712939 - Convert existing JS_STATIC_ASSERT users to static_assert

Status: ASSIGNED

Description

[Jeff Walden [:Waldo]]

Now that MOZ_STATIC_ASSERT exists, we should convert everyone[0] using JS_STATIC_ASSERT over to it.  Getting everyone on the same page is a nice reduction in complexity, and it increases the approachability of our code base to not have many ways to do the same thing.

This conversion isn't just search-and-replace.  J_S_A takes only a compile-time condition as an argument.  M_S_A works like C++11's static_assert and takes two arguments: a condition and a string literal explaining the assertion.  (Compilers that implement the C++11 bit will display the reason if the assertion fails when compiling.)  Existing uses thus need reasons associated with them, which requires getting a little familiar with the relevant code to figure out those reasons.  Also, converted files probably should #include "mozilla/Assertions.h" to get the macro -- not easy to search-and-replace into place.

I'm willing to help anyone interested through this, especially as regards assertions where the reasons behind them aren't obvious.

Comment 1

[Berker Peksag [:berkerpeksag]]

Hi,

I'd like to work on this bug. I have contributed to various Mozilla projects like DevTools and Kitsune. Can you please tell me what is the next step is?

Thanks!

Comment 2

[Jeff Walden [:Waldo]]

Excellent!  The next step here is pretty simple.

http://mxr.mozilla.org/mozilla-central/search?string=JS_STATIC_ASSERT

That's the list of all the current users of JS_STATIC_ASSERT.  Basically, this bug's about going through those uses and changing them to MOZ_STATIC_ASSERT by 1) changing the macro used, and 2) adding an extra argument to each macro call, as an "explanation" string for why the assertion should hold.  Let's take, say, js/src/jsfun.h for example.

http://hg.mozilla.org/mozilla-central/annotate/9eda39f28e5a/js/src/jsfun.h#l198

    static uintN offsetOfNativeOrScript() {
        JS_STATIC_ASSERT(offsetof(U, n.native) == offsetof(U, i.script_));
        JS_STATIC_ASSERT(offsetof(U, n.native) == offsetof(U, nativeOrScript));
        return offsetof(JSFunction, u.nativeOrScript);
    }

As it says, the function returns the offset of JSFunction::u.nativeOrScript.  The static assertions before it verify that JSFunction::U::native is the same as both JSFunction::U::i.script_ and JSFunction::U::nativeOrScript.  Now look at the definition of JSFunction:

struct JSFunction : public JSObject
{
    uint16_t        nargs;        /* maximum number of specified arguments,
                                     reflected as f.length/f.arity */
    uint16_t        flags;        /* flags, see JSFUN_* below and in jsapi.h */
    union U {
        struct Native {
            js::Native  native;   /* native method pointer or null */
            js::Class   *clasp;   /* class of objects constructed
                                     by this function */
        } n;
        struct Scripted {
            JSScript    *script_; /* interpreted bytecode descriptor or null;
                                     use the accessor! */
            JSObject    *env_;    /* environment for new activations;
                                     use the accessor! */
        } i;
        void            *nativeOrScript;
    } u;
    //...

Given how unions work (I'm assuming you're familiar with C++, right?), it should be reasonably clear that what's happening is that we're using the offset of nativeOrScript as a stand-in for the offset of |native| and the offset of |script_|.  The static assertions simply verify that's the case.

So, then, for those assertions the right fix might be this:

    static uintN offsetOfNativeOrScript() {
        MOZ_STATIC_ASSERT(offsetof(U, n.native) == offsetof(U, i.script_),
                          "this method requires that native and script_ be at the same "
                          "offset within JSFunction");
        MOZ_STATIC_ASSERT(offsetof(U, n.native) == offsetof(U, nativeOrScript),
                          "moreover, that offset must be the same as for nativeOrScript");
        return offsetof(JSFunction, u.nativeOrScript);
    }

Does that make sense?

That covers two uses in the codebase.  From there you'd go on to others, in whatever order suits you.  #jsapi on IRC has people who should be able to explain all the existing uses (although sometimes we might have to look a little to figure out the reason), if you can't figure any out.  (But since there's no reason this has to be done in any order, you could just move on to another use for the moment, anyway.)  I'm on there as "Waldo" fairly often, too, and am more than happy to walk through any static assertions you find that don't have an obvious reason for existence.  That said, given the time of the year, I probably won't be on IRC much for the next few days, likely til the day after Christmas.  If you have questions during that time, feel free to ask them here (there are a bunch of people watching JS bugmail who know as much as I do -- more in many areas, too :-) ), or send me email directly if you want.

Comment 3

[Nicholas Nethercote [:njn]]

I don't want to discourage Berker from contributing, but I think it's worth pointing out that the "replace JS_ASSERT with MOZ_ASSERT" patch was rejected (bug 712873), and it's highly likely that this change would be rejected on the same grounds.

Comment 4

[Jeff Walden [:Waldo]]

Why?  JS_STATIC_ASSERT doesn't accept the reason string MOZ_STATIC_ASSERT does, instead passing along the useless string "JS_STATIC_ASSERT".  It would clearly be better to explain these assertions and generate useful compiler errors in modern C++11 compilers.  (And we're not done yet in bug 712873 just yet, either.  :-P )

