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>
4 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.