1208674 - ARM64: BaselineScript::toggleDebugTraps() is clobbering unrelated instructions

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Jakob Stoklund Olesen [:jolesen]]

1. Build SpiderMonkey with --enable-simulator=arm64
2. js --baseline-eager js/src/jit-test/tests/debug/Debugger-debuggees-28.js

Unallocated instruction at 0x101e6c350: 0xffff000c
Redirecting call to abort() to mozalloc_abort

Hit MOZ_CRASH() at /Users/jolesen/gecko-dev/memory/mozalloc/mozalloc_abort.cpp:33
Segmentation fault: 11

Comment 1

[Jakob Stoklund Olesen [:jolesen]]

Initial analysis:

The unallocated instruction 0xffff000c is a constant pool header.
When the constant pool was emitted, a guard branch was inserted before the pool at 0x101e6c34c.
The guard branch gets overwritten by Assembler::ToggleCall() as called by BaselineScript::toggleDebugTraps().

I suspect that PCMappingIndexEntry has incorrect nativeOffset entries. This could be related to constant pools that are inserted by executableCopy().

See bug 1207827 - Simplify AssemblerBufferWithConstantPools for ARM and ARM64

Comment 2

[Jakob Stoklund Olesen [:jolesen]]

The problem is related to constant pools, but not the rewriting of buffer offsets as I initially thought:

MacroAssemblerCompat::toggledCall() inserts a patchable call instruction sequence that can be turned on and off by Assembler::ToggleCall(). The enabled call sequence is:

  ldr scratchreg, pool_imm64
  blr scratchreg

The ldr instruction is a pc-relative load of a 64-bit constant pool entry created by toggledCall(). The disabled sequence is:

  adr scratchreg, pool_imm64
  nop

The two instructions may be preceded by a stack pointer synchronizing instruction: "mov sp, r28".

The toggledCall() function is trying to prevent the emitted instructions from being separated by a constant pool by calling armbuffer_.flushPool() first. At the same time, ToggleCall() attempts to handle an inserted constant pool correctly by using the NextInstruction() function which understands how to skip over a guard branch and constant pool.

Unfortunately, both mechanisms fail. This is the code generated by toggledCall():

returned_offset:
  b after_pool ; guard branch
  .data 0xffff000c ; pool header, length=12 words incl header.
  .data poolword1, ..., poolword11
after_pool:
  ldr scratchreg, pool_imm64
  blr scratchreg

The two mechanisms for handling constant pools fail like this:

1. The buffer offset returned from toggledCall() is the label returned_offset, not the label after_pool where the call instruction sequence lives.
2. Assembler::ToggleCall() can deal with an injected constant pool, except if it is placed at the first word passed. Which is the case here.

