JDK-8214976 : Warn about uses of functions replaced for portability
  • Type: Enhancement
  • Component: hotspot
  • Sub-Component: runtime
  • Affected Version: 12
  • Priority: P4
  • Status: Resolved
  • Resolution: Fixed
  • OS: generic
  • CPU: generic
  • Submitted: 2018-12-06
  • Updated: 2022-07-22
  • Resolved: 2022-06-08
The Version table provides details related to the release that this issue/RFE will be addressed.

Unresolved : Release in which this issue/RFE will be addressed.
Resolved: Release in which this issue/RFE has been resolved.
Fixed : Release in which this issue/RFE has been fixed. The release containing this fix may be available for download as an Early Access Release or a General Availability Release.

To download the current JDK release, click here.
JDK 19
19 b26Fixed
Related Reports
Blocks :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Relates :  
Description
Hotspot provides a portability layer (via the os "namespace").  This provides portable implementations of various functions with common semantics over all supported platforms.  This is used to provide functions not present on some platforms, and to hide behavioral variations between platforms where the functions do exist.

It is possible for direct uses of the native functions to creep into the code, because only developer care and code review presently prevents that from occurring in some places.  For example, if a function is natively available everywhere but with different semantics, it is all too easy to call it while failing to have a test that hits those differences.

For gcc/clang-based platforms we can use __attribute__ declarations to cause warnings to be generated for references to such functions.  These attribute declarations can be added to utilities/compilerWarnings.hpp.

For functions which should never be called, we can use (for example)

extern "C" char* strerror(int)
__attribute__((__warning__("use os::strerror")));

For functions which should never be called outside the implementation of the os replacement, we can use (for example)

extern "C" int vsnprintf(char*, size_t, const char*, va_list)
__attribute__((__deprecated__("use os::vsnprintf")));

and in the definition of os::vsnprintf, locally disable the deprecation warning with the appropriate diagnostic #pragma.

Comments
We found this caused build failures with the latest Clang, as Clang does not support merging the "warning" function attribute. Our colleagues kindly opened discussion in https://github.com/llvm/llvm-project/issues/56519 to determine whether Clang should support merging the attribute. To fix the build failure, we could disable this check in compilerWarnings_gcc.hpp if OpenJDK is building with Clang. However, I think we could wait for a conclusion from the LLVM discussion thread first.
13-07-2022

Changeset: 04f02ac6 Author: Kim Barrett <kbarrett@openjdk.org> Date: 2022-06-08 19:16:46 +0000 URL: https://git.openjdk.java.net/jdk/commit/04f02ac6b2ce496b86642987bb7e25d21b52a5b6
08-06-2022

A pull request was submitted for review. URL: https://git.openjdk.java.net/jdk/pull/8982 Date: 2022-06-01 22:02:29 +0000
02-06-2022

For Windows I think we might be able to get much of what we want via a combination of *not* defining either of _CRT_SECURE_NO_WARNINGS or _CRT_NONSTDC_NO_DEPRECATE, along with not globally suppressing deprecation warnings (which we currently do, sigh). But there's a fair amount of work to get there.
10-05-2022

I've not found a way to do something like this for Windows / Visual Studio. The nearest I've found is using deprecation warnings. And that would need to be done with the __pragma(deprecate(name)) rather than __declspec(deprecate(alternative)). The __declspec approach runs afoul of such declarations not being additive, unlike gcc attributes, and we don't reliably know what attributes are applied to a given function in the library headers. And it gets worse, as using __pragma-based deprecation doesn't seem to work either. With such a pragma in place for a global function (and deprecation warnings not globally disabled - see below), the compiler issues a deprecation warning about the *declaration* of an os:: function of the same name. Seems like a pretty strange compiler bug. (This is with VS2019.) But that's all moot, at least for now, since we globally disable deprecation warnings when building for Windows. We also define _CRT_SECURE_NO_DEPRECATION (which is an older name for _CRT_SECURE_NO_WARNINGS - JDK-8286262). Just removing those warning disables doesn't really give us what we want for this enhancement request. We might want to consider such removal anyway for other reasons, and this change and associated cleanups might be a useful preliminary step toward that, but it should be dealt with as a separate issue.
06-05-2022

Clang 14+ has the warning (and error) attributes and -W[no-]attribute-warning, compatible with gcc.
05-05-2022

[~dholmes] I see what you mean. A generic platform-independent os layer makes of course sense. Naming them like Posix functions may have been a bad choice then since it misleads newcomers who know the standard functions but don't know the hotspot versions. This RFE introduces a way to forbid the former and propose the latter as a replacement during build time. Many will just do that. I mean, the prototypes are usually identical, so it should be okay, right? But it will cause problems if VM infrastructure is used that limits the functionality of the replacement. And the tests often don't cover that. As a simple example, if the os:: variant queries command line parameters, it may misbehave when called before argument parsing. Or just do nothing (e.g. UL). If it uses any form of Thread::current (e.g. ResourceArea), it may crash when called from within a native thread that is not attached to the VM. And so forth. All these are real examples I saw many times during my time with the hotspot. Maybe because we do a lot of platform porting. We often work in the outer edges. I always thought assigning "rings" to our VM parts would be cool, where APIs in the inner ring need hotspot infrastructure and can call everything, and APIs in the outer ring are more tolerant, work at every point in time, but are not allowed to call into the inner ring. Over time one could work on making more infrastructure outer-ring-capable (e.g. UL).
02-02-2022

