src: allow setting a dir for all diagnostic output

AshCripps · addaleax · commit 403deb71d5a2 · 2020-09-22T23:47:59.000+02:00
Add a flag that allows for the setting of a directory where all diagnostic output will be written to. e.g. --redirect-warnings Refs: #33010 (comment) PR-URL: #33584 Reviewed-By: James M Snell <jasnell@gmail.com> Reviewed-By: Richard Lau <riclau@uk.ibm.com> Reviewed-By: Beth Griggs <Bethany.Griggs@uk.ibm.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>
diff --git a/doc/api/cli.md b/doc/api/cli.md
@@ -108,6 +108,9 @@ added: v12.0.0
 Specify the directory where the CPU profiles generated by `--cpu-prof` will
 be placed.
 
+The default value is controlled by the
+[--diagnostic-dir](#cli_diagnostic_dir_directory) command line option.
+
 ### `--cpu-prof-interval`
 <!-- YAML
 added: v12.2.0
@@ -127,6 +130,16 @@ added: v12.0.0
 
 Specify the file name of the CPU profile generated by `--cpu-prof`.
 
+### `--diagnostic-dir=directory`
+
+Set the directory to which all diagnostic output files will be written to.
+Defaults to current working directory.
+
+Affects the default output directory of:
+* [--cpu-prof-dir](#cli_cpu_prof_dir)
+* [--heap-prof-dir](#cli_heap_prof_dir)
+* [--redirect-warnings](#cli_redirect_warnings_file)
+
 ### `--disable-proto=mode`
 <!--YAML
 added: v12.17.0
@@ -325,6 +338,9 @@ added: v12.4.0
 Specify the directory where the heap profiles generated by `--heap-prof` will
 be placed.
 
+The default value is controlled by the
+[--diagnostic-dir](#cli_diagnostic_dir_directory) command line option.
+
 ### `--heap-prof-interval`
 <!-- YAML
 added: v12.4.0
@@ -623,6 +639,10 @@ file will be created if it does not exist, and will be appended to if it does.
 If an error occurs while attempting to write the warning to the file, the
 warning will be written to stderr instead.
 
+The `file` name may be an absolute path. If it is not, the default directory it
+will be written to is controlled by the
+[--diagnostic-dir](#cli_diagnostic_dir_directory) command line option.
+
 ### `--report-compact`
 <!-- YAML
 added: v12.17.0
@@ -1141,6 +1161,7 @@ node --require "./a.js" --require "./b.js"
 
 Node.js options that are allowed are:
 <!-- node-options-node start -->
+* `--diagnostic-dir`
 * `--disable-proto`
 * `--enable-fips`
 * `--enable-source-maps`
diff --git a/doc/node.1 b/doc/node.1
@@ -89,6 +89,9 @@ with a generated file name.
 The directory where the CPU profiles generated by
 .Fl -cpu-prof
 will be placed.
+The default value is controlled by the
+.Fl -diagnostic-dir .
+command line option.
 .
 .It Fl -cpu-prof-interval
 The sampling interval in microseconds for the CPU profiles generated by
@@ -100,6 +103,17 @@ The default is
 File name of the V8 CPU profile generated with
 .Fl -cpu-prof
 .
+.It Fl -diagnostic-dir
+Set the directory for all diagnostic output files.
+Default is current working directory.
+Set the directory to which all diagnostic output files will be written to.
+Defaults to current working directory.
+.
+Affects the default output directory of:
+.Fl -cpu-prof-dir .
+.Fl -heap-prof-dir .
+.Fl -redirect-warnings .
+.
 .It Fl -disable-proto Ns = Ns Ar mode
 Disable the `Object.prototype.__proto__` property. If
 .Ar mode
@@ -178,6 +192,9 @@ with a generated file name.
 The directory where the heap profiles generated by
 .Fl -heap-prof
 will be placed.
+The default value is controlled by the
+.Fl -diagnostic-dir .
+command line option.
 .
 .It Fl -heap-prof-interval
 The average sampling interval in bytes for the heap profiles generated by
@@ -304,6 +321,10 @@ in a compact format, single-line JSON.
 Location at which the
 .Sy diagnostic report
 will be generated.
+The `file` name may be an absolute path. If it is not, the default directory it will
+be written to is controlled by the
+.Fl -diagnostic-dir .
+command line option.
 .
 .It Fl -report-filename
 Name of the file to which the
diff --git a/lib/internal/process/warning.js b/lib/internal/process/warning.js
@@ -11,13 +11,21 @@ const { ERR_INVALID_ARG_TYPE } = require('internal/errors').codes;
 let fs;
 let fd;
 let warningFile;
+let options;
 
 function lazyOption() {
   // This will load `warningFile` only once. If the flag is not set,
   // `warningFile` will be set to an empty string.
   if (warningFile === undefined) {
-    warningFile = require('internal/options')
-                  .getOptionValue('--redirect-warnings');
+    options = require('internal/options');
+    if (options.getOptionValue('--diagnostic-dir') !== '') {
+      warningFile = options.getOptionValue('--diagnostic-dir');
+    }
+    if (options.getOptionValue('--redirect-warnings') !== '') {
+      warningFile = options.getOptionValue('--redirect-warnings');
+    } else {
+      warningFile = '';
+    }
   }
   return warningFile;
 }
diff --git a/src/node_options.cc b/src/node_options.cc
@@ -148,6 +148,10 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
     }
   }
 
+  if (cpu_prof && cpu_prof_dir.empty() && !diagnostic_dir.empty()) {
+      cpu_prof_dir = diagnostic_dir;
+    }
+
   if (!heap_prof) {
     if (!heap_prof_name.empty()) {
       errors->push_back("--heap-prof-name must be used with --heap-prof");
@@ -161,6 +165,11 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {
       errors->push_back("--heap-prof-interval must be used with --heap-prof");
     }
   }
+
+  if (heap_prof && heap_prof_dir.empty() && !diagnostic_dir.empty()) {
+    heap_prof_dir = diagnostic_dir;
+  }
+
   debug_options_.CheckOptions(errors);
 #endif  // HAVE_INSPECTOR
 }
@@ -274,6 +283,11 @@ DebugOptionsParser::DebugOptionsParser() {
 }
 
 EnvironmentOptionsParser::EnvironmentOptionsParser() {
+  AddOption("--diagnostic-dir",
+            "set dir for all output files"
+            " (default: current working directory)",
+            &EnvironmentOptions::diagnostic_dir,
+            kAllowedInEnvironment);
   AddOption("--enable-source-maps",
             "experimental Source Map V3 support",
             &EnvironmentOptions::enable_source_maps,
diff --git a/src/node_options.h b/src/node_options.h
@@ -139,6 +139,7 @@ class EnvironmentOptions : public Options {
   bool heap_prof = false;
 #endif  // HAVE_INSPECTOR
   std::string redirect_warnings;
+  std::string diagnostic_dir;
   bool test_udp_no_try_send = false;
   bool throw_deprecation = false;
   bool trace_deprecation = false;
diff --git a/test/sequential/test-diagnostic-dir-cpu-prof.js b/test/sequential/test-diagnostic-dir-cpu-prof.js
@@ -0,0 +1,76 @@
+'use strict';
+
+// This test is to ensure that --diagnostic-dir does not change the directory
+// for --cpu-prof when --cpu-prof-dir is specified
+
+const common = require('../common');
+const fixtures = require('../common/fixtures');
+common.skipIfInspectorDisabled();
+
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const { spawnSync } = require('child_process');
+
+const tmpdir = require('../common/tmpdir');
+const {
+  getCpuProfiles,
+  kCpuProfInterval,
+  env,
+  verifyFrames
+} = require('../common/cpu-prof');
+
+// Test --diagnostic-dir changes the default for --cpu-prof
+
+{
+  tmpdir.refresh();
+  const dir = path.join(tmpdir.path, 'prof');
+  const output = spawnSync(process.execPath, [
+    '--cpu-prof',
+    '--cpu-prof-interval',
+    kCpuProfInterval,
+    '--diagnostic-dir',
+    dir,
+    fixtures.path('workload', 'fibonacci.js'),
+  ], {
+    cwd: tmpdir.path,
+    env
+  });
+  if (output.status !== 0) {
+    console.log(output.stderr.toString());
+  }
+  assert.strictEqual(output.status, 0);
+  assert(fs.existsSync(dir));
+  const profiles = getCpuProfiles(dir);
+  assert.strictEqual(profiles.length, 1);
+  verifyFrames(output, profiles[0], 'fibonacci.js');
+}
+
+// Test --cpu-prof-dir overwrites --diagnostic-dir
+
+{
+  tmpdir.refresh();
+  const dir = path.join(tmpdir.path, 'diag');
+  const dir2 = path.join(tmpdir.path, 'prof');
+  const output = spawnSync(process.execPath, [
+    '--cpu-prof',
+    '--cpu-prof-interval',
+    kCpuProfInterval,
+    '--diagnostic-dir',
+    dir,
+    '--cpu-prof-dir',
+    dir2,
+    fixtures.path('workload', 'fibonacci.js'),
+  ], {
+    cwd: tmpdir.path,
+    env
+  });
+  if (output.status !== 0) {
+    console.log(output.stderr.toString());
+  }
+  assert.strictEqual(output.status, 0);
+  assert(fs.existsSync(dir2));
+  const profiles = getCpuProfiles(dir2);
+  assert.strictEqual(profiles.length, 1);
+  verifyFrames(output, profiles[0], 'fibonacci.js');
+}
diff --git a/test/sequential/test-diagnostic-dir-heap-prof.js b/test/sequential/test-diagnostic-dir-heap-prof.js
@@ -0,0 +1,117 @@
+'use strict';
+
+// This test is to ensure that --diagnostic-dir does not change the directory
+// for --cpu-prof when --cpu-prof-dir is specified
+
+const common = require('../common');
+const fixtures = require('../common/fixtures');
+common.skipIfInspectorDisabled();
+
+const assert = require('assert');
+const fs = require('fs');
+const path = require('path');
+const { spawnSync } = require('child_process');
+
+const tmpdir = require('../common/tmpdir');
+
+function findFirstFrameInNode(root, func) {
+  const first = root.children.find(
+    (child) => child.callFrame.functionName === func
+  );
+  if (first) {
+    return first;
+  }
+  for (const child of root.children) {
+    const first = findFirstFrameInNode(child, func);
+    if (first) {
+      return first;
+    }
+  }
+  return undefined;
+}
+
+function findFirstFrame(file, func) {
+  const data = fs.readFileSync(file, 'utf8');
+  const profile = JSON.parse(data);
+  const first = findFirstFrameInNode(profile.head, func);
+  return { frame: first, roots: profile.head.children };
+}
+
+function verifyFrames(output, file, func) {
+  const { frame, roots } = findFirstFrame(file, func);
+  if (!frame) {
+    // Show native debug output and the profile for debugging.
+    console.log(output.stderr.toString());
+    console.log(roots);
+  }
+  assert.notDeepStrictEqual(frame, undefined);
+}
+
+const kHeapProfInterval = 128;
+const TEST_ALLOCATION = kHeapProfInterval * 2;
+
+const env = {
+  ...process.env,
+  TEST_ALLOCATION,
+  NODE_DEBUG_NATIVE: 'INSPECTOR_PROFILER'
+};
+
+function getHeapProfiles(dir) {
+  const list = fs.readdirSync(dir);
+  return list
+    .filter((file) => file.endsWith('.heapprofile'))
+    .map((file) => path.join(dir, file));
+}
+
+// Test --diagnostic-dir changes the default for --cpu-prof
+{
+  tmpdir.refresh();
+  const dir = path.join(tmpdir.path, 'prof');
+  const output = spawnSync(process.execPath, [
+    '--heap-prof',
+    '--diagnostic-dir',
+    dir,
+    '--heap-prof-interval',
+    kHeapProfInterval,
+    fixtures.path('workload', 'allocation.js'),
+  ], {
+    cwd: tmpdir.path,
+    env
+  });
+  if (output.status !== 0) {
+    console.log(output.stderr.toString());
+  }
+  assert.strictEqual(output.status, 0);
+  assert(fs.existsSync(dir));
+  const profiles = getHeapProfiles(dir);
+  assert.strictEqual(profiles.length, 1);
+  verifyFrames(output, profiles[0], 'runAllocation');
+}
+
+// Test --heap-prof-dir overwrites --diagnostic-dir
+{
+  tmpdir.refresh();
+  const dir = path.join(tmpdir.path, 'diag');
+  const dir2 = path.join(tmpdir.path, 'prof');
+  const output = spawnSync(process.execPath, [
+    '--heap-prof',
+    '--heap-prof-interval',
+    kHeapProfInterval,
+    '--diagnostic-dir',
+    dir,
+    '--heap-prof-dir',
+    dir2,
+    fixtures.path('workload', 'allocation.js'),
+  ], {
+    cwd: tmpdir.path,
+    env
+  });
+  if (output.status !== 0) {
+    console.log(output.stderr.toString());
+  }
+  assert.strictEqual(output.status, 0);
+  assert(fs.existsSync(dir2));
+  const profiles = getHeapProfiles(dir2);
+  assert.strictEqual(profiles.length, 1);
+  verifyFrames(output, profiles[0], 'runAllocation');
+}

Original file line number	Diff line number	Diff line change
`@@ -148,6 +148,10 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {`
`148`	`148`	`}`
`149`	`149`	`}`
`150`	`150`
	`151`	`+ if (cpu_prof && cpu_prof_dir.empty() && !diagnostic_dir.empty()) {`
	`152`	`+ cpu_prof_dir = diagnostic_dir;`
	`153`	`+ }`
	`154`	`+`
`151`	`155`	`if (!heap_prof) {`
`152`	`156`	`if (!heap_prof_name.empty()) {`
`153`	`157`	`errors->push_back("--heap-prof-name must be used with --heap-prof");`
`@@ -161,6 +165,11 @@ void EnvironmentOptions::CheckOptions(std::vector<std::string>* errors) {`
`161`	`165`	`errors->push_back("--heap-prof-interval must be used with --heap-prof");`
`162`	`166`	`}`
`163`	`167`	`}`
	`168`	`+`
	`169`	`+ if (heap_prof && heap_prof_dir.empty() && !diagnostic_dir.empty()) {`
	`170`	`+ heap_prof_dir = diagnostic_dir;`
	`171`	`+ }`
	`172`	`+`
`164`	`173`	`debug_options_.CheckOptions(errors);`
`165`	`174`	`#endif // HAVE_INSPECTOR`
`166`	`175`	`}`
`@@ -274,6 +283,11 @@ DebugOptionsParser::DebugOptionsParser() {`
`274`	`283`	`}`
`275`	`284`
`276`	`285`	`EnvironmentOptionsParser::EnvironmentOptionsParser() {`
	`286`	`+ AddOption("--diagnostic-dir",`
	`287`	`+ "set dir for all output files"`
	`288`	`+ " (default: current working directory)",`
	`289`	`+ &EnvironmentOptions::diagnostic_dir,`
	`290`	`+ kAllowedInEnvironment);`
`277`	`291`	`AddOption("--enable-source-maps",`
`278`	`292`	`"experimental Source Map V3 support",`
`279`	`293`	`&EnvironmentOptions::enable_source_maps,`