RFR: 8252148: vmError::controlled_crash should be #ifdef ASSERT [v2]

[Coleen Phillimore]

On Fri, 11 Dec 2020 08:29:11 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Coleen Phillimore has updated the pull request incrementally with one additional commit since the last revision:
>> 
>>   Fixed va_copy problem, and added changes from pull request.
>
> Hi Coleen,
> 
> this is a good cleanup, I like this a lot.
> 
> About moving ErrorHandlerTest.java to gtest: this is a slick way of doing these tests. However, some things to consider:
> - Strictly speaking we now do not test the real error handler output but the artificial output `ExecutingUnitTests` gives us. The real printing code is now untested. This may have bitten you right here, since I believe in `print_error_for_unit_test` you miss a `va_copy` which should have messed up detail_args and garbled the real output later on.
> - moving these tests out of jtreg means we give up flexibility (excluding them or moving them to another tier). Do they impact the gtest run time noticeably?
> 
> There are also remaining problems with gtest death tests, which I hope we can fix at some point. I don't hold my breath though since we decided to not own the gtest code and cannot make modifications:
> - Death tests are easily confused by stderr output. See: https://bugs.openjdk.java.net/browse/JDK-8257229. So whenever we have unconditional stderr output in the VM (eg if  some large page alloc failed) these death tests fail too.
> - Death tests don't tell you the assert message which happened if it did not match their expectations. Thats plain annoying. See https://bugs.openjdk.java.net/browse/JDK-8257227.

I posted a comment on https://bugs.openjdk.java.net/browse/JDK-8257227.  I don't know how to deal with https://bugs.openjdk.java.net/browse/JDK-8257229.  These tests would fail because of that also.  The assert message tests are nice and imo better than having crashing jtreg tests, but they have these limitations.

-------------

PR: https://git.openjdk.java.net/jdk/pull/1723