-
Notifications
You must be signed in to change notification settings - Fork 5.8k
JDK-8334217 : [AIX] Misleading error messages after JDK-8320005 #19887
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.
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
Conversation
👋 Welcome back sroy! A progress list of the required criteria for merging this PR into |
@suchismith1993 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:
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 230 new commits pushed to the
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. As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@JoKern65, @MBaesken) but any other Committer may sponsor as well. ➡️ To flag this PR as ready for integration with the above commit message, type |
@suchismith1993 The following label will be automatically applied to this pull request:
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. |
49ad3b0
to
1ec92b5
Compare
Webrevs
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now
Are these strings not locale dependent? How would this work with a different locale? |
In principle this might be the case, but our special code does the existence check beforehand (you know the 'loading a dll twice without getting a new handle') and this check returns the string as is. See |
Okay. But grepping for an english error string verbatim seems really iffy. @suchismith1993 Please factor out the string at least, e.g. as a common define. |
You mean using a #define ? or declaring a string --> const error_report[] = "No Such File or Directory"; |
What I meant is to define the string somewhere - be it a constexpr or a define - and pull that definition into both places where its used. Tying important implementation decisions to scanning error message literals is super breaky and violates the principle of least surprise. Someone may change the error message in the future, maybe to enrich it. And nobody would expect someone to scan that message literally and base decisions on that. Better would be introducing clear error numbers. But by using a central common definition for the string you at least minimize the chance of this breaking if someone changes the error messages, and signal too the code reader that the message may be used somewhere else. |
Great point. Thanks, thats good learning. |
No, since you still only use it in one place, and in porting_aix.cpp we still use a literal string. |
Hi Suchi, what was the problem with error number? I would enrich the Then you can return the errno from Aix_dlopen in two places
and
In os_aix.cpp you have to add and modify the following
|
#16604 (comment) This was the discussion |
Yes, that's right. But my new proposal follows the points mentioned in the discussion above. A pointer to an integer is passed down to exactly the two places, where the |
Yes. So should I wait till you push it out, need to mark it as dependent on you change then ? |
No. This was meant as a sketch for you how YOU can solve the problem without the string hassle. |
Where is the errno getting set here ? |
errno is an implicit global variable set/modified by almost all runtime APIs in case of an error. In our case errno is set/modified in the ::dlopen() call. So the logic is first reset errno to 0 (no error), then call the API (here ::dlopen) which will set errno in case of an error with one of the values described in the APIs man-page, and directly after the API call but at least before the next API call save the content of errno into a local variable for further evaluation (here we return the value of eno to your function for the eno == ENOENT check). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It was intentional, that I used the variable name eno or if you want err_no but not errno to transport the content of the global errno variable. Have you compiled this beforehand?
I will mark my changes were needed.
Not it was not intentional. I corrected it after i understood your comment about errno. |
src/hotspot/os/aix/os_aix.cpp
Outdated
@@ -113,6 +113,7 @@ | |||
#error Hotspot on AIX must be compiled with -D_LARGE_FILES | |||
#endif | |||
|
|||
#define ERROR_NO_SUCH_FILE "No such file or directory" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not needed anymore. Remove the #define line.
src/hotspot/os/aix/os_aix.cpp
Outdated
@@ -988,7 +989,7 @@ bool os::dll_address_to_library_name(address addr, char* buf, | |||
return true; | |||
} | |||
|
|||
static void* dll_load_library(const char *filename, char *ebuf, int ebuflen) { | |||
static void* dll_load_library(const char *filename, int *errno, char *ebuf, int ebuflen) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use the global errno variable name here, instead use *eno
src/hotspot/os/aix/os_aix.cpp
Outdated
@@ -1016,7 +1017,7 @@ static void* dll_load_library(const char *filename, char *ebuf, int ebuflen) { | |||
void* result; | |||
const char* error_report = nullptr; | |||
JFR_ONLY(NativeLibraryLoadEvent load_event(filename, &result);) | |||
result = Aix_dlopen(filename, dflags, &error_report); | |||
result = Aix_dlopen(filename, errno, dflags, &error_report); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use the global errno variable name here, instead use *eno
src/hotspot/os/aix/os_aix.cpp
Outdated
snprintf(pointer_to_dot, sizeof(old_extension), "%s", new_extension); | ||
result = dll_load_library(file_path, ebuf, ebuflen); | ||
result = dll_load_library(file_path, &errno, ebuf, ebuflen); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do not use the global errno variable name here, instead use &eno
src/hotspot/os/aix/porting_aix.cpp
Outdated
@@ -1035,7 +1035,7 @@ static bool search_file_in_LIBPATH(const char* path, struct stat64x* stat) { | |||
// specific AIX versions for ::dlopen() and ::dlclose(), which handles the struct g_handletable | |||
// This way we mimic dl handle equality for a library | |||
// opened a second time, as it is implemented on other platforms. | |||
void* Aix_dlopen(const char* filename, int Flags, const char** error_report) { | |||
void* Aix_dlopen(const char* filename, int *eno, int Flags, const char** error_report) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you also have to adapt the Aix_dlopen()
declaration in the header file porting_aix.hpp
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok. I understood. The reason i was not getting error was i was compiling on Linux machine. I thought the jdk would throw errors atleast.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please switch Flags and eno
old: void* Aix_dlopen(const char* filename, int eno, int Flags, const char* error_report) {
new: void* Aix_dlopen(const char* filename, int Flags, int eno, const char* error_report) {
and I have to emphasize please also add this additional int *eno
to the declaration of Aix_dlopen()
in the porting_aix.hpp
file
src/hotspot/os/aix/os_aix.cpp
Outdated
@@ -1016,7 +1016,7 @@ static void* dll_load_library(const char *filename, char *ebuf, int ebuflen) { | |||
void* result; | |||
const char* error_report = nullptr; | |||
JFR_ONLY(NativeLibraryLoadEvent load_event(filename, &result);) | |||
result = Aix_dlopen(filename, dflags, &error_report); | |||
result = Aix_dlopen(filename, eno, dflags, &error_report); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please switch dflags and eno
old: result = Aix_dlopen(filename, eno, dflags, &error_report);
new: result = Aix_dlopen(filename, dflags, eno, &error_report);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ok. why are we switching the order of arguments / parameters ? is it some order of the arguments to be processed ?
src/hotspot/os/aix/porting_aix.cpp
Outdated
@@ -1035,7 +1035,7 @@ static bool search_file_in_LIBPATH(const char* path, struct stat64x* stat) { | |||
// specific AIX versions for ::dlopen() and ::dlclose(), which handles the struct g_handletable | |||
// This way we mimic dl handle equality for a library | |||
// opened a second time, as it is implemented on other platforms. | |||
void* Aix_dlopen(const char* filename, int Flags, const char** error_report) { | |||
void* Aix_dlopen(const char* filename, int *eno, int Flags, const char** error_report) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please switch Flags and eno
old: void* Aix_dlopen(const char* filename, int eno, int Flags, const char* error_report) {
new: void* Aix_dlopen(const char* filename, int Flags, int eno, const char* error_report) {
and I have to emphasize please also add this additional int *eno
to the declaration of Aix_dlopen()
in the porting_aix.hpp
file
src/hotspot/os/aix/porting_aix.cpp
Outdated
@@ -1035,7 +1035,7 @@ static bool search_file_in_LIBPATH(const char* path, struct stat64x* stat) { | |||
// specific AIX versions for ::dlopen() and ::dlclose(), which handles the struct g_handletable | |||
// This way we mimic dl handle equality for a library | |||
// opened a second time, as it is implemented on other platforms. | |||
void* Aix_dlopen(const char* filename, int *eno, int Flags, const char** error_report) { | |||
void* Aix_dlopen(const char* filename,int Flags, int *eno, const char** error_report) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please insert a space between ',' and 'int Flags'.
Otherwise it looks good now and I will check with a testbuild.
Thanks Suchi. For me it looks good now. Test build with your changes also succeeded. I will wait for the test runs next night and approve then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM now and test runs succeeded.
Thanks. @tstuefe Could you kindly provide a review. |
/integrate |
@suchismith1993 |
@JoKern65 Could you sponsor this ? |
/sponsor |
Going to push as commit 8713628.
Your commit was automatically rebased without conflicts. |
@MBaesken @suchismith1993 Pushed as commit 8713628. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
dll_load_library() call fails is not analyzed (the ebuf content is ignored)
dll_load_library does not analyze the contents of ebuf which leads to misleading error message when library loading fails.
We now call the second dll_load_library() only if the first returns with a 'No such file or directory' error message.
If the first dll_load_library() found the library is not able to load it by any reason, we do not try again with a .a extension.
JDK-8334217
Progress
Issue
Reviewers
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/19887/head:pull/19887
$ git checkout pull/19887
Update a local copy of the PR:
$ git checkout pull/19887
$ git pull https://git.openjdk.org/jdk.git pull/19887/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 19887
View PR using the GUI difftool:
$ git pr show -t 19887
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/19887.diff
Webrev
Link to Webrev Comment