Comment 5

[Berker Peksag [:berkerpeksag]]

Jeff, thanks for your detailed answer!

Comment 6

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

We use static_assert rather than MOZ_STATIC_ASSERT now.

Comment 8

[Sahil Chelaramani]

Created attachment 814797 [details] [diff] [review]
Converted all instances of JS_STATIC_ASSERT to static_assert

Made the changes from JS_STATIC_ASSERT to static_assert. This is a rough patch to do this, and the messages to static_assert can be refined. Any suggestions are welcome. Please review.

Comment 9

[Jeff Walden [:Waldo]]

Comment on attachment 814797 [details] [diff] [review]
Converted all instances of JS_STATIC_ASSERT to static_assert

Review of attachment 814797 [details] [diff] [review]:
-----------------------------------------------------------------

Phew, that's big!  Two issues.

First, I think we want to do this a few files at a time.  This patch likely doesn't apply already.  :-\  Smaller bites at a time won't break so quickly, and they're faster to review, so you don't get long delays going through *all* of a large patch, when comments on parts of it get finished much sooner.

Second, the explanations shouldn't just be restating whatever condition is checked.  They should be explaining *why* the condition matters.  For example, consider this from your Assembler-arm.cpp changes:

static_assert(sizeof(PaddedFloat32) == sizeof(double),"sizeof PaddedFloat32 should be equal to sizeof double");

It's true we want those types to have the same size.  But the condition states that adequately.  The *reason* is what matters: that float32 constants are stored in the double constant pool, using the PaddedFloat32 type to access the relevant memory.  So for this we'd want (wrapped to 79 characters, with at least one space, perhaps a line break after the comma in the middle of the static_assert):

static_assert(sizeof(PaddedFloat32) == sizeof(double),
              "float32 constants are stored in the double constant pool using "
              "the PaddedFloat32 struct, which must be the same size as the "
              "doubles in the pool");

Figuring out those underlying motivations may be a bit tricky for an ostensibly good first bug.  :-\  At least, depending how "first" one wants it to be.  If you want to track down these reasons, with patches fixing a file or two at a time, that's great!  But I think it may be the case that mentored/first bugs are generally geared to be somewhat simpler, and this bug is a bit much for that, unless you're willing to do some deeper diving to figure out reasons that aren't necessarily obvious at first glance.  So I'm going to unmark it as such a bug -- don't take that as suggesting you shouldn't try it, and I'm still fine to review potential patches (just make them smaller :-) ), but it seems like I should better set expectations about the difficulty of this bug.  :-)

Comment 10

[Jeff Walden [:Waldo]]

Just to restate: anyone should feel free to work on this, in small patches touching a few files at a time.  But it seems to be technically more difficult than the average mentored bug, so I don't want to mislead people about how hard it might be to fix properly.

Comment 11

[Chris Peterson [:cpeterson]]

Created attachment 8377420 [details] [diff] [review]
replace-JS_STATIC_ASSERT-outside-js-directory.patch

Replace the three instances of JS_STATIC_ASSERT outside the js/ directory with static_assert. JS_STATIC_ASSERT only remains in js/public/ and js/src/.

Comment 12

[Chris Peterson [:cpeterson]]

Comment on attachment 8377420 [details] [diff] [review]
replace-JS_STATIC_ASSERT-outside-js-directory.patch

Asking luke for r? because Waldo is on PTO.

Replace the three instances of JS_STATIC_ASSERT outside the js/ directory with static_assert. JS_STATIC_ASSERT only remains in js/public/ and js/src/.

Comment 13

[Luke Wagner [:luke]]

Since these patches have a small bit of style choice to them, I'd feel more comfortable if you got a Gecko person to review them (whoever hg annotate on the surrounding code points to).

Comment 14

[Andrew McCreight [:mccr8]]

Comment on attachment 8377420 [details] [diff] [review]
replace-JS_STATIC_ASSERT-outside-js-directory.patch

