RFR: 8347719: [REDO] Portable implementation of FORBID_C_FUNCTION and ALLOW_C_FUNCTION
Kim Barrett
kbarrett at openjdk.org
Sat Apr 12 13:53:24 UTC 2025
Please review this second attempt. It's mostly similar to the original
attempt:
https://bugs.openjdk.org/browse/JDK-8313396
https://github.com/openjdk/jdk/pull/22890
but improves the workarounds for one clang issue, and adds a workaround for
another clang issue
https://bugs.openjdk.org/browse/JDK-8347649
See globalDefinitions_gcc.hpp for more details about those issues and the
workarounds.
Additions to the testing done for the earlier attempt (see below)
mach5 tier4-5. There is an Oracle-internal build configuration in tier5 that
failed with the earlier attempt.
Local manual build and tier1 test on linux with clang.
For testing on linux with clang, be aware of these issues:
https://bugs.openjdk.org/browse/JDK-8354316
https://bugs.openjdk.org/browse/JDK-8354467
Below is a repeat of the PR summary for the earlier attempt.
----------
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
[note: for REDO this is changed to permit_forbidden_function, per prior review]
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:
- improve/add clang-specific workarounds
- apply original change
Changes: https://git.openjdk.org/jdk/pull/24608/files
Webrev: https://webrevs.openjdk.org/?repo=jdk&pr=24608&range=00
Issue: https://bugs.openjdk.org/browse/JDK-8347719
Stats: 627 lines in 32 files changed: 464 ins; 64 del; 99 mod
Patch: https://git.openjdk.org/jdk/pull/24608.diff
Fetch: git fetch https://git.openjdk.org/jdk.git pull/24608/head:pull/24608
PR: https://git.openjdk.org/jdk/pull/24608
More information about the graal-dev
mailing list