8329862: libjli GetApplicationHome cleanups and enhance jli tracing #18699

MBaesken · 2024-04-09T15:28:08Z

We have already good JLI tracing capabilities. But GetApplicationHome and GetApplicationHomeFromDll lack some tracing and should be enhanced.

Progress

Change must not contain extraneous whitespace
Commit message must refer to an issue
Change must be properly reviewed (2 reviews required, with at least 1 Reviewer, 1 Author)

Issue

JDK-8329862: libjli GetApplicationHome cleanups and enhance jli tracing (Bug - P4)

Reviewers

Christoph Langer (@RealCLanger - Reviewer)
Thomas Stuefe (@tstuefe - Reviewer) ⚠️ Review applies to 4d998244

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/18699/head:pull/18699
$ git checkout pull/18699

Update a local copy of the PR:
$ git checkout pull/18699
$ git pull https://git.openjdk.org/jdk.git pull/18699/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 18699

View PR using the GUI difftool:
$ git pr show -t 18699

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/18699.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-04-09T15:28:41Z

👋 Welcome back mbaesken! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

openjdk · 2024-04-09T15:29:40Z

@MBaesken This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8329862: libjli GetApplicationHome cleanups and enhance jli tracing

Reviewed-by: clanger, stuefe

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 243 new commits pushed to the master branch:

006f090: 8331150: RISC-V: Fix "bad AD file" bug
a407dc9: 8327512: JShell does not work correctly when a class named Object or Throwable is defined
ea06129: 8322847: java.lang.classfile.BufWriter should specify @throws for its writeXXX methods
b3bcc49: 8330181: Move PcDesc cache from nmethod header
1c238d4: 8319990: Update CLDR to Version 45.0
ce9eac3: 8331113: createJMHBundle.sh support configurable maven repo mirror
5af6b45: 8330615: avoid signed integer overflows in zip_util.c readCen / hashN
4dfaa9b: 8329805: Deprecate for removal ObjectOutputStream.PutField.write
b9927aa: 8329194: Cleanup Type::cmp definition and usage
c944201: 8331031: unify os::dont_yield and os::naked_yield across Posix platforms
... and 233 more: https://git.openjdk.org/jdk/compare/5fb5e6c8f04e325cbb782431d51251edde4c2618...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

openjdk · 2024-04-09T15:30:05Z

@MBaesken The following label will be automatically applied to this pull request:

core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

mlbridge · 2024-04-09T15:33:16Z

Webrevs

MBaesken · 2024-04-09T15:42:08Z

Btw. I noticed that GetModuleFileName return value is not checked, should this be done (at other locations in the JDk codebase it is done) ?
See https://learn.microsoft.com/en-us/windows/win32/api/libloaderapi/nf-libloaderapi-getmodulefilenamea

AlanBateman · 2024-04-09T17:26:46Z

This looks very ad hoc. Not opposed to expanding the set of trace messages when this env variable is set but what is proposed here looks like trace messages that are left over from a debugging session. If we expand the tracing then I think it should be consistent with the existing tracing.

MBaesken · 2024-04-10T07:16:49Z

If we expand the tracing then I think it should be consistent with the existing tracing.

I just added some tracing calls where I missed them. Do you think I should do some more adjustments ?
Btw. is the GetApplicationHomeFromDll (which in fact is getting the image location from the jli shared lib location) approach for the launcher documented somewhere or is this something that 'just works' for a log time ?

MBaesken · 2024-04-15T12:47:08Z

If we expand the tracing then I think it should be consistent with the existing tracing.

What exactly do you see as inconsistent ?

RealCLanger

To me this looks useful, although maybe the overall JLI tracing could be revisited.

RealCLanger · 2024-04-16T09:09:00Z

src/java.base/windows/native/libjli/java_md.c

@@ -321,6 +323,8 @@ GetJREPath(char *path, jint pathsize)
        }
    }

+    JLI_TraceLauncher("GetJREPath - attempt to get JRE location from shared lib of the image\n");


Maybe add a trace also in the USE_REGISTRY_LOOKUP section

Hi Christoph, seems the USE_REGISTRY_LOOKUP is currently unused (at least without additional defines that are not present usually)