Review of attachment 8377420 [details] [diff] [review]:
-----------------------------------------------------------------

The comment is a bit literal.  Maybe something like "length of trace_types should match the number of cases of JSGCTraceKind".

r=me for the CycleCollectedJSRuntime changes.

mrgiggles is a handy way to figure out who can review a particular file.  "mrgiggles: who can review nsNPAPIPlugin.h?" etc.

Comment 15

[Nicholas Nethercote [:njn]]

> mrgiggles is a handy way to figure out who can review a particular file. 
> "mrgiggles: who can review nsNPAPIPlugin.h?" etc.

In case it's not clear, mrgiggles is an IRC bot, so you'd be typing that question into IRC. He's present on #jsapi and possibly some other channels.

Comment 16

[Chris Peterson [:cpeterson]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3a8893b28489

Comment 17

[Daniel Holbert [:dholbert]]

Backed out, along with bug 712873, for the bustage noted in bug 712873 comment 13. (Likely just caused by that bug, but I backed this out as well in case this bug depends on that bug or anything, since they sound related and landed together):
 https://hg.mozilla.org/integration/mozilla-inbound/rev/a6425a31c4c4

Comment 18

[Chris Peterson [:cpeterson]]

relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9da5cb817aae

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9da5cb817aae

Comment 20

[Andrew McCreight [:mccr8]]

I didn't actually review all of the changes here, only the CycleCollectedJSRuntime.cpp ones.  You should find somebody to look at those...

Comment 21

[Andrew McCreight [:mccr8]]

backed out:  https://hg.mozilla.org/integration/mozilla-inbound/rev/7bc9adddf5fa

Sorry for not being more explicit when I reviewed about needing other reviewers.

Comment 22

[Andrew McCreight [:mccr8]]

Comment on attachment 8377420 [details] [diff] [review]
replace-JS_STATIC_ASSERT-outside-js-directory.patch

Benoit for the profiler changes, bsmedberg for the nsNPAPIPlugin changes.

Comment 23

[Benoit Girard (:BenWa)]

Comment on attachment 8377420 [details] [diff] [review]
replace-JS_STATIC_ASSERT-outside-js-directory.patch

Review of attachment 8377420 [details] [diff] [review]:
-----------------------------------------------------------------

ty

Comment 24

[Ryan VanderMeulen [:RyanVM]]

(In reply to Andrew McCreight [:mccr8] from comment #21)
> backed out: 
> https://hg.mozilla.org/integration/mozilla-inbound/rev/7bc9adddf5fa

Merge of backout: https://hg.mozilla.org/mozilla-central/rev/7bc9adddf5fa

Comment 25

[Chris Peterson [:cpeterson]]

Sorry for misunderstanding! re-relanded:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d5114a4f9059

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d5114a4f9059

Comment 27

[Chris Peterson [:cpeterson]]

I'm not working on this bug.

Comment 28

[Jeff Walden [:Waldo]]

Created attachment 8510547 [details] [diff] [review]
First pass of conversion

This isn't going to get done unless someone sits down and does it bit by bit, a little bit every day for awhile.  I've started doing that.  Here's a first pass.  Note that it's not just mechanical replacement, and the process of providing reasons isn't necessarily just providing a reason, either.  Probably one reason that, in hindsight, this wasn't the greatest first bug.  :-\

Feel free to foist off sub-parts of this onto other reviewers if necessary.  I took some mini-leaps in deciding what the point of some of these were, and if you don't agree, or are unsure whether you agree or disagree, another person won't hurt.

I picked out you because you expressed confidence in the idea of "just changing them", so you get to see how it's not entirely that simple.  :-P

The pn_link/dn_uses changes are the squirreliest bits of this.  I don't think there's actually any requirement that the two overlap, so I removed the assertion.  The overlap is simply a "freebie" to conserve space.  Still, if I missed some actual reason for them to be at the same place, we can add it to the comment on the union.

Comment 29

[Jan de Mooij [:jandem]]

Comment on attachment 8510547 [details] [diff] [review]
First pass of conversion

Review of attachment 8510547 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/builtin/MapObject.cpp
@@ +1302,4 @@
>      MOZ_ASSERT(MapObject::is(args.thisv()));
>  
>      ValueMap &map = extract(args);
> +    static_assert(sizeof map.count() <= sizeof(uint32_t),

Interesting, I didn't know "sizeof map.count()" works.

::: js/src/frontend/ParseNode.h
@@ +533,5 @@
> +     * below, with a lengthy comment that you should read.)  Each binding
> +     * has one canonical Definition; all uses of that definition are reached
> +     * starting from dn_uses, then following subsequent pn_link pointers.
> +     *
> +     * The dn_uses chain elements are unordered.  Any apparent ordering in some

Nice comment and cleanup. Single space after period?

Comment 30

[Jeff Walden [:Waldo]]

Created attachment 8512368 [details] [diff] [review]
Pass two

Haven't landed the first patch yet, probably tomorrow.

Comment 31

[Jan de Mooij [:jandem]]

Comment on attachment 8512368 [details] [diff] [review]
Pass two

Review of attachment 8512368 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.h
@@ +2447,5 @@
> +                  "JSPropertySpec::setter must be compact");
> +    static_assert(offsetof(SelfHostedWrapper, funname) == offsetof(JSPropertyOpWrapper, info),
> +                  "JS_SELF_HOSTED* macros below require that "
> +                  "SelfHostedWrapper::funname overlay "
> +                  "JSPropertyOpWrapper::info");

Nit: should fit on two lines AFAICS, unless we're using 80 columns for those strings?

::: js/src/vm/String.h
@@ +872,5 @@
> +                      "fill subsequent cells and thus are wasteful");
> +        static_assert(MAX_LENGTH_LATIN1 + 1 ==
> +                      (sizeof(JSFatInlineString) -
> +                       offsetof(JSFatInlineString, d.inlineStorageLatin1)) / sizeof(char),
> +                      "MAX_LENGTH_LATIN1_BYTE must be one less than inline "

Nit: s/_BYTE//

Comment 32

[Jeff Walden [:Waldo]]

Given that static_assert reasons are closer to comments than code, and we wrap comments early for readability, I've always wrapped static_assert reasons at the lower boundary.  FYI.

https://hg.mozilla.org/integration/mozilla-inbound/rev/c8cdf92b78f5
https://hg.mozilla.org/integration/mozilla-inbound/rev/2afd2caa8f74

Comment 33

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/c8cdf92b78f5
https://hg.mozilla.org/mozilla-central/rev/2afd2caa8f74

Comment 34

[Jeff Walden [:Waldo]]

Created attachment 8523321 [details] [diff] [review]
More conversions

Comment 35

[Jeff Walden [:Waldo]]

Created attachment 8523328 [details] [diff] [review]
Convert some typed-enum sizing static assertions

This also includes some JS_ENUM_* nonsense.  Too bad for bizarrely buggy gcc here.

Comment 36

[Jeff Walden [:Waldo]]

Created attachment 8523330 [details] [diff] [review]
More conversions

The char16_t characteristics asserted previously are already asserted in mfbt/Char16.h, so it's sort of pointless asserting them yet again.  In general we really should just assume language guarantees like that actually hold, unless we know they don't somewhere for some reason.  And the initial definition should include any static assertions, when we have to emulate stuff, which we already do for char16_t.

Comment 37

[Jeff Walden [:Waldo]]

Created attachment 8523336 [details] [diff] [review]
Centralize/structure sizeof(Value) assertions in the JITs when doing address calculations, shifts, etc.

This is a bit more than just assertion conversions -- I added a few helpers to centralize these things and not require the same static_assert ubiquitously, with individual sizeofs of inlining of finicky constants at every place.  It's probable I've missed some of the places where this matters -- I targeted relevant JS_STATIC_ASSERTs in these files, and I did some searching for sizeof(Value) and similar.  But this seems like a good start.

Comment 38

[Jeff Walden [:Waldo]]

Created attachment 8523338 [details] [diff] [review]
More random conversions

Comment 39

[Jan de Mooij [:jandem]]

Comment on attachment 8523321 [details] [diff] [review]
More conversions

Review of attachment 8523321 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/json.cpp
@@ +58,5 @@
> +                  "'\\n' must be treated as special in the result below");
> +    static_assert('\r' < ' ',
> +                  "'\\r' must be treated as special in the result below");
> +    static_assert('\t' < ' ',
> +                  "'\\t' must be treated as special in the result below");