The first problem is explained:

    CodeOffsetLabel toggledCall(JitCode* target, bool enabled) {
        // The returned offset must be to the first instruction generated,
        // for the debugger to match offset with Baseline's pcMappingEntries_.
        BufferOffset offset = nextOffset();

It would be useful if toggledOffset() were allowed to emit some code before the returned code offset. Then ToggleCall wouldn't need to be able to skip both stack pointer synchronizers and constant pools.

Second:

        // TODO: Random pool insertion between instructions below is terrible.
        // Unfortunately, we can't forbid pool prevention, because we're trying
        // to add an entry to a pool. So as a temporary fix, just flush the pool
        // now, so that it won't add later. If you're changing this, also
        // check ToggleCall(), which will probably break.
        armbuffer_.flushPool();

It should be possible to prevent constant pool insertion between the two critical instruction, even if one of them requires adding a constant pool entry. As long as the no-pools region is shorter than the reach of the ldr instruction inserted, the pool can be emitted after the critical instructions.

OTOH, if ToggleCall() does support inserted constant pools, why prevent their insertion at all?

Third: AssemblerBufferWithConstantPools can be in a state where attempting to insert even a single instruction causes the current constant pool to be flushed first. This means that nextOffset() does not always return the offset that will be given to the next instruction inserted. Perhaps the pool should be flushed immediately if there isn't even room for one more instruction. The pool is going to be flushed at that point anyway. OTOH, the pool could be in a state where adding a pool entry would require the pool to be flushed first. If the next instruction added requires a pool entry, the pool may require flushing anyway.

It would be possible to flush the pool early such that it is always possible to allocate a single instruction and, say, a 64-bit pool entry without triggering an immediate pool flush. This would not even cause the constant pools to move compared to their current positions, except for very special circumstances. This would mean that nextOffset() becomes trustworthy as a pointer to the next added instruction.

Comment 3

[Jakob Stoklund Olesen [:jolesen]]

Two ways of fixing this problem:

1. Make Assembler::ToggleCall() more robust so it knows how to skip constant pools even at the very first instruction it is looking at. Stop flushing the constant pool in MacroAssemblerCompat::toggledCall().

2. Make MacroAssemblerCompat::currentOffset() force a constant pool flush if there isn't even room for emitting three instructions + 64 bits of pool entries. Assert in toggledCall() that no pools were flushed. Simplify Assembler::ToggleCall() which no longer needs to know about inserted constant pools.

I'm leaning towards solution (2) because the lies currently coming from armbuffer_.nextOffset() also caused problems over in bug 1205621, and the fix is similar: Add a nextInstrOffset() method which forces a pool flush if there is not room for the new instruction. These two approaches could be combined.

Comment 4

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Add ToggleCall assertions.

Assert that the ToggleCall() function does not overwrite any unexpected
instructions.

This does not fix this bug, but it exposes it earlier and more clearly. Rather
than crashing when trying to execute the garbled code clobbered by ToggleCall,
assert in ToggleCall when attempting to overwrite the wrong instructions.

Comment 5

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Fix ToggleCall to handle constant pools.

This is solution (1) above. Making ToggleCall() robust seems like a good idea
regardless. We can simplify the handling of constant pools as part of Bug
1207827.

Comment 6

[Jakob Stoklund Olesen [:jolesen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=21e05a0fbea7

Comment 7

[Sean Stangl [:sstangl]]

Comment on attachment 8668609 [details] [diff] [review]
Fix ToggleCall to handle constant pools.

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

::: js/src/jit/arm64/Assembler-arm64.cpp
@@ +389,5 @@
> +// This function does not skip constant pools with a natural guard branch. It is assumed that
> +// anyone inspecting the instruction stream understands about branches that were inserted
> +// naturally.
> +static const Instruction*
> +SkipConstantPool(const Instruction* instr)

The name sounds like it requires instr to be a constant pool guard. Maybe "SkipAnyConstantPool()"?

ARM does the same thing with Instruction::skipPool().

@@ +399,5 @@
> +    // Check for a constant pool header.
> +    // Bit 15 indicates a natural pool guard. It must be clear.
> +    uint32_t header = reinterpret_cast<const uint32_t*>(instr)[1];
> +    if ((header & 0xffff8000) != 0xffff0000)
> +        return instr;

If (instr->Bit(15))
    return instr;

I'm not sure what Bit 15 is. Is there some name in jit/arm64/vixl/Constants-vixl.h that could be given to it?

@@ +413,5 @@
>      Instruction* load;
>      Instruction* call;
>  
> +    // Skip the stack pointer restore instruction.
> +    if (first->InstructionBits() == 0x9100039f)

Oh boy! I realize you didn't introduce this, but that's a hell of a hardcoded constant. Could we instead construct that value using helpful terminology in Constants-vixl.h?

@@ +417,5 @@
> +    if (first->InstructionBits() == 0x9100039f)
> +        first = SkipConstantPool(first + 4);
> +
> +    load = const_cast<Instruction*>(first);
> +    call = const_cast<Instruction*>(SkipConstantPool(load + 4));

"kInstructionSize" instead of 4.

Comment 8

[Jakob Stoklund Olesen [:jolesen]]

(In reply to Sean Stangl [:sstangl] from comment #7)
> Comment on attachment 8668609 [details] [diff] [review]
> Fix ToggleCall to handle constant pools.
> 
> Review of attachment 8668609 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/jit/arm64/Assembler-arm64.cpp
> @@ +389,5 @@
> > +// This function does not skip constant pools with a natural guard branch. It is assumed that
> > +// anyone inspecting the instruction stream understands about branches that were inserted
> > +// naturally.
> > +static const Instruction*
> > +SkipConstantPool(const Instruction* instr)
> 
> The name sounds like it requires instr to be a constant pool guard. Maybe
> "SkipAnyConstantPool()"?
> 
> ARM does the same thing with Instruction::skipPool().

Thanks, I'll copy what ARM did.

> 
> @@ +399,5 @@
> > +    // Check for a constant pool header.
> > +    // Bit 15 indicates a natural pool guard. It must be clear.
> > +    uint32_t header = reinterpret_cast<const uint32_t*>(instr)[1];
> > +    if ((header & 0xffff8000) != 0xffff0000)
> > +        return instr;
> 
> If (instr->Bit(15))
>     return instr;
> 
> I'm not sure what Bit 15 is. Is there some name in
> jit/arm64/vixl/Constants-vixl.h that could be given to it?

Better yet, I'll implement InstIsGuard, like ARM does.
 
> @@ +413,5 @@
> >      Instruction* load;
> >      Instruction* call;
> >  
> > +    // Skip the stack pointer restore instruction.
> > +    if (first->InstructionBits() == 0x9100039f)
> 
> Oh boy! I realize you didn't introduce this, but that's a hell of a
> hardcoded constant. Could we instead construct that value using helpful
> terminology in Constants-vixl.h?

This is totally gross, and that hex constant even appears elsewhere too. I'll see if I can fix it.

> @@ +417,5 @@
> > +    if (first->InstructionBits() == 0x9100039f)
> > +        first = SkipConstantPool(first + 4);
> > +
> > +    load = const_cast<Instruction*>(first);
> > +    call = const_cast<Instruction*>(SkipConstantPool(load + 4));
> 
> "kInstructionSize" instead of 4.

Thanks!

Comment 9

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Fix ToggleCall to handle constant pools.

Handle constant pools inserted anywhere, even at the initial pointer
location. Don't attempt handling constant pools with natural guard
branches. They are not relevant here (and actually never generated
currently).

Stop flushing constant pools in toggledCall(). Since ToggleCall() does
handle constant pools at any point, there is no need to prevent their
insertion.

Replace the NextInstruction() functions with Instruction::skipPool()
which mimics the ARM implementation.

Add an Instruction::IsStackPtrSync() which recognizes the instructions
inserted by syncStackPtr().

Comment 10

[Sean Stangl [:sstangl]]

Comment on attachment 8670281 [details] [diff] [review]
Fix ToggleCall to handle constant pools.

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

::: js/src/jit/arm64/Assembler-arm64.cpp
@@ +399,5 @@
> +
> +    // The call instruction follows the load, but there may be an injected
> +    // constant pool.
> +    call = const_cast<Instruction*>(
> +      load->InstructionAtOffset(vixl::kInstructionSize)->skipPool());

nit: 4 space indentation.

Our column limit is 100; it looks like this should fit without needing wrap. If it does wrap, it would be better to break up "load->InstructionAtOffset(vixl::kInstructionSize)" onto a separate line, and give it a name.

Comment 11

[Jakob Stoklund Olesen [:jolesen]]

Attachment: Fix ToggleCall to handle constant pools.

Fix formatting nit and a build error caused by UNIFIED.

Comment 12

[Jakob Stoklund Olesen [:jolesen]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7e736aa3ddb1

Comment 13

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d6d3fcb7c0bc
https://hg.mozilla.org/integration/mozilla-inbound/rev/b541b2706161

Comment 14

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d6d3fcb7c0bc
https://hg.mozilla.org/mozilla-central/rev/b541b2706161