1391248 - MIPS32: Add missing instruction and other miscellaneous fixes for the simulator.

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, enhancement)

Description

[Dragan Mladjenovic]

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/51.0.2704.106 Safari/537.36

Comment 1

[Dragan Mladjenovic]

Attachment: bug1391248.patch

Moved form bug 1329650.
Added support for instructions needed for atomics and few others. Removed support for floating point instructions that consume or produce 64-bit integers. Since simulator implements only FR=0 mode (see bug 1329650) their usage should not be allowed. Added support for wasm unaligned access, interrupt support, new oom handling.

Comment 2

[Lars T Hansen [:lth]]

Nice!  I need a little time to review this - ie might be Monday before I get to it - but first impressions are good.

Comment 3

[Benjamin Bouvier [:bbouvier] (inactive)]

(Similarly, I'll take a look at this next week)

Comment 4

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8898300 [details] [diff] [review]
bug1391248.patch

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

It's hard to review without more context. Can you please change your mercurial patch context to something like 8, please? Here are the settings in my .hgrc file, generated from Mozilla recommendations:

[diff]
git = 1
unified = 8
showfunc = 1


I'd like to see a new version of this patch, because of a few things:
- this doesn't compile a debug mips32 build
- FramePointer still is not defined (see comment near handleWasmInterrupt)
- it'd be nicer to keep the crashing behavior in case of unaligned memory access
- some other minor questions and suggestions below

Thanks for the patch!

::: js/src/jit/mips32/Simulator-mips32.cpp
@@ +1270,4 @@
>      break_pc_ = nullptr;
>      break_instr_ = 0;
>  
> +

nit: please remove this line

@@ +1292,4 @@
>          exceptions[i] = 0;
>  
>      lastDebuggerInput_ = nullptr;
> +    lastLLValue_ = 0;

can you group this definition with the LLBIt_ / LLAddr_ too?

@@ +1365,1 @@
>          }

extra-nit: no {} around single-line if bodies

