1320353 - AnyRegister::Invalid is an orphan / AnyRegister() constructs garbage, not an invalid value

Status: RESOLVED FIXED

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

Description

[Lars T Hansen [:lth]]

AnyRegister::Invalid is not used anywhere, and indeed it cannot be used: there is no accessor for it, unlike in Register and Register64, and the obvious workaround AnyRegister::FromCode(AnyRegister::Invalid) asserts because the Invalid code is not a valid code for this function.

Not sure what to do here.  I actually found myself needing an invalid AnyRegister but I've not had time to test the consequences of implementing it.

(Could one reliably use AnyRegister(Register::Invalid())?)

Comment 1

[Lars T Hansen [:lth]]

This bit me again.  It is possible to construct an AnyRegister that contains garbage by AnyRegister() but it can't be tested for validity.

Comment 2

[Lars T Hansen [:lth]]

... and that is because AnyRegister can be part of a union in TypedOrValueRegister and needs to have a default constructor that does nothing.  Sigh.

Comment 3

[Lars T Hansen [:lth]]

Attachment: bug1320353-anyregister-invalid.patch

What do you think of this?  It's a halfway solution - code that needs to signal that the AnyRegister is invalid can do so, but there's still an option to construct an AnyRegister containing garbage, so as not to have to change the code where an AnyRegister is in a union.

I have some cases in the ARM back-end where AnyRegister can be invalid and therefore wants to be created as one that is recognizably so (updates to that code are included in this patch); there's more code like that coming the ARM64 back-end.

Comment 4

[Jan de Mooij [:jandem]]

(In reply to Lars T Hansen [:lth] from comment #2)
> ... and that is because AnyRegister can be part of a union in
> TypedOrValueRegister and needs to have a default constructor that does
> nothing.  Sigh.

Can we store AnyRegister::Code in the union?

Comment 5

[Lars T Hansen [:lth]]

That's a good idea.  I'll experiment.  I don't know how many unions were affected, there could be more than the one.

Comment 6

[Lars T Hansen [:lth]]

Attachment: bug1320353-no-garbage-anyregister.patch

Alternative solution, probably the superior approach.  Only the one union needed to change.

Comment 7

[Jan de Mooij [:jandem]]

Comment on attachment 8946261 [details] [diff] [review]
bug1320353-no-garbage-anyregister.patch

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

Nice. Register has the same garbage-initialization I think; ideally we would use this pattern everywhere.

Comment 8

[Lars T Hansen [:lth]]

(In reply to Jan de Mooij [:jandem] from comment #7)

> Register has the same garbage-initialization I think; ideally we would
> use this pattern everywhere.

Filed as bug 1434539.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/593bad5d26d6
Avoid garbage AnyRegister. r=jandem

Comment 10

[Andreea Pavel [:apavel]]

https://hg.mozilla.org/mozilla-central/rev/593bad5d26d6