581702 - nHints is uninitialized

Status: RESOLVED FIXED

(Core Graveyard :: Nanojit, defect, P1)

Comment 1

[Edwin Smith]

When importing bug 574969 into TR, a crash showed up that appeared to be caused by the LIR opcode shuffling from introducing LIR_dasq and qasd.  However, chasing the crash led to finding that Assembler::nHints[] is not initialized.  Random values caused subtle changes in register allocation, which caused the crash.

Initializing nHints to zeros (before backends fill in specific hints) fixes the problem;  after that, generated x64 code is identical with different LIR opcode encodings.

One would not expect bad hints to cause crashes, so there might be a subtle bug in the register allocator.  It is possible that entries in nHints might randomly == PREFER_SPECIAL, and call the backend nHint() function, but I still wouldn't expect any evil, just more spillage.

I also noticed this, in assembler.cpp:

    // Per-opcode register hint table.  Default to no hints for all
    // instructions.  It's not marked const because individual back-ends can
    // install hint values for opcodes of interest in nInit().
    RegisterMask hints[LIR_sentinel+1] = {

note spelling: "hints" vs "nHints".  The table is dead code.  If it were spelled nHints[], then the code in various backends would not be initializing this static table because of the nHints declaration in Assembler.h.

I'm not sure what the intention was:

a) should nHints[] be static, defaulting to 0s, and mutated once, each time Assembler() is constructed?   If so, maybe we should also staticly set a flag so we only fill in the table once.  (TR instantiates Assembler once per method).

b) nHints should be stack allocated, the static-non-const table should be removed, and we need to memset the table each time.

(Shame on me not spotting this during review!)

Comment 2

[Edwin Smith]

Attachment: Patch for option (b) - memset nHints[] to zero

This was my first try at fixing the bug.  If this lands, we should also delete hints[] in Assembler.cpp.

Comment 3

[Edwin Smith]

Attachment: incomplete patch for option (a)

Sketching out the other way to do it.  Other backends would need tweaking to skip init the 2nd time.  Come to think of it, the couple extra stores are probably irrelevant.

Anyway - we should land one of these to avoid reading uninitialized memory, but i'm more worried about the implied regalloc bug.  Not even sure how to test it, maybe try fuzzing with random nHints tables for a while?

Comment 4

[Nicholas Nethercote [inactive]]

(In reply to comment #1)
>
> One would not expect bad hints to cause crashes, so there might be a subtle bug
> in the register allocator.  It is possible that entries in nHints might
> randomly == PREFER_SPECIAL, and call the backend nHint() function, but I still
> wouldn't expect any evil, just more spillage.

Or some assertion failures.

> I also noticed this, in assembler.cpp:
> 
>     // Per-opcode register hint table.  Default to no hints for all
>     // instructions.  It's not marked const because individual back-ends can
>     // install hint values for opcodes of interest in nInit().
>     RegisterMask hints[LIR_sentinel+1] = {
> 
> note spelling: "hints" vs "nHints".  The table is dead code.

Yeargh!  My bad.

> If it were
> spelled nHints[], then the code in various backends would not be initializing
> this static table because of the nHints declaration in Assembler.h.

Oh geez.
 
> I'm not sure what the intention was:
> 
> a) should nHints[] be static, defaulting to 0s, and mutated once, each time
> Assembler() is constructed?   If so, maybe we should also staticly set a flag
> so we only fill in the table once.  (TR instantiates Assembler once per
> method).

That was the intention.  As for the backends filling in a handful of entries, it would be better if it only happened once but I couldn't see how to do that, and I thought that cost seemed insignificant.

> (Shame on me not spotting this during review!)

Shame on me for writing the wretched code.  Thanks for working this out.

Comment 5

[Nicholas Nethercote [inactive]]

Comment on attachment 460059 [details] [diff] [review]
incomplete patch for option (a)

The hints/nHints change is good.  So what was happening before... the hints were all completely bogus?  All zero?  Yikes.

I'm not sure if the test in nInit() is necessary -- is this code run enough for it to matter?  I'll let you decide.  If so, a short comment would be good, and the other backends should be modified accordingly.

Comment 6

[Nicholas Nethercote [inactive]]

(In reply to comment #5)
> So what was happening before... the hints
> were all completely bogus?  All zero?  Yikes.

Here's what was happening:  nHints[] was zeroed because the allocators used with NJ tend to zero memory, at least, on TM they do, because it ends up being allocated with calloc().  TR may be different.

And then nHints had some of its entries set as expected by nInit().  Meanwhile, hints[] was explicitly zeroed but never used.  So the behaviour was as expected, at least for TM.

This makes me think we shouldn't zero by default;  Valgrind would have caught this problem if we didn't.  Graydon, I have a vague memory that TM uses calloc() because it somehow gave better performance -- do you remember if that's right?

You're right that even if the hints were random, it shouldn't cause crashes.  The hint should always be ignored if it's inappropriate (eg. the register(s) isn't available), see Assembler::registerAlloc().

Comment 7

[Edwin Smith]

Comment on attachment 460059 [details] [diff] [review]
incomplete patch for option (a)

I'll remove the if-first-time logic and land this; I agree a few unconditional static stores are no biggie.

Comment 8

[Edwin Smith]

NJ: http://hg.mozilla.org/projects/nanojit-central/rev/d8dccd548db1
TR: http://hg.mozilla.org/tamarin-redux/rev/927e7ad60a2e

Comment 9

[Nicholas Nethercote [inactive]]

http://hg.mozilla.org/tracemonkey/rev/ab3e0cc11532

Comment 10

[Robert Sayre]

http://hg.mozilla.org/mozilla-central/rev/ab3e0cc11532