@@ +1652,5 @@
>  void
>  Simulator::handleWasmInterrupt()
>  {
> +    void* pc = (void*)get_pc();
> +    void* fp = (void*)getRegister(s8);

Pretty sure this is still incorrect: the argument passed to getRegister in the ARM simulator is the same register as the FramePointer (as defined in Assembler-arm.h). But for MIPS, FramePointer is defined in Assembler-mips-shared.h to InvalidPtr, so generating a wasm entry (which makes use of FramePointer) would crash. This will fail wasm jit-tests; have you checked if they passed or not with your patch applied?

@@ +1712,5 @@
> +// Note that simulator runs have the runtime system running directly on the host
> +// system and only generated code is executed in the simulator.
> +// Since the host is typically IA32 it will not trap on unaligned memory access.
> +// We assume that that executing correct generated code will not produce unaligned
> +// memory access, so we explicitly check for address alignment and trap.

Instead of assuming that the generated code is correct and does not trap, what do you think about keeping the previous code that crashed in this case? If this is not supposed to happen, crashing is a nice way to make sure this is the case.

@@ +1719,4 @@
>  int
>  Simulator::readW(uint32_t addr, SimInstruction* instr)
>  {
> +    if (handleWasmFault(addr,4)) {

nit: space after comma (there are a few other instances of this in the file)

@@ +1724,1 @@
>      }

nit: no braces around single-line if bodies (other instances in this file)

@@ +1940,5 @@
> +        MOZ_CRASH();
> +    }
> +
> +    if ((addr & kPointerAlignmentMask) == 0) {
> +        SharedMem<int32_t*> ptr = SharedMem<int32_t*>::shared(reinterpret_cast<int32_t*>(addr));

Can you move this after the if (!LLBit_) check?

@@ -2649,4 @@
>                  }
>                  break;
>                }
> -              case ff_cvt_l_fmt: {  // Mips32r2: Truncate float to 64-bit long-word.

Why are these instructions removed?

@@ -2798,4 @@
>                case ff_cvt_s_fmt:  // Convert double to float (single).
>                  setFpuRegisterFloat(fd_reg, static_cast<float>(ds_value));
>                  break;
> -              case ff_cvt_l_fmt: {  // Mips32r2: Truncate double to 64-bit long-word.

ditto?

::: js/src/jit/mips32/Simulator-mips32.h
@@ +343,5 @@
>      uint32_t FCSR_;
>  
> +    bool LLBit_;
> +    uint32_t LLAddr_;
> +    int32_t lastLLValue_;

How about the renaming suggestions I made on the other bug?

@@ +345,5 @@
> +    bool LLBit_;
> +    uint32_t LLAddr_;
> +    int32_t lastLLValue_;
> +
> +

nit: please remove one blank line

Comment 5

[Dragan Mladjenovic]

(In reply to Benjamin Bouvier [:bbouvier] from comment #4)

Thanks for the review.
I would like to ask a few more questions and hopefully provide some answers.

> @@ +1652,5 @@
> >  void
> >  Simulator::handleWasmInterrupt()
> >  {
> > +    void* pc = (void*)get_pc();
> > +    void* fp = (void*)getRegister(s8);
> 
> Pretty sure this is still incorrect: the argument passed to getRegister in
> the ARM simulator is the same register as the FramePointer (as defined in
> Assembler-arm.h). But for MIPS, FramePointer is defined in
> Assembler-mips-shared.h to InvalidPtr, so generating a wasm entry (which
> makes use of FramePointer) would crash. This will fail wasm jit-tests; have
> you checked if they passed or not with your patch applied?

Wasm jit-tests are not passing due to various issues, one of them FramePointer being defines as InvalidReg on mips32.
I've managed to fix up most of the tests failures excluding the debug wasm tests which looks to require wasm baseline tier (not implemented on MIPS for now). This patch is amalgam of fixes that needed to be done to simulator code to make all the tests pass. At the time being the change looks out of place because required bits of mips32 codegen are missing. I hope to be able to send more codegen patches during this week.  

> @@ +1712,5 @@
> > +// Note that simulator runs have the runtime system running directly on the host
> > +// system and only generated code is executed in the simulator.
> > +// Since the host is typically IA32 it will not trap on unaligned memory access.
> > +// We assume that that executing correct generated code will not produce unaligned
> > +// memory access, so we explicitly check for address alignment and trap.
> 
> Instead of assuming that the generated code is correct and does not trap,
> what do you think about keeping the previous code that crashed in this case?
> If this is not supposed to happen, crashing is a nice way to make sure this
> is the case.

Just to clarify tho old behavior is to always crass on unaligned access and the new one is to only crass if not executing wasm code. It was needed to make a large number of wasm tests pass. I took ARM's approach to allow unaligned access for wasm code because performance is not of high importance right now. At some time later I would like to iterate this part of the patch to maybe provide an option to always crash on unaligned access in order to help us to prune unaligned acess from wasm jited code if that is actually possible. 

> @@ -2649,4 @@
> >                  }
> >                  break;
> >                }
> > -              case ff_cvt_l_fmt: {  // Mips32r2: Truncate float to 64-bit long-word.
> 
> Why are these instructions removed?

These instructions in FR=0 mode produce undefined result and should not be used. I've found few places inside mips-shared code where we do. This is just to prevent any such further mistakes.

> ::: js/src/jit/mips32/Simulator-mips32.h
> @@ +343,5 @@
> >      uint32_t FCSR_;
> >  
> > +    bool LLBit_;
> > +    uint32_t LLAddr_;
> > +    int32_t lastLLValue_;
> 
> How about the renaming suggestions I made on the other bug?

The are few differences between  ARM's exclusive monitor and MIPS's LL/SC pair, so the probably should not use same terminology.
That said LLBit_ and LLAddr_ correspond to "real" architecture CP0 registers of the same name. The lastLLValue_ is the state unique for the needs of the simulator and I'm not particularly fond of it's naming, but I could not think of better one. Any suggestions are welcome.  

> @@ +345,5 @@
> > +    bool LLBit_;
> > +    uint32_t LLAddr_;
> > +    int32_t lastLLValue_;
> > +
> > +
> 
> nit: please remove one blank line

Sorry about those. They always slip trough. I've noticed .clang-format files laying around in js folder. Do you provide something like make check-style that utilizes them?

Thanks in advance.

Comment 6

[Lars T Hansen [:lth]]

Comment on attachment 8898300 [details] [diff] [review]
bug1391248.patch

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

Clearing review in anticipation of new patch as per benjamin's request.

Comment 7

[Benjamin Bouvier [:bbouvier] (inactive)]

(In reply to Dragan Mladjenovic from comment #5)
> Wasm jit-tests are not passing due to various issues, one of them
> FramePointer being defines as InvalidReg on mips32.
> I've managed to fix up most of the tests failures excluding the debug wasm
> tests which looks to require wasm baseline tier (not implemented on MIPS for
> now). This patch is amalgam of fixes that needed to be done to simulator
> code to make all the tests pass. At the time being the change looks out of
> place because required bits of mips32 codegen are missing. I hope to be able
> to send more codegen patches during this week.

OK, thanks for explaining.
  
> 
> > @@ +1712,5 @@
> > > +// Note that simulator runs have the runtime system running directly on the host
> > > +// system and only generated code is executed in the simulator.
> > > +// Since the host is typically IA32 it will not trap on unaligned memory access.
> > > +// We assume that that executing correct generated code will not produce unaligned
> > > +// memory access, so we explicitly check for address alignment and trap.
> > 
> > Instead of assuming that the generated code is correct and does not trap,
> > what do you think about keeping the previous code that crashed in this case?
> > If this is not supposed to happen, crashing is a nice way to make sure this
> > is the case.
> 
> Just to clarify tho old behavior is to always crass on unaligned access and
> the new one is to only crass if not executing wasm code. It was needed to
> make a large number of wasm tests pass. I took ARM's approach to allow
> unaligned access for wasm code because performance is not of high importance
> right now. At some time later I would like to iterate this part of the patch
> to maybe provide an option to always crash on unaligned access in order to
> help us to prune unaligned acess from wasm jited code if that is actually
> possible. 

But what does MIPS hardware do in general? SInce it's a simulator, we'd like to stay close to the behavior that hardware would do. It seems the previous comment and behavior were closer to this real behavior, for that matter. Note there are already some machinery in the ARM simulator to automatically fix up unaligned accesses (as it *can* be done on some ARM devices). Could you maybe split these parts related to unaligned accesses to a different patch (but under the same bug), please?

> 
> > @@ -2649,4 @@
> > >                  }
> > >                  break;
> > >                }
> > > -              case ff_cvt_l_fmt: {  // Mips32r2: Truncate float to 64-bit long-word.
> > 
> > Why are these instructions removed?
> 
> These instructions in FR=0 mode produce undefined result and should not be
> used. I've found few places inside mips-shared code where we do. This is
> just to prevent any such further mistakes.

I'm afraid we're reaching the limits of my MIPS knowledge. Are there any clues in the code that indicate the size of FP registers here? Can the simulator be affected a particular mode FR=0 or FR=1? (otherwise, how hard would it be to add it) If so, could we just assert that we're in mode FR=1 when executing these instructions, instead? (Again, a separate small patch would make easy to spot all the changes related to this)

> The are few differences between  ARM's exclusive monitor and MIPS's LL/SC
> pair, so the probably should not use same terminology.
> That said LLBit_ and LLAddr_ correspond to "real" architecture CP0 registers
> of the same name. The lastLLValue_ is the state unique for the needs of the
> simulator and I'm not particularly fond of it's naming, but I could not
> think of better one. Any suggestions are welcome.  

OK, I like the consistency between LLAddr_ and lastLLValue_, so we could keep those. Thanks for explaining!

> 
> > @@ +345,5 @@
> > > +    bool LLBit_;
> > > +    uint32_t LLAddr_;
> > > +    int32_t lastLLValue_;
> > > +
> > > +
> > 
> > nit: please remove one blank line
> 
> Sorry about those. They always slip trough. I've noticed .clang-format files
> laying around in js folder. Do you provide something like make check-style
> that utilizes them?
> 
> Thanks in advance.

We do have a `make check-style` but that only checks some annotations in MacroAssembler.h (annotations which are _disabled_ on MIPS at the moment). I kinda remember people talked about using clang format in the past in the JS engine, but our particular rules (namely, switch cases must have half-indents) weren't well handled by clang format. So no, there doesn't seem to be good ways to automatically find those (maybe a grep \n\n on the patch files would do?).

Comment 8

[Dragan Mladjenovic]

(In reply to Benjamin Bouvier [:bbouvier] from comment #7)
Hi,
Thanks for the replay. 
> > 
> > > @@ +1712,5 @@
> > > > +// Note that simulator runs have the runtime system running directly on the host
> > > > +// system and only generated code is executed in the simulator.
> > > > +// Since the host is typically IA32 it will not trap on unaligned memory access.
> > > > +// We assume that that executing correct generated code will not produce unaligned
> > > > +// memory access, so we explicitly check for address alignment and trap.
> > > 
> > > Instead of assuming that the generated code is correct and does not trap,
> > > what do you think about keeping the previous code that crashed in this case?
> > > If this is not supposed to happen, crashing is a nice way to make sure this
> > > is the case.
> > 
> > Just to clarify tho old behavior is to always crass on unaligned access and
> > the new one is to only crass if not executing wasm code. It was needed to
> > make a large number of wasm tests pass. I took ARM's approach to allow
> > unaligned access for wasm code because performance is not of high importance
> > right now. At some time later I would like to iterate this part of the patch
> > to maybe provide an option to always crash on unaligned access in order to
> > help us to prune unaligned acess from wasm jited code if that is actually
> > possible. 
> 
> But what does MIPS hardware do in general? SInce it's a simulator, we'd like
> to stay close to the behavior that hardware would do. It seems the previous
> comment and behavior were closer to this real behavior, for that matter.
> Note there are already some machinery in the ARM simulator to automatically
> fix up unaligned accesses (as it *can* be done on some ARM devices). Could
> you maybe split these parts related to unaligned accesses to a different
> patch (but under the same bug), please?
In general hardware will always trap on unaligned access and Linux kernel will emulate it 
(unaligned access emulation is on by default), so situation is a bit simpler than on arm. 
Unaligned access will be allowed when executing wasm code because it is necessary in 
order to successfully run the tests. For the rest the old behavior is preserved because it 
helps track down the performance bugs due to unaligned access.

> > 
> > > @@ -2649,4 @@
> > > >                  }
> > > >                  break;
> > > >                }
> > > > -              case ff_cvt_l_fmt: {  // Mips32r2: Truncate float to 64-bit long-word.
> > > 
> > > Why are these instructions removed?
> > 
> > These instructions in FR=0 mode produce undefined result and should not be
> > used. I've found few places inside mips-shared code where we do. This is
> > just to prevent any such further mistakes.
> 
> I'm afraid we're reaching the limits of my MIPS knowledge. Are there any
> clues in the code that indicate the size of FP registers here? Can the
> simulator be affected a particular mode FR=0 or FR=1? (otherwise, how hard
> would it be to add it) If so, could we just assert that we're in mode FR=1
> when executing these instructions, instead? (Again, a separate small patch
> would make easy to spot all the changes related to this)

There are in setFpuRegisterDouble and similar. Currently simulator implements only FR=0 mode as assumed by the jit. 
May at some point in the future the support for FR=1 will be added, but for now allowing this instructions to execute would be incorrect. You can see more info in Appendix C. FPU hardware modes, https://dmz-portal.imgtec.com/wiki/MIPS_O32_ABI_-_FR0_and_FR1_Interlinking and http://cdn2.imgtec.com/documentation/MD00086-2B-MIPS32BIS-AFP-05.04.pdf (see CVT.L.fmt).

Comment 9

[Dragan Mladjenovic]

Attachment: 0001-Bug-1391248-Use-AutoEnterOOMUnsafeRegion-in-mips32-s.patch

Comment 10

[Dragan Mladjenovic]

Attachment: 0002-Bug-1391248-Add-ll-sc-sync-instruction-support-to-mi.patch

Comment 11

[Dragan Mladjenovic]

Attachment: 0003-Bug-1391248-Add-mov-.s-instruction-support-and-missi.patch

Comment 12

[Dragan Mladjenovic]

Attachment: 0004-Bug-1391248-Crash-mips32-simulator-on-instructions-w.patch

Comment 13

[Dragan Mladjenovic]

Attachment: 0005-Bug-1391248-Add-asynchronous-wasm-interrupt-trap-sup.patch

Comment 14

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8900373 [details] [diff] [review]
0001-Bug-1391248-Use-AutoEnterOOMUnsafeRegion-in-mips32-s.patch

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

Looks good; can you make it a proper mercurial patch, though, please? (upload it again and mark it r+ since I've reviewed it here)

Comment 15

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8900374 [details] [diff] [review]
0002-Bug-1391248-Add-ll-sc-sync-instruction-support-to-mi.patch

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

Assuming the codegen bits are correct, requesting review from Lars for the atomic calls happening in storeConditionalW and the load function.

Comment 16

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8900375 [details] [diff] [review]
0003-Bug-1391248-Add-mov-.s-instruction-support-and-missi.patch

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

Thanks (ditto, please make it a proper HG patch and don't include the diffstats in the changeset message)

Comment 17

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8900377 [details] [diff] [review]
0005-Bug-1391248-Add-asynchronous-wasm-interrupt-trap-sup.patch

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

Just one change requested here. Thanks.

::: js/src/jit/mips32/Simulator-mips32.cpp
@@ +1622,5 @@
>  Simulator::startInterrupt(WasmActivation* activation)
>  {
> +    JS::ProfilingFrameIterator::RegisterState state;
> +    state.pc = (void*) get_pc();
> +    state.fp = (void*) getRegister(fp);

(note: this should be in sync with handleWasmInterrupt, in which case fp := s8)

@@ +1637,5 @@
>  void
>  Simulator::handleWasmInterrupt()
>  {
> +    void* pc = (void*)get_pc();
> +    void* fp = (void*)getRegister(s8);

Can you add a TODO in a comment, explaining that the choice of s8 will have to be fixed at some point?

@@ +1672,5 @@
> +    void* pc = reinterpret_cast<void*>(get_pc());
> +    uint8_t* fp = reinterpret_cast<uint8_t*>(getRegister(s8));
> +
> +    // Cache the wasm::Code to avoid lookup on every load/store.
> +    if (!wasm_code_ || !wasm_code_->containsCodePC(pc))

As far as I can see, wasm_code_ doesn't need to be an attribute member of the Simulator and could just be a local variable. Can you change this, please?

Comment 18

[Lars T Hansen [:lth]]

Comment on attachment 8900374 [details] [diff] [review]
0002-Bug-1391248-Add-ll-sc-sync-instruction-support-to-mi.patch

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

Looks OK to me, it's the same structure we use for the ARM simulator, and it's nicely compatible with the revamping of the AtomicOperations (bug 1389669, bug 1146817).

Comment 19

[Dragan Mladjenovic]

Attachment: 0001-Bug-1391248-Use-AutoEnterOOMUnsafeRegion-in-mips32-s.patch

Comment 20

[Dragan Mladjenovic]

Attachment: 0002-Bug-1391248-Add-ll-sc-sync-instruction-support-to-mi.patch

Comment 21

[Dragan Mladjenovic]

Attachment: 0003-Bug-1391248-Add-mov-.s-instruction-support-and-missi.patch

Comment 22

[Dragan Mladjenovic]

Attachment: 0004-Bug-1391248-Crash-mips32-simulator-on-instructions-w.patch

Comment 23

[Dragan Mladjenovic]

Attachment: 0005-Bug-1391248-Add-asynchronous-wasm-interrupt-trap-sup.patch

Comment 24

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8900750 [details] [diff] [review]
0005-Bug-1391248-Add-asynchronous-wasm-interrupt-trap-sup.patch

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

Still the wasm_code_ issue.

Please also note that the changeset message should end with r=bbouvier, not r-bbouvier.

::: js/src/jit/mips32/Simulator-mips32.h
@@ +358,2 @@
>      bool wasm_interrupt_;
> +    wasm::SharedCode wasm_code_;

As asked in the previous review, can you get rid of wasm_code_ and use a local variable in the function that used it instead, please?

Comment 25

[Dragan Mladjenovic]

(In reply to Benjamin Bouvier [:bbouvier] from comment #24)
> Comment on attachment 8900750 [details] [diff] [review]
> 0005-Bug-1391248-Add-asynchronous-wasm-interrupt-trap-sup.patch
> 
> Review of attachment 8900750 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Still the wasm_code_ issue.
> 
> Please also note that the changeset message should end with r=bbouvier, not
> r-bbouvier.
> 
> ::: js/src/jit/mips32/Simulator-mips32.h
> @@ +358,2 @@
> >      bool wasm_interrupt_;
> > +    wasm::SharedCode wasm_code_;
> 
> As asked in the previous review, can you get rid of wasm_code_ and use a
> local variable in the function that used it instead, please?

Sorry for not replaying in timely manner.
It is used to cache wasm::SharedCode instance between Simulator::handleWasmFault calls.
ARM simulator chose to use it to avoid repetitive calls to WasmCompartment::lookupCode.
I'm not sure of lookupCode call overhead, but I would like to keep this code similar 
to that of ARM to ease any further code maintenance.

Comment 26

[Benjamin Bouvier [:bbouvier] (inactive)]

Comment on attachment 8900750 [details] [diff] [review]
0005-Bug-1391248-Add-asynchronous-wasm-interrupt-trap-sup.patch

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

OK then, sorry for not checking what the ARM simulator did and thank you for explaining.

Comment 27

[Dragan Mladjenovic]

Attachment: 0001-Bug-1391248-Use-AutoEnterOOMUnsafeRegion-in-mips32-s.patch

Comment 28

[Dragan Mladjenovic]

Attachment: 0002-Bug-1391248-Add-ll-sc-sync-instruction-support-to-mi.patch

Comment 29

[Dragan Mladjenovic]

Attachment: 0003-Bug-1391248-Add-mov-.s-instruction-support-and-missi.patch

Comment 30

[Dragan Mladjenovic]

Attachment: 0004-Bug-1391248-Crash-mips32-simulator-on-instructions-w.patch

Comment 31

[Dragan Mladjenovic]

Attachment: 0005-Bug-1391248-Add-asynchronous-wasm-interrupt-trap-sup.patch

Comment 32

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d7502c932452
Use AutoEnterOOMUnsafeRegion in mips32 simulator. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/ee2061564115
Add ll,sc,sync instruction support to mips32 simulator. r=lth
https://hg.mozilla.org/integration/mozilla-inbound/rev/e452433ec516
Add mov*.s instruction support and missing redirection signatures to mips32 simulator. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/220bc56b5036
Crash mips32 simulator on instructions whose result is undefined under FR=0 mode. r=bbouvier
https://hg.mozilla.org/integration/mozilla-inbound/rev/3c6a4705b511
Add asynchronous wasm interrupt/trap support to mips32 simulator. r=bbouvier

Comment 33

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d7502c932452
https://hg.mozilla.org/mozilla-central/rev/ee2061564115
https://hg.mozilla.org/mozilla-central/rev/e452433ec516
https://hg.mozilla.org/mozilla-central/rev/220bc56b5036
https://hg.mozilla.org/mozilla-central/rev/3c6a4705b511