[~stuefe] I don't agree with your characterisation here. The os:: functions only need to exist for two very simple reasons: 1. To provide an abstraction layer for shared code when the actual OS API's are very different. 2. To provide some essential/desirable semantic enhancement to the native OS functionality The issue of using VM infrastructure, Thread::current() etc should really not come into it at all. But if you need that for the semantic enhancement then you need it.
02-02-2022

There are cases where I really want to call the raw system function, not the os variant. I usually mark these calls with an explicit global scope, both to avoid clashes with symbols in the containing namespace and as an indication that yes, I thought about this, this is what I want to call. I find myself doing this in situations where the os variant does too much stuff or has too many dependencies (e.g. does not work during initialization or signal handling). I also use system functions directly when I really don't want code to bitrot: os functions are more "volatile" over time. People add stuff. System APIs are stable and almost never change. I think shared os:: functions are often misleadingly named. They look and feel like standard POSIX functions, but are not. Since we seldom comment prototypes, you need to read the code to know what they do. This is worse than if they were named something else entirely. E.g. abort. os::abort() feels like UNIX abort, but it is not. It only writes a core dump if CreateCoredumpOnCrash is true, which is surprising and not documented. Then, it alludes to platform independence, but ::abort is a UNIX concept. On Windows, we try to mimic it by writing a minidump, then exit with a very complicated dance to avoid some OS bug. But, sometimes you just want ::abort. If I think a bit, I am sure I can come up with better examples. I think this RFE makes sense, but we should think about other steps. If we replace a general-purpose system API with an os:: variant, and force everyone to use it, then that os::variant should limit its VM infrastructure use. Example: a general-purpose function should not depend on VM initialization, should not use os::malloc, should not log using UL, should not need Thread::current to exist. It would be nice if these assumptions were documented and tested. So that things break if someone e.g. accidentally adds a log line into os::abort().
02-02-2022

A pull request was submitted for review. URL: https://git.openjdk.java.net/jdk/pull/7248 Date: 2022-01-27 19:18:10 +0000
27-01-2022

Re: poisoning the malloc/free calls instead of doing it in the build system at link time. This seems much nicer. Let's have another RFE for this with more details specific to this case? Thanks.
05-01-2022

A pull request was submitted for review. URL: https://git.openjdk.java.net/jdk/pull/6961 Date: 2022-01-04 19:23:24 +0000
04-01-2022

This warning attribute mechanism might also be used to poison at compile time the global allocation and deallocation operators. For platforms where we can do so, this could provide stronger and more immediate and accurate protection than our existing link time check or our existing (non-product) runtime check.
01-11-2019

Just be mindful that this really only applies when shared code calls these functions. In os specific code it may be necessary/desirable to call the platform specific functions rather than the generic OS:: version.
27-10-2019

gcc9 added -Wno-attribute-warning for controlling the warning attribute. This suggests the following implementation, which avoids the use of deprecation warnings, which might require -Wdeprecated and encounter other problems. #if __GNUC__ >= 9 #define FORBID_C_FUNCTION(signature, alternative) \ extern "C" signature __attribute__((__warning__("use" alternative))) #define PRAGMA_PERMIT_FORBIDDEN_C_FUNCTION(name) \ PRAGMA_DISABLE_GCC_WARNING("-Wattribute-warning") #endif // __GNUC__ >= 9
27-10-2019

Some candidate os functions (xxx => os::xxx) abort close closedir connect exit flockfile fopen free fsync ftruncate funlockfile lseek malloc open opendir random read readdir realloc recv send signal sleep snprintf socket stat strdup strerror unsetenv vsnprintf write Others: strtok => (forbid, use strtok_r; see JDK-8214773) readdir_r (forbid, use readdir; see JDK-8202353)
09-12-2018

I like the sound of this! Thanks [~kbarrett]
07-12-2018

Locally disabling deprecation warnings can be done with another new macro in compilerWarnings.hpp. Something like #ifdef TARGET_COMPILER_gcc #define PRAGMA_DISABLE_DEPRECATED_DECLARATIONS_WARNINGS \ _Pragma("GCC diagnostic ignored \"-Wdeprecated-declarations\"") #endif // TARGET_COMPILER_gcc #ifndef PRAGMA_DISABLE_DEPRECATED_DECLARATIONS_WARNINGS #define PRAGMA_DISABLE_DEPRECATED_DECLARATIONS_WARNINGS #endif Then, for example, in os_posix.cpp: PRAGMA_DIAG_PUSH PRAGMA_DISABLE_DEPRECATION_WARNINGS int os::vsnprintf(char* buf, size_t len, const char* fmt, va_list args) { ... } PRAGMA_DIAG_POP
07-12-2018

It is generally okay to add such attributes. From the gcc documentation: "Compatible attribute specifications on distinct declarations of the same function are merged. An attribute specification that is not compatible with attributes already applied to a declaration of the same function is ignored with a warning."
06-12-2018