From a6bd2c2f81a0a238bf0f45174fee70b7870f8ef2 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Thu, 12 Mar 2020 21:32:07 -0400 Subject: [PATCH 1/6] src: unconditionally include report feature This commit removes all #ifdef NODE_REPORT checks in the src directory. PR-URL: https://github.com/nodejs/node/pull/32242 Fixes: https://github.com/nodejs/node/issues/26293 Reviewed-By: Richard Lau Reviewed-By: David Carlier Reviewed-By: Gireesh Punathil Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- src/node.cc | 7 +------ src/node_binding.cc | 8 +------- src/node_errors.cc | 6 ++---- src/node_options.cc | 8 +------- src/node_options.h | 8 -------- 5 files changed, 5 insertions(+), 32 deletions(-) diff --git a/src/node.cc b/src/node.cc index 1fec85aa793dc4..290d36d2d2058a 100644 --- a/src/node.cc +++ b/src/node.cc @@ -35,6 +35,7 @@ #include "node_options-inl.h" #include "node_perf.h" #include "node_process.h" +#include "node_report.h" #include "node_revert.h" #include "node_v8_platform-inl.h" #include "node_version.h" @@ -67,10 +68,6 @@ #include "large_pages/node_large_page.h" -#ifdef NODE_REPORT -#include "node_report.h" -#endif - #if defined(__APPLE__) || defined(__linux__) #define NODE_USE_V8_WASM_TRAP_HANDLER 1 #else @@ -781,11 +778,9 @@ int InitializeNodeWithArgs(std::vector* argv, // Make inherited handles noninheritable. uv_disable_stdio_inheritance(); -#ifdef NODE_REPORT // Cache the original command line to be // used in diagnostic reports. per_process::cli_options->cmdline = *argv; -#endif // NODE_REPORT #if defined(NODE_V8_OPTIONS) // Should come before the call to V8::SetFlagsFromCommandLine() diff --git a/src/node_binding.cc b/src/node_binding.cc index 37c64000650085..592d0ca2a397e2 100644 --- a/src/node_binding.cc +++ b/src/node_binding.cc @@ -17,12 +17,6 @@ #define NODE_BUILTIN_ICU_MODULES(V) #endif -#if NODE_REPORT -#define NODE_BUILTIN_REPORT_MODULES(V) V(report) -#else -#define NODE_BUILTIN_REPORT_MODULES(V) -#endif - #if HAVE_INSPECTOR #define NODE_BUILTIN_PROFILER_MODULES(V) V(profiler) #else @@ -67,6 +61,7 @@ V(pipe_wrap) \ V(process_wrap) \ V(process_methods) \ + V(report) \ V(serdes) \ V(signal_wrap) \ V(spawn_sync) \ @@ -94,7 +89,6 @@ NODE_BUILTIN_STANDARD_MODULES(V) \ NODE_BUILTIN_OPENSSL_MODULES(V) \ NODE_BUILTIN_ICU_MODULES(V) \ - NODE_BUILTIN_REPORT_MODULES(V) \ NODE_BUILTIN_PROFILER_MODULES(V) \ NODE_BUILTIN_DTRACE_MODULES(V) diff --git a/src/node_errors.cc b/src/node_errors.cc index 9d038e3d1683e7..ae1da87ef6b0e0 100644 --- a/src/node_errors.cc +++ b/src/node_errors.cc @@ -4,9 +4,7 @@ #include "debug_utils-inl.h" #include "node_errors.h" #include "node_internals.h" -#ifdef NODE_REPORT #include "node_report.h" -#endif #include "node_process.h" #include "node_v8_platform-inl.h" #include "util-inl.h" @@ -405,14 +403,14 @@ void OnFatalError(const char* location, const char* message) { } else { FPrintF(stderr, "FATAL ERROR: %s\n", message); } -#ifdef NODE_REPORT + Isolate* isolate = Isolate::GetCurrent(); Environment* env = Environment::GetCurrent(isolate); if (env == nullptr || env->isolate_data()->options()->report_on_fatalerror) { report::TriggerNodeReport( isolate, env, message, "FatalError", "", Local()); } -#endif // NODE_REPORT + fflush(stderr); ABORT(); } diff --git a/src/node_options.cc b/src/node_options.cc index 3bf1031f1667ba..35dd274d6fe673 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -76,7 +76,7 @@ void PerProcessOptions::CheckOptions(std::vector* errors) { void PerIsolateOptions::CheckOptions(std::vector* errors) { per_env->CheckOptions(errors); -#ifdef NODE_REPORT + if (per_env->experimental_report) { // Assign the report_signal default value here. Once the // --experimental-report flag is dropped, move this initialization to @@ -117,7 +117,6 @@ void PerIsolateOptions::CheckOptions(std::vector* errors) { "--report-uncaught-exception option is valid only when " "--experimental-report is set"); } -#endif // NODE_REPORT } void EnvironmentOptions::CheckOptions(std::vector* errors) { @@ -361,12 +360,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { &EnvironmentOptions::experimental_vm_modules, kAllowedInEnvironment); AddOption("--experimental-worker", "", NoOp{}, kAllowedInEnvironment); -#ifdef NODE_REPORT AddOption("--experimental-report", "enable report generation", &EnvironmentOptions::experimental_report, kAllowedInEnvironment); -#endif // NODE_REPORT AddOption("--experimental-wasi-unstable-preview1", "experimental WASI support", &EnvironmentOptions::experimental_wasi, @@ -620,8 +617,6 @@ PerIsolateOptionsParser::PerIsolateOptionsParser( "disable runtime allocation of executable memory", V8Option{}, kAllowedInEnvironment); - -#ifdef NODE_REPORT AddOption("--report-uncaught-exception", "generate diagnostic report on uncaught exceptions", &PerIsolateOptions::report_uncaught_exception, @@ -650,7 +645,6 @@ PerIsolateOptionsParser::PerIsolateOptionsParser( " (default: current working directory of Node.js process)", &PerIsolateOptions::report_directory, kAllowedInEnvironment); -#endif // NODE_REPORT Insert(eop, &PerIsolateOptions::get_per_env_options); } diff --git a/src/node_options.h b/src/node_options.h index 470007f06b6342..958eaf29571e01 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -151,9 +151,7 @@ class EnvironmentOptions : public Options { bool syntax_check_only = false; bool has_eval_string = false; -#ifdef NODE_REPORT bool experimental_report = false; -#endif // NODE_REPORT bool experimental_wasi = false; std::string eval_string; bool print_eval = false; @@ -187,15 +185,12 @@ class PerIsolateOptions : public Options { std::shared_ptr per_env { new EnvironmentOptions() }; bool track_heap_objects = false; bool no_node_snapshot = false; - -#ifdef NODE_REPORT bool report_uncaught_exception = false; bool report_on_signal = false; bool report_on_fatalerror = false; std::string report_signal; std::string report_filename; std::string report_directory; -#endif // NODE_REPORT inline EnvironmentOptions* get_per_env_options(); void CheckOptions(std::vector* errors) override; }; @@ -239,10 +234,7 @@ class PerProcessOptions : public Options { #endif std::string use_largepages = "off"; bool trace_sigint = false; - -#ifdef NODE_REPORT std::vector cmdline; -#endif // NODE_REPORT inline PerIsolateOptions* get_per_isolate_options(); void CheckOptions(std::vector* errors) override; From f1d3d927e1225f0b8a959d36024788bd65600c5b Mon Sep 17 00:00:00 2001 From: cjihrig Date: Thu, 12 Mar 2020 22:20:36 -0400 Subject: [PATCH 2/6] build: remove node_report option in node.gyp PR-URL: https://github.com/nodejs/node/pull/32242 Fixes: https://github.com/nodejs/node/issues/26293 Reviewed-By: Richard Lau Reviewed-By: David Carlier Reviewed-By: Gireesh Punathil Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- node.gyp | 53 +++++++++++++---------------------------------------- 1 file changed, 13 insertions(+), 40 deletions(-) diff --git a/node.gyp b/node.gyp index 8a23ec2f1f398b..c8e7387c2a8cf1 100644 --- a/node.gyp +++ b/node.gyp @@ -337,6 +337,8 @@ 'type': 'executable', 'defines': [ + 'NODE_ARCH="<(target_arch)"', + 'NODE_PLATFORM="<(OS)"', 'NODE_WANT_INTERNALS=1', ], @@ -424,13 +426,6 @@ 'OTHER_LDFLAGS': [ '-Wl,-rpath,@loader_path', ], }, }], - [ 'node_report=="true"', { - 'defines': [ - 'NODE_REPORT', - 'NODE_ARCH="<(target_arch)"', - 'NODE_PLATFORM="<(OS)"', - ], - }], ['OS=="win"', { 'libraries': [ 'Dbghelp.lib', @@ -596,6 +591,9 @@ 'src/node_process_events.cc', 'src/node_process_methods.cc', 'src/node_process_object.cc', + 'src/node_report.cc', + 'src/node_report_module.cc', + 'src/node_report_utils.cc', 'src/node_serdes.cc', 'src/node_sockaddr.cc', 'src/node_stat_watcher.cc', @@ -685,6 +683,7 @@ 'src/node_perf_common.h', 'src/node_platform.h', 'src/node_process.h', + 'src/node_report.h', 'src/node_revert.h', 'src/node_root_certs.h', 'src/node_sockaddr.h', @@ -781,6 +780,7 @@ 'libraries': [ 'Dbghelp', 'Psapi', + 'Ws2_32', ], }], [ 'node_use_etw=="true"', { @@ -865,23 +865,6 @@ 'src/tls_wrap.h' ], }], - [ 'node_report=="true"', { - 'sources': [ - 'src/node_report.cc', - 'src/node_report_module.cc', - 'src/node_report_utils.cc', - ], - 'defines': [ - 'NODE_REPORT', - 'NODE_ARCH="<(target_arch)"', - 'NODE_PLATFORM="<(OS)"', - ], - 'conditions': [ - ['OS=="win"', { - 'libraries': [ 'Ws2_32' ], - }], - ], - }], [ 'OS in "linux freebsd mac" and ' 'target_arch=="x64" and ' 'node_target_type=="executable"', { @@ -1138,7 +1121,11 @@ 'test/cctest', ], - 'defines': [ 'NODE_WANT_INTERNALS=1' ], + 'defines': [ + 'NODE_ARCH="<(target_arch)"', + 'NODE_PLATFORM="<(OS)"', + 'NODE_WANT_INTERNALS=1', + ], 'sources': [ 'src/node_snapshot_stub.cc', @@ -1155,6 +1142,7 @@ 'test/cctest/test_linked_binding.cc', 'test/cctest/test_per_process.cc', 'test/cctest/test_platform.cc', + 'test/cctest/test_report_util.cc', 'test/cctest/test_sockaddr.cc', 'test/cctest/test_traced_value.cc', 'test/cctest/test_util.cc', @@ -1192,21 +1180,6 @@ 'OTHER_LDFLAGS': [ '-Wl,-rpath,@loader_path', ], }, }], - [ 'node_report=="true"', { - 'sources': [ - 'test/cctest/test_report_util.cc', - ], - 'defines': [ - 'NODE_REPORT', - 'NODE_ARCH="<(target_arch)"', - 'NODE_PLATFORM="<(OS)"', - ], - 'conditions': [ - ['OS=="win"', { - 'libraries': [ 'Ws2_32' ], - }], - ], - }], ['OS=="win"', { 'libraries': [ 'Dbghelp.lib', From 765c3759eff73851bdc5c4f1f8f63819eed9a2e9 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Thu, 12 Mar 2020 22:39:12 -0400 Subject: [PATCH 3/6] build: make --without-report a no-op This commit makes the configure --without-report flag a no-op. This commit also updates a test that depends on the report CLI flags being conditionally present. PR-URL: https://github.com/nodejs/node/pull/32242 Fixes: https://github.com/nodejs/node/issues/26293 Reviewed-By: Richard Lau Reviewed-By: David Carlier Reviewed-By: Gireesh Punathil Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- configure.py | 7 +++---- .../test-process-env-allowed-flags-are-documented.js | 2 -- 2 files changed, 3 insertions(+), 6 deletions(-) diff --git a/configure.py b/configure.py index 0190e31b41a214..e98801f2279f6b 100755 --- a/configure.py +++ b/configure.py @@ -538,12 +538,12 @@ dest='without_npm', help='do not install the bundled npm (package manager)') +# Dummy option for backwards compatibility parser.add_option('--without-report', action='store_true', - dest='without_report', - help='build without report') + dest='unused_without_report', + help=optparse.SUPPRESS_HELP) -# Dummy option for backwards compatibility parser.add_option('--with-snapshot', action='store_true', dest='unused_with_snapshot', @@ -1004,7 +1004,6 @@ def configure_node(o): o['variables']['OS'] = 'android' o['variables']['node_prefix'] = options.prefix o['variables']['node_install_npm'] = b(not options.without_npm) - o['variables']['node_report'] = b(not options.without_report) o['variables']['debug_node'] = b(options.debug_node) o['default_configuration'] = 'Debug' if options.debug else 'Release' diff --git a/test/parallel/test-process-env-allowed-flags-are-documented.js b/test/parallel/test-process-env-allowed-flags-are-documented.js index f356f88fe9c740..01999cc46241c6 100644 --- a/test/parallel/test-process-env-allowed-flags-are-documented.js +++ b/test/parallel/test-process-env-allowed-flags-are-documented.js @@ -59,8 +59,6 @@ const conditionalOpts = [ filter: (opt) => opt === '--icu-data-dir' }, { include: process.features.inspector, filter: (opt) => opt.startsWith('--inspect') || opt === '--debug-port' }, - { include: process.config.variables.node_report, - filter: (opt) => opt.includes('-report') }, ]; documented.forEach((opt) => { conditionalOpts.forEach(({ include, filter }) => { From 4c64e7c59a655142367a361bae873683208b9f9b Mon Sep 17 00:00:00 2001 From: cjihrig Date: Thu, 12 Mar 2020 22:58:29 -0400 Subject: [PATCH 4/6] test: remove common.skipIfReportDisabled() The report feature won't ever be disabled moving forward, so checking for its existence in the tests is no longer needed. PR-URL: https://github.com/nodejs/node/pull/32242 Fixes: https://github.com/nodejs/node/issues/26293 Reviewed-By: Richard Lau Reviewed-By: David Carlier Reviewed-By: Gireesh Punathil Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- test/common/index.js | 7 ------- test/report/test-report-config.js | 1 - test/report/test-report-fatal-error.js | 1 - test/report/test-report-getreport.js | 1 - test/report/test-report-signal.js | 2 +- test/report/test-report-uncaught-exception.js | 1 - test/report/test-report-uv-handles.js | 1 - test/report/test-report-worker.js | 1 - test/report/test-report-writereport.js | 1 - 9 files changed, 1 insertion(+), 15 deletions(-) diff --git a/test/common/index.js b/test/common/index.js index 28ce841c48cc3f..e77e7b95059947 100644 --- a/test/common/index.js +++ b/test/common/index.js @@ -553,12 +553,6 @@ function skipIfInspectorDisabled() { } } -function skipIfReportDisabled() { - if (!process.config.variables.node_report) { - skip('Diagnostic reporting is disabled'); - } -} - function skipIf32Bits() { if (bits < 64) { skip('The tested feature is not available in 32bit builds'); @@ -700,7 +694,6 @@ const common = { skipIf32Bits, skipIfEslintMissing, skipIfInspectorDisabled, - skipIfReportDisabled, skipIfWorker, get enoughTestCpu() { diff --git a/test/report/test-report-config.js b/test/report/test-report-config.js index 4f6fdf2a80e111..49781ac9582adc 100644 --- a/test/report/test-report-config.js +++ b/test/report/test-report-config.js @@ -1,7 +1,6 @@ // Flags: --experimental-report --report-on-fatalerror --report-on-signal --report-uncaught-exception 'use strict'; const common = require('../common'); -common.skipIfReportDisabled(); const assert = require('assert'); common.expectWarning('ExperimentalWarning', diff --git a/test/report/test-report-fatal-error.js b/test/report/test-report-fatal-error.js index 8ecd058cf8a4de..60eeef6c454ba7 100644 --- a/test/report/test-report-fatal-error.js +++ b/test/report/test-report-fatal-error.js @@ -1,7 +1,6 @@ 'use strict'; const common = require('../common'); -common.skipIfReportDisabled(); const assert = require('assert'); // Testcase to produce report on fatal error (javascript heap OOM) if (process.argv[2] === 'child') { diff --git a/test/report/test-report-getreport.js b/test/report/test-report-getreport.js index ba645df69fdd7b..efadaa73f10cb9 100644 --- a/test/report/test-report-getreport.js +++ b/test/report/test-report-getreport.js @@ -1,7 +1,6 @@ // Flags: --experimental-report 'use strict'; const common = require('../common'); -common.skipIfReportDisabled(); const assert = require('assert'); const helper = require('../common/report'); diff --git a/test/report/test-report-signal.js b/test/report/test-report-signal.js index 51244fcade2196..c1c98d2588aba5 100644 --- a/test/report/test-report-signal.js +++ b/test/report/test-report-signal.js @@ -2,7 +2,7 @@ 'use strict'; // Test producing a report via signal. const common = require('../common'); -common.skipIfReportDisabled(); + if (common.isWindows) return common.skip('Unsupported on Windows.'); diff --git a/test/report/test-report-uncaught-exception.js b/test/report/test-report-uncaught-exception.js index 4a2627c13c88ff..d35b5f276f9f4e 100644 --- a/test/report/test-report-uncaught-exception.js +++ b/test/report/test-report-uncaught-exception.js @@ -2,7 +2,6 @@ 'use strict'; // Test producing a report on uncaught exception. const common = require('../common'); -common.skipIfReportDisabled(); const assert = require('assert'); const helper = require('../common/report'); const tmpdir = require('../common/tmpdir'); diff --git a/test/report/test-report-uv-handles.js b/test/report/test-report-uv-handles.js index ea0c189b859c1a..bf5e53d154b32d 100644 --- a/test/report/test-report-uv-handles.js +++ b/test/report/test-report-uv-handles.js @@ -5,7 +5,6 @@ const common = require('../common'); if (common.isIBMi) common.skip('IBMi does not support fs.watch()'); -common.skipIfReportDisabled(); if (process.argv[2] === 'child') { // Exit on loss of parent process const exit = () => process.exit(2); diff --git a/test/report/test-report-worker.js b/test/report/test-report-worker.js index 39a2c87f65f044..6e77deecf57137 100644 --- a/test/report/test-report-worker.js +++ b/test/report/test-report-worker.js @@ -1,7 +1,6 @@ // Flags: --experimental-report 'use strict'; const common = require('../common'); -common.skipIfReportDisabled(); const assert = require('assert'); const { Worker } = require('worker_threads'); const { once } = require('events'); diff --git a/test/report/test-report-writereport.js b/test/report/test-report-writereport.js index a72744fcd4fcb7..f04a19f3e01bc0 100644 --- a/test/report/test-report-writereport.js +++ b/test/report/test-report-writereport.js @@ -3,7 +3,6 @@ // Test producing a report via API call, using the no-hooks/no-signal interface. const common = require('../common'); -common.skipIfReportDisabled(); const assert = require('assert'); const { spawnSync } = require('child_process'); const fs = require('fs'); From 9d1a3b6f608d864f3e4b1e8dc667f733f5acdcb8 Mon Sep 17 00:00:00 2001 From: cjihrig Date: Thu, 12 Mar 2020 23:24:01 -0400 Subject: [PATCH 5/6] doc,lib,src,test: make --experimental-report a nop This commit makes the --experimental-report CLI flag a no-op. PR-URL: https://github.com/nodejs/node/pull/32242 Fixes: https://github.com/nodejs/node/issues/26293 Reviewed-By: Richard Lau Reviewed-By: David Carlier Reviewed-By: Gireesh Punathil Reviewed-By: James M Snell Reviewed-By: Matteo Collina --- doc/api/cli.md | 24 ++++------ doc/api/report.md | 11 ++--- doc/node.1 | 22 ++++----- lib/internal/bootstrap/pre_execution.js | 9 ---- src/node_options.cc | 46 +------------------ src/node_options.h | 3 +- test/addons/worker-addon/test.js | 1 - test/parallel/test-bootstrap-modules.js | 2 + ...rocess-env-allowed-flags-are-documented.js | 1 + test/report/test-report-config.js | 6 +-- test/report/test-report-fatal-error.js | 3 +- test/report/test-report-getreport.js | 7 +-- test/report/test-report-signal.js | 5 +- test/report/test-report-uncaught-exception.js | 5 +- test/report/test-report-uv-handles.js | 8 +--- test/report/test-report-worker.js | 1 - test/report/test-report-writereport.js | 15 ++---- 17 files changed, 35 insertions(+), 134 deletions(-) diff --git a/doc/api/cli.md b/doc/api/cli.md index 66cb665fa82e9f..9a023bbb488245 100644 --- a/doc/api/cli.md +++ b/doc/api/cli.md @@ -199,13 +199,6 @@ added: v10.0.0 Enable experimental top-level `await` keyword support in REPL. -### `--experimental-report` - - -Enable experimental diagnostic report feature. - ### `--experimental-specifier-resolution=mode` Enables report to be generated upon receiving the specified (or predefined) -signal to the running Node.js process, if `--experimental-report` is enabled. -The signal to trigger the report is specified through `--report-signal`. +signal to the running Node.js process. The signal to trigger the report is +specified through `--report-signal`. ### `--report-signal=signal` -Enables report to be generated on un-caught exceptions, if -`--experimental-report` is enabled. Useful when inspecting JavaScript stack in -conjunction with native stack and other runtime environment data. +Enables report to be generated on uncaught exceptions. Useful when inspecting +the JavaScript stack in conjunction with native stack and other runtime +environment data. ### `--throw-deprecation` +> Stability: 1 - Experimental + Enables the report to be triggered on fatal errors (internal errors within the Node.js runtime such as out of memory) that lead to termination of the application. Useful to inspect various diagnostic data elements such as heap, @@ -636,6 +644,9 @@ error. -> Stability: 1 - Experimental - * {Object} `process.report` is an object whose methods are used to generate diagnostic @@ -1796,10 +1798,12 @@ reports for the current process. Additional documentation is available in the ### `process.report.directory` -> Stability: 1 - Experimental - * {string} Directory where the report is written. The default value is the empty string, @@ -1813,10 +1817,12 @@ console.log(`Report directory is ${process.report.directory}`); ### `process.report.filename` -> Stability: 1 - Experimental - * {string} Filename where the report is written. If set to the empty string, the output @@ -1830,10 +1836,12 @@ console.log(`Report filename is ${process.report.filename}`); ### `process.report.getReport([err])` -> Stability: 1 - Experimental - * `err` {Error} A custom error used for reporting the JavaScript stack. * Returns: {Object} @@ -1871,10 +1879,12 @@ console.log(`Report on fatal error: ${process.report.reportOnFatalError}`); ### `process.report.reportOnSignal` -> Stability: 1 - Experimental - * {boolean} If `true`, a diagnostic report is generated when the process receives the @@ -1887,10 +1897,12 @@ console.log(`Report on signal: ${process.report.reportOnSignal}`); ### `process.report.reportOnUncaughtException` -> Stability: 1 - Experimental - * {boolean} If `true`, a diagnostic report is generated on uncaught exception. @@ -1902,10 +1914,12 @@ console.log(`Report on exception: ${process.report.reportOnUncaughtException}`); ### `process.report.signal` -> Stability: 1 - Experimental - * {string} The signal used to trigger the creation of a diagnostic report. Defaults to @@ -1918,10 +1932,12 @@ console.log(`Report signal: ${process.report.signal}`); ### `process.report.writeReport([filename][, err])` -> Stability: 1 - Experimental - * `filename` {string} Name of the file where the report is written. This should be a relative path, that will be appended to the directory specified in `process.report.directory`, or the current working directory of the Node.js diff --git a/doc/api/report.md b/doc/api/report.md index 976d470bc59311..7762424ec5f7b6 100644 --- a/doc/api/report.md +++ b/doc/api/report.md @@ -3,7 +3,7 @@ -> Stability: 1 - Experimental +> Stability: 2 - Stable