Skip to content
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

8214976: Warn about uses of functions replaced for portability #7248

Closed
wants to merge 6 commits into from

Conversation

hseigel
Copy link
Member

@hseigel hseigel commented Jan 27, 2022

Please review this new attempt to resolve JDK-8214976. This fix adds Pragmas to generate compilation errors, when using gcc, if calling a native system function instead of the os:: version of the function. The fix includes changes to calls in non-shared code because it is cleaner than adding PRAGMAs and, for some cases, the os:: version of a function has added value, such as asserts and RESTARTABLE. This fix slightly changes the signature of os::abort() so it wouldn't conflict with native abort() functions. Changes to Windows code is left for a future RFE.

This fix was tested with Mach5 tiers 1-2 on Linux, Mac OS, and Windows, Mach5 tiers 3-5 on Linux x64, and Mach5 builds of Zero, PPC, and s390.

Thanks, Harold


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8214976: Warn about uses of functions replaced for portability

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/7248/head:pull/7248
$ git checkout pull/7248

Update a local copy of the PR:
$ git checkout pull/7248
$ git pull https://git.openjdk.java.net/jdk pull/7248/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7248

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/7248.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Jan 27, 2022

👋 Welcome back hseigel! 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 openjdk bot added the rfr label Jan 27, 2022
@openjdk
Copy link

openjdk bot commented Jan 27, 2022

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

  • hotspot

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.

@openjdk openjdk bot added the hotspot label Jan 27, 2022
@mlbridge
Copy link

mlbridge bot commented Jan 27, 2022

Webrevs

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Harold,

Still have reservations about the awkwardness of this.

Quite a few comments below.

Shouldn't we generate a warning for all external functions for which there is an os:: replacement e.g. pread is called by read_at; gethostbyname is called by get_host_by_name; ...

Thanks,
David

@@ -2496,17 +2496,17 @@ bool os::dir_is_empty(const char* path) {
DIR *dir = NULL;
struct dirent *ptr;

dir = opendir(path);
dir = os::opendir(path);
Copy link
Member

@dholmes-ora dholmes-ora Jan 28, 2022

Choose a reason for hiding this comment

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

Just to clarify, as we are in the scope of the os class both opendir and os::opendir are the same thing here - and similarly for other code in the os class - right?

Choose a reason for hiding this comment

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

Yes, that's correct. So an unqualified opendir here should not trigger a forbidden warning.

Copy link
Member Author

@hseigel hseigel Feb 1, 2022

Choose a reason for hiding this comment

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

I removed "os:::" from the os class files.

static void abort(bool dump_core, void *siginfo, const void *context);
static void abort(bool dump_core = true);
static void abort(bool dump_core);
Copy link
Member

@dholmes-ora dholmes-ora Jan 28, 2022

Choose a reason for hiding this comment

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

I don't understand why the change to the default arg was needed. There should be no conflict between os::abort() and ::abort().

Copy link
Member Author

@hseigel hseigel Feb 1, 2022

Choose a reason for hiding this comment

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

I reverted the abort() changes. Thanks for correcting this.

// AIX also needs a 64 bit NULL to work as a null address pointer.
// Most system includes on AIX would define it as an int 0 if not already defined with one
// exception: /usr/include/dirent.h will unconditionally redefine NULL to int 0 again.
// In this case you need to copy the following defines to a position after #include <dirent.h>
#include <dirent.h>
#ifdef _LP64
#undef NULL
#define NULL 0L
#else
#ifndef NULL
#define NULL 0
#endif
#endif
Copy link
Member

@dholmes-ora dholmes-ora Jan 28, 2022

Choose a reason for hiding this comment

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

This is really ugly just because we include dirent.h so we can add the warning for a few functions; and even uglier because it is only needed for AIX, and even uglier still because based on the existing code we only compile AIX with xlc - no? Otherwise we would already need this hack for gcc.

Choose a reason for hiding this comment

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

We only compile AIX with xclang these days. I don't know how our "xlc" compiler platform mechanism interacts with our "gcc" (which is really both gcc and clang) compiler platform, or if it interacts, or if it should. But none of that matters for the dirent.h problem. The problem there is that it's a system header, irrespective of what compiler is being used, and it has this problem. So whether we need this NULL cruft here depends on whether AIX with xclang uses this file or not. One option would be to just not deal with the dirent stuff yet, saving that for a followup focused on that problem.

Copy link
Member

@tstuefe tstuefe Jan 29, 2022

Choose a reason for hiding this comment

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

Sorry, I'm confused. We build AIX with xlc. I don't believe we even include this file on AIX. How does this help AIX?

Copy link
Member Author

@hseigel hseigel Feb 1, 2022

Choose a reason for hiding this comment

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

I removed the changes for the dirent functions and removed the above code. I also reverted all changes to os_aix.cpp.

Copy link
Member

@tstuefe tstuefe Feb 2, 2022

Choose a reason for hiding this comment

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

Thank you @hseigel !

@@ -611,13 +611,16 @@ void fileStream::flush() {
}
}

