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

[Coleen Phillimore]

On Fri, 11 Dec 2020 08:24:27 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.
>
> src/hotspot/share/utilities/debug.cpp line 243:
> 
>> 241: static char _detail_msg[1024];
>> 242: 
>> 243: static void print_error_for_unit_test(const char* message, const char* detail_fmt, va_list detail_args) {
> 
> Trying to understand why you changed this code (beside factoring it out). We have:
> 
> - report_vm_out_of_memory -> message=NULL, detail set
> - report_and_die from real crash -> message=NULL, detail=""
> - report_fatal -> message="fatal error", detail set
> - report_vm_error (assert, guarantee) -> message="assert (bla) failed", detail is set
> 
> so sometimes message is NULL, sometimes details are empty. What a mess :( Since you do gtests for all these cases now, you need to print the assert message correctly for them. You use _detail_msg as scratch buffer, since its already there. Later we print the detail message again into it. But I think that is fine.
> 
> I have a patch on the back burner somewhere simplifying these functions. Maybe we can improve matters. Then this function becomes easier too.

I changed it to factor it out.  I think I got the combinations.

> src/hotspot/share/utilities/debug.cpp line 246:
> 
>> 244: #ifdef ASSERT
>> 245:   if (ExecutingUnitTests) {
>> 246:     if (detail_fmt != NULL) {
> 
> You miss va_copy for detail_args here.

va_list detail_args_copy;
      va_copy(detail_args_copy, detail_args);
      jio_vsnprintf(_detail_msg, sizeof(_detail_msg), detail_fmt, detail_args_copy);

I fixed this but I didn't see the message garbled (?)

You're right in your comment that we are testing the assert message through print_error_for_unit_test.  I think the original motivation was to move the test that checked for a specific message out of the error handling tests, and these other ones just came for the ride.

> src/hotspot/share/utilities/vmError.hpp line 187:
> 
>> 185: 
>> 186:   // Test vmassert(), fatal(), guarantee(), etc.
>> 187:   DEBUG_ONLY(static void test_error_handler();)
> 
> May I suggest:
> - remove the almost empty `test_error_handler` completely
> - remove the "case 0" in `controlled_crash` 
> - leave `controlled_crash` public
> - In create_vm, doing this instead:
> if (ErrorHandlerTest != 0) {
>   VMError::controlled_crash(ErrorHandlerTest);
> }
> 
> This would have two advantages, it would be clearer and leave us with a "controlled_crash" API to use in quick tests (I do that sometimes). 
> 
> Both are not hard reasons and a matter of taste, so feel free to ignore my proposal.

Yes, I like this suggestion.  I'll do this.

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

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