RFR: 8214976: Warn about uses of functions replaced for portability [v2]

[David Holmes]

On Wed, 5 Jan 2022 20:41:07 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
>
> Harold Seigel has updated the pull request incrementally with one additional commit since the last revision:
> 
>   revert strdup() changes and address some of Kim's comments

I'm having very mixed feelings about this one. I'd like to see the warnings for shared code using potentially OS-specific API's, but I don't like seeing OS-specific code having to call os::* functions unnecessarily - but there seems no neat way to achieve this. We can argue it is harmless for the OS-specific code to call the os::* functions, but it looks really odd to see code like:

os::close(fd);
::unlink(fd);

why is `close()` an os api where `unlink()` is not? There's no obvious answer.

Examining that question closer both os_windows.cpp and os_posix.cpp just simply call `::close` - so `os::close` could actually be defined in os.cpp the same as `os::read` - but then taking that one step further, why do close and read need to be part of the os API at all? Maybe there was a reason when Solaris was still supported - I haven't checked. But should we re-examine the os API and actually drop any functions that are either not OS-specific, or which don't need some VM-specific custom handling? And if so, in which order should we do that simplification and the current PR?

David

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

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