RFR: 8214976: Warn about uses of functions replaced for portability

[David Holmes]

On Thu, 27 Jan 2022 19:18:10 GMT, Harold Seigel <hseigel at openjdk.org> wrote:

> 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

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

src/hotspot/os/aix/os_aix.cpp line 2499:

> 2497:   struct dirent *ptr;
> 2498: 
> 2499:   dir = os::opendir(path);

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?

src/hotspot/share/runtime/os.hpp line 533:

> 531:   // platforms that support such things.  This calls shutdown() and then aborts.
> 532:   static void abort(bool dump_core, void *siginfo, const void *context);
> 533:   static void abort(bool dump_core);

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

src/hotspot/share/utilities/compilerWarnings_gcc.hpp line 80:

> 78:     #define NULL 0
> 79:   #endif
> 80: #endif

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.

src/hotspot/share/utilities/compilerWarnings_gcc.hpp line 97:

> 95: FORBID_C_FUNCTION(FILE*    fopen(const char*, const char*),           "use os::fopen");
> 96: FORBID_C_FUNCTION(int      fsync(int),                                "use os::fsync");
> 97: FORBID_C_FUNCTION(int      ftruncate(int, off_t),                     "use os::ftruncate");

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

src/hotspot/share/utilities/compilerWarnings_gcc.hpp line 99:

> 97: FORBID_C_FUNCTION(int      ftruncate(int, off_t),                     "use os::ftruncate");
> 98: FORBID_C_FUNCTION(void     funlockfile(FILE *),                       "use os::funlockfile");
> 99: FORBID_C_FUNCTION(off_t    lseek(int, off_t, int),                    "use os::lseek");

Similarly there should be a lseek64 definition too.

src/hotspot/share/utilities/ostream.cpp line 615:

> 613: 
> 614: PRAGMA_DIAG_PUSH
> 615: PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION(write);

Why do we not call os::write here?

-------------

PR: https://git.openjdk.java.net/jdk/pull/7248