RFR: 8313396: Portable implementation of FORBID_C_FUNCTION and ALLOW_C_FUNCTION
Kim Barrett
kbarrett at openjdk.org
Sun Dec 29 08:16:50 UTC 2024
Please review this change to how HotSpot prevents the use of certain C library
functions (e.g. poisons references to those functions), while permitting a
subset to be used in restricted circumstances. Reasons for poisoning a
function include it being considered obsolete, or a security concern, or there
is a HotSpot function (typically in the os:: namespace) providing similar
functionality that should be used instead.
The old mechanism, based on -Wattribute-warning and the associated attribute,
only worked for gcc. (Clang's implementation differs in an important way from
gcc, which is the subject of a clang bug that has been open for years. MSVC
doesn't provide a similar mechanism.) It also had problems with LTO, due to a
gcc bug.
The new mechanism is based on deprecation warnings, using [[deprecated]]
attributes. We redeclare or forward declare the functions we want to prevent
use of as being deprecated. This relies on deprecation warnings being
enabled, which they already are in our build configuration. All of our
supported compilers support the [[deprecated]] attribute.
Another benefit of using deprecation warnings rather than warning attributes
is the time when the check is performed. Warning attributes are checked only
if the function is referenced after all optimizations have been performed.
Deprecation is checked during initial semantic analysis. That's better for
our purposes here. (This is also part of why gcc LTO has problems with the
old mechanism, but not the new.)
Adding these redeclarations or forward declarations isn't as simple as
expected, due to differences between the various compilers. We hide the
differences behind a set of macros, FORBID_C_FUNCTION and related macros. See
the compiler-specific parts of those macros for details.
In some situations we need to allow references to these poisoned functions.
One common case is where our poisoning is visible to some 3rd party code we
don't want to modify. This is typically 3rd party headers included in HotSpot
code, such as from Google Test or the C++ Standard Library. For these the
BEGIN/END_ALLOW_FORBIDDEN_FUNCTIONS pair of macros are used demark the context
where such references are permitted.
Some of the poisoned functions are needed to implement associated HotSpot os::
functions, or in other similarly restricted contexts. For these, a wrapper
function is provided that calls the poisoned function with the warning
suppressed. These wrappers are defined in the permit_forbidden_functions
namespace, and called using the qualified name. This makes the use of these
functions stand out, suggesting they need careful scrutiny in code reviews and
the like. There are several benefits to this approach vs the old
ALLOW_C_FUNCTION macro. We can centralize the set of such functions. The
syntax for use is simpler (there were syntactic bugs with the old mechanism
that weren't always noticed for a while). The permitted reference is explicit;
there can't be an ALLOW_C_FUNCTION use that isn't actually needed.
Testing:
mach5 tier1-3, which includes various build variants such as slowdebug.
GHA sanity tests
Manual testing for warnings for direct calls to poisoned functions with all 3
compilers, and that the error messages look sane and helpful.
gcc:
<file/containing/reference>: In function 'void test_exit(int)':
<file/containing/reference>:<line>:<column>: error: 'void exit(int)' is deprecated: use os::exit [-Werror=deprecated-declarations]
32 | void test_exit(int status) { return exit(status); }
| ~~~~^~~~~~~~
... and more stuff about the declaration ...
clang:
<file/containing/reference>:<line>:<column>: error: 'exit' is deprecated: use os::exit [-Werror,-Wdeprecated-declarations]
void test_exit(int status) { return exit(status); }
^
... and more stuff about the declaration ...
Visual Studio:
<file/containing/reference>(<line>): warning C4996: 'exit': use os::exit
-------------
Commit messages:
- new poisoning
Changes: https://git.openjdk.org/jdk/pull/22890/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=22890&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8313396
Stats: 713 lines in 35 files changed: 553 ins; 71 del; 89 mod
Patch: https://git.openjdk.org/jdk/pull/22890.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/22890/head:pull/22890
PR: https://git.openjdk.org/jdk/pull/22890
More information about the hotspot-dev
mailing list