PRAGMA_DIAG_PUSH
PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION(write);
Copy link
Member

@dholmes-ora dholmes-ora Jan 28, 2022

Choose a reason for hiding this comment

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

Why do we not call os::write here?

Copy link
Member Author

@hseigel hseigel Feb 1, 2022

Choose a reason for hiding this comment

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

fixed

FORBID_C_FUNCTION(void flockfile(FILE*), "use os::flockfile");
FORBID_C_FUNCTION(FILE* fopen(const char*, const char*), "use os::fopen");
FORBID_C_FUNCTION(int fsync(int), "use os::fsync");
FORBID_C_FUNCTION(int ftruncate(int, off_t), "use os::ftruncate");
Copy link
Member

@dholmes-ora dholmes-ora Jan 28, 2022

Choose a reason for hiding this comment

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

Shouldn't this be ftruncate for BSD and ftruncate64 for other Posix (not sure what Windows has)?

Copy link
Member Author

@hseigel hseigel Feb 1, 2022

Choose a reason for hiding this comment

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

Platform agnostic code would call ftruncate(), not ftruncate64(). So I think this is correct as is.

Copy link
Member

@dholmes-ora dholmes-ora Feb 2, 2022

Choose a reason for hiding this comment

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

You need to enable the warning for the function that we would use, which we are not supposed to use and that would be ftruncate64 on Linux.

FORBID_C_FUNCTION(int fsync(int), "use os::fsync");
FORBID_C_FUNCTION(int ftruncate(int, off_t), "use os::ftruncate");
FORBID_C_FUNCTION(void funlockfile(FILE *), "use os::funlockfile");
FORBID_C_FUNCTION(off_t lseek(int, off_t, int), "use os::lseek");
Copy link
Member

@dholmes-ora dholmes-ora Jan 28, 2022

Choose a reason for hiding this comment

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

Similarly there should be a lseek64 definition too.

Copy link
Member Author

@hseigel hseigel Feb 1, 2022

Choose a reason for hiding this comment

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

Like ftruncate(), platform agnostic code would call lseek(), not lseek64(). So I think this is correct as is.

Copy link
Member

@dholmes-ora dholmes-ora Feb 2, 2022

Choose a reason for hiding this comment

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

I disagree - you are not enabling the warnings for all the functions that would be used.

@kimbarrett
Copy link

kimbarrett commented Jan 28, 2022

Shouldn't we generate a warning for all external functions for which there is an os:: replacement e.g. pread is called by read_at; gethostbyname is called by get_host_by_name; ...

Thanks, David

That seems like a good goal, but I don't think we have to get complete coverage in one PR.

