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 InspectorStarted macro guard #13167

Closed
wants to merge 1 commit into from

Conversation

@danbev
Copy link
Member

commented May 23, 2017

Currently the InspectorStarted function is guarded by the else clause of
the NODE_USE_V8_PLATFORM macro. If node is configured --without-ssl then
NODE_USE_V8_PLATFORM will be 1 but the nested HAVE_INSPECTOR macro
will not be 0 which will lead to that there will be no InspectorStarted
function defined.

If building --without-inspector or --without-ssl the following
compilation error will occur:

../src/node.cc:4470:57: error: no member named 'InspectorStarted' in
'node::(anonymous struct at ../src/node.cc:241:8)'
  if (debug_options.inspector_enabled() &&
!v8_platform.InspectorStarted(&env))
                                            ~~~~~~~~~~~ ^
../src/node.cc:4470:57: error: no member named 'InspectorStarted' in
'node::(anonymous struct at ../src/node.cc:241:8)'
  if (debug_options.inspector_enabled() &&
!v8_platform.InspectorStarted(&env))
                                            ~~~~~~~~~~~ ^
1 error generated.

This commit adds a separate if preprocessor directive to catch the case
when either --without-ssl/--without-inspector and --without-v8-platform
combinations are used to configure node.

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines
Affected core subsystem(s)
@danbev

This comment has been minimized.

Copy link
Member Author

commented May 23, 2017

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented May 23, 2017

Random observations about the src/node.cc in master:

  1. The call to v8_platform.StartInspector() is guarded by a #ifdef HAVE_INSPECTOR but the call to v8_platform.InspectorStarted() is not.

  2. The v8_platform.StartInspector() for the NODE_USE_V8_PLATFORM == 0 case is never called because of point 1.

  3. There are several (guarded) calls to env->inspector_agent()->IsStarted(), there is only one call to v8_platform.InspectorStarted(). The patch below reduces the line count and is arguably more straightforward.

  4. The logic in node.gyp and node.gypi seems wrong: if the inspector depends on libplatform, then node_use_v8_platform=="false" should imply v8_enable_inspector==0 - i.e., if NODE_USE_V8_PLATFORM == 0, then logically HAVE_INSPECTOR == 0.

I think if you enforce 4, you'll be able to cut quite a bit of code and ifdef logic. Good idea, bad idea?

diff --git a/src/node.cc b/src/node.cc
index a5c86d4..81e1dd7 100644
--- a/src/node.cc
+++ b/src/node.cc
@@ -260,10 +260,6 @@ static struct {
                       const node::DebugOptions& options) {
     return env->inspector_agent()->Start(platform_, script_path, options);
   }
-
-  bool InspectorStarted(Environment *env) {
-    return env->inspector_agent()->IsStarted();
-  }
 #endif  // HAVE_INSPECTOR
 
   void StartTracingAgent() {
@@ -293,9 +289,6 @@ static struct {
                     "so event tracing is not available.\n");
   }
   void StopTracingAgent() {}
-  bool InspectorStarted(Environment *env) {
-    return false;
-  }
 #endif  // !NODE_USE_V8_PLATFORM
 } v8_platform;
 
@@ -4467,8 +4460,13 @@ inline int Start(Isolate* isolate, IsolateData* isolate_data,
   const char* path = argc > 1 ? argv[1] : nullptr;
   StartDebug(&env, path, debug_options);
 
-  if (debug_options.inspector_enabled() && !v8_platform.InspectorStarted(&env))
-    return 12;  // Signal internal error.
+  if (debug_options.inspector_enabled()) {
+#if HAVE_INSPECTOR
+    if (!env->inspector_agent()->IsStarted()) return 12;  // Internal error.
+#else
+    return 12;  // Internal error.
+#endif
+  }
 
   env.set_abort_on_uncaught_exception(abort_on_uncaught_exception);
 
@danbev

This comment has been minimized.

Copy link
Member Author

commented May 23, 2017

I think if you enforce 4, you'll be able to cut quite a bit of code and ifdef logic. Good idea, bad idea?

Sounds good to me. I'll take a look at this.

Just for my understanding of when --without-v8-platform would be used. When configured, the responsibility of setting the platform (v8::V8::InitializePlatform(platform_);) is up to the caller and building node --without-v8-platform is expected to fail with error from V8 stating this. I'm guessing this is use by someone embedding node, perhaps Electron?

src/node.cc Outdated
@@ -293,10 +293,13 @@ static struct {
"so event tracing is not available.\n");
}
void StopTracingAgent() {}
#endif // !NODE_USE_V8_PLATFORM

