RFR: 8344013: "bad tag in log" assert with +LogCompilation +CITimeVerbose [v2]

[Christian Hagedorn]

On Mon, 25 Nov 2024 15:36:00 GMT, Sonia Zaldana Calles <szaldana at openjdk.org> wrote:

>> Hi folks, 
>> 
>> This PR addresses [8344013](https://bugs.openjdk.org/browse/JDK-8344013).
>> 
>> Sometimes the writing to xmlStream is mixed from several threads, and therefore the xmlStream tag stack can end up in a bad state. When this occurs, the VM crashes in `xmlStream::pop_tag` with `assert(false) failed: bad tag in log`. 
>> 
>> In this case, running `java -XX:+LogCompilation -XX:CompileCommand="log,*.*" -XX:+CITimeVerbose -Xcomp -Xbatch -version` , `xmlStream::pop_tag` is expecting to pop the tag `task` but finds `phase` instead. 
>> 
>> I found the issue stems from [8330157](https://bugs.openjdk.org/browse/JDK-8330157). The problematic code is in the destructor for [TracePhase](https://github.com/openjdk/jdk/blame/master/src/hotspot/share/opto/compile.cpp#L4337).
>> 
>> Note how the constructor adds the [phase tag](https://github.com/openjdk/jdk/blame/master/src/hotspot/share/opto/compile.cpp#L4327).
>> 
>> However, in the destructor, if we return early, we don’t pop that tag, leading to the xmlStream tag stack to end up in a bad state. With this patch, I made sure we pop the tag even if we return early. 
>> 
>> Cheers, 
>> Sonia
>
> Sonia Zaldana Calles has updated the pull request incrementally with one additional commit since the last revision:
> 
>   Adding regression test

Thanks for adding the test! Two minor comments, otherwise, looks good.

test/hotspot/jtreg/compiler/debug/TestLogStackAssert.java line 31:

> 29:  * @requires vm.debug == true & vm.compiler2.enabled
> 30:  * @summary Verify the xmlStream log stack is not left in a bad state
> 31:  * @library /test/lib /

Not required and can be removed.
Suggestion:

test/hotspot/jtreg/compiler/debug/TestLogStackAssert.java line 32:

> 30:  * @summary Verify the xmlStream log stack is not left in a bad state
> 31:  * @library /test/lib /
> 32:  * @run main/othervm -XX:+LogCompilation -XX:CompileCommand=log,*.* -XX:+CITimeVerbose -Xcomp -Xbatch compiler.debug.TestLogStackAssert

`-Xbatch` is implied by `-Xcomp`.
Suggestion:

 * @run main/othervm -XX:+LogCompilation -XX:CompileCommand=log,*.* -XX:+CITimeVerbose -Xcomp compiler.debug.TestLogStackAssert

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

Marked as reviewed by chagedorn (Reviewer).

PR Review: https://git.openjdk.org/jdk/pull/22331#pullrequestreview-2459549758
PR Review Comment: https://git.openjdk.org/jdk/pull/22331#discussion_r1857296710
PR Review Comment: https://git.openjdk.org/jdk/pull/22331#discussion_r1857297827