RFR: 8329581: Java launcher no longer prints a stack trace [v9]

[Alan Bateman]

On Thu, 9 May 2024 19:48:53 GMT, Sonia Zaldana Calles <szaldana at openjdk.org> wrote:

>> Pre-existing: Man, I cannot grok the complex return code handling, tbh.
>> 
>> We have the local `ret` variable holding a return code. We also hand codes to CHECK_EXCEPTION_LEAVE as macro argument. But we don't hand codes to CHECK_EXCEPTION_NULL_LEAVE. LEAVE uses the locally defined `ret` instead of getting the return code as argument. CHECK_EXCEPTION_LEAVE modifies the local `ret`, but CHECK_EXCEPTION_NULL_LEAVE does not.
>> 
>> CHECK_EXCEPTION_NULL_LEAVE does not set `ret`. So, in case of an error, it would cause the launcher to return OK, but this does not happen because the local `ret` gets initialized to 1 before the first call to CHECK_EXCEPTION_NULL_LEAVE (line 566 resp. 560). Not sure if this was intentional, but it surely is very brittle. We rely on the content of `ret`, and that changes several times throughout JavaMain.
>> 
>> CHECK_EXCEPTION_NULL_LEAVE argument is named CENL_exception, which I don't understand.
>> 
>> To confuse matters more, the logic for internal error codes and the launcher return code is reversed: internally, 0 means error, and externally, 0 means success. And we only use numerical literals (`1`, `0`) instead of clearly named constants.
>> 
>> This may be food for another RFE, to keep this patch minimal. But a good solution, to me, would be like this:
>> 
>> - have the same logic for return codes (1 = error, 0 = success) to ease understanding
>> - have clearly named constants (e.g. "LAUNCHER_OK" 0, "LAUNCHER_ERR" = 1)
>> - have the LEAVE macro take the launcher return code as argument
>> - have all xxx_LEAVE macros pass in LAUNCHER_ERR to LEAVE
>> - call the final LEAVE with LAUNCHER_OK
>> - optionally, define something like "LEAVE_ERR" and "LEAVE_OK" that call LEAVE with either LAUNCHER_ERR or LAUNCHER_OK, for more concise coding.
>> 
>> For this patch, I think the return code logic is okay, but I would feel better if others double-checked.
>
>> This may be food for another RFE, to keep this patch minimal. But a good solution, to me, would be like this:
>> 
>> * have the same logic for return codes (1 = error, 0 = success) to ease understanding
>> * have clearly named constants (e.g. "LAUNCHER_OK" 0, "LAUNCHER_ERR" = 1)
>> * have the LEAVE macro take the launcher return code as argument
>> * have all xxx_LEAVE macros pass in LAUNCHER_ERR to LEAVE
>> * call the final LEAVE with LAUNCHER_OK
>> * optionally, define something like "LEAVE_ERR" and "LEAVE_OK" that call LEAVE with either LAUNCHER_ERR or LAUNCHER_OK, for more concise coding.
>> 
>> For this patch, I think the return code logic is okay, but I would feel better if others double-checked.
> 
> @tstuefe Agreed, I can look into opening another issue to track this after we fix the regression.

@SoniaZaldana I plan to review this shortly. It's an important issue that slipped through due to insufficient tests.

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

PR Comment: https://git.openjdk.org/jdk/pull/18786#issuecomment-2104216259