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 1fec85aa793d..290d36d2d205 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 37c640006500..592d0ca2a397 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 9d038e3d1683..ae1da87ef6b0 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 3bf1031f1667..35dd274d6fe6 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 470007f06b63..958eaf29571e 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 8a23ec2f1f39..c8e7387c2a8c 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 0190e31b41a2..e98801f2279f 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 f356f88fe9c7..01999cc46241 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 28ce841c48cc..e77e7b950599 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 4f6fdf2a80e1..49781ac9582a 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 8ecd058cf8a4..60eeef6c454b 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 ba645df69fdd..efadaa73f10c 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 51244fcade21..c1c98d2588ab 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 4a2627c13c88..d35b5f276f9f 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 ea0c189b859c..bf5e53d154b3 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 39a2c87f65f0..6e77deecf571 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 a72744fcd4fc..f04a19f3e01b 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 66cb665fa82e..9a023bbb4882 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 976d470bc593..7762424ec5f7 100644 --- a/doc/api/report.md +++ b/doc/api/report.md @@ -3,7 +3,7 @@ -> Stability: 1 - Experimental +> Stability: 2 - Stable