Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8347719: [REDO] Portable implementation of FORBID_C_FUNCTION and ALLOW_C_FUNCTION #24608

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

kimbarrett
Copy link

@kimbarrett kimbarrett commented Apr 12, 2025

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:

<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

Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue

Issue

  • JDK-8347719: [REDO] Portable implementation of FORBID_C_FUNCTION and ALLOW_C_FUNCTION (Enhancement - P4)

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

@bridgekeeper
Copy link

bridgekeeper bot commented Apr 12, 2025

👋 Welcome back kbarrett! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Apr 12, 2025

❗ This change is not yet ready to be integrated.
See the Progress checklist in the description for automated requirements.

@openjdk openjdk bot added the rfr Pull request is ready for review label Apr 12, 2025
@openjdk
Copy link

openjdk bot commented Apr 12, 2025

@kimbarrett The following labels will be automatically applied to this pull request:

  • graal
  • hotspot

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org labels Apr 12, 2025
@mlbridge
Copy link

mlbridge bot commented Apr 12, 2025

Webrevs

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
graal graal-dev@openjdk.org hotspot hotspot-dev@openjdk.org rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

1 participant