src: fix building --without-v8-platform #11088

mykmelez · 2017-01-31T18:33:50Z

Node fails to compile when configured --without-v8-platform because of two issues with the implementation in src/node.cc when !NODE_USE_V8_PLATFORM. This branch fixes those two issues.

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes (Note: make -j4 test passes with these changes when I build with the default configure options. It fails when I build --without-v8-platform, but since previously it didn't even compile with that option, these changes are still an improvement.)
commit message follows commit guidelines

Affected core subsystem(s)

The affected core subsystem appears to be "src", or possibly "build." The only file that is changed is src/node.cc.

v8_platform.platform_ is referenced by node::Start without regard to the value of NODE_USE_V8_PLATFORM, so it should be declared unconditionally, otherwise Node fails to compile when !NODE_USE_V8_PLATFORM.

The call signature of v8_platform.StartInspector needs to be the same whether or not NODE_USE_V8_PLATFORM, otherwise Node will fail to compile if HAVE_INSPECTOR and !NODE_USE_V8_PLATFORM.

bnoordhuis · 2017-02-01T14:38:23Z

Second commit LGTM but @matthewloring should review the first one because I don't think passing a nullptr to node::tracing::Agent::Start() is allowed.

matthewloring · 2017-02-01T17:19:37Z

That is correct. I think we will need to follow the same pattern used by the inspector agent here which starts the agent if NODE_USE_V8_PLATFORM is set and throwing an exception otherwise. We could then call the function in place of the call here.

node::tracing::Agent::Start can't accept a nullptr argument to its platform parameter, so don't call it when Node is compiled with NODE_USE_V8_PLATFORM=0.

mykmelez · 2017-02-01T22:35:18Z

@bnoordhuis, @matthewloring: We can't throw an exception, because node::Start() calls node::tracing::Agent::Start() before the Environment gets defined, so Environment::ThrowError() is not yet available. However, we can still refactor the call to node::tracing::Agent::Start() into v8_platform and avoid calling it unless NODE_USE_V8_PLATFORM=1. I've done that in 0e9c0cb.

Note: I opted to warn rather than exiting if NODE_USE_V8_PLATFORM=0 and --trace-events-enabled, because exiting would complicate node::Start(), which would have to check the return value of v8_platform.StartTracingAgent() and conditionally exit early. However, I can change it to exit early if you think it's important for the process to die in that case.

Note that I had to move the call to node::tracing::Agent::Stop() into v8_platform as well in order to ensure it doesn't get called if NODE_USE_V8_PLATFORM=0.

matthewloring · 2017-02-01T22:43:10Z

I don't have a preference between an error and a warning. The approach looks good on my end.

mykmelez · 2017-02-01T22:45:40Z

Note that I had to move the call to node::tracing::Agent::Stop() into v8_platform as well in order to ensure it doesn't get called if NODE_USE_V8_PLATFORM=0.

Erm, to be precise: I didn't have to move that call, since Agent::Stop() returns early if !IsStarted(). But if v8_platform is conditionally calling Agent::Start(), then it seems intuitive for it to conditionally call Agent::Stop() as well.

bnoordhuis · 2017-02-02T13:23:41Z

src/node.cc

@@ -228,16 +228,31 @@ static struct {
  }
 #endif  // HAVE_INSPECTOR

+  void StartTracingAgent() {
+    tracing_agent = new tracing::Agent();


Can you make this a property of v8_platform while you're here (and call it tracing_agent_)? Maybe also insert a CHECK(tracing_agent_ == nullptr) to detect double calls.

Yes, I've done both of these things in b93584b. In the process, I also spotted another direct call to tracing_agent->Stop() and replaced it with a call to v8_platform.StopTracingAgent().

Move tracing_agent global into the v8_platform struct, renaming it to tracing_agent_; CHECK(tracing_agent_ == nullptr) in StartTracingAgent() to detect double calls; and relace another tracing_agent->Stop() call with a call to StopTracingAgent().

bnoordhuis

Thanks, LGTM. CI: https://ci.nodejs.org/job/node-test-pull-request/6171/

mhdawson

LGTM

* declare v8_platform.platform_ unconditionally v8_platform.platform_ is referenced by node::Start without regard to the value of NODE_USE_V8_PLATFORM, so it should be declared unconditionally, otherwise Node fails to compile when !NODE_USE_V8_PLATFORM. * update v8_platform.StartInspector signature The call signature of v8_platform.StartInspector needs to be the same whether or not NODE_USE_V8_PLATFORM, otherwise Node will fail to compile if HAVE_INSPECTOR and !NODE_USE_V8_PLATFORM. * don't call tracing_agent->Start w/nullptr node::tracing::Agent::Start can't accept a nullptr argument to its platform parameter, so don't call it when Node is compiled with NODE_USE_V8_PLATFORM=0. * refactor tracing_agent into v8_platform Move tracing_agent global into the v8_platform struct, renaming it to tracing_agent_; CHECK(tracing_agent_ == nullptr) in StartTracingAgent() to detect double calls; and relace another tracing_agent->Stop() call with a call to StopTracingAgent(). PR-URL: #11088 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

jasnell · 2017-02-03T22:53:00Z

Landed in 046f66a

italoacasas · 2017-02-04T00:49:35Z

This commits depends on a semver-major, do we have a plan to backport this to v7.-x?

mykmelez · 2017-02-04T01:45:08Z

This commits depends on a semver-major, do we have a plan to backport this to v7.-x?

The issue doesn't exist in v7.x, so there's no need to backport anything.

mykmelez · 2017-02-04T02:00:50Z

The issue doesn't exist in v7.x, so there's no need to backport anything.

Erm, correction: one of the issues addressed in this PR does exist in v7.x, so I've opened #11157 to backport its fix.

* declare v8_platform.platform_ unconditionally v8_platform.platform_ is referenced by node::Start without regard to the value of NODE_USE_V8_PLATFORM, so it should be declared unconditionally, otherwise Node fails to compile when !NODE_USE_V8_PLATFORM. * update v8_platform.StartInspector signature The call signature of v8_platform.StartInspector needs to be the same whether or not NODE_USE_V8_PLATFORM, otherwise Node will fail to compile if HAVE_INSPECTOR and !NODE_USE_V8_PLATFORM. * don't call tracing_agent->Start w/nullptr node::tracing::Agent::Start can't accept a nullptr argument to its platform parameter, so don't call it when Node is compiled with NODE_USE_V8_PLATFORM=0. * refactor tracing_agent into v8_platform Move tracing_agent global into the v8_platform struct, renaming it to tracing_agent_; CHECK(tracing_agent_ == nullptr) in StartTracingAgent() to detect double calls; and relace another tracing_agent->Stop() call with a call to StopTracingAgent(). PR-URL: nodejs#11088 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Michael Dawson <michael_dawson@ca.ibm.com>

gibfahn · 2017-06-17T20:10:03Z

Marking dont-land-on-v6.x due to #11157 (comment), if this is incorrect let me know.

mykmelez added 2 commits January 31, 2017 10:14

src: declare v8_platform.platform_ unconditionally

9c95606

v8_platform.platform_ is referenced by node::Start without regard to the value of NODE_USE_V8_PLATFORM, so it should be declared unconditionally, otherwise Node fails to compile when !NODE_USE_V8_PLATFORM.

src: update v8_platform.StartInspector signature

e619725

The call signature of v8_platform.StartInspector needs to be the same whether or not NODE_USE_V8_PLATFORM, otherwise Node will fail to compile if HAVE_INSPECTOR and !NODE_USE_V8_PLATFORM.

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Jan 31, 2017

mykmelez changed the title ~~fix building --without-v8-platform~~ src: fix building --without-v8-platform Jan 31, 2017

src: don't call tracing_agent->Start w/nullptr

0e9c0cb

node::tracing::Agent::Start can't accept a nullptr argument to its platform parameter, so don't call it when Node is compiled with NODE_USE_V8_PLATFORM=0.

mykmelez force-pushed the fix-without-v8-platform branch from 907f1f2 to 0e9c0cb Compare February 1, 2017 22:31

bnoordhuis requested changes Feb 2, 2017

View reviewed changes

bnoordhuis approved these changes Feb 2, 2017

View reviewed changes

mhdawson approved these changes Feb 3, 2017

View reviewed changes

jasnell closed this Feb 3, 2017

italoacasas added the dont-land-on-v7.x label Feb 4, 2017

mykmelez deleted the fix-without-v8-platform branch February 4, 2017 01:38

mykmelez mentioned this pull request Feb 4, 2017

src: fix building --without-v8-platform (backport v7.x) #11157

Closed

2 tasks

jasnell mentioned this pull request Apr 4, 2017

8.0.0 Release Proposal #12220

Closed

gibfahn mentioned this pull request Jun 15, 2017

Auditing for 6.11.1 nodejs/Release#230

Closed

3 tasks

gibfahn added the dont-land-on-v6.x label Jun 17, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

src: fix building --without-v8-platform #11088

src: fix building --without-v8-platform #11088

mykmelez commented Jan 31, 2017

bnoordhuis commented Feb 1, 2017

matthewloring commented Feb 1, 2017

mykmelez commented Feb 1, 2017

matthewloring commented Feb 1, 2017 •

edited

Loading

mykmelez commented Feb 1, 2017

bnoordhuis Feb 2, 2017

mykmelez Feb 2, 2017

bnoordhuis left a comment

mhdawson left a comment

jasnell commented Feb 3, 2017

italoacasas commented Feb 4, 2017

mykmelez commented Feb 4, 2017

mykmelez commented Feb 4, 2017

gibfahn commented Jun 17, 2017

src: fix building --without-v8-platform #11088

src: fix building --without-v8-platform #11088

Conversation

mykmelez commented Jan 31, 2017

Checklist

Affected core subsystem(s)

bnoordhuis commented Feb 1, 2017

matthewloring commented Feb 1, 2017

mykmelez commented Feb 1, 2017

matthewloring commented Feb 1, 2017 • edited Loading

mykmelez commented Feb 1, 2017

bnoordhuis Feb 2, 2017

Choose a reason for hiding this comment

mykmelez Feb 2, 2017

Choose a reason for hiding this comment

bnoordhuis left a comment

Choose a reason for hiding this comment

mhdawson left a comment

Choose a reason for hiding this comment

jasnell commented Feb 3, 2017

italoacasas commented Feb 4, 2017

mykmelez commented Feb 4, 2017

mykmelez commented Feb 4, 2017

gibfahn commented Jun 17, 2017

matthewloring commented Feb 1, 2017 •

edited

Loading