#if NODE_USE_V8_PLATFORM == 0 || HAVE_INSPECTOR == 0

This comment has been minimized.

Copy link
@refack

refack May 23, 2017

Member

shouldn't it be #if !defined(NODE_USE_V8_PLATFORM) || !defined(HAVE_INSPECTOR)?

This comment has been minimized.

Copy link
@danbev

danbev May 23, 2017

Author Member

I thought so too, but we need to check the value as the names will always be defined as macros.

This comment has been minimized.

Copy link
@refack

refack May 23, 2017

Member

sorry to nag, but what about #if !NODE_USE_V8_PLATFORM || !HAVE_INSPECTOR

This comment has been minimized.

Copy link
@danbev

danbev May 23, 2017

Author Member

sorry to nag, but what about #if !NODE_USE_V8_PLATFORM || !HAVE_INSPECTOR

Yes, that works as it is equivalent to the current statement. I'm fine with either.

This comment has been minimized.

Copy link
@danbev

danbev May 26, 2017

Author Member

I'll make this change.

@mscdex mscdex added the inspector label May 23, 2017

@bnoordhuis

This comment has been minimized.

Copy link
Member

commented May 23, 2017

Just for my understanding of when --without-v8-platform would be used. When configured, the responsibility of setting the platform (v8::V8::InitializePlatform(platform_);) is up to the caller and building node --without-v8-platform is expected to fail with error from V8 stating this. I'm guessing this is use by someone embedding node, perhaps Electron?

Yes, correct.

@bnoordhuis
Copy link
Member

left a comment

This PR LGTM as a stop-gap measure, by the way.

@kfarnung
Copy link
Member

left a comment

Ran into this problem in my fork and had a similar fix in mind. 👍

@kfarnung kfarnung referenced this pull request May 25, 2017
2 of 2 tasks complete
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request May 25, 2017
src: fix InspectorStarted macro guard
Currently the InspectorStarted function is guarded by the else clause of
the NODE_USE_V8_PLATFORM macro. If node is configured --without-ssl then
NODE_USE_V8_PLATFORM will be 1 but the nested HAVE_INSPECTOR macro
will not be 0 which will lead to that there will be no InspectorStarted
function defined.

If building --without-inspector or --without-ssl the following
compilation error will occur:
../src/node.cc:4470:57: error: no member named 'InspectorStarted' in
'node::(anonymous struct at ../src/node.cc:241:8)'
  if (debug_options.inspector_enabled() &&
!v8_platform.InspectorStarted(&env))
                                            ~~~~~~~~~~~ ^
../src/node.cc:4470:57: error: no member named 'InspectorStarted' in
'node::(anonymous struct at ../src/node.cc:241:8)'
  if (debug_options.inspector_enabled() &&
!v8_platform.InspectorStarted(&env))
                                            ~~~~~~~~~~~ ^
1 error generated.

This commit adds a separate if preprocessor directive to catch the case
when either --without-ssl/--without-inspector and --without-v8-platform
combinations are used to configure node.

PR-URL: nodejs/node#13167
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
kfarnung added a commit to kfarnung/node-chakracore that referenced this pull request May 25, 2017
src: fix InspectorStarted macro guard
Currently the InspectorStarted function is guarded by the else clause of
the NODE_USE_V8_PLATFORM macro. If node is configured --without-ssl then
NODE_USE_V8_PLATFORM will be 1 but the nested HAVE_INSPECTOR macro
will not be 0 which will lead to that there will be no InspectorStarted
function defined.

If building --without-inspector or --without-ssl the following
compilation error will occur:
../src/node.cc:4470:57: error: no member named 'InspectorStarted' in
'node::(anonymous struct at ../src/node.cc:241:8)'
  if (debug_options.inspector_enabled() &&
!v8_platform.InspectorStarted(&env))
                                            ~~~~~~~~~~~ ^
../src/node.cc:4470:57: error: no member named 'InspectorStarted' in
'node::(anonymous struct at ../src/node.cc:241:8)'
  if (debug_options.inspector_enabled() &&
!v8_platform.InspectorStarted(&env))
                                            ~~~~~~~~~~~ ^
1 error generated.

This commit adds a separate if preprocessor directive to catch the case
when either --without-ssl/--without-inspector and --without-v8-platform
combinations are used to configure node.

PR-URL: nodejs/node#13167
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Kyle Farnung <kfarnung@microsoft.com>
src: fix InspectorStarted macro guard
Currently the InspectorStarted function is guarded by the else clause of
the NODE_USE_V8_PLATFORM macro. If node is configured --without-ssl then
NODE_USE_V8_PLATFORM will be 1 but the nested HAVE_INSPECTOR macro
will not be 0 which will lead to that there will be no InspectorStarted
function defined.

