eclipse-openj9 / openj9

Eclipse OpenJ9: A Java Virtual Machine for OpenJDK that's optimized for small footprint, fast start-up, and high throughput. Builds on Eclipse OMR (https://github.com/eclipse/omr) and combines with the Extensions for OpenJDK for OpenJ9 repo.
Other
3.28k stars 721 forks source link

1TIDMPDURATION always returns 64d on Windows #17319

Closed kgibm closed 1 year ago

kgibm commented 1 year ago

Java -version output

java -version
openjdk version "17.0.6" 2023-01-17
IBM Semeru Runtime Open Edition 17.0.6.0 (build 17.0.6+10)
Eclipse OpenJ9 VM 17.0.6.0 (build openj9-0.36.0, JRE 17 Windows 11 amd64-64-Bit Compressed References 20230117_367 (JIT enabled, AOT enabled)
OpenJ9   - e68fb241f
OMR      - f491bbf6f
JCL      - 927b34f84c8 based on jdk-17.0.6+10)

Summary of problem

On Windows, the new java dump line 1TIDMPDURATION added by PR #15964 incorrectly returns a constant 64d:

1TIDMPDURATION Approximate time to produce this dump: 64d ms

javacore.20230502.061258.26772.0001.txt

kgibm commented 1 year ago

@keithc-ca I can take a look at this but before I do, have you seen issues with printf and OMR_PRId64 on Windows before?

kgibm commented 1 year ago

Maybe it should have been PRIu64?

keithc-ca commented 1 year ago

Yes, if we were talking to sprintf() it should have used OMR_PRIu64 instead of OMR_PRId64. Here it is ultimately using omrstr_vprintf() so the format we want is "%llu".

kgibm commented 1 year ago

Okay, thanks. I'll test this out and open a PR.

keithc-ca commented 1 year ago

omrstr_vprintf() doesn't handle %I so that part just disappears.

keithc-ca commented 1 year ago

Okay, thanks. I'll test this out and open a PR.

Any updates to share on that PR?

kgibm commented 1 year ago

Any updates to share on that PR?

I got stuck building locally on Windows, but will try again today.

kgibm commented 1 year ago

Given the block on building on Windows 11, could I just submit a PR and one of you could spark a Jenkins build and then I could test that?

pshipton commented 1 year ago

You don't need a PR, if you have a branch on your fork let me know what it is and I will kick a build with it. You might want to rebase it on the latest.

pshipton commented 1 year ago

~Do you need to build on Windows? It's easier to build on Linux if you want to set that up.~ nm, I see this is a Windows problem.

kgibm commented 1 year ago

You don't need a PR, if you have a branch on your fork let me know what it is and I will kick a build with it. You might want to rebase it on the latest.

Thanks, please build this branch (rebased on latest): https://github.com/kgibm/openj9/tree/issue17319

pshipton commented 1 year ago

https://openj9-jenkins.osuosl.org/job/Pipeline-Build-Test-Personal/396/

kgibm commented 1 year ago

Thanks Peter.

Tested the build (with OMR_PRIu64) but now it just shows:

Approximate time to produce this dump: 64u ms

@keithc-ca Thoughts?

keithc-ca commented 1 year ago

That code should not use any of the OMR_PRIxxx macros; the format string is interpreted by OMR, not printf and friends. It should use "%llu" like line 681.

kgibm commented 1 year ago

I notice another part of the code has the same issue; I presume I should update that as well:

#if defined(J9VM_OPT_JITSERVER)
        if (NULL != jitConfig) {
                if (0 != jitConfig->clientUID) {
                        _OutputStream.writeCharacters("1CICLIENTID    Client UID ");
                        _OutputStream.writeInteger64(jitConfig->clientUID, "%" OMR_PRIu64);
                        _OutputStream.writeCharacters("\n");
                }
                if (0 != jitConfig->serverUID) {
                        _OutputStream.writeCharacters("1CISERVERID    Server UID ");
                        _OutputStream.writeInteger64(jitConfig->serverUID, "%" OMR_PRIu64);
                        _OutputStream.writeCharacters("\n");
                }
        }
#endif /* J9VM_OPT_JITSERVER */                                                                   
kgibm commented 1 year ago

@pshipton Please start a new personal build for https://github.com/kgibm/openj9/tree/issue17319

pshipton commented 1 year ago

https://openj9-jenkins.osuosl.org/job/Pipeline-Build-Test-Personal/397/

keithc-ca commented 1 year ago

I notice another part of the code has the same issue; I presume I should update that as well:

Yes, please and thanks.