Changed dependency list overflow assert to fatal #4962
Conversation
If not capturing jit log having a non fatal assert could cause list overflow to slip under the radar Signed-off-by: Shubham Verma <shubhamv.sv@gmail.com>
| // Printf added so it triggers some output even in prod build. | ||
| // If this failure is triggered in a prod build, you might | ||
| // not get a SEGV nor any meaningful error msg. | ||
| TR_ASSERT(0,"ERROR: addPostCondition list overflow\n"); | ||
| TR_ASSERT_FATAL(0,"ERROR: addPostCondition list overflow\n"); | ||
| _cg->comp()->failCompilation<TR::CompilationException>("addPostCondition list overflow, abort compilation\n"); |
There was a problem hiding this comment.
The extra failCompilation is no longer needed since TR_ASSERT_FATAL terminates the process.
This whole if statement can be simplified to
TR_ASSERT_FATAL(_addCursorForPost < _numPostConditions, "addPostCondition list overflow");
(The extra "ERROR" at the start of the message adds no value since an assert failure is obviously an error).
There was a problem hiding this comment.
Excellent point Leo. Further to that, let's think about what would happen if we hit this assert in the wild. Would there be any other information that we could print that would be helpful in this case? I'd imagine the first thing one may want to know when debugging such a problem is what is the value of _numPostConditions? It would be helpful to print that as part of the assert, along with any other information you may think would be useful.
There was a problem hiding this comment.
Besides _numPostConditions and _addCursorForPost, is there anything else that could be useful while debugging? I was just printing those two while debugging a bug in inlineVectorizedStringIndexOf Could register name be helpful?
There was a problem hiding this comment.
Yes, the virtual register pointer and name would be very useful, in addition to _numPostConditions.
I don't think capturing a jit log has anything to do with the area of code being modified. An overflow in the dependency list would cause a method compilation failure, for which if there is a fallback such as an interpreter to execute the method, it could result in a silent performance problem. The purpose of this change is to ensure the developer correctly sizes the dependency list during development and to avoid potential silent performance problems. Could we alter the commit message accordingly? |
Since the assert will process to halt, we no longer need to raise a compilation exception. Added _addCursorForPost, _numPostConditions, virtual register name and virtual register pointer to assert print statement Signed-off-by: Shubham Verma <shubhamv.sv@gmail.com>
|
@genie-omr build all |
With initial implementation, using a fallback such as an interpreter to execute method for which compilation failed due to dependency list overflow could result in silent performance issues.
Changing the assert to fatal helps avoid any potential silent performance issues by ensuring initialization of dependency list with correct capacity.
Signed-off-by: Shubham Verma shubhamv.sv@gmail.com