If building --without-inspector or --without-ssl the following
compilation error will occur:
../src/node.cc:4470:57: error: no member named 'InspectorStarted' in
'node::(anonymous struct at ../src/node.cc:241:8)'
  if (debug_options.inspector_enabled() &&
!v8_platform.InspectorStarted(&env))
                                            ~~~~~~~~~~~ ^
../src/node.cc:4470:57: error: no member named 'InspectorStarted' in
'node::(anonymous struct at ../src/node.cc:241:8)'
  if (debug_options.inspector_enabled() &&
!v8_platform.InspectorStarted(&env))
                                            ~~~~~~~~~~~ ^
1 error generated.

This commit adds a separate if preprocessor directive to catch the case
when either --without-ssl/--without-inspector and --without-v8-platform
combinations are used to configure node.

@danbev danbev force-pushed the danbev:move-inspectorstarted branch to 7bb3c4e May 26, 2017

@danbev

This comment has been minimized.

Copy link
Member Author

commented May 26, 2017

If there are no objections I'd like to merge this and then follow up with a separate PR when I have a little more time (hopefully next week) to address the comments from @bnoordhuis.

@danbev

This comment has been minimized.

Copy link
Member Author

commented May 26, 2017

@refack
refack approved these changes May 26, 2017
danbev added a commit to danbev/node that referenced this pull request May 27, 2017
src: fix InspectorStarted macro guard
Currently the InspectorStarted function is guarded by the else clause of
the NODE_USE_V8_PLATFORM macro. If node is configured --without-ssl then
NODE_USE_V8_PLATFORM will be 1 but the nested HAVE_INSPECTOR macro
will not be 0 which will lead to that there will be no InspectorStarted
function defined.

If building --without-inspector or --without-ssl the following
compilation error will occur:
../src/node.cc:4470:57: error: no member named 'InspectorStarted' in
'node::(anonymous struct at ../src/node.cc:241:8)'
  if (debug_options.inspector_enabled() &&
!v8_platform.InspectorStarted(&env))
                                            ~~~~~~~~~~~ ^
../src/node.cc:4470:57: error: no member named 'InspectorStarted' in
'node::(anonymous struct at ../src/node.cc:241:8)'
  if (debug_options.inspector_enabled() &&
!v8_platform.InspectorStarted(&env))
                                            ~~~~~~~~~~~ ^
1 error generated.

This commit adds a separate if preprocessor directive to catch the case
when either --without-ssl/--without-inspector and --without-v8-platform
combinations are used to configure node.

PR-URL: nodejs#13167
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@danbev

This comment has been minimized.

Copy link
Member Author

commented May 27, 2017

Landed in ae5e65c

@danbev danbev closed this May 27, 2017

jasnell added a commit that referenced this pull request May 28, 2017
src: fix InspectorStarted macro guard
Currently the InspectorStarted function is guarded by the else clause of
the NODE_USE_V8_PLATFORM macro. If node is configured --without-ssl then
NODE_USE_V8_PLATFORM will be 1 but the nested HAVE_INSPECTOR macro
will not be 0 which will lead to that there will be no InspectorStarted
function defined.

If building --without-inspector or --without-ssl the following
compilation error will occur:
../src/node.cc:4470:57: error: no member named 'InspectorStarted' in
'node::(anonymous struct at ../src/node.cc:241:8)'
  if (debug_options.inspector_enabled() &&
!v8_platform.InspectorStarted(&env))
                                            ~~~~~~~~~~~ ^
../src/node.cc:4470:57: error: no member named 'InspectorStarted' in
'node::(anonymous struct at ../src/node.cc:241:8)'
  if (debug_options.inspector_enabled() &&
!v8_platform.InspectorStarted(&env))
                                            ~~~~~~~~~~~ ^
1 error generated.

This commit adds a separate if preprocessor directive to catch the case
when either --without-ssl/--without-inspector and --without-v8-platform
combinations are used to configure node.

PR-URL: #13167
Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl>
Reviewed-By: Refael Ackermann <refack@gmail.com>
@jasnell jasnell referenced this pull request May 28, 2017
@gibfahn gibfahn referenced this pull request Jun 15, 2017
2 of 3 tasks complete

@danbev danbev deleted the danbev:move-inspectorstarted branch Jun 28, 2017

@MylesBorins

This comment has been minimized.

Copy link
Member

commented Jul 17, 2017

This does not land cleanly in LTS. Please feel free to manually backport. Please also feel free to replace the backport request label with do-not-land if it shouldn't land

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