1020834 - IonMonkey: (ARM) Correct some poorly handled pool placement cases and improve test coverage for these issues.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Description

[Douglas Crosher [:dougc]]

For the ARM assembly, constant pools can be dumped between instructions unless inhibited. This can cause code references to point to the pool rather than the expected instruction and for code sequences to have a varying length.

This bug adds support to emit a given number of distinctive NOP instructions at all locations at which a pool might have been dumped, and this alone can help catch many issues. This test can be enabled via the a JS shell command line argument or an environment variable.

This testing reveals an number of issues and these are patched here.

Comment 2

[Douglas Crosher [:dougc]]

Attachment: Correct some poorly handled pool placement cases and improve test coverage for these issues.

The NOP fill is enabled via the js shell argument --arm-asm-nop-fill=<num> or by the environment variable ARM_ASM_NOP_FILL.

The function skipPool() has been added and is used when creating an InstructionIterator, and called directly in some instances. This code has been reworked to skip all the fill-NOPs and artificial pool guards.

A markAsBranch argument has been added to insertEntry() so that the branch positions are noted accurately. The function writeBranchInst() has been added, like writeInst() but it sets markAsBranch.

There were issues with the bailout table, and with the toggled calls, which have been addressed.

It passes jit-tests locally. Shall do some more testing, and make sure non-debug builds compile etc.

Comment 3

[Douglas Crosher [:dougc]]

Would this test help with testing, and would it be usable?

Comment 4

[Douglas Crosher [:dougc]]

https://tbpl.mozilla.org/?tree=Try&rev=da593e460ef3

Comment 5

[Christian Holler (:decoder)]

I think this would help testing :) You just have to let us know what value to use for <num> typically.

Comment 6

[Douglas Crosher [:dougc]]

Attachment: Correct some poorly handled pool placement cases and improve test coverage for these issues.

Rebased. This was all split out from the asm buffer simplification patch. These are fixes that stand on their own. They will be needed for a solid 31 ESR and block the asm buffer simplification patch.

Comment 7

[Douglas Crosher [:dougc]]

Attachment: Correct some poorly handled pool placement cases and improve test coverage for these issues.

Backport for beta (31).

Comment 8

[Jan de Mooij [:jandem]]

Comment on attachment 8441165 [details] [diff] [review]
Correct some poorly handled pool placement cases and improve test coverage for these issues.

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

Thanks for adding this; when it lands we should inform the fuzzing people so they can use this flag. Some minor comments below.

