From 6db1a08ae972ab398f10f83bc696948e2ec9c3a5 Mon Sep 17 00:00:00 2001 From: bcoe Date: Sun, 1 Dec 2019 12:55:54 -0800 Subject: [PATCH 1/3] build: remove (almost) unused macros/constants Macros, like CHECK, cause issues for tracking coverage because they modify the source before it's placed in V8. Upon investigation it seemed that we only used this functionality in two places: internal/vm/module.js, and internal/async_hooks.js (in comments). Given this, it seemed to make more sense to move CHECK to JavaScript, and retire a mostly unused build step. --- lib/.eslintrc.yaml | 15 --- lib/internal/per_context/primordials.js | 9 ++ lib/internal/vm/module.js | 1 + node.gyp | 14 +-- tools/js2c.py | 144 +----------------------- tools/js2c_macros/check_macros.py | 8 -- tools/js2c_macros/dcheck_macros.py | 9 -- tools/js2c_macros/nodcheck_macros.py | 9 -- tools/js2c_macros/notrace_macros.py | 12 -- 9 files changed, 15 insertions(+), 206 deletions(-) delete mode 100644 tools/js2c_macros/check_macros.py delete mode 100644 tools/js2c_macros/dcheck_macros.py delete mode 100644 tools/js2c_macros/nodcheck_macros.py delete mode 100644 tools/js2c_macros/notrace_macros.py diff --git a/lib/.eslintrc.yaml b/lib/.eslintrc.yaml index 2823b7aa0d1def..ff9a0f595d2225 100644 --- a/lib/.eslintrc.yaml +++ b/lib/.eslintrc.yaml @@ -46,21 +46,6 @@ rules: node-core/non-ascii-character: error globals: Intl: false - # Assertions - CHECK: false - CHECK_EQ: false - CHECK_GE: false - CHECK_GT: false - CHECK_LE: false - CHECK_LT: false - CHECK_NE: false - DCHECK: false - DCHECK_EQ: false - DCHECK_GE: false - DCHECK_GT: false - DCHECK_LE: false - DCHECK_LT: false - DCHECK_NE: false # Parameters passed to internal modules global: false require: false diff --git a/lib/internal/per_context/primordials.js b/lib/internal/per_context/primordials.js index 9e17b3541c78af..38cf45c839c93a 100644 --- a/lib/internal/per_context/primordials.js +++ b/lib/internal/per_context/primordials.js @@ -124,5 +124,14 @@ primordials.SafePromise = makeSafe( copyPrototype(original.prototype, primordials, `${name}Prototype`); }); +// Helper caried over from js2c_macros: exits process and prints debug message +// iff node_debug_lib flag is set. +primordials.CHECK = function(x) { + if (process.config.variables.node_debug_lib === false) return; + if (!x) { + (process._rawDebug('CHECK: x == true'), process.abort()); + } +}; + Object.setPrototypeOf(primordials, null); Object.freeze(primordials); diff --git a/lib/internal/vm/module.js b/lib/internal/vm/module.js index 88a276e217a7fd..c48d739af9a638 100644 --- a/lib/internal/vm/module.js +++ b/lib/internal/vm/module.js @@ -2,6 +2,7 @@ const { ArrayIsArray, + CHECK, ObjectCreate, ObjectDefineProperty, Symbol, diff --git a/node.gyp b/node.gyp index 8becce9062edef..0a6bbd61009e84 100644 --- a/node.gyp +++ b/node.gyp @@ -892,23 +892,11 @@ # Put the code first so it's a dependency and can be used for invocation. 'tools/js2c.py', '<@(library_files)', - 'config.gypi', - 'tools/js2c_macros/check_macros.py' + 'config.gypi' ], 'outputs': [ '<(SHARED_INTERMEDIATE_DIR)/node_javascript.cc', ], - 'conditions': [ - [ 'node_use_dtrace=="false" and node_use_etw=="false"', { - 'inputs': [ 'tools/js2c_macros/notrace_macros.py' ] - }], - [ 'node_debug_lib=="false"', { - 'inputs': [ 'tools/js2c_macros/nodcheck_macros.py' ] - }], - [ 'node_debug_lib=="true"', { - 'inputs': [ 'tools/js2c_macros/dcheck_macros.py' ] - }] - ], 'action': [ 'python', '<@(_inputs)', '--target', '<@(_outputs)', diff --git a/tools/js2c.py b/tools/js2c.py index 1346b2a87046d3..4594694a2cab0d 100755 --- a/tools/js2c.py +++ b/tools/js2c.py @@ -45,137 +45,6 @@ def ReadFile(filename): return lines -def ReadMacroFiles(filenames): - """ - - :rtype: List(str) - """ - result = [] - for filename in filenames: - with open(filename, "rt") as f: - # strip python-like comments and whitespace padding - lines = [line.split('#')[0].strip() for line in f] - # filter empty lines - result.extend(filter(bool, lines)) - return result - - -def ExpandConstants(lines, constants): - for key, value in constants.items(): - lines = lines.replace(key, str(value)) - return lines - - -def ExpandMacros(lines, macros): - def expander(s): - return ExpandMacros(s, macros) - - for name, macro in macros.items(): - name_pattern = re.compile("\\b%s\\(" % name) - pattern_match = name_pattern.search(lines, 0) - while pattern_match is not None: - # Scan over the arguments - height = 1 - start = pattern_match.start() - end = pattern_match.end() - assert lines[end - 1] == '(' - last_match = end - arg_index = [0] # Wrap state into array, to work around Python "scoping" - mapping = {} - - def add_arg(s): - # Remember to expand recursively in the arguments - if arg_index[0] >= len(macro.args): - return - replacement = expander(s.strip()) - mapping[macro.args[arg_index[0]]] = replacement - arg_index[0] += 1 - - while end < len(lines) and height > 0: - # We don't count commas at higher nesting levels. - if lines[end] == ',' and height == 1: - add_arg(lines[last_match:end]) - last_match = end + 1 - elif lines[end] in ['(', '{', '[']: - height = height + 1 - elif lines[end] in [')', '}', ']']: - height = height - 1 - end = end + 1 - # Remember to add the last match. - add_arg(lines[last_match:end - 1]) - if arg_index[0] < len(macro.args) - 1: - lineno = lines.count(os.linesep, 0, start) + 1 - raise Exception( - 'line %s: Too few arguments for macro "%s"' % (lineno, name)) - result = macro.expand(mapping) - # Replace the occurrence of the macro with the expansion - lines = lines[:start] + result + lines[end:] - pattern_match = name_pattern.search(lines, start + len(result)) - return lines - - -class TextMacro: - def __init__(self, args, body): - self.args = args - self.body = body - - def expand(self, mapping): - result = self.body - for key, value in mapping.items(): - result = result.replace(key, value) - return result - - -class PythonMacro: - def __init__(self, args, fun): - self.args = args - self.fun = fun - - def expand(self, mapping): - args = [] - for arg in self.args: - args.append(mapping[arg]) - return str(self.fun(*args)) - - -CONST_PATTERN = re.compile('^const\s+([a-zA-Z0-9_]+)\s*=\s*([^;]*);$') -MACRO_PATTERN = re.compile('^macro\s+([a-zA-Z0-9_]+)\s*\(([^)]*)\)\s*=\s*([^;]*);$') -PYTHON_MACRO_PATTERN = re.compile('^python\s+macro\s+([a-zA-Z0-9_]+)\s*\(([^)]*)\)\s*=\s*([^;]*);$') - - -def ReadMacros(macro_files): - lines = ReadMacroFiles(macro_files) - constants = {} - macros = {} - for line in lines: - line = line.split('#')[0].strip() - if len(line) == 0: - continue - const_match = CONST_PATTERN.match(line) - if const_match: - name = const_match.group(1) - value = const_match.group(2).strip() - constants[name] = value - else: - macro_match = MACRO_PATTERN.match(line) - if macro_match: - name = macro_match.group(1) - args = [p.strip() for p in macro_match.group(2).split(',')] - body = macro_match.group(3).strip() - macros[name] = TextMacro(args, body) - else: - python_match = PYTHON_MACRO_PATTERN.match(line) - if python_match: - name = python_match.group(1) - args = [p.strip() for p in macro_match.group(2).split(',')] - body = python_match.group(3).strip() - fun = eval("lambda " + ",".join(args) + ': ' + body) - macros[name] = PythonMacro(args, fun) - else: - raise Exception("Illegal line: " + line) - return constants, macros - - TEMPLATE = """ #include "env-inl.h" #include "node_native_module.h" @@ -243,10 +112,8 @@ def GetDefinition(var, source, step=30): return definition, len(code_points) -def AddModule(filename, consts, macros, definitions, initializers): +def AddModule(filename, definitions, initializers): code = ReadFile(filename) - code = ExpandConstants(code, consts) - code = ExpandMacros(code, macros) name = NormalizeFileName(filename) slug = SLUGGER_RE.sub('_', name) var = slug + '_raw' @@ -267,15 +134,12 @@ def NormalizeFileName(filename): def JS2C(source_files, target): - # Process input from all *macro.py files - consts, macros = ReadMacros(source_files['.py']) - # Build source code lines definitions = [] initializers = [] for filename in source_files['.js']: - AddModule(filename, consts, macros, definitions, initializers) + AddModule(filename, definitions, initializers) config_def, config_size = handle_config_gypi(source_files['config.gypi']) definitions.append(config_def) @@ -341,8 +205,8 @@ def main(): global is_verbose is_verbose = options.verbose source_files = functools.reduce(SourceFileByExt, options.sources, {}) - # Should have exactly 3 types: `.js`, `.py`, and `.gypi` - assert len(source_files) == 3 + # Should have exactly 2 types: `.js`, and `.gypi` + assert len(source_files) == 2 # Currently config.gypi is the only `.gypi` file allowed assert source_files['.gypi'] == ['config.gypi'] source_files['config.gypi'] = source_files.pop('.gypi')[0] diff --git a/tools/js2c_macros/check_macros.py b/tools/js2c_macros/check_macros.py deleted file mode 100644 index f24d47c9ee40bf..00000000000000 --- a/tools/js2c_macros/check_macros.py +++ /dev/null @@ -1,8 +0,0 @@ -# flake8: noqa -macro CHECK(x) = do { if (!(x)) (process._rawDebug("CHECK: x == true"), process.abort()) } while (0); -macro CHECK_EQ(a, b) = CHECK((a) === (b)); -macro CHECK_GE(a, b) = CHECK((a) >= (b)); -macro CHECK_GT(a, b) = CHECK((a) > (b)); -macro CHECK_LE(a, b) = CHECK((a) <= (b)); -macro CHECK_LT(a, b) = CHECK((a) < (b)); -macro CHECK_NE(a, b) = CHECK((a) !== (b)); diff --git a/tools/js2c_macros/dcheck_macros.py b/tools/js2c_macros/dcheck_macros.py deleted file mode 100644 index f22c08598fd694..00000000000000 --- a/tools/js2c_macros/dcheck_macros.py +++ /dev/null @@ -1,9 +0,0 @@ -# flake8: noqa - -macro DCHECK(x) = do { if (!(x)) (process._rawDebug("DCHECK: x == true"), process.abort()) } while (0); -macro DCHECK_EQ(a, b) = DCHECK((a) === (b)); -macro DCHECK_GE(a, b) = DCHECK((a) >= (b)); -macro DCHECK_GT(a, b) = DCHECK((a) > (b)); -macro DCHECK_LE(a, b) = DCHECK((a) <= (b)); -macro DCHECK_LT(a, b) = DCHECK((a) < (b)); -macro DCHECK_NE(a, b) = DCHECK((a) !== (b)); diff --git a/tools/js2c_macros/nodcheck_macros.py b/tools/js2c_macros/nodcheck_macros.py deleted file mode 100644 index 0a59001b549fe5..00000000000000 --- a/tools/js2c_macros/nodcheck_macros.py +++ /dev/null @@ -1,9 +0,0 @@ -# flake8: noqa - -macro DCHECK(x) = void(x); -macro DCHECK_EQ(a, b) = void(a, b); -macro DCHECK_GE(a, b) = void(a, b); -macro DCHECK_GT(a, b) = void(a, b); -macro DCHECK_LE(a, b) = void(a, b); -macro DCHECK_LT(a, b) = void(a, b); -macro DCHECK_NE(a, b) = void(a, b); diff --git a/tools/js2c_macros/notrace_macros.py b/tools/js2c_macros/notrace_macros.py deleted file mode 100644 index 334f9c0c7f7a87..00000000000000 --- a/tools/js2c_macros/notrace_macros.py +++ /dev/null @@ -1,12 +0,0 @@ -# This file is used by tools/js2c.py to preprocess out the DTRACE symbols in -# builds that don't support DTrace. This is not used in builds that support -# DTrace. - -# flake8: noqa - -macro DTRACE_HTTP_CLIENT_REQUEST(x) = ; -macro DTRACE_HTTP_CLIENT_RESPONSE(x) = ; -macro DTRACE_HTTP_SERVER_REQUEST(x) = ; -macro DTRACE_HTTP_SERVER_RESPONSE(x) = ; -macro DTRACE_NET_SERVER_CONNECTION(x) = ; -macro DTRACE_NET_STREAM_END(x) = ; From 3765e06a276294ceabb35a4bf21f8f6a3d2bb3ea Mon Sep 17 00:00:00 2001 From: bcoe Date: Sun, 1 Dec 2019 21:19:26 -0800 Subject: [PATCH 2/3] chore: address code review --- lib/internal/per_context/primordials.js | 9 --------- lib/internal/vm/module.js | 10 +++++++++- 2 files changed, 9 insertions(+), 10 deletions(-) diff --git a/lib/internal/per_context/primordials.js b/lib/internal/per_context/primordials.js index 38cf45c839c93a..9e17b3541c78af 100644 --- a/lib/internal/per_context/primordials.js +++ b/lib/internal/per_context/primordials.js @@ -124,14 +124,5 @@ primordials.SafePromise = makeSafe( copyPrototype(original.prototype, primordials, `${name}Prototype`); }); -// Helper caried over from js2c_macros: exits process and prints debug message -// iff node_debug_lib flag is set. -primordials.CHECK = function(x) { - if (process.config.variables.node_debug_lib === false) return; - if (!x) { - (process._rawDebug('CHECK: x == true'), process.abort()); - } -}; - Object.setPrototypeOf(primordials, null); Object.freeze(primordials); diff --git a/lib/internal/vm/module.js b/lib/internal/vm/module.js index c48d739af9a638..cc42330cf24bfc 100644 --- a/lib/internal/vm/module.js +++ b/lib/internal/vm/module.js @@ -2,7 +2,6 @@ const { ArrayIsArray, - CHECK, ObjectCreate, ObjectDefineProperty, Symbol, @@ -59,6 +58,15 @@ const kContext = Symbol('kContext'); const kPerContextModuleId = Symbol('kPerContextModuleId'); const kLink = Symbol('kLink'); +// Helper caried over from js2c_macros: exits process and prints debug message +// iff node_debug_lib flag is set. +function CHECK(x) { + if (process.config.variables.node_debug_lib === false) return; + if (!x) { + (process._rawDebug('CHECK: x == true'), process.abort()); + } +} + class Module { constructor(options) { emitExperimentalWarning('VM Modules'); From 2530062b7af868e48f57a83b927a0305d600452e Mon Sep 17 00:00:00 2001 From: bcoe Date: Wed, 4 Dec 2019 22:55:49 -0800 Subject: [PATCH 3/3] chore: switch to assert --- lib/internal/vm/module.js | 15 ++++++--------- 1 file changed, 6 insertions(+), 9 deletions(-) diff --git a/lib/internal/vm/module.js b/lib/internal/vm/module.js index cc42330cf24bfc..ee2fec2e431380 100644 --- a/lib/internal/vm/module.js +++ b/lib/internal/vm/module.js @@ -1,5 +1,6 @@ 'use strict'; +const { fail } = require('internal/assert'); const { ArrayIsArray, ObjectCreate, @@ -58,13 +59,9 @@ const kContext = Symbol('kContext'); const kPerContextModuleId = Symbol('kPerContextModuleId'); const kLink = Symbol('kLink'); -// Helper caried over from js2c_macros: exits process and prints debug message -// iff node_debug_lib flag is set. -function CHECK(x) { - if (process.config.variables.node_debug_lib === false) return; - if (!x) { - (process._rawDebug('CHECK: x == true'), process.abort()); - } +function failIfDebug() { + if (process.features.debug === false) return; + fail('VM Modules'); } class Module { @@ -127,7 +124,7 @@ class Module { syntheticExportNames, syntheticEvaluationSteps); } else { - CHECK(false); + failIfDebug(); } wrapToModuleMap.set(this[kWrap], this); @@ -383,7 +380,7 @@ class SyntheticModule extends Module { identifier, }); - this[kLink] = () => this[kWrap].link(() => { CHECK(false); }); + this[kLink] = () => this[kWrap].link(() => { failIfDebug(); }); } setExport(name, value) {