Closed Bug 657881 Opened 13 years ago Closed 13 years ago

TI: Make type failures abort like assertions


(Core :: JavaScript Engine, defect)

Not set





(Reporter: decoder, Unassigned)



(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
Attachment #533209 - Flags: review?(bhackett1024)
Comment on attachment 533209 [details] [diff] [review]

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

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.
Closed: 13 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.