@@ -2496,17 +2496,17 @@ bool os::dir_is_empty(const char* path) {
DIR *dir = NULL;
struct dirent *ptr;

dir = opendir(path);
dir = os::opendir(path);

Choose a reason for hiding this comment

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

Yes, that's correct. So an unqualified opendir here should not trigger a forbidden warning.

#else

#define FORBID_C_FUNCTION(signature, alternative)
#define PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION(name)

Choose a reason for hiding this comment

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

These aren't needed. The default empty definitions in compilerWarnings.hpp cover this case.

Copy link
Member Author

@hseigel hseigel Feb 1, 2022

Choose a reason for hiding this comment

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

fixed

FORBID_C_FUNCTION(char* strerror(int), "use os::strerror");
FORBID_C_FUNCTION(ssize_t write(int, const void*, size_t ), "use os::write");

FORBID_C_FUNCTION(char* strtok(char*, const char*), "use strtok_r");

Choose a reason for hiding this comment

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

Some of these functions are portable and ought to be forbidden in a platform agnostic location, so the restriction also applies if/when we have real support on other platforms. I think almost none are gcc (or clang) specific, but are instead probably posix and not windows, so maybe should go in a different place as well. Basically I think the structure / placement considerations need some more work.

Copy link
Member

@dholmes-ora dholmes-ora Feb 15, 2022

Choose a reason for hiding this comment

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

Can we put the list of forbidden functions in os.hpp?

// AIX also needs a 64 bit NULL to work as a null address pointer.
// Most system includes on AIX would define it as an int 0 if not already defined with one
// exception: /usr/include/dirent.h will unconditionally redefine NULL to int 0 again.
// In this case you need to copy the following defines to a position after #include <dirent.h>
#include <dirent.h>
#ifdef _LP64
#undef NULL
#define NULL 0L
#else
#ifndef NULL
#define NULL 0
#endif
#endif

Choose a reason for hiding this comment

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

We only compile AIX with xclang these days. I don't know how our "xlc" compiler platform mechanism interacts with our "gcc" (which is really both gcc and clang) compiler platform, or if it interacts, or if it should. But none of that matters for the dirent.h problem. The problem there is that it's a system header, irrespective of what compiler is being used, and it has this problem. So whether we need this NULL cruft here depends on whether AIX with xclang uses this file or not. One option would be to just not deal with the dirent stuff yet, saving that for a followup focused on that problem.

@hseigel
Copy link
Member Author

hseigel commented Feb 1, 2022

The second commit contains minor changes in response to review comments. The changes include removing unneeded "os::" from os_*.cpp files, reverting all changes to os_aix.cpp, reverting changes to abort(), removing "#include <dirent.h>" and related funcations from compilerWarnings_gcc.hpp, and changing "::write()" to "os::write()" in ostream.cpp.

This update does not address bigger issues such as structure and placement concerns and whether or not to do this change at all.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Harold,

This is looking better - thanks - but I think the lseek64 situation needs handling differently.

Thanks,
David

@@ -4921,14 +4921,18 @@ int os::create_binary_file(const char* path, bool rewrite_existing) {
return ::open64(path, oflags, S_IREAD | S_IWRITE);
}

off64_t call_lseek64(int fd, off64_t offset, int whence) {
Copy link
Member

@dholmes-ora dholmes-ora Feb 14, 2022

Choose a reason for hiding this comment

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

I think it would be better to just change the lseek64 calls to os::lseek rather than introduce this wrapper function.

Copy link
Member Author

@hseigel hseigel Feb 15, 2022

Choose a reason for hiding this comment

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

Thanks David. I changed the code to call os::lseek().

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Thanks Harold,

This seems acceptable to me now. Only remaining issue is the placement issue Kim raised - see query/suggestion below.

David

FORBID_C_FUNCTION(char* strerror(int), "use os::strerror");
FORBID_C_FUNCTION(ssize_t write(int, const void*, size_t ), "use os::write");

FORBID_C_FUNCTION(char* strtok(char*, const char*), "use strtok_r");
Copy link
Member

@dholmes-ora dholmes-ora Feb 15, 2022

Choose a reason for hiding this comment

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

Can we put the list of forbidden functions in os.hpp?

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 16, 2022

@hseigel This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 13, 2022

@hseigel This pull request has been inactive for more than 8 weeks and will now be automatically closed. If you would like to continue working on this pull request in the future, feel free to reopen it! This can be done using the /open pull request command.

@bridgekeeper bridgekeeper bot closed this Apr 13, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot rfr
4 participants