Doesn't really matter but IMO in such cases it's more readable when each assert is on a single line.

Comment 40

[Jan de Mooij [:jandem]]

Comment on attachment 8523328 [details] [diff] [review]
Convert some typed-enum sizing static assertions

Review of attachment 8523328 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/Value.h
@@ +146,5 @@
> +/*
> + * All our supported compilers implement C++11 |enum Foo : T| syntax, so don't
> + * expose these macros.  (This macro exists *only* because gcc bug 51242
> + * <https://gcc.gnu.org/bugzilla/show_bug.cgi?id=51242> makes bit-fields of
> + * typed enums trigger a warning that can't be turned off.  Don't expose it

Hrm :(

(Btw, SM style is single space after period right?)

Comment 41

[Jan de Mooij [:jandem]]

Comment on attachment 8523336 [details] [diff] [review]
Centralize/structure sizeof(Value) assertions in the JITs when doing address calculations, shifts, etc.

Review of attachment 8523336 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jit/shared/Assembler-shared.h
@@ +319,5 @@
> +// that aren't object elements or object slots (for example, values on the
> +// stack, values in an arguments object, &c.).  If you're indexing into an
> +// object's elements or slots, don't use this directly!  Use
> +// BaseObject{Element,Slot}Index instead.
> +struct BaseValueIndex : BaseIndex

Ah pretty nice to get rid of the Scale argument everywhere.

::: js/src/vm/NativeObject.h
@@ +550,5 @@
>      bool shadowingShapeChange(ExclusiveContext *cx, const Shape &shape);
>      bool clearFlag(ExclusiveContext *cx, BaseShape::Flag flag);
>  
> +    static void slotsSizeMustNotOverflow() {
> +        static_assert((NativeObject::NELEMENTS_LIMIT - 1) * sizeof(JS::Value) <= INT32_MAX,

Hm, does this do the right thing if the multiplication overflows?

@@ +903,5 @@
> +    static void elementsSizeMustNotOverflow() {
> +        static_assert((NativeObject::NELEMENTS_LIMIT - 1) * sizeof(JS::Value) <= INT32_MAX,
> +                      "every caller of this method require that an element "
> +                      "count multiplied by sizeof(Value) can't overflow "
> +                      "uint32_t (and sometimes int32_t ,too)");

Nit: typo: "int32, too"

Comment 42

[Jan de Mooij [:jandem]]

Comment on attachment 8523338 [details] [diff] [review]
More random conversions

Review of attachment 8523338 [details] [diff] [review]:
-----------------------------------------------------------------

Nice.

::: js/src/asmjs/AsmJSSignalHandlers.cpp
@@ +476,5 @@
>  ContextToPC(x86_thread_state_t &state)
>  {
>  # if defined(JS_CPU_X64)
> +    static_assert(sizeof(state.uts.ts64.__rip) == sizeof(void*),
> +                  "stored IP should be compile-time pointer-sized");

Nit: what do you think about removing the "compile-time" part?

::: js/src/vm/TypedArrayObject.cpp
@@ +848,5 @@
>  TypedArrayObjectTemplate<NativeType>::getIndexValue(JSObject *tarray, uint32_t index)
>  {
> +    static_assert(sizeof(NativeType) < 4,
> +                  "this method must only handle NativeType values that are "
> +                  "always exact int32_ values");

s/int32_/int32/ ?

Comment 43

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/dcb90c3786e0
https://hg.mozilla.org/integration/mozilla-inbound/rev/37314ece19fa
https://hg.mozilla.org/integration/mozilla-inbound/rev/f371cd11e4da
https://hg.mozilla.org/integration/mozilla-inbound/rev/295cb63e5546
https://hg.mozilla.org/integration/mozilla-inbound/rev/a59f355fe056

I made the static_asserts in json single-line by removing a few extra words.  (They were only not single-line because of the length exceeding 80ch, which we try not to exceed for comments, and I think the rationale applies to static assertion messages as well.)

I am never going to remember spacing after periods.  (Nor do I think it's valuable at all to be consistent about that aspect of style, given prose is so much less constrained than code, but I digress.)

I think you're right, the multiplication overflowing would make the static_assert not work.  That's not really a problem on 64-bit, because size_t is going to be 64 bits and we're not going to have 60-some bits of element limit there.  On 32-bit it could conceivably overflow, tho.  I did the usual a * b <= c ⇒ a <= c / b trick to address this.

int32_t is deliberate, since that's the type name.  :-)  We use the name all over in comments, fairly sure.

I kind of like the "compile-time" bit to distinguish from the pointer size of the architecture in the state being queried, but it's not a super-strong preference.

Comment 44

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dcb90c3786e0
https://hg.mozilla.org/mozilla-central/rev/37314ece19fa
https://hg.mozilla.org/mozilla-central/rev/f371cd11e4da
https://hg.mozilla.org/mozilla-central/rev/295cb63e5546
https://hg.mozilla.org/mozilla-central/rev/a59f355fe056

Comment 45

[Release mgmt bot [:sylvestre / :calixte]]

The leave-open keyword is there and there is no activity for 6 months.
:Waldo, maybe it's time to close this bug?

Comment 46

[Jeff Walden [:Waldo]]

No, there are still a bunch of JS_STATIC_ASSERTs that need to die.