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

[Kim Barrett]

On Tue, 4 Jan 2022 19:23:24 GMT, Harold Seigel <hseigel at openjdk.org> wrote:

> Please review this change for JDK-8214976.  This change adds attribute warnings to header file compilerWarnings.hpp so that compilation warnings get issued when certain system functions are called directly, instead of hotspot's os:: versions of the functions.  Many additional files were changed because of compilation warnings resulting from the compilerWarnings.hpp changes.
> 
> A sample warning is:
> 
> .../open/test/hotspot/gtest/logging/test_log.cpp:63:19: error: call to 'fopen' declared with attribute warning: use os::fopen [-Werror=attribute-warning]
>    63 |   FILE* fp = fopen(TestLogFileName, "r");
>       |              ~~~~~^~~~~~~~~~~~~~~~~~~~~~
> 
> 
> Note that changing src/hotspot/os/linux/gc/z/zMountPoint_linux.cpp to call os:: functions requires adding "#include "runtime/os.hpp" and caused test gc/z/TestAllocateHeapAt.java to fail.  So, for now, I just added PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION to zMountPoint_linux.cpp.  There's a similar issue with gtest/logging/test_logDecorators.cpp.
> 
> Attribute warnings for additional functions, such as malloc(), were not included in this change because they require lots of source code changes.
> 
> This change was tested by running mach5 tiers 1-2 on Linux, Mac OS, and Windows, Mach5 tiers 3-5 on Linux x64.  Also, builds were done on Linux-zero, Linux-s390, and Linux-ppc.
> 
> Thanks, Harold

Changes requested by kbarrett (Reviewer).

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

> 109: #include <sys/vminfo.h>
> 110: 
> 111: PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION(close); // prevents compiler warnings for all functions

Why suppress the warning for this file?  I think there are only 3 calls to `close`, so it seems like they could just be fixed.

src/hotspot/os/bsd/os_bsd.cpp line 110:

> 108: #endif
> 109: 
> 110: PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION(close); // prevents compiler warnings for all functions

Again here, why suppress the warning rather than fix the small number of calls in this file.

src/hotspot/os/linux/gc/z/zMountPoint_linux.cpp line 37:

> 35: #define PROC_SELF_MOUNTINFO        "/proc/self/mountinfo"
> 36: 
> 37: PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION(fopen);

The intended usage for this pragma is to scope it narrowly, using `PRAGMA_DIAG_PUSH/POP`.

src/hotspot/os/linux/os_linux.cpp line 153:

> 151: };
> 152: 
> 153: PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION(closedir);  // prevents compiler warning for all functions

Why suppress this warning rather than fix the one call.  Note the `closedir` in `os::dir_is_empty` is in the `os` "namespace" and hence is already calling the `os` wrapper.  It could be explicitly qualified anyway, if that seems confusing.

src/hotspot/os/posix/os_posix.cpp line 93:

> 91: #define guarantee_with_errno(cond, msg) check_with_errno(guarantee, cond, msg)
> 92: 
> 93: PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION(closedir); // prevents compiler warnings for all functions

This should be scoped over the one use, in the implementation of `os::closedir`, rather than applied to the whole file.

src/hotspot/os/windows/os_windows.cpp line 105:

> 103: #include <winsock2.h>
> 104: 
> 105: PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION(close); // prevents compiler warnings for all functions

The scope of this should be limited to the implementation of the `os::close` function.

src/hotspot/share/compiler/compilerEvent.cpp line 102:

> 100: 
> 101:     index = phase_names->length();
> 102:     phase_names->append(use_strdup ? os::strdup(phase_name) : phase_name);

I'm not sure this change to use `os::strdup` is currently correct.  I'm not sure where the copied string gets freed, but that might be using `::free` rather than `os::free`.  `os::strdup` uses NMT when enabled, and then using `::free` will leak the tracking data.

src/hotspot/share/jvmci/jvmciCodeInstaller.cpp line 511:

> 509:       JVMCI_ERROR_OK("stub should have a name");
> 510:     }
> 511:     char* name = os::strdup(jvmci_env()->as_utf8_string(stubName));

Another `os::strdup` that I'm not sure is correct because I'm not sure where corresponding `free` might be, and whether it is `::free` or `os::free`.

src/hotspot/share/runtime/os.cpp line 93:

> 91: #endif
> 92: 
> 93: PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION(fopen); // prevents compiler warnings for all functions

Scope should be limited to the implementation of `os::fopen`.

src/hotspot/share/utilities/compilerWarnings.hpp line 87:

> 85:   PRAGMA_DISABLE_GCC_WARNING("-Wattribute-warning")
> 86: 
> 87: FORBID_C_FUNCTION(void abort(void),                            "use os::abort");

It would be better to put all of these after all the `#endif`, so that if we add macro implementations for other platforms (like windows), these will be covered by the additional platforms.

src/hotspot/share/utilities/compilerWarnings.hpp line 112:

> 110: 
> 111: #else
> 112: 

I think, but have not tested it, that this facility can be implemented for Visual Studio using `__declspec(deprecated)` and suppressing warning C4996.  Of course, doing that may trigger a bunch of warnings in Windows-specific files, so it might be best to do that as a followup change.

test/hotspot/gtest/logging/test_logDecorators.cpp line 29:

> 27: #include "unittest.hpp"
> 28: 
> 29: PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION(strdup);

Why suppress this warning?  Why not just fix the couple of calls, and remember to also fix the corresponding calls to `::free` to instead call `os::free`.

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

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