::: js/src/jit/arm/Assembler-arm.cpp
@@ +1853,3 @@
>      if (l->bound()) {
> +        // Note only one instruction is emitted here, the NOP is overwritten.
> +        BufferOffset ret = writeBranchInst(0xe320f000); // NOP

Can we use InstNOP here to avoid hardcoding this constant in multiple places?

@@ +1914,3 @@
>      if (l->bound()) {
> +        // Note only one instruction is emitted here, the NOP is overwritten.
> +        BufferOffset ret = writeBranchInst(0xe320f000); // NOP

Same here.

@@ +2812,5 @@
> +
> +uint32_t Assembler::nopFill = 0;
> +
> +uint32_t
> +Assembler::getNopFill() {

This method can also be static right?

::: js/src/jit/arm/Assembler-arm.h
@@ +1235,5 @@
>      uint32_t actualOffset(uint32_t) const;
>      uint32_t actualIndex(uint32_t) const;
>      static uint8_t *PatchableJumpAddress(JitCode *code, uint32_t index);
>      BufferOffset actualOffset(BufferOffset) const;
> +    static uint32_t nopFill;

Nit: it's static so NopFill

::: js/src/jit/arm/CodeGenerator-arm.cpp
@@ +213,5 @@
>      JS_ASSERT_IF(frameClass_ != FrameSizeClass::None(),
>                   frameClass_.frameSize() == masm.framePushed());
>  
>      if (assignBailoutId(snapshot)) {
> +        uint8_t *code = masm.BailoutTableStart(deoptTable_->raw()) + snapshot->bailoutId() * BAILOUT_TABLE_ENTRY_SIZE;

Nit: (Macro)Assembler::BailoutTableStart?

::: js/src/jit/shared/IonAssemblerBufferWithConstantPools.h
@@ +466,5 @@
>              }
>          }
>      }
>  
> +    void InsertNopFill() {

Nit: insertNopFill as it's a non-static member.

Comment 9

[Douglas Crosher [:dougc]]

Attachment: Correct some poorly handled pool placement cases and improve test coverage for these issues.

Address reviewer feedback. Thank you for the quick feedback.

Read through the style guide and looked for other issue. Sorry, it is still not 100% clear when to capitalize the first letter of names.

Comment 10

[Douglas Crosher [:dougc]]

Attachment: Correct some poorly handled pool placement cases and improve test coverage for these issues.

Some rebasing and some small clean up.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8445806 [details] [diff] [review]
Correct some poorly handled pool placement cases and improve test coverage for these issues.

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

Sorry for the delay. Just some style nits; most of them pre-existing, that makes it harder to pick it up so don't worry too much about it :)

(In reply to Douglas Crosher [:dougc] from comment #9)
> Read through the style guide and looked for other issue. Sorry, it is still
> not 100% clear when to capitalize the first letter of names.

Functions that are not class/struct members are always capitalized. Same for static class/struct members.

::: js/src/jit/arm/Assembler-arm.cpp
@@ +2803,5 @@
>      // NOTE: we don't update the Auto Flush Cache!  this function is currently only called from
>      // within AsmJSModule::patchHeapAccesses, which does that for us.  Don't call this!
>  }
>  
>  InstructionIterator::InstructionIterator(Instruction *i_) : i(i_) {

Nit: { on its own line.

@@ +2812,5 @@
> +
> +uint32_t Assembler::NopFill = 0;
> +
> +uint32_t
> +Assembler::getNopFill() {

Same here.

::: js/src/jit/arm/Assembler-arm.h
@@ +1236,5 @@
>      uint32_t actualIndex(uint32_t) const;
>      static uint8_t *PatchableJumpAddress(JitCode *code, uint32_t index);
>      BufferOffset actualOffset(BufferOffset) const;
> +    static uint32_t NopFill;
> +    static uint32_t getNopFill();

Nit: we capitalize the names of static methods: GetNopFill. At least in jit/*, code outside jit/* doesn't always do this...

@@ +1391,5 @@
>      // instruction gets written into the instruction stream. If dest is not null
>      // it is interpreted as a pointer to the location that we want the
>      // instruction to be written.
>      BufferOffset writeInst(uint32_t x, uint32_t *dest = nullptr);
> +    // As above, but also mark the instruction as a branch.

Nit: blank line before this comment.

@@ +1397,1 @@
>      // A static variant for the cases where we don't want to have an assembler

And here.

Comment 12

[Douglas Crosher [:dougc]]

Attachment: Correct some poorly handled pool placement cases and improve test coverage for these issues.

Address reviewer feedback, thank you.

Comment 13

[Douglas Crosher [:dougc]]

Carrying forward the r+ above.

Comment 14

[Till Schneidereit [:till]]

You'll need a try server link in the bug for the sheriffs to check a patch in for you. This was announced on dev.platform a while ago: https://groups.google.com/forum/#!searchin/mozilla.dev.platform/checkin-needed/mozilla.dev.platform/9w0-Bh_3vVI/xWebYK64GdwJ

Comment 15

[Douglas Crosher [:dougc]]

Attachment: Correct some poorly handled pool placement cases and improve test coverage for these issues.

Rebase. Carrying forward r+.
https://tbpl.mozilla.org/?tree=Try&rev=5942dabfb4c3

Comment 16

[Douglas Crosher [:dougc]]

Attachment: Backport to Aurora 32.

https://tbpl.mozilla.org/?tree=Try&rev=4facd2560028

Comment 17

[Douglas Crosher [:dougc]]

Attachment: Backport for Beta 31.

https://tbpl.mozilla.org/?tree=Try&rev=fc7e91883a6e

Comment 18

[Douglas Crosher [:dougc]]

(In reply to Till Schneidereit [:till] from comment #14)
> You'll need a try server link in the bug for the sheriffs to check a patch
> in for you. This was announced on dev.platform a while ago:
> https://groups.google.com/forum/#!searchin/mozilla.dev.platform/checkin-
> needed/mozilla.dev.platform/9w0-Bh_3vVI/xWebYK64GdwJ

Thank you, I had not seen this.

Comment 19

[Douglas Crosher [:dougc]]

Comment on attachment 8447074 [details] [diff] [review]
Backport to Aurora 32.

Approval Request Comment
[Feature/regressing bug #]: Long standing issues.

[User impact if declined]: Crashes, bad computation, maybe exploitable vulnerabilities. Asm.js code seems to tickle these issues, and they are more problematic on ARMv6 hardware, making this a show stopper for asm.js on ARMv6 devices.

[Describe test coverage new/current, TBPL]: This has been fuzz tested in bug 1026919, wrt m-c, with no reported issues. Tested locally on the Aurora 32 branch using the tbpl jit-tests and it passes all these tests for ARMv7 code generation, and it reduces the number of failures from 22 to 6 for ARMv6 code generation (the remaining failures are addressed in bug 1026919). Note that the moz tbpl testing of the ARMv6 Android builds tests on ARMv7 hardware so the JIT compiler is generation ARMv7 code, so these failures have been hidden! Tested on a range of large asm.js demos.

[Risks and why]: There is some risk, there is some complexity here, and this is a low level change. Fwiw the changes are isolated to the ARM backend. I ask you weight this up against the risks associated with the bugs this fixes.

[String/UUID change made/needed]: n/a

Comment 20

[Douglas Crosher [:dougc]]

Comment on attachment 8447075 [details] [diff] [review]
Backport for Beta 31.

Approval Request Comment
[Feature/regressing bug #]: Long standing issues.

[User impact if declined]: Crashes, bad computation, maybe exploitable vulnerabilities. Asm.js code seems to tickle these issues, and they are more problematic on ARMv6 hardware, making this a show stopper for asm.js on ARMv6 devices.

[Describe test coverage new/current, TBPL]: This has been fuzz tested in bug 1026919, wrt m-c, with no reported issues. Tested locally on the Beta 31 branch using the tbpl jit-tests and it passes all these tests for ARMv7 code generation, and it reduces the number of failures from 23 to 7 for ARMv6 code generation. Tested on a range of large asm.js demos.

[Risks and why]: There is some risk, there is some complexity here, and this is a low level change. Fwiw the changes are isolated to the ARM backend. I ask you weight this up against the risks associated with the bugs this fixes.

[String/UUID change made/needed]: n/a

Comment 21

[Douglas Crosher [:dougc]]

Repeating some try builds that had a little more orange than expected:
m-c: https://tbpl.mozilla.org/?tree=Try&rev=19234d62b4f6
Beta: https://tbpl.mozilla.org/?tree=Try&rev=403c695196d9

Comment 22

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8447075 [details] [diff] [review]
Backport for Beta 31.

Given that this bug has been in Firefox for a while and has risk, I prefer to let ride the 32 train.

Comment 23

[Douglas Crosher [:dougc]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #22)
> Comment on attachment 8447075 [details] [diff] [review]
> Backport for Beta 31.
> 
> Given that this bug has been in Firefox for a while and has risk, I prefer
> to let ride the 32 train.

I respect your decision. Hopefully many people will upgrade to 32 soon anyway, and as you note this is a long standing issue. Such a fresh change also has it's own unknown risks and some more testing on 32 and 33 would help reduce this risk. Deferring the 31 release would leave people using the un-patched code anyway.

However this decision will block bug 1026919 so 31 will have a somewhat fragile and potentially insecure JIT compiler backend.

ESR 31 will have a long ride to an upgrade so will it be possible in future to request approval to uplift to ESR 31?  If not then are there any other processes I should be aware of for informing ESR builders and making patches available?

Comment 24

[Sylvestre Ledru [:Sylvestre]]

OK. With the ESR in mind and bug 1026919, I could reconsider this choice. 
However, I don't see a reviewed patch for bug 1026919 and the current patch seems quite big (and too big to be approved so late in the 31 cycle).
I propose that we skip it for 31. We approve both of them asap in 32 and we discuss about a potential approval in ESR 31.1.0 or 31.2.0.
Sounds good?

Comment 25

[Douglas Crosher [:dougc]]

(In reply to Sylvestre Ledru [:sylvestre] from comment #24)
> OK. With the ESR in mind and bug 1026919, I could reconsider this choice. 
> However, I don't see a reviewed patch for bug 1026919 and the current patch
> seems quite big (and too big to be approved so late in the 31 cycle).
> I propose that we skip it for 31. We approve both of them asap in 32 and we
> discuss about a potential approval in ESR 31.1.0 or 31.2.0.
> Sounds good?

Yes, thank you. I'll work towards this.

Comment 27

[Ryan VanderMeulen [:RyanVM]]

Wait, when did this land on m-c?

Comment 28

[Douglas Crosher [:dougc]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #27)
> Wait, when did this land on m-c?

This has not landed on m-c. If it could be checked in when the ARM builds on m-c are stable it would be appreciated.

Comment 29

[Ryan VanderMeulen [:RyanVM]]

Is there any reason to land it on Aurora prior to that?

Comment 30

[Douglas Crosher [:dougc]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #29)
> Is there any reason to land it on Aurora prior to that?

No, it does not seem that urgent. I defer to your judgement. It will need testing on Aurora soon, and it is unlikely that testing on m-c will show any problems about the other noise there, but it would be prudent to land on m-c first.

I brought forward the Aurora and Beta approval requests because we are playing catchup and these requests may have needed some discussion.

Comment 31

[Ryan VanderMeulen [:RyanVM]]

(In reply to Douglas Crosher [:dougc] from comment #28)
> This has not landed on m-c. If it could be checked in when the ARM builds on
> m-c are stable it would be appreciated.

I'm not sure exactly what you're referring to here. Please just put checkin-needed on the bug if you feel it's ready to land on m-c. The Try run looks green enough IMO.

Comment 32

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f1bacafe789c

Comment 33

[Ryan VanderMeulen [:RyanVM]]

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

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/261407ec0ea7

Leaving status-firefox31 set to affected because there aren't esr31 flags to set yet and we're still tracking this bug for that.