Closed
Bug 1482931
Opened 6 years ago
Closed 6 years ago
Cleanup static/globals in js/src
Categories
(Core :: JavaScript Engine, enhancement, P5)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla63
Tracking | Status | |
---|---|---|
firefox63 | --- | fixed |
People
(Reporter: tcampbell, Assigned: tcampbell)
Details
Attachments
(2 files, 1 obsolete file)
7.08 KB,
patch
|
jandem
:
review+
|
Details | Diff | Splinter Review |
21.32 KB,
patch
|
tcampbell
:
review+
|
Details | Diff | Splinter Review |
We should cleanup our global state to make read-only things as const when possible. This moves them into .data section instead of .rodata. This also reduces false-positives when auditing for global mutable state (which we want to reduce because it can hide side-effects). This also removes a few calls to getenv from static initializers (which was a bad practice do to initialization ordering fun).
Assignee | ||
Comment 1•6 years ago
|
||
Mutable globals are gross, so let's make sure the read-only ones are marked as const. This also removes some errant 'static' qualifies that affect pointer storage when the author probably intended it to affect the pointee.
Attachment #8999674 -
Flags: review?(jwalden+bmo)
Assignee | ||
Comment 2•6 years ago
|
||
Avoid calling getenv in static initializers that run before main. This seems to be not super well defined and it is easy for us to avoid.
Attachment #8999676 -
Flags: review?(jdemooij)
Comment 3•6 years ago
|
||
Comment on attachment 8999674 [details] [diff] [review] 0001-Bug-1482931-Cleanup-const-ness-of-statics-in-js-src.patch Review of attachment 8999674 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/GC.cpp @@ +1037,5 @@ > *frequency = zealFrequency; > *scheduled = nextScheduled; > } > > +const char* const gc::ZealModeHelpText = Seems like this would be better as const char gc::ZealModeHelpText[] = and the same at the extern declaration site. ::: js/src/jit/ScalarReplacement.cpp @@ +369,5 @@ > void loadOffset(MInstruction* ins, size_t offset); > void visitObjectGuard(MInstruction* ins, MDefinition* operand); > }; > > +const char* const ObjectMemoryView::phaseName = "Scalar Replacement of Object"; const char[] and at declaration. ::: js/src/proxy/ScriptedProxyHandler.cpp @@ +34,5 @@ > // Step 2. > if (!current.object()) { > // Step 2a-b,e. As |O| is always undefined, steps 2c-d fall away. > if (!extensible) { > + const char* DETAILS_NOT_EXTENSIBLE = static const char DNE[] @@ +72,5 @@ > // Step 5. > if (!current.configurable()) { > // Step 5a. > if (desc.hasConfigurable() && desc.configurable()) { > + const char* DETAILS_CANT_REPORT_NC_AS_C = static const char DCRNAC[] @@ +80,5 @@ > } > > // Step 5b. > if (desc.hasEnumerable() && desc.enumerable() != current.enumerable()) { > + const char* DETAILS_ENUM_DIFFERENT = static const char DED[] @@ +95,5 @@ > // Step 7. > if (current.isDataDescriptor() != desc.isDataDescriptor()) { > // Steps 7a, 11. As |O| is always undefined, steps 2b-c fall away. > if (!current.configurable()) { > + const char* DETAILS_CURRENT_NC_DIFF_TYPE = static const char DCNDT[] @@ +107,5 @@ > if (current.isDataDescriptor()) { > MOZ_ASSERT(desc.isDataDescriptor()); // by step 7 > if (!current.configurable() && !current.writable()) { > if (desc.hasWritable() && desc.writable()) { > + const char* DETAILS_CANT_REPORT_NW_AS_W = static const char DCRNAW[] @@ +118,5 @@ > bool same; > if (!SameValue(cx, desc.value(), current.value(), &same)) > return false; > if (!same) { > + const char* DETAILS_DIFFERENT_VALUE = static const char DDV[] @@ +136,5 @@ > > if (current.configurable()) > return true; > if (desc.hasSetterObject() && (desc.setter() != current.setter())) { > + const char* DETAILS_SETTERS_DIFFERENT = ...I'm gonna stop noting 'em in this file now. :-) ::: js/src/vm/BytecodeUtil.cpp @@ +116,5 @@ > MOZ_CRASH("Unexpected op"); > } > } > > +const char* const PCCounts::numExecName = "interp"; const char PCCounts::numExecName[]? ::: js/src/wasm/WasmFrameIter.cpp @@ +1288,5 @@ > + const char* importJitDescription = "fast exit trampoline (in wasm)"; > + const char* importInterpDescription = "slow exit trampoline (in wasm)"; > + const char* builtinNativeDescription = "fast exit trampoline to native (in wasm)"; > + const char* trapDescription = "trap handling (in wasm)"; > + const char* debugTrapDescription = "debug trap handling (in wasm)"; Can't these all be static const char fooDescription[] = ... ?
Attachment #8999674 -
Flags: review?(jwalden+bmo) → review+
Assignee | ||
Comment 4•6 years ago
|
||
Carrying r=waldo on Comment 3. Replaced the |static const char* x = ".."| cases with |static const char x[] = ".."| which is a much better idea.
Attachment #8999674 -
Attachment is obsolete: true
Attachment #8999709 -
Flags: review+
Comment 5•6 years ago
|
||
Comment on attachment 8999676 [details] [diff] [review] 0002-Bug-1482931-Avoid-calling-getenv-in-static-initializ.patch Review of attachment 8999676 [details] [diff] [review]: ----------------------------------------------------------------- AFAIK static initializers inside functions run when we call the function the first time and the spec requires this to be thread-safe? This does look simpler (and slightly faster) though. ::: js/src/vm/Initialization.cpp @@ +105,5 @@ > #if defined(DEBUG) || defined(JS_OOM_BREAKPOINT) > RETURN_IF_FAIL(js::oom::InitThreadType()); > #endif > > + js::gDisablePoisoning = (bool) getenv("JSGC_DISABLE_POISONING"); We usually prefer C++ casts so bool(getenv("JSGC_DISABLE_POISONING"))
Attachment #8999676 -
Flags: review?(jdemooij) → review+
Assignee | ||
Comment 6•6 years ago
|
||
(In reply to Jan de Mooij [:jandem] from comment #5) > AFAIK static initializers inside functions run when we call the function the > first time and the spec requires this to be thread-safe? I apparently was mistaken here. Looks like they are deferred to first run. Will still land patch for the simplification.
Pushed by tcampbell@mozilla.com: https://hg.mozilla.org/integration/mozilla-inbound/rev/490b0d605859 Cleanup const-ness of statics in js/src. r=waldo https://hg.mozilla.org/integration/mozilla-inbound/rev/a77d82cbe1cc Simplify some static initializers in js/src. r=jandem
Comment 8•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/490b0d605859 https://hg.mozilla.org/mozilla-central/rev/a77d82cbe1cc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox63:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in
before you can comment on or make changes to this bug.
Description
•