src/java.base/windows/native/libjli/java_md.c:52:#ifdef USE_REGISTRY_LOOKUP src/java.base/windows/native/libjli/java_md.c:333:#ifdef USE_REGISTRY_LOOKUP

I am not sure if this even works any more. Maybe Alan could comment on this ?

I am not sure if this even works any more. Maybe Alan could comment on this ?

The GetPublicJREHome function was removed at some point, I think JDK 9, as it didn't make sense to have in the OpenJDK project. However, Oracle installer did depend on it and will require a bit of effort to see if removing it now will cause an issue now or not.

@MBaesken I checked into the building with -DUSE_REGISTRY_LOOKUP as that compiled in code that the Oracle installer needed when it copied java.exe to somewhere. This is indeed left over code and can be removed from java_md.c.

Hi Alan, thanks for checking this !
I removed the USE_REGISTRY_LOOKUP usages.

RealCLanger · 2024-04-16T09:14:50Z

If we expand the tracing then I think it should be consistent with the existing tracing.

What exactly do you see as inconsistent ?

Maybe the output of the tracing should look similar to other traces done through JLI_TraceLauncher? E.g. not mention method names but just tell what the program is doing... ?

MBaesken · 2024-04-16T09:17:49Z

What exactly do you see as inconsistent ?

Maybe the output of the tracing should look similar to other traces done through JLI_TraceLauncher? E.g. not mention method names but just tell what the program is doing... ?

Okay, makes sense ! I will change that to make the output similar to existing traces.

AlanBateman · 2024-04-16T10:20:23Z

I think this is way too ad hoc and looks like lefts over from a debugging session. So I don't think it should be integrated without stepping back and thinking more about what this tracing option is intended for.

AlanBateman · 2024-04-16T10:21:44Z

/reviewers 2

openjdk · 2024-04-16T10:21:55Z

@AlanBateman
The total number of required reviews for this PR (including the jcheck configuration and the last /reviewers command) is now set to 2 (with at least 1 Reviewer, 1 Author).

MBaesken · 2024-04-16T14:36:31Z

I think this is way too ad hoc and looks like lefts over from a debugging session. So I don't think it should be integrated without stepping back and thinking more about what this tracing option is intended for.

Currently there seem to be two (with the unused registry stuff 3, if it is still valid ?) approaches to find the 'JRE' location (should this better be named image location?) - using the launcher exe location and using the location of the libjli shared lib.
Unfortunately those approaches are not well supported by JLI-traces , this is what the change is about.
We could probably adjust/reduce the added tracing code but I would like to know a bit more about the concerns raised.

MBaesken · 2024-04-17T15:30:15Z

I adjusted the trace messages a bit to make the coding more consistent to the existing Jli trace code,
I removed the print of the function name because this is usually avoided in the existing cases.
I removed the 'did not succeed' Jli trace messages in GetJREPath because in case of success we would getJli trace output ('JRE path is ') so missing success output means it failed.

RealCLanger · 2024-04-19T09:20:27Z

src/java.base/unix/native/libjli/java_md_common.c

