build: enable -DV8_ENABLE_CHECKS flag #61327
Conversation
|
Review requested:
|
nodejs-github-bot
added
c++
configure.py
Outdated
| def set_configuration_variable_and_defines(configs, name, define, release=None, debug=None): | ||
| set_configuration_variable(configs, name, release, debug) | ||
| if configs['Debug'].get('defines') is None: | ||
| configs['Debug']['defines'] = [] | ||
| if configs['Release'].get('defines') is None: | ||
| configs['Release']['defines'] = [] | ||
| configs['Debug']['defines'].append(define) | ||
| configs['Release']['defines'].append(define) | ||
|
|
There was a problem hiding this comment.
This might be a GYP issue, but it seems that variables defined inside a conditions block are not reflected when evaluating other conditions.
In this case, the value of v8_enable_v8_checks set in configure.py (https://github.com/nodejs/node/blob/main/configure.py#L1530-L1532
) does not appear to be used when evaluating the condition 'v8_enable_v8_checks==1' in tools/v8_gypfiles/features.gypi (https://github.com/nodejs/node/blob/main/tools/v8_gypfiles/features.gypi#L405-L407
).
Therefore, I’m setting the defines directly.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #61327 +/- ##
========================================
Coverage 88.51% 88.51%
========================================
Files 703 704 +1
Lines 208430 208745 +315
Branches 40198 40273 +75
========================================
+ Hits 184491 184780 +289
- Misses 15951 15969 +18
- Partials 7988 7996 +8
🚀 New features to boost your workflow:
|
lib/util.js
Outdated
| frameCount = MathFloor(frameCount); | ||
| return binding.getCallSites(frameCount); |
There was a problem hiding this comment.
| frameCount = MathFloor(frameCount); | |
| return binding.getCallSites(frameCount); | |
| return binding.getCallSites(MathFloor(frameCount)); |
There was a problem hiding this comment.
Since this is experimental, would it make more sense to switch to validateInteger?
There was a problem hiding this comment.
Agree, I switched to validateInteger to limit it to integers.
| @@ -441,7 +442,7 @@ void BuiltinLoader::SaveCodeCache(const std::string& id, Local<Data> data) { | |||
| new_cached_data.reset( | |||
| ScriptCompiler::CreateCodeCache(mod->GetUnboundModuleScript())); | |||
| } else { | |||
| Local<Function> fun = data.As<Function>(); | |||
| Local<Function> fun = data.As<Value>().As<Function>(); | |||
There was a problem hiding this comment.
When V8_ENABLE_CHECKS is enabled, Local::As() fails because v8::Function only supports Cast(Value*), causing a compile error.
https://github.com/nodejs/node/blob/main/deps/v8/include/v8-local-handle.h#L424.
| } else { | ||
| CHECK(value->IsNumber()); | ||
| v8::Isolate* isolate = v8::Isolate::GetCurrent(); | ||
| v8::Local<v8::Context> context = isolate->GetCurrentContext(); | ||
| v8::Maybe<uint32_t> maybe = value->Uint32Value(context); |
There was a problem hiding this comment.
in line 684 why don't you just check for IsUint32?
There was a problem hiding this comment.
Because we need to accept signed values for some call sites (e.g. fs.chown), not only strict Uint32.
Co-authored-by: Yagiz Nizipli <yagiz@nizipli.com>
| configs['Release']['variables'][name] = release | ||
| configs['Debug']['variables'][name] = debug | ||
|
|
||
| def set_configuration_variable_and_defines(configs, name, release=None, debug=None, release_define=None, debug_define=None): |
There was a problem hiding this comment.
According to https://github.com/nodejs/gyp-next/blob/main/docs/InputFormatReference.md#processing-order, I think there is no way for us to generate a single config.gypi supporting two configurations at the same time, with variable expansions based on configurations, since the variables are expanded before merging the configuration objects.
So I'd suggest updating this to be a simple way: instead of
Lines 2499 to 2503 in e1fc3dc
we could do:
if options.debug:
variables = variables | config_debug_vars
else:
variables = variables | config_release_vars
5 participants
Fixes: #61301
I’ve made changes so that V8_ENABLE_CHECKS is enabled in debug builds.
In addition, I’ve fixed code that caused errors when V8_ENABLE_CHECKS is enabled.