Omr: Should TR::checkILCondition trap or throw an exception upon failure?

[dchopra001]

While investigating some tril failures I found that an IlValidator DeathTest ( IlValidatorTest/IllformedTrees) spits out 14 core files (1 for each test). This is expected behavior because TR::checkILCondition is designed to trap if it detects a validation error:
https://github.com/eclipse/omr/blob/38f19f5a9254a3a14b1258c8c0dee634319a8198/compiler/ras/ILValidationUtils.cpp#L45-L64

My question is whether we want this behavior, or should we instead throw an exception and then change the GoogleTest from Death tests. If we want to keep this behavior, then we need to discuss what to do to make sure the cores are cleaned up as it seems wasteful to generate them right now.

@rbanerjee @Leonardo2718 @mgaudet thoughts?

[fjeremic]

IMO we do not want this behavior. Running this Tril test on a system with a ulimit -c unlimited set will result in tons of cores generated and tons of wasted space. I would think if we encountered invalid IL instead of trapping we should abort the compile via a C++ exception.

In debug mode uncaught aborted compiles should be fatal while in production mode they are silent failures only observed in verbose log as compilation failures.

IMO this death test should be changed to catch a compilation abort exception due to invalid IL rather than expect a crash.

@mpirvu @dsouzai thoughts?

[mgaudet]

So, yes, definitely. Though, I do wonder if this may be a bit of a google test bug.

Charlie has investigated issues with death tests on OS/X and switching to exceptions would be the resolution there too.

[dchopra001]

Side Note: CommoningDeathTest will also need to be changed if we decide to go the exception route.

[Leonardo2718]

Interesting. In the original implementation, I used a macro instead of calling TR::trap() directly. The macro mapped to TR::trap() only in DEBUG builds. In PROD builds, it threw an exception to abort compilation without taking down the VM.

I tend to agree that throwing an exception is preferable. Ideally, this exception should be able to propagate to the VM or test harness for testing purposes. In a production build, it would just abort compilation. This would make it easy to test that invalid IL is rejected. The death tests would be changed to expect a specific exception to be thrown.

However, it is also possible to check the return code of the compile method call. This is not ideal but will work for most testing purposes.

Maybe @dsouzai or @mpirvu have ideas for making the exception mechanism work?

[rbanerjee]

I used a macro instead of calling TR::trap() directly. The macro mapped to TR::trap() only in DEBUG builds. In PROD builds, it threw an exception to abort compilation without taking down the VM.

Hmm, I think I remember the macro used in the initial refactoring of TreeVerifier. I also remember not being a big fan of it :)

IMO the correct behaviour here should be to throw an exception in both DEBUG and PROD builds.
(Where the PROD build just ~dies~ fails silently). I think I have a better understanding of what the macro was trying to accomplish then. (Though I still believe there are better ways to do what it was trying to achieve). Maybe this would be a good opportunity to test out the different solutions, and take into account the OS/X Death Test failure Issue while trying to figure out which one's more "optimal".

Thanks for bringing this up @dchopra001!

[0xdaryl]

I'm in favour of an exception approach. Some work will have to be done to allow any thrown exception for this purpose to escape compilation, but I don't think that should pose too many challenges.

[dsouzai]

From what I can tell, because the Tril::SimpleCompiler invokes the compiler in the following way:

https://github.com/eclipse/omr/blob/38f19f5a9254a3a14b1258c8c0dee634319a8198/fvtest/tril/tril/simple_compiler.cpp#L73-L80

If the TR::trap() is changed to something like comp->failCompilation<TR::CompilationException>("Failed as expected");

The rc will not be 0 (COMPILATION_SUCCEEDED) and so the test will complete successfully without any cores being produced.

[dsouzai]

If we really want to make sure that the failure is due to the bad IL input, then we can:

1. define a new exception type
2. define a new CompilationReturnCodes

Then we add a new catch block below to catch the new exception type

https://github.com/eclipse/omr/blob/38f19f5a9254a3a14b1258c8c0dee634319a8198/compiler/control/CompileMethod.cpp#L435-L446

And set the rc to the new CompilationReturnCodes. Then in

https://github.com/eclipse/omr/blob/38f19f5a9254a3a14b1258c8c0dee634319a8198/fvtest/compilertriltest/ILValidatorTest.cpp#L37-L38

we make sure that the rc is what we expect it to be.