Skip to content

8346059: [ASAN] awt_LoadLibrary.c reported compile warning ignoring return value of function by clang17 #22701

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
sendaoYan wants to merge 1 commit into from
Closed

8346059: [ASAN] awt_LoadLibrary.c reported compile warning ignoring return value of function by clang17 #22701

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion src/java.desktop/unix/native/libawt/awt/awt_LoadLibrary.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,9 @@ AWT_OnLoad(JavaVM *vm, void *reserved)
} else {
/* Get address of this library and the directory containing it. */
dladdr((void *)AWT_OnLoad, &dlinfo);
realpath((char *)dlinfo.dli_fname, buf);
if (realpath((char *)dlinfo.dli_fname, buf) == NULL) {
perror((char *)dlinfo.dli_fname);
Copy link
Member

@magicus magicus Dec 12, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Having error handling is better than no error handling, but is perror the best we can do?

OTOH, maybe it doesn't matter as much. Something would be very, very broken if this were to fail.

Copy link
Contributor

@prrace prrace Dec 12, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I saw this in another bug report too from the same submitter.
I don't know that I want to encourage this as a pattern, except in debug builds. better might be to just copy dlinfo.dli_fname into buf ..

Copy link
Member Author

@sendaoYan sendaoYan Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

perror will print the given message accrording the previous errno info to stderr, so this change will not change the original behaviour.

Copy link
Member Author

@sendaoYan sendaoYan Dec 13, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @prrace,

I saw this in another bug report too from the same submitter.

I did create 3 clang17 compile waring issues, the compile warings occur in different files, and maybe use should different solutions to solve the issue, so I create the separate issues.

Copy link
Member Author

@sendaoYan sendaoYan Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better might be to just copy dlinfo.dli_fname into buf

"Something would be very, very broken if this were to fail.", So I think perror maybe better, it will print which file and the reason get realpath fail.

Copy link
Member

@magicus magicus Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should I combine 3 similar issues(JDK-8346104/JDK-8346103/JDK-8346059) to 1 issue

I personally would probably have started out by making a single issue for these, since they all try to address the same new warning in clang, and are all in client code, and only split it up later if reviewers had requested it. But since you have created separate issues, I suggest you do not change things now. Also, the other two I believe need a more serious response; a write to a pipe has a somewhat more realistic chance at failure than this filename lookup.

Copy link
Member Author

@sendaoYan sendaoYan Dec 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry for the confuse. I mean this PR change nothing except it will print a message to stderr when realpath return NULL. Thanks magicus.

Copy link
Contributor

@prrace prrace Dec 17, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did you integrate this ?
It did not yet have my approval even though I was clearly reviewing it.
Nor did it have the required 2nd reviewer, in fact did it not have ANY reviewer from the client team which is responsible for this code.

Copy link
Member Author

@sendaoYan sendaoYan Dec 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @prrace , sorry for the rash integrated. I can backout or redo this change quickly if it's necessary. If I should redo or backport this PR, please let me known.

I will pay more attention for the future PRs.

Apologize for the rash integrated again.

Copy link
Member

@magicus magicus Dec 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@prrace I don't think it is reasonable for you to blame @sendaoYan for integrating. He had a review from a Reviewer, and he waited 24 hours after that before integrating. The general JDK rule is that one review is enough; Hotspot as a special case requires two. Your comment did not say that you wanted to review it further, nor did you clearly request any changes, just a vague idea of replacing realpath with a strcpy (which seems like much more risky, and potentially incorrect, change!)

The responsibility for this PR is mine. I had reviewed and approved the PR. I did consider if I should increase the number of reviewers and state that a client team reviewer was also needed, but decided against it. That was my decision as a reviewer. The PR was clearly unproblematic. It added some extra logging in case something gets terribly wrong. Without this fix, AWT would just break without any indication whatsoever if something did go wrong with realpath. With this fix, this unlikely problem will at least get some indication. I did not think this needed confirmation from the client team as well.

}
len = strlen(buf);
p = strrchr(buf, '/');

Expand Down