1476500 - Harden code generator against changed types elsewhere

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Paul Bone [:pbone]]

There are a number of places in the JIT where a void * or even a const void * is passed in, and then the JITed code treats it as a more specific type such as a uint32_t*.

I have a patch to add extra assignments to the JIT to ensure that these types are correct before performing the implicit void* coersion in the Address and AbsoluteAddress constructors.  The types of these things are updated elsewhere in the engine.  Now if one of these types changes, it will create a compilation error in the JIT code drawing a developer's attention there.

Comment 1

[Paul Bone [:pbone]]

Attachment: Bug 1476500 - Fix some prose in the MacroAssembler class comment

Comment 2

[Paul Bone [:pbone]]

Attachment: Bug 1476500 - Add extra assignments and make some types more specific

Comment 3

[Paul Bone [:pbone]]

Attachment: Bug 1476500 - Add a comment explaining why we need this value

Comment 4

[Jan de Mooij [:jandem]]

One reason for returning const void* from CompileWrappers, for instance addressOfRandomNumberGenerator, is that this code can be called off-thread so we want to give the caller as little as possible to work with (read: to introduce races etc).

Comment 5

[Jan de Mooij [:jandem]]

(In reply to Jan de Mooij [:jandem] from comment #4)
> One reason for returning const void* from CompileWrappers, for instance
> addressOfRandomNumberGenerator, is that this code can be called off-thread
> so we want to give the caller as little as possible to work with (read: to
> introduce races etc).

(That said, better type safety may be worth it; I just wanted to mention this as a trade-off to keep in mind. I guess ideally we'd wrap these pointers in a C++ template that wouldn't let you dereference them, but that might be overkill.)

Comment 6

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8992852 [details] [diff] [review]
Bug 1476500 - Fix some prose in the MacroAssembler class comment

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

Thanks for fixing my typos :/

Comment 7

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8992853 [details] [diff] [review]
Bug 1476500 - Add extra assignments and make some types more specific

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

Adding more type is a good thing. However, I am not sure about having assignments versus having a templated AbsoluteAddress constructor.
I guess this is a first step towards having a (phantom-)typed MacroAssembler, starting with AbsoluteAddress / Address / ImmPtr.

Comment 8

[Paul Bone [:pbone]]

(In reply to Jan de Mooij [:jandem] from comment #5)
> (In reply to Jan de Mooij [:jandem] from comment #4)
> > One reason for returning const void* from CompileWrappers, for instance
> > addressOfRandomNumberGenerator, is that this code can be called off-thread
> > so we want to give the caller as little as possible to work with (read: to
> > introduce races etc).
> 
> (That said, better type safety may be worth it; I just wanted to mention
> this as a trade-off to keep in mind. I guess ideally we'd wrap these
> pointers in a C++ template that wouldn't let you dereference them, but that
> might be overkill.)

I figured there would be such trade-offs, but I didn't know exactly which types they would apply to.

Alternatively I can create a parent class of the random number generator class that contains only the parts required to do code generation and return a pointer to that for addressOfRandomNumberGenerator.

Comment 9

[Paul Bone [:pbone]]

(In reply to Nicolas B. Pierron [:nbp] {backlog: 39} from comment #7)
> Comment on attachment 8992853 [details] [diff] [review]
> Bug 1476500 - Add extra assignments and make some types more specific
> 
> Review of attachment 8992853 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Adding more type is a good thing. However, I am not sure about having
> assignments versus having a templated AbsoluteAddress constructor.
> I guess this is a first step towards having a (phantom-)typed
> MacroAssembler, starting with AbsoluteAddress / Address / ImmPtr.

Here's where my C++ is a little weak.  WOuld the type be visible in the constructor call?  Eg:

    movPtr(AbsoluteAddress<uint32_t*>(addressOfX()), temp);

Comment 10

[Ted Campbell [:tcampbell]]

Comment on attachment 8992852 [details] [diff] [review]
Bug 1476500 - Fix some prose in the MacroAssembler class comment

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

There's a pre-existing stale comment here that you can help fix :)

::: js/src/jit/MacroAssembler.h
@@ +60,4 @@
>  // - If the declaration is "inline", then the method definition(s) would be in
>  //   the "-inl.h" variant of the same file(s).
>  //
> +// The script check_macroassembler_style.py (check-masm target of the

|check-masm| target was removed in https://searchfox.org/mozilla-central/commit/d04734856692be4604a15e5ae2363f058b006927 and runs on every build now. 

> (run on every build)

Comment 11

[Paul Bone [:pbone]]

Attachment: Bug 1476500 - Make the JIT's use of the XorShift128PlusRNG is type-correct

Comment 12

[Jan de Mooij [:jandem]]

Comment on attachment 8993212 [details] [diff] [review]
Bug 1476500 - Make the JIT's use of the XorShift128PlusRNG is type-correct

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

Thanks for writing the patch!

However this makes the XorShift128PlusRNG code in MFBT harder to read and I don't expect the Ion RNG JIT code to change much over time, so IMO it's fine to just use the XorShift128PlusRNG* pointer. If we're worried we could add an IonOffThreadPtr<XorShift128PlusRNG*> template that only let's you get the raw pointer out of it as void* or uint8_t*, but that's probably not worth it for this bug.

Comment 13

[Paul Bone [:pbone]]

(In reply to Jan de Mooij [:jandem] from comment #12)
> Comment on attachment 8993212 [details] [diff] [review]
> Bug 1476500 - Make the JIT's use of the XorShift128PlusRNG is type-correct
> 
> Review of attachment 8993212 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Thanks for writing the patch!
> 
> However this makes the XorShift128PlusRNG code in MFBT harder to read and I
> don't expect the Ion RNG JIT code to change much over time, so IMO it's fine
> to just use the XorShift128PlusRNG* pointer. If we're worried we could add
> an IonOffThreadPtr<XorShift128PlusRNG*> template that only let's you get the
> raw pointer out of it as void* or uint8_t*, but that's probably not worth it
> for this bug.

Okay, I'll do it the simple way.

However it's not to protect against the Ion RNG JIT code from changing, but from the underlying representation of the RNG from changing.

Comment 14

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8992854 [details] [diff] [review]
Bug 1476500 - Add a comment explaining why we need this value

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

::: js/src/jit/CompileWrappers.cpp
@@ +206,5 @@
>  CompileZone::addressOfStringNurseryCurrentEnd()
>  {
> +    // Unlike addressOfStringNurseryPosition() this is defined.  Because unlike
> +    // addressOfNurseryCurrentEnd() this will be null if nursery strings are
> +    // disabled.

The wording is a little confusing. What does "defined" mean here? And is the return value of this function null (no) or the value pointed to (yes)?

Oh! I guess the point here is that this is the address of a pointer that is usually the same as that pointed to by addressOfNurseryCurrentEnd(), and justifying that it exists because it can be nullptr if strings are disabled. Ok, that's valuable, but I think it needs to be rephrased to be less confusing.

Though the real reason why this exists is because I started but did not finish creating a separate nursery for strings, so the (eventual) intent is for this to point to somewhere entirely different than the other one. I don't know if that's worth describing before it exists, or not.

You are right that this function desperately needs a comment!

Comment 15

[Paul Bone [:pbone]]

sfink and I discussed this on IRC just now. he said I can write a suitable comment and land that last patch with r=sfink.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/6db05e2cc96f
Fix some prose in the MacroAssembler class comment r=tcampbell,nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/47be709bc6ca
Add extra assignments and make some types more specific r=nbp
https://hg.mozilla.org/integration/mozilla-inbound/rev/77027e772894
Add a comment explaining why we need this value r=sfink

Comment 17

[Tiberius Oros[:tiberius_oros]]

https://hg.mozilla.org/mozilla-central/rev/6db05e2cc96f
https://hg.mozilla.org/mozilla-central/rev/47be709bc6ca
https://hg.mozilla.org/mozilla-central/rev/77027e772894

Comment 18

[Steve Fink [:sfink] [:s:]]

(In reply to Paul Bone [:pbone] from comment #15)
> sfink and I discussed this on IRC just now. he said I can write a suitable
> comment and land that last patch with r=sfink.

I really like the comment you ended up with. Thanks! (It's much more clear than anything I was coming up with.)

Comment 19

[Paul Bone [:pbone]]

(In reply to Steve Fink [:sfink] [:s:] from comment #18)
> (In reply to Paul Bone [:pbone] from comment #15)
> > sfink and I discussed this on IRC just now. he said I can write a suitable
> > comment and land that last patch with r=sfink.
> 
> I really like the comment you ended up with. Thanks! (It's much more clear
> than anything I was coming up with.)

Thanks.