8347719: [REDO] Portable implementation of FORBID_C_FUNCTION and ALLOW_C_FUNCTION #24608
+563
−163
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Please review this second attempt. It's mostly similar to the original
attempt:
https://bugs.openjdk.org/browse/JDK-8313396
#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:
... and more stuff about the declaration ...
clang:
... and more stuff about the declaration ...
Visual Studio:
Progress
Issue
Reviewing
Using
git
Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/24608/head:pull/24608
$ git checkout pull/24608
Update a local copy of the PR:
$ git checkout pull/24608
$ git pull https://git.openjdk.org/jdk.git pull/24608/head
Using Skara CLI tools
Checkout this PR locally:
$ git pr checkout 24608
View PR using the GUI difftool:
$ git pr show -t 24608
Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/24608.diff
Using Webrev
Link to Webrev Comment