Closed Bug 1482931 Opened Last year Closed Last year

Cleanup static/globals in js/src

Categories

(Core :: JavaScript Engine, enhancement, P5)

enhancement

Tracking

()

RESOLVED FIXED
mozilla63
Tracking Status
firefox63 --- fixed

People

(Reporter: tcampbell, Assigned: tcampbell)

Details

Attachments

(2 files, 1 obsolete file)

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).
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)
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 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+
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 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+
(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
https://hg.mozilla.org/mozilla-central/rev/490b0d605859
https://hg.mozilla.org/mozilla-central/rev/a77d82cbe1cc
Status: ASSIGNED → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla63
You need to log in before you can comment on or make changes to this bug.