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

[Axel Boldt-Christmas]

On Mon, 7 Nov 2022 15:25:14 GMT, Thomas Stuefe <stuefe at openjdk.org> wrote:

>> Refactor the STEP macro in VMError::report to improve readability.
>> Right now the macro contains multiple statements on one line and the non-conventional control flow is even harder to understand.
>> 
>> This enhancement aims to do two things:
>>  1. It splits the macro into multiple lines with indentations which makes the structure of the C++ code generated by the preprocessor clearer.
>>   2. Separates the internal step logic from the decision logic which decides if a step should be taken with a STEP_IF(step_name_str, condition) macro
>> 
>> Testing: tier 1 + GHA
>
> 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.)

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

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