From 64a0b118d67674d90eb72430c9c45fbb3cb13fa4 Mon Sep 17 00:00:00 2001 From: Refael Ackermann Date: Sat, 23 Feb 2019 17:21:56 -0500 Subject: [PATCH] src: refactor node options parsers to mitigate MSVC bug PR-URL: https://github.com/nodejs/node/pull/26280 Fixes: https://github.com/nodejs/node/issues/25593 Reviewed-By: Joyee Cheung --- src/node.cc | 2 +- src/node_options.cc | 99 +++++++++++++++++++++++++++++++-------------- src/node_options.h | 45 ++++++--------------- src/node_worker.cc | 2 +- 4 files changed, 83 insertions(+), 65 deletions(-) diff --git a/src/node.cc b/src/node.cc index e46232405f4ca4..cbef51960a4ebe 100644 --- a/src/node.cc +++ b/src/node.cc @@ -546,7 +546,7 @@ int ProcessGlobalArgs(std::vector* args, std::vector v8_args; Mutex::ScopedLock lock(per_process::cli_options_mutex); - options_parser::PerProcessOptionsParser::instance.Parse( + options_parser::Parse( args, exec_args, &v8_args, diff --git a/src/node_options.cc b/src/node_options.cc index e89282f8d2d3da..206c05797ed86f 100644 --- a/src/node_options.cc +++ b/src/node_options.cc @@ -102,19 +102,60 @@ void EnvironmentOptions::CheckOptions(std::vector* errors) { namespace options_parser { -// Explicitly access the singelton instances in their dependancy order. -// This was moved here to workaround a compiler bug. -// Refs: https://github.com/nodejs/node/issues/25593 +class DebugOptionsParser : public OptionsParser { + public: + DebugOptionsParser(); +}; + +class EnvironmentOptionsParser : public OptionsParser { + public: + EnvironmentOptionsParser(); + explicit EnvironmentOptionsParser(const DebugOptionsParser& dop) + : EnvironmentOptionsParser() { + Insert(&dop, &EnvironmentOptions::get_debug_options); + } +}; -#if HAVE_INSPECTOR -const DebugOptionsParser DebugOptionsParser::instance; -#endif // HAVE_INSPECTOR +class PerIsolateOptionsParser : public OptionsParser { + public: + PerIsolateOptionsParser() = delete; + explicit PerIsolateOptionsParser(const EnvironmentOptionsParser& eop); +}; -const EnvironmentOptionsParser EnvironmentOptionsParser::instance; +class PerProcessOptionsParser : public OptionsParser { + public: + PerProcessOptionsParser() = delete; + explicit PerProcessOptionsParser(const PerIsolateOptionsParser& iop); +}; -const PerIsolateOptionsParser PerIsolateOptionsParser::instance; +#if HAVE_INSPECTOR +const DebugOptionsParser _dop_instance{}; +const EnvironmentOptionsParser _eop_instance{_dop_instance}; +#else +const EnvironmentOptionsParser _eop_instance{}; +#endif // HAVE_INSPECTOR +const PerIsolateOptionsParser _piop_instance{_eop_instance}; +const PerProcessOptionsParser _ppop_instance{_piop_instance}; + +template <> +void Parse( + StringVector* const args, StringVector* const exec_args, + StringVector* const v8_args, + PerIsolateOptions* const options, + OptionEnvvarSettings required_env_settings, StringVector* const errors) { + _piop_instance.Parse( + args, exec_args, v8_args, options, required_env_settings, errors); +} -const PerProcessOptionsParser PerProcessOptionsParser::instance; +template <> +void Parse( + StringVector* const args, StringVector* const exec_args, + StringVector* const v8_args, + PerProcessOptions* const options, + OptionEnvvarSettings required_env_settings, StringVector* const errors) { + _ppop_instance.Parse( + args, exec_args, v8_args, options, required_env_settings, errors); +} // XXX: If you add an option here, please also add it to doc/node.1 and // doc/api/cli.md @@ -273,14 +314,10 @@ EnvironmentOptionsParser::EnvironmentOptionsParser() { AddAlias("-i", "--interactive"); AddOption("--napi-modules", "", NoOp{}, kAllowedInEnvironment); - -#if HAVE_INSPECTOR - Insert(&DebugOptionsParser::instance, - &EnvironmentOptions::get_debug_options); -#endif // HAVE_INSPECTOR } -PerIsolateOptionsParser::PerIsolateOptionsParser() { +PerIsolateOptionsParser::PerIsolateOptionsParser( + const EnvironmentOptionsParser& eop) { AddOption("--track-heap-objects", "track heap object allocations for heap snapshots", &PerIsolateOptions::track_heap_objects, @@ -336,11 +373,11 @@ PerIsolateOptionsParser::PerIsolateOptionsParser() { kAllowedInEnvironment); #endif // NODE_REPORT - Insert(&EnvironmentOptionsParser::instance, - &PerIsolateOptions::get_per_env_options); + Insert(&eop, &PerIsolateOptions::get_per_env_options); } -PerProcessOptionsParser::PerProcessOptionsParser() { +PerProcessOptionsParser::PerProcessOptionsParser( + const PerIsolateOptionsParser& iop) { AddOption("--title", "the process title to use on startup", &PerProcessOptions::title, @@ -446,8 +483,7 @@ PerProcessOptionsParser::PerProcessOptionsParser() { #endif #endif - Insert(&PerIsolateOptionsParser::instance, - &PerProcessOptions::get_per_isolate_options); + Insert(&iop, &PerProcessOptions::get_per_isolate_options); } inline std::string RemoveBrackets(const std::string& host) { @@ -513,10 +549,8 @@ void GetOptions(const FunctionCallbackInfo& args) { per_process::cli_options->per_isolate = original_per_isolate; }); - const auto& parser = PerProcessOptionsParser::instance; - Local options = Map::New(isolate); - for (const auto& item : parser.options_) { + for (const auto& item : _ppop_instance.options_) { Local value; const auto& option_info = item.second; auto field = option_info.field; @@ -534,29 +568,34 @@ void GetOptions(const FunctionCallbackInfo& args) { } break; case kBoolean: - value = Boolean::New(isolate, *parser.Lookup(field, opts)); + value = Boolean::New(isolate, + *_ppop_instance.Lookup(field, opts)); break; case kInteger: - value = Number::New(isolate, *parser.Lookup(field, opts)); + value = Number::New(isolate, + *_ppop_instance.Lookup(field, opts)); break; case kUInteger: - value = Number::New(isolate, *parser.Lookup(field, opts)); + value = Number::New(isolate, + *_ppop_instance.Lookup(field, opts)); break; case kString: - if (!ToV8Value(context, *parser.Lookup(field, opts)) + if (!ToV8Value(context, + *_ppop_instance.Lookup(field, opts)) .ToLocal(&value)) { return; } break; case kStringList: if (!ToV8Value(context, - *parser.Lookup>(field, opts)) + *_ppop_instance.Lookup(field, opts)) .ToLocal(&value)) { return; } break; case kHostPort: { - const HostPort& host_port = *parser.Lookup(field, opts); + const HostPort& host_port = + *_ppop_instance.Lookup(field, opts); Local obj = Object::New(isolate); Local host; if (!ToV8Value(context, host_port.host()).ToLocal(&host) || @@ -597,7 +636,7 @@ void GetOptions(const FunctionCallbackInfo& args) { } Local aliases; - if (!ToV8Value(context, parser.aliases_).ToLocal(&aliases)) return; + if (!ToV8Value(context, _ppop_instance.aliases_).ToLocal(&aliases)) return; Local ret = Object::New(isolate); if (ret->Set(context, env->options_string(), options).IsNothing() || diff --git a/src/node_options.h b/src/node_options.h index e92fd3c983191f..cc91d708ef284b 100644 --- a/src/node_options.h +++ b/src/node_options.h @@ -320,12 +320,12 @@ class OptionsParser { // // If `*error` is set, the result of the parsing should be discarded and the // contents of any of the argument vectors should be considered undefined. - virtual void Parse(std::vector* const args, - std::vector* const exec_args, - std::vector* const v8_args, - Options* const options, - OptionEnvvarSettings required_env_settings, - std::vector* const errors) const; + void Parse(std::vector* const args, + std::vector* const exec_args, + std::vector* const v8_args, + Options* const options, + OptionEnvvarSettings required_env_settings, + std::vector* const errors) const; private: // We support the wide variety of different option types by remembering @@ -406,33 +406,12 @@ class OptionsParser { friend void GetOptions(const v8::FunctionCallbackInfo& args); }; -class DebugOptionsParser : public OptionsParser { - public: - DebugOptionsParser(); - - static const DebugOptionsParser instance; -}; - -class EnvironmentOptionsParser : public OptionsParser { - public: - EnvironmentOptionsParser(); - - static const EnvironmentOptionsParser instance; -}; - -class PerIsolateOptionsParser : public OptionsParser { - public: - PerIsolateOptionsParser(); - - static const PerIsolateOptionsParser instance; -}; - -class PerProcessOptionsParser : public OptionsParser { - public: - PerProcessOptionsParser(); - - static const PerProcessOptionsParser instance; -}; +using StringVector = std::vector; +template +void Parse( + StringVector* const args, StringVector* const exec_args, + StringVector* const v8_args, OptionsType* const options, + OptionEnvvarSettings required_env_settings, StringVector* const errors); } // namespace options_parser diff --git a/src/node_worker.cc b/src/node_worker.cc index b6a56b9b94b08e..e22503863d7db4 100644 --- a/src/node_worker.cc +++ b/src/node_worker.cc @@ -476,7 +476,7 @@ void Worker::New(const FunctionCallbackInfo& args) { // Using invalid_args as the v8_args argument as it stores unknown // options for the per isolate parser. - options_parser::PerIsolateOptionsParser::instance.Parse( + options_parser::Parse( &exec_argv, &exec_argv_out, &invalid_args,