Closed Bug 657881 Opened 9 years ago Closed 9 years ago

TI: Make type failures abort like assertions

Categories

(Core :: JavaScript Engine, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: decoder, Unassigned)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (obsolete) — Splinter Review
Currently, the TI branch uses the function [TypeFailure] to print a custom "infer failure" message and to terminate the engine afterwards with a crash. I propose to change this behavior: Instead of printing a custom message and crashing, I suggest to use [JS_Assert] to print an assertion instead and cause an abort. The main benefits are:

* Standard tools (e.g. fuzzers) can use the assertion message to differentiate between several failures more easily
* JS_Assert contains code for multiple OSes

Note that JS_Assert is active during release builds.

I've made a small patch that shows how this could be done. Please let me know what needs to be changed in order to get this into TI. What I'm not fully sure about is this statement:

cx->compartment->types.print(cx, cx->compartment);

I've never seen this output something, so I don't know what its purpose is.

With the patch applied, the message looks for example like this:

$ ./js -n -a test.js 
Assertion failure: [infer failure] Missing type at #2:00021 pushed 0: string, at jsinfer.cpp:285
Aborted
Attachment #533209 - Flags: review?(bhackett1024)
Comment on attachment 533209 [details] [diff] [review]
Patch

Looks good, do you need this pushed?

If the INFERFLAGS environment variable is set to 'result' or 'full', the cx->compartment->types.print dumps the current type state of all scripts and type objects to stdout.
Attachment #533209 - Flags: review?(bhackett1024) → review+
Attached patch Improved patchSplinter Review
Attached is an improved patch which changes the following things:

- It first prints out the regular failure message (without the [infer failure] though) with all its details
- It then aborts with an assertion that has the same message minus stuff that I consider too unique for the assertion. In this case I removed the pc and index variables from the message.
- The "unknown bytecode" message should now include the unknown byte in the message and assertion.


Example with bug 657193:

$ ./js -m -n -a test.js
Missing type at #3:00029 pushed 0: void
Assertion failure: [infer failure] Missing type, pushed 0: void, at js/src/jsinfer.cpp:287
Aborted


Is it ok like that? :)
Attachment #533209 - Attachment is obsolete: true
Attachment #536916 - Flags: review?(bhackett1024)
Attachment #536916 - Flags: review?(bhackett1024) → review+
Looks good, will push within the next couple hours.
http://hg.mozilla.org/projects/jaegermonkey/rev/11714be33655
Status: NEW → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Blocks: 676763
You need to log in before you can comment on or make changes to this bug.