RFR: 8296470: Refactor VMError::report STEP macro to improve readability

[Thomas Stuefe]

On Mon, 7 Nov 2022 16:28:34 GMT, Axel Boldt-Christmas <aboldtch at openjdk.org> wrote:

>> src/hotspot/share/utilities/vmError.cpp line 593:
>> 
>>> 591:       controlled_crash(TestCrashInErrorHandler);
>>> 592:       return true;
>>> 593:     }())
>> 
>> I don't think this is necessary. The tests for secondary crash handling are exhaustive, there is no reason why it should differ from crash condition handling outside of a crash condition. If the signal handler is correctly set up, it should work. If it is not, the prior step is sufficient.
>
>> I don't think it adds much readability, but it is a wide-spread patch that will add unnecessary noise for us backporters. Just my opinion, lets see what others think.
> 
> I was worried about that too. This patch initially came about when I was writing #11017 which required rewriting the macro logic and add extra nested scope. The fact that the common case for a step was checking the verbose flag made me refactor out just the verbose case. And then I just refactored out the rest the few extra conditions as well. 
> 
> But I am pretty ambivalent about this change. #11017 contains the newline changes as well. So if there are more opinions that this makes back porting work harder it can be dropped.
>  
> 
>> I don't think this is necessary. The tests for secondary crash handling are exhaustive, there is no reason why it should differ from crash condition handling outside of a crash condition. If the signal handler is correctly set up, it should work. If it is not, the prior step is sufficient.
> 
> I added it because in my initial change (9449501f063e7662a530adf921ae53dadd0312a0), a theoretical crash in a condition would recover but not proceed to the next step. The test change is to avoid such a regression if someone thinks of doing the same simplification of the macro condition logic. (It would crash before `_current_step` was updated.)

Ah, I understand. But you changed this to two nested ifs. So this is no problem anymore. 

I still don't think it is needed with your new version. IIUC it prevents us from the problem you describe if someone changes the STEP macros in the future *and* has a very complex if statement that would crash.

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

PR: https://git.openjdk.org/jdk/pull/11018