Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 4 commits into from

Conversation

@mykmelez
Copy link
Contributor

commented Jan 31, 2017

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.

mykmelez added 2 commits Jan 31, 2017
src: 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.
src: 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.

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

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented Feb 1, 2017

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

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2017

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.

src: 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.

@mykmelez mykmelez force-pushed the mykmelez:fix-without-v8-platform branch to 0e9c0cb Feb 1, 2017

@mykmelez

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2017

@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

This comment has been minimized.

Copy link
Contributor

commented Feb 1, 2017

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

@mykmelez

This comment has been minimized.

Copy link
Contributor Author

commented Feb 1, 2017

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.

src/node.cc Outdated
@@ -228,16 +228,31 @@ static struct {
}
#endif // HAVE_INSPECTOR

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

This comment has been minimized.

Copy link
@bnoordhuis

bnoordhuis Feb 2, 2017

Member

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.

This comment has been minimized.

Copy link
@mykmelez

mykmelez Feb 2, 2017

Author Contributor

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().

src: 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().
@bnoordhuis
Copy link
Member

left a comment

@mhdawson
Copy link
Member

left a comment

LGTM

jasnell added a commit that referenced this pull request Feb 3, 2017
src: fix building --without-v8-plartform
* 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

This comment has been minimized.

Copy link
Member

commented Feb 3, 2017

Landed in 046f66a

@jasnell jasnell closed this Feb 3, 2017

@italoacasas

This comment has been minimized.

Copy link
Member

commented Feb 4, 2017

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

@mykmelez mykmelez deleted the mykmelez:fix-without-v8-platform branch Feb 4, 2017

@mykmelez

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2017

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 mykmelez referenced this pull request Feb 4, 2017
2 of 2 tasks complete
@mykmelez

This comment has been minimized.

Copy link
Contributor Author

commented Feb 4, 2017

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.

KryDos added a commit to KryDos/node that referenced this pull request Feb 25, 2017
src: fix building --without-v8-plartform
* 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>
@jasnell jasnell referenced this pull request Apr 4, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete
@gibfahn

This comment has been minimized.

Copy link
Member

commented Jun 17, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.