1256324 - Bail after

Status: RESOLVED FIXED

(Core :: JavaScript Engine: JIT, defect)

Comment 1

[Hannes Verschore [:h4writer]]

Attachment: BailAfter

I created the function "bailAfter(number)" which will bail after 'number' of fallible LIR's have been executed at runtime. In order to make it easier for fuzzers to find issues with our resume points. This has been sprouted out of the discussion of recover instructions that potentially encodes instructions wrongly sometimes. This has been an area the fuzzers couldn't fuzz easily and this function should help.

Now before r? and landing this, I did a bit of testing:
I automatically enabled bailing every 100th instructions encounter at runtime. This is already giving us:
[ 9977|   85|    9|    0] 100% ======================================>| 781.5s

I haven't dug into the details and don't know exactly where the issues are,
but it seems we have some work to do. We should get them investigated and fixed.
Opening this bug to investigate the failures in depending bugs, before we can land this new feature.

Comment 2

[Hannes Verschore [:h4writer]]

Attachment: Failure log

Comment 3

[Nicolas B. Pierron [:nbp]]

Comment on attachment 8730214 [details] [diff] [review]
BailAfter

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

Nice work!

::: js/src/builtin/TestingFunctions.cpp
@@ +3766,5 @@
>  
> +    JS_FN_HELP("bailAfter", testingFunc_bailAfter, 1, 0,
> +"bailAfter(number)",
> +"  Start a counter to bail once after passing the given amount of possible bailout positions in\n"
> +"  ionmonkey.\n"),

I think it might be good to have a bailTest function for the fuzzers, in addition to bailAfter.

The reason why I think it might be better, is that it would be easier for reducers to get rid of all the code which is not necessary in the test case, as the code would become independent of the number of bailing instructions.  (similar to oomAfterAllocation and oomTest issue)

