1434539 - Register() should not create a garbage register, but an invalid register

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement, P3)

Description

[Lars T Hansen [:lth]]

Register() now creates a Register holding garbage; this is because Register is held in unions.  But those unions could hold Register::Code instead, and use Register::FromCode to construct Register values on dereference.  Doing so would improve our error checking.

See bug 1320353 for a similar change for AnyRegister.

Comment 1

[Lars T Hansen [:lth]]

Marking as good-first-bug though in truth it may be a good-second-bug.

Comment 2

[Daniel Almeida]

Please assign to me if you're willing to mentor as I'm new.

Comment 3

[Daniel Almeida]

Attachment: bug-1434539-01.patch

Please check this first try.

Comment 4

[Tooru Fujisawa [:arai]]

Comment on attachment 8964146 [details] [diff] [review]
bug-1434539-01.patch

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

Thank you for your patch!
the change looks fine :)
forwarding to lth.

Comment 5

[Lars T Hansen [:lth]]

Comment on attachment 8964146 [details] [diff] [review]
bug-1434539-01.patch

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

Not quite as simple as this :)  ValueOperand is a complicated beast with different 32-bit and 64-bit definitions, and that complexity needs to be reflected when TypedOrValueRegister uses it.

On 32-bit, ValueOperand has two Register fields type_ and payload_, and we need both when constructing a ValueOperand.  So likely the union in TypedOrValueRegister contains a field that is just a Register::Code on 64-bit systems but might be a struct containing two Register::Code fields on 32-bit systems.

::: js/src/jit/RegisterSets.h
@@ +191,4 @@
>      MOZ_IMPLICIT TypedOrValueRegister(ValueOperand value)
>        : type_(MIRType::Value)
>      {
> +        data.value = value.valueReg().code();

ValueOperand::valueReg() is only defined on 64-bit platforms (it's under an #ifdef JS_PUNBOX64).  So this needs a different solution for 32-bit platforms.

@@ +213,4 @@
>  
>      ValueOperand valueReg() const {
>          MOZ_ASSERT(hasValue());
> +        return ValueOperand{Register::FromCode(this->data.value)};

I'm surprised this compiles, because ValueOperand has constructors and private fields, and TypedOrValueRegister should not be allowed to construct one like this.  In any case the appropriate solution would be to use one of the defined constructors on ValueOperand.  And again, this is only appropriate on 64-bit systems, since on 32-bit systems ValueOperand has two fields.

Comment 6

[Daniel Almeida]

Attachment: 0001-Register-should-not-create-a-garbage-register-but-an.patch

Hello!

I hadn't realized that! Please check whether this new solution is acceptable.

Comment 7

[Daniel Almeida]

Attachment: 0001-Register-should-not-create-a-garbage-register-but-an.patch

Comment 8

[Daniel Almeida]

Attachment: 0001-Register-should-not-create-a-garbage-register-but-an.patch

Comment 9

[Lars T Hansen [:lth]]

Comment on attachment 8968285 [details] [diff] [review]
0001-Register-should-not-create-a-garbage-register-but-an.patch

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

This code does not compile on 32-bit systems because there are reference to "value" and "TypeReg" that are not resolved.  I suggest you fix those problems and the formatting issues I've pointed out below, and then we'll do another round.

I'm not 100% sure that I like having these ifdefs in TypedOrValueRegister, it might instead be better to create a new type called, say, ValueOperandWithCodes, to encapsulate that, but as I'm not sure what I think about that I suggest we leave that issue for the next round.

I know 32-bit builds can be a little tricky to test since everyone has a 64-bit build system these days.  If you're on Mac or Linux you can usually select a 32-bit compilation by setting some environment variables:

export HOST_CC='gcc'
export HOSTCFLAGS='-mfpmath=sse -msse -msse2'
export HOST_CXX='g++'
export HOSTCXXFLAGS='-mfpmath=sse -msse -msse2'

and then configure the build with eg --target=i386-pc-linux-gnu in addition to the options you would normally use.  (I think if you're on Windows and you're using the mozilla build infrastructure there are also possibilities but I'll have to investigate; let me know.)

::: js/src/jit/RegisterSets.h
@@ +175,4 @@
>  
>      union U {
>          AnyRegister::Code typed;
> +        #if defined JS_PUNBOX64

#if etc are always aligned to column 1

@@ +180,5 @@
> +        #elif defined JS_NUNBOX32
> +        Register::Code _type;
> +        Register::Code _payload;
> +        #else
> +        #error "Bad architecture"

And then when you have nested directives like here the # is in column 1 and there are spaces to show nesting, eg, "# error "Bad architecture".

@@ +182,5 @@
> +        Register::Code _payload;
> +        #else
> +        #error "Bad architecture"
> +        #endif
> +       

Remove this blank line and in general most blank lines you've added below.

@@ +201,5 @@
>      {
> +        #if defined JS_PUNBOX64
> +        data.value = value.valueReg().code();
> +        #elif defined JS_NUNBOX32
> +        data.value._type = value.TypeReg().code();

This should probably just be data._type?  Ditto next line.  Also, since there's no 'value' field the right hand side won't work.

::: js/src/jit/Registers.h
@@ +40,5 @@
>       : reg_(e)
>      { }
> +
> +    Register()
> +     : reg_(Codes::Invalid)

Should probably be reg_(Encoding(Codes::Invalid)), compare with the code in the Invalid() method.

Comment 10

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 11

[Garima Mazumdar]

Hi, I'm Garima, an Outreachy participant, and I'd like to work on this bug :)
Could someone guide me on how the start? From the above comments, I'm still kinda confused about ValueOperand.

Comment 12

[Ashita]

@Garima, are you working on it or should i do it?Since i got the approach to do

Comment 13

[Iain Ireland [:iain]]

(This has been sorted out in #spidermonkey; setting assignee accordingly)

Comment 14

[Garima Mazumdar]

Attachment: Bug 1434539 - Register() should not create a garbage register, but an invalid register. r?arai

Comment 15

[BugBot [:suhaib / :marco/ :calixte]]

This good-first-bug hasn't had any activity for 2 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Comment 16

[lukas.bernhard]

Attachment: Bug 1434539 - Register() should not create a garbage register, but an invalid register. r?arai

Comment 17

[Tooru Fujisawa [:arai]]

SM(cgc) job in try run with the above patch hits the following

https://treeherder.mozilla.org/jobs?repo=try&revision=82bd77a22f71bea97706a1a96d0970f9e1419200&group_state=expanded&selectedTaskRun=El3JxBIASKOYPQgKRMItcQ.0

TEST-UNEXPECTED-FAIL | js/src/jit-test/tests/gc/weakmap-nursery-value.js | /builds/worker/checkouts/gecko/js/src/jit-test/tests/gc/weakmap-nursery-value.js:18:8 Error: Incremental GC already in progress (code 3, args "") [0.2 s]

(for PROCESS-CRASH in the log, it's pre-existing, and I've filed bug 1718819)

:jonco, can you take a look what's going on there?

Comment 18

[Tooru Fujisawa [:arai]]

the failure seems to be also pre-existing
filed bug 1718823

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b0a4d29a5351
Register() should not create a garbage register, but an invalid register. r=arai

Comment 20

[Atila Butkovits]

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