@@ -82,6 +82,8 @@ GetApplicationHome(char *buf, jint bufsize)
 {
    const char *execname = GetExecName();
    if (execname != NULL) {
+        JLI_TraceLauncher("Launcher executable path is %s\n", execname);


This trace seems a bit unsymmetric to its Windows counterpart. Maybe it should be left out here, too, since there is tracing in GetJREPath.

This trace seems a bit unsymmetric to its Windows counterpart. Maybe it should be left out here, too, since there is tracing in GetJREPath.

I removed the JLI trace output for the launcher exe path.

RealCLanger · 2024-04-19T09:21:49Z

Generally this looks more consistent now. Let's see what @AlanBateman thinks... 😄

MBaesken · 2024-04-22T11:30:41Z

Hi, any additional comments / reviews ?
Thanks Matthias

AlanBateman · 2024-04-22T11:57:19Z

Hi, any additional comments / reviews ? Thanks Matthias

It still looks like left over trace messages from a debugging session, need to think about about what tracing make sense here.

magicus · 2024-04-22T17:19:14Z

What is the use case for this tracing functionality? I recently was quite helped by it when debugging static java launching. And in that case, the more logging the better. But that sounds like a very special case. Is this something that end users are supposed to use to help solve problems?

RealCLanger · 2024-04-22T17:35:27Z

It helped to understand the issue behind https://bugs.openjdk.org/browse/JDK-8329653

MBaesken · 2024-04-23T11:50:35Z

But that sounds like a very special case.

Not sure if it is really such a special case. Currently both modes (getting the image path from launcher binary path , and getting the image path from 'the dll' / GetApplicationHomeFromDll (libjli.so on Linux) exist and are to my knowledge supported. At least one jtreg test fails when the GetApplicationHomeFromDll does not work any more.
So I think it is natural that both modes are also supported/augmented by some tracing.
We could probably reduce the trace message(s) added to just one if this is desired .

magicus · 2024-04-23T13:38:47Z

I agree; the launcher uses two different modes for discovery. If you want to troubleshoot, knowing which of these are attempted is helpful.

@AlanBateman If this is not acceptable to add, I must once again iterate my question: What is the use case for the tracing functionality then?

AlanBateman · 2024-04-23T14:08:06Z

@AlanBateman If this is not acceptable to add, I must once again iterate my question: What is the use case for the tracing functionality then?

The tracing was mostly for the options and timing when it was originally added. I don't object to expanding the tracing. The issue is that the proposed additions are inconsistent with the existing trace messages, e.g. GetJREPath's existing trace messages use "JRE path", the proposed messages switch to "JRE location".

BTW: the vestiges of the "JRE" should really be removed. The GetJREPath function has no business looking for jre/lib as that location does not exist since JDK 9.

MBaesken · 2024-04-23T14:29:05Z

I adjusted the output ot 'JRE path' .

The GetJREPath function has no business looking for jre/lib as that location does not exist since JDK 9.

Is the at/below

/* Does the app ship a private JRE in <apphome>/jre directory? */

part meant? This looks indeed obsolete .

MBaesken · 2024-04-24T13:55:46Z

I changed the added output to 'JRE path' to have more consistency with the existing JLI trace messages.

tstuefe

LGTM

AlanBateman · 2024-04-25T12:57:48Z

/* Does the app ship a private JRE in <apphome>/jre directory? */

part meant? This looks indeed obsolete .

Yes, this is code that doesn't make sense since JDK 9 and should be removed/cleanup at some point. I suspect we had to leave it at the time because of the issue of installers copying java.exe into a shared location and expecting it to work with any JRE or JDK release, something that doesn't make sense now of course.

MBaesken · 2024-04-25T13:17:51Z

I removed the mentioned 'private JRE' check and also the related size check.

RealCLanger · 2024-04-25T14:07:21Z

Still looks good. We should maybe change the synopsis (title) of the bug to something like "libjli GetApplicationHome cleanups"?

tstuefe · 2024-04-26T07:00:07Z

Change still good

Still looks good. We should maybe change the synopsis (title) of the bug to something like "libjli GetApplicationHome cleanups"?

Well, it does enhance tracing, so the title is not lying :)

MBaesken · 2024-04-26T07:46:14Z

Still looks good. We should maybe change the synopsis (title) of the bug to something like "libjli GetApplicationHome cleanups"?

I changed the synopsis because it was indeed now not really reflecting any more what the change does.

MBaesken · 2024-04-26T08:11:38Z

Thanks for the reviews !

/integrate

openjdk · 2024-04-26T08:12:11Z

Going to push as commit 377f2e5.
Since your change was applied there have been 243 commits pushed to the master branch:

006f090: 8331150: RISC-V: Fix "bad AD file" bug
a407dc9: 8327512: JShell does not work correctly when a class named Object or Throwable is defined
ea06129: 8322847: java.lang.classfile.BufWriter should specify @throws for its writeXXX methods
b3bcc49: 8330181: Move PcDesc cache from nmethod header
1c238d4: 8319990: Update CLDR to Version 45.0
ce9eac3: 8331113: createJMHBundle.sh support configurable maven repo mirror
5af6b45: 8330615: avoid signed integer overflows in zip_util.c readCen / hashN
4dfaa9b: 8329805: Deprecate for removal ObjectOutputStream.PutField.write
b9927aa: 8329194: Cleanup Type::cmp definition and usage
c944201: 8331031: unify os::dont_yield and os::naked_yield across Posix platforms
... and 233 more: https://git.openjdk.org/jdk/compare/5fb5e6c8f04e325cbb782431d51251edde4c2618...master

Your commit was automatically rebased without conflicts.

openjdk · 2024-04-26T08:12:18Z

@MBaesken Pushed as commit 377f2e5.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

AlanBateman · 2024-04-26T08:38:16Z

I removed the mentioned 'private JRE' check and also the related size check.

Good, I'll look at it as soon as I can. I suspect we'll need some follow on issues as there are several issues here.

JDK-8329862

128300a

openjdk bot added the rfr Pull request is ready for review label Apr 9, 2024

openjdk bot added the core-libs core-libs-dev@openjdk.org label Apr 9, 2024

openjdk bot changed the title ~~JDK-8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing~~ 8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing Apr 10, 2024

RealCLanger approved these changes Apr 16, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 16, 2024

openjdk bot removed the ready Pull request is ready to be integrated label Apr 16, 2024

adjust trace messages

344d1b8

remove obsolete USE_REGISTRY_LOOKUP usages

ddb3e0f

RealCLanger reviewed Apr 19, 2024

View reviewed changes

remove launcher executable path trace output

8b7a9d5

adjust output

4d99824

tstuefe approved these changes Apr 25, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Apr 25, 2024

remove /jre path check

4d8b680

MBaesken changed the title ~~8329862: GetApplicationHome and GetApplicationHomeFromDll enhance jli tracing~~ 8329862: libjli GetApplicationHome cleanups and enhance jli tracing Apr 26, 2024

RealCLanger approved these changes Apr 26, 2024

View reviewed changes

openjdk bot added the integrated Pull request has been integrated label Apr 26, 2024

openjdk bot closed this Apr 26, 2024

openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Apr 26, 2024

8329862: libjli GetApplicationHome cleanups and enhance jli tracing #18699

8329862: libjli GetApplicationHome cleanups and enhance jli tracing #18699

Conversation

MBaesken commented Apr 9, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Apr 9, 2024

openjdk bot commented Apr 9, 2024 • edited Loading

openjdk bot commented Apr 9, 2024

mlbridge bot commented Apr 9, 2024 • edited Loading

Webrevs

MBaesken commented Apr 9, 2024

AlanBateman commented Apr 9, 2024

MBaesken commented Apr 10, 2024

MBaesken commented Apr 15, 2024

RealCLanger left a comment

Choose a reason for hiding this comment

RealCLanger Apr 16, 2024

Choose a reason for hiding this comment

MBaesken Apr 16, 2024

Choose a reason for hiding this comment

AlanBateman Apr 16, 2024

Choose a reason for hiding this comment

AlanBateman Apr 18, 2024

Choose a reason for hiding this comment

MBaesken Apr 18, 2024

Choose a reason for hiding this comment

RealCLanger commented Apr 16, 2024

MBaesken commented Apr 16, 2024

AlanBateman commented Apr 16, 2024

AlanBateman commented Apr 16, 2024

openjdk bot commented Apr 16, 2024

MBaesken commented Apr 16, 2024

MBaesken commented Apr 17, 2024

RealCLanger Apr 19, 2024

Choose a reason for hiding this comment

MBaesken Apr 19, 2024

Choose a reason for hiding this comment

RealCLanger commented Apr 19, 2024

MBaesken commented Apr 22, 2024

AlanBateman commented Apr 22, 2024

magicus commented Apr 22, 2024

RealCLanger commented Apr 22, 2024

MBaesken commented Apr 23, 2024

magicus commented Apr 23, 2024

AlanBateman commented Apr 23, 2024

MBaesken commented Apr 23, 2024

MBaesken commented Apr 24, 2024

tstuefe left a comment

Choose a reason for hiding this comment

AlanBateman commented Apr 25, 2024 • edited Loading

MBaesken commented Apr 25, 2024

RealCLanger commented Apr 25, 2024

tstuefe commented Apr 26, 2024

MBaesken commented Apr 26, 2024

MBaesken commented Apr 26, 2024

openjdk bot commented Apr 26, 2024

openjdk bot commented Apr 26, 2024

AlanBateman commented Apr 26, 2024

MBaesken commented Apr 9, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Apr 9, 2024 •

edited

Loading

mlbridge bot commented Apr 9, 2024 •

edited

Loading

AlanBateman commented Apr 25, 2024 •

edited

Loading