::: js/src/jit/CodeGenerator.cpp
@@ +4604,5 @@
> +void
> +CodeGenerator::emitForceBailing(LInstruction* lir)
> +{
> +    if (!lir->snapshot())
> +        return;

Note, some instruction have a snapshot, and never use it.  Some this might lead to a few false positive.
For example, MStart / LStart.

::: js/src/jit/CodeGenerator.h
@@ +399,5 @@
>      void visitRecompileCheck(LRecompileCheck* ins);
>  
>      void visitRandom(LRandom* ins);
>  
> +    void emitForceBailing(LInstruction* lir);

nit: emitMaybeForceBailout.

Comment 4

[Nicolas B. Pierron [:nbp]]

How many issue remain, with Bug 1239075's patch?

Comment 5

[Hannes Verschore [:h4writer]]

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> I think it might be good to have a bailTest function for the fuzzers, in
> addition to bailAfter.
> 
> The reason why I think it might be better, is that it would be easier for
> reducers to get rid of all the code which is not necessary in the test case,
> as the code would become independent of the number of bailing instructions. 
> (similar to oomAfterAllocation and oomTest issue)

Not sure I understand what bailTest should do. But I already added a "bailout()" function before. Which will force a bailout.
Though I have never seen that create new testcases :(

> ::: js/src/jit/CodeGenerator.cpp
> @@ +4604,5 @@
> > +void
> > +CodeGenerator::emitForceBailing(LInstruction* lir)
> > +{
> > +    if (!lir->snapshot())
> > +        return;
> 
> Note, some instruction have a snapshot, and never use it.  Some this might
> lead to a few false positive.
> For example, MStart / LStart.

Why do we have a snapshot in that case?
=> though this gave indeed false positives.

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> How many issue remain, with Bug 1239075's patch?

Like stated above. I haven't been able to classify them.
I even think there are more non-recoverable issues that I'm currently seeing.
I think I only saw 1 related to it yet.
I big downside is that we don't have a --ion-recover=disable.
That would have made it easy to classify the recover issues from the other ones.
And it is non-trivial to disable by adjusting the code.

(In reply to Nicolas B. Pierron [:nbp] from comment #3)
> feedback+

This is a bit premature. The code is just a dump of code, not polished. Wrong names, wrong code flow ...
Totally not ready for f? or r?

Comment 6

[Nicolas B. Pierron [:nbp]]

(In reply to Hannes Verschore [:h4writer] from comment #5)
> (In reply to Nicolas B. Pierron [:nbp] from comment #3)
> > I think it might be good to have a bailTest function for the fuzzers, in
> > addition to bailAfter.
> > 
> > The reason why I think it might be better, is that it would be easier for
> > reducers to get rid of all the code which is not necessary in the test case,
> > as the code would become independent of the number of bailing instructions. 
> > (similar to oomAfterAllocation and oomTest issue)
> 
> Not sure I understand what bailTest should do. But I already added a
> "bailout()" function before. Which will force a bailout.
> Though I have never seen that create new testcases :(

Basically, the bailTest function should look like:

function bailTest(f) {
  var i = 0;
  while (bailCounterReached()) {
    gc(); gc(); // get rid of all previous compilation
    // reset counters?
    // get rid of baseline ?

    // Set the next bailout to trigger a failure.
    bailAfter(i++);

    // re-execute f.
    f();
  }
}

But, as with the the oomTest, we should implement this one in C++ and not in JS, to prevent it from being reduced away.

The goal of this function is too fuzz bailouts across all the execution of a function, causing the next bailout to fail after the next iteration.

> > ::: js/src/jit/CodeGenerator.cpp
> > @@ +4604,5 @@
> > > +void
> > > +CodeGenerator::emitForceBailing(LInstruction* lir)
> > > +{
> > > +    if (!lir->snapshot())
> > > +        return;
> > 
> > Note, some instruction have a snapshot, and never use it.  Some this might
> > lead to a few false positive.
> > For example, MStart / LStart.
> 
> Why do we have a snapshot in that case?
> => though this gave indeed false positives.

I think this is a hack to get an entry-resume point for the lir graph, but this snapshot captures the result of the MParameter mir, as-if they were MPhi.  Thus would cause us to bail before reading anything.

(I wonder how the register allocator deals with that :/)

I guess the proper fix here would be to have StackAllocations MIR for the stack slots.

> (In reply to Nicolas B. Pierron [:nbp] from comment #4)
> > How many issue remain, with Bug 1239075's patch?
> 
> Like stated above. I haven't been able to classify them.
> I even think there are more non-recoverable issues that I'm currently seeing.
> I think I only saw 1 related to it yet.
> I big downside is that we don't have a --ion-recover=disable.
> That would have made it easy to classify the recover issues from the other
> ones.
> And it is non-trivial to disable by adjusting the code.

I guess we could add such flag for debug builds if necessary.
Making every canRecoverOnBailout function return false. (not sure about MAssertRecoveredOnBailout though)

> (In reply to Nicolas B. Pierron [:nbp] from comment #3)
> > feedback+
> 
> This is a bit premature. The code is just a dump of code, not polished.
> Wrong names, wrong code flow ...
> Totally not ready for f? or r?

The feedback flag has no strong meaning, and no meaning for the landing process, I frequently set it to mean that I think a patch is going in the right direction.  So I think this is a good way to cheer others up, while leaving comments.

Comment 7

[Hannes Verschore [:h4writer]]

(In reply to Nicolas B. Pierron [:nbp] from comment #6)
> But, as with the the oomTest, we should implement this one in C++ and not in
> JS, to prevent it from being reduced away.
> 
> The goal of this function is too fuzz bailouts across all the execution of a
> function, causing the next bailout to fail after the next iteration.

I see. Not sure if it is necessary. If the bailAfter works good enough, I wouldn't implement bailTest. 

> > (In reply to Nicolas B. Pierron [:nbp] from comment #4)
> > > How many issue remain, with Bug 1239075's patch?
> > 
> > Like stated above. I haven't been able to classify them.
> > I even think there are more non-recoverable issues that I'm currently seeing.
> > I think I only saw 1 related to it yet.
> > I big downside is that we don't have a --ion-recover=disable.
> > That would have made it easy to classify the recover issues from the other
> > ones.
> > And it is non-trivial to disable by adjusting the code.
> 
> I guess we could add such flag for debug builds if necessary.
> Making every canRecoverOnBailout function return false. (not sure about
> MAssertRecoveredOnBailout though)

Yeah having such a flag would be so awesome!
But is currently not trivial. Too much code assumes canRecoverOnBailout will return true!
That made it such a mess.
E.g.
- MArrayState / MObjectState
- Sink
- EliminateDeadResumePointOperands
- CloneForDeadBranches
- truncate

I even wasn't able to remove the dependency in:
- EnsureOperandNotFloat32

> > (In reply to Nicolas B. Pierron [:nbp] from comment #3)
> > > feedback+
> > 
> > This is a bit premature. The code is just a dump of code, not polished.
> > Wrong names, wrong code flow ...
> > Totally not ready for f? or r?
> 
> The feedback flag has no strong meaning, and no meaning for the landing
> process, I frequently set it to mean that I think a patch is going in the
> right direction.  So I think this is a good way to cheer others up, while
> leaving comments.

Like you said it gives the message that the patch is going in the right direction. That was totally not the reason to post that patch. The only reason to post that patch is to make it possible for others to replicate this work. In whatever state this patch was.

Therefore I'm glad with the cheers in the comments, but the f+ on the code/patch is premature and actually wrong. It should have been f-.

e.g.
- The code doesn't work as advertised.

e.g.
- nit: emitMaybeForceBailout.

I even didn't think about the fact if it would be better to put the test in that function or in the parent function. As a result the naming could still be anything.

Comment 8

[Hannes Verschore [:h4writer]]

I disabled recover instructions forcefully. Didn't succeed totally and potentially also disabled other things,
but it gave me three non-recoverable instructions issue categories:

> js::TypeMonitorResult (cx=cx@entry=0xb7a74020, script=0xb47ed430, pc=pc@entry=0xb7a9bbd2 "\270", rval=...) at /home/h4writer/Build/mozilla-inbound/js/src/vm/ObjectGroup.h:112
>     --ion-eager --ion-offthread-compile=off /home/h4writer/Build/mozilla-inbound/js/src/jit-test/tests/modules/eval-module-oom.js
> 
> MagicValue that escapes!
>     --ion-eager --ion-offthread-compile=off /home/h4writer/Build/mozilla-inbound/js/src/jit-test/tests/TypedObject/bug1004527.js
>     --baseline-eager /home/h4writer/Build/mozilla-inbound/js/src/jit-test/tests/TypedObject/jit-write-u16-into-u16-array-in-struct.js
>     --ion-eager --ion-offthread-compile=off /home/h4writer/Build/mozilla-inbound/js/src/jit-test/tests/TypedObject/bug1004527.js
>     --baseline-eager /home/h4writer/Build/mozilla-inbound/js/src/jit-test/tests/TypedObject/jit-write-u16-into-u16-array-in-struct.js
>     --ion-eager --ion-offthread-compile=off /home/h4writer/Build/mozilla-inbound/js/src/jit-test/tests/TypedObject/jit-write-u16-into-u16-array-in-struct.js
>     --ion-eager --ion-offthread-compile=off /home/h4writer/Build/mozilla-inbound/js/src/jit-test/tests/TypedObject/jit-write-u32-into-mdim-array.js
> 
> IsInsideNursery (cell=0x2f2f2f2f) at /home/h4writer/Build/mozilla-inbound/js/src/build-32-debug-opt/dist/include/js/HeapAPI.h:334
>     --baseline-eager /home/h4writer/Build/mozilla-inbound/js/src/jit-test/tests/ion/bug779245.js
>     --ion-eager --ion-offthread-compile=off /home/h4writer/Build/mozilla-inbound/js/src/jit-test/tests/ion/bug779245.js

Comment 9

[Hannes Verschore [:h4writer]]

That leaves the recoverable instruction issues at:

> Assertion failure: snapshot_.numAllocationsRead() == numAllocations(), at /home/h4writer/Build/mozilla-inbound/js/src/jit/JitFrameIterator.h:499
>     --baseline-eager /home/h4writer/Build/mozilla-inbound/js/src/jit-test/tests/SIMD/recover.js
>     --ion-eager --ion-offthread-compile=off /home/h4writer/Build/mozilla-inbound/js/src/jit-test/tests/SIMD/recover.js
>     --baseline-eager /home/h4writer/Build/mozilla-inbound/js/src/jit-test/tests/ion/dce-with-rinstructions.js
>     --ion-eager --ion-offthread-compile=off /home/h4writer/Build/mozilla-inbound/js/src/jit-test/tests/ion/dce-with-rinstructions.js
>     --ion-pgo=on --ion-eager --ion-offthread-compile=off /home/h4writer/Build/mozilla-inbound/js/src/jit-test/tests/ion/recover-object-bug1175233.js
>     --ion-pgo=on --ion-eager --ion-offthread-compile=off --no-unboxed-objects /home/h4writer/Build/mozilla-inbound/js/src/jit-test/tests/ion/recover-object-bug1175233.js

Comment 10

[Hannes Verschore [:h4writer]]

Attachment: BailAfter

Comment 11

[Hannes Verschore [:h4writer]]

Attachment: BailAfter

I got side-tracked and forgot to land this. Current state is that this is working and there are no failures, even when I adjusted it to bail after every 100th place.

I will also open this bug. There is no security sensitive data in this bug.

Comment 12

[Hannes Verschore [:h4writer]]

Comment on attachment 8754732 [details] [diff] [review]
BailAfter

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

::: js/src/jit/CodeGenerator.h
@@ +415,5 @@
>      void visitRotate(LRotate* ins);
>  
>      void visitRandom(LRandom* ins);
>  
> +    void emitForceBailing(LInstruction* lir);

I stuck with "emitForceBailing" instead of the suggested "emitMaybeForceBailing" by nbp. This is similar to "emitDebugResultCheck", which also doesn't always add a result check. There we also didn't added "Maybe"

Comment 13

[Hannes Verschore [:h4writer]]

Opening

Comment 14

[Eric Faust [:efaust]]

Comment on attachment 8754732 [details] [diff] [review]
BailAfter

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

Thanks for adding this! I'm glad it caught a few things, and at least it's something for the fuzzers to play with.

::: js/src/jit/CodeGenerator.h
@@ +415,5 @@
>      void visitRotate(LRotate* ins);
>  
>      void visitRandom(LRandom* ins);
>  
> +    void emitForceBailing(LInstruction* lir);

can we do emitDebugForceBailing()? :P

::: js/src/jit/MacroAssembler.cpp
@@ +1519,5 @@
>  static void
>  Printf1_(const char* output, uintptr_t value) {
>      char* line = JS_sprintf_append(nullptr, output, value);
> +    if (!line)
> +        MOZ_CRASH("OOM at masm.printf");

This should probably use AutoOOMUnsafeRegion instead?

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0f74961b048e

Comment 16

[Hannes Verschore [:h4writer]]

bailAfter(x) has landed in the shell. This is quite useful for the fuzzers. It makes it possible to force bail after x fallible instructions. That makes it possible to test every possible bailing location.

Therefore I think it might be handy to add "bailAfter(100)" or something in the fuzzer suite for the fuzzers to sometimes try that instruction.

Comment 17

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/8c9c8720264d

Comment 18

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

What would be recommended values to pass to bailAfter? e.g. 0 - 999?

Comment 19

[Hannes Verschore [:h4writer]]

(In reply to Gary Kwong [:gkw] [:nth10sd] from comment #18)
> What would be recommended values to pass to bailAfter? e.g. 0 - 999?

If you can use a range, 0-999 is definitely a good range!

If you want a specific value. That is harder. Having lower numbers will bail closer to the "bailAfter" instruction. Which means less strange things can happen, before we force it to bail. So in that case I would go to somewhere between 100-1000. But it is hard to assess if fuzzers will find more with 100 or 1000 ... 

I think decoder mentioned he has a pool of statements that get added arbitrary to the testcase. Can we add the following to it? And take at random one of those?
bailAfter(100)
bailAfter(1000)
bailAfter(10000)

Comment 20

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/0f74961b048e
https://hg.mozilla.org/mozilla-central/rev/8c9c8720264d

Comment 21

[Christian Holler (:decoder)]

I've added bailAfter() to LangFuzz's custom code fragments and it should be used now with 10, 100 and 1000. Let's see if we find something.

Comment 22

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Jesse has added support for bailAfter here: https://github.com/MozillaSecurity/funfuzz/issues/41

Comment 23

[Jesse Ruderman]

Also added to jsfunfuzz & DOMFuzz. It will use N between 1 and 10000 (uniform distribution of log).

https://github.com/MozillaSecurity/funfuzz/commit/f152d4b49f60c4488faa92cf433b6f6a901d7aff

Comment 24

[Jesse Ruderman]

I guess that if we hit bugs with bailAfter(N) for any N, we'll need to modify the testcase to use the "bailTest" gadget from comment 6, in order to reduce it and create a stable bug report. I hope the JS version of bailTest works well enough for this.

Comment 25

[Hannes Verschore [:h4writer]]

(In reply to Jesse Ruderman from comment #24)
> I guess that if we hit bugs with bailAfter(N) for any N, we'll need to
> modify the testcase to use the "bailTest" gadget from comment 6, in order to
> reduce it and create a stable bug report. I hope the JS version of bailTest
> works well enough for this.

The BailTest desribed in comment 6 is just an example. The code doesn't work. I can make a bailTest function if that is easier for fuzzers?