8313396: Portable implementation of FORBID_C_FUNCTION and ALLOW_C_FUNCTION #22890
Conversation
|
👋 Welcome back kbarrett! A progress list of the required criteria for merging this PR into |
|
@kimbarrett This change now passes all automated pre-integration checks. ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details. After integration, the commit message for the final commit will be: You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed. At the time when this comment was updated there had been 38 new commits pushed to the
As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details. ➡️ To integrate this PR with the above commit message to the |
|
@kimbarrett The following labels will be automatically applied to this pull request:
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. |
Webrevs
|
| inline char* _fullpath(char* absPath, const char* relPath, size_t maxLength) { | ||
| return ::_fullpath(absPath, relPath, maxLength); | ||
| } | ||
|
|
There was a problem hiding this comment.
Could we forbid the non Standard _snprintf too?
There was a problem hiding this comment.
Sure, I'll add that. We should also technically be forbidding snprintf in favor of os::snprintf, but the difference
is pretty minuscule and there are a significant number of occurrences (125 today).
| // these written-out wrapper functions. All that have been tried don't work | ||
| // for one reason or another. | ||
|
|
||
| namespace permit_forbidden_functions { |
There was a problem hiding this comment.
permit_forbidden_functions sounds like a bit of a mouthful, maybe permit or permitted could work better in this case? If a call to free is to be permitted for example, permit::free(ptr) or permitted::free(ptr) sounds like "permit free", which conveys the intent pretty well, at least in my opinion
There was a problem hiding this comment.
It is intentionally a mouthful. It's not intended to be used except in pretty rare situations, and it should stand
out as unusual and needing additional scrutiny by readers.
| // noreturn attribute. | ||
| #ifdef __clang__ | ||
| #define FORBID_NORETURN_C_FUNCTION(Signature, Alternative) \ | ||
| FORBID_C_FUNCTION(__attribute__((__noreturn__)) Signature, Alternative) |
There was a problem hiding this comment.
clang isn't the only one that makes a distinction between [[gnu::noreturn]] and [[noreturn]], gcc does too. It's a little strange that gcc doesn't face this same issue
There was a problem hiding this comment.
I don't know what [[gnu::noreturn]] has to do with this? And in what way does gcc treat them differently?
There was a problem hiding this comment.
[[gnu::noreturn]] is equal to __attribute__((noreturn)), I just referred to it as its scoped attribute form to make it clear that I wasn't talking about the Standard noreturn. There is funnily enough no documentation on how gcc treats them differently, I only learnt that it does while reading through gcc's source code one day
/* We used to treat C++11 noreturn attribute as equivalent to GNU's,
but no longer: we have to be able to tell [[noreturn]] and
__attribute__((noreturn)) apart.
Similarly for C++14 deprecated attribute, we need to emit extra
diagnostics for [[deprecated]] compared to [[gnu::deprecated]]. */
/* C++17 fallthrough attribute is equivalent to GNU's. */There was a problem hiding this comment.
gcc does treat [[noreturn]] differently from [[gnu::noreturn]] /
attribute((noreturn)) when there is a preceding declaration that doesn't have
either kind of attribute. Compile this:
void frob(int);
//__attribute__((__noreturn__)) void frob(int);
//[[gnu::noreturn]] void frob(int);
[[noreturn]] void frob(int);
and you'll get an error:
error: function 'void frob(int)' declared '[[noreturn]]' but its first declaration was not
But uncomment either of the gnu-specific attributes and the error goes away.
C++14 7.6.3 "Noreturn attribute" says "The first declaration of a function
shall specify the noreturn attribute if any declaration of that function
specifies the noreturn attribute."
So without the declarations with gcc-specific attributes it errors, and that's
fine. The gcc-specific attributes never complained in that situation, and
continue to not complain, for backward compatibility. But other than that
error checking difference, the gcc-specific attributes seem to be treated as
equivalent to the standard attribute.
There was a problem hiding this comment.
The difference between [[deprecated]] and [[gnu::deprecated]] is discussed here:
https://gcc.gnu.org/git/?p=gcc.git;a=commit;f=gcc/cp/parser.cc;h=cd647514a539943ade6461efbf056a7c3f4305c6
TL;DR - [[gnu::deprecated]] is allowed in more places than [[deprecated]].
|
Overall nice change, my only complaint is that to know which FORBID macro to use (IMPORTED, NORETURN, etc), you have to delve into implementation details to select the right one. That quickly becomes confusing. But unfortunately all the other approaches thus far have failed, so there aren't really any alternatives that are any better than this approach |
Unfortunately, I couldn't find any way to determine whether IMPORTED or not Whether to use a NORETURN variant seems pretty straightforward. It would be But that doesn't work both because of the clang weirdness, and because all We'll have to come up with something new if we ever want to forbid a function One option would be to have distinct arguments for the signature, like we do with attributes at the end or something like that. |
| @@ -36,15 +37,15 @@ void MallocInfoDcmd::execute(DCmdSource source, TRAPS) { | |||
| #ifdef __GLIBC__ | |||
| char* buf; | |||
| size_t size; | |||
| ALLOW_C_FUNCTION(::open_memstream, FILE* stream = ::open_memstream(&buf, &size);) | |||
| FILE* stream = ::open_memstream(&buf, &size); | |||
There was a problem hiding this comment.
Note that open_memstream was never forbidden, so the ALLOW_C_FUNCTION was not needed.
| if (stream == nullptr) { | ||
| _output->print_cr("Error: Could not call malloc_info(3)"); | ||
| return; | ||
| } | ||
|
|
||
| int err = os::Linux::malloc_info(stream); | ||
| if (err == 0) { | ||
| ALLOW_C_FUNCTION(::fflush, fflush(stream);) | ||
| fflush(stream); |
There was a problem hiding this comment.
Note that fflush was never forbidden, so the ALLOW_C_FUNCTION was not needed.
| @@ -54,8 +55,8 @@ void MallocInfoDcmd::execute(DCmdSource source, TRAPS) { | |||
| } else { | |||
| ShouldNotReachHere(); | |||
| } | |||
| ALLOW_C_FUNCTION(::fclose, ::fclose(stream);) | |||
| ALLOW_C_FUNCTION(::free, ::free(buf);) | |||
| ::fclose(stream); | |||
There was a problem hiding this comment.
Note that fclose was never forbidden, so the ALLOW_C_FUNCTION was not needed.
|
Unfortunately, this doesn't compile on AIX: |
Ugh! The clang workaround for noreturn handling is going to need to be more extensive. |
|
Hi, I really like the syntactic change of this feature, and it's very nice that we get to have working auto-complete (makes it easier to find out that a specific function isn't forbidden). The syntactic change isn't shown in the PR description, isn't that useful to add? I have a bike shedding request: Could we skip the S at the end and have it be |
|
Hi Johan, which syntactic change are you referring to? |
Oh, just that we don't have to repeat the name of the function we want to use. No more macro, just a namespace and a function! |
| #include <stdarg.h> // for va_list | ||
| #include <stddef.h> // for size_t | ||
|
|
||
| #include OS_HEADER(forbiddenFunctions) |
There was a problem hiding this comment.
I thought there was an OS_VARIANT_HEADER to dispatch directly to the posix variant, but I guess it doesn't exist. Using the semaphore example, this could save your dispatch header files, at the cost of an #ifdef. Not sure which is worse:
#if defined(LINUX) || defined(AIX)
# include "semaphore_posix.hpp"
#else
# include OS_HEADER(semaphore)
#endif
There was a problem hiding this comment.
park.hpp has the same:
#if defined(LINUX) || defined(AIX) || defined(BSD)
# include "park_posix.hpp"
#else
# include OS_HEADER(park)
#endif
There was a problem hiding this comment.
If I was going to go that route, I'd be more inclined toward
#if !define(_WINDOWS)
#include "forbiddenFunctions_windows.hpp"
#else
#include "forbiddenFunctions_posix.hpp"
#endif
rather than list all (or maybe just a subset) of the posix-like ports. My
inclination is to leave it as-is with OS_HEADERS, since I think that's the
"intended" idiom.
There was a problem hiding this comment.
Overall I like this change. I appreciate the effort that has been put in to try and find an elegant solution to this problem.
but having OS specific files created just to include the posix version runs counter to why we have the posix variants in the first place IMO. Please select one of the above approaches so that the new aix/bsd/linux specific files can be removed in favour of the posix one. Thanks.
There was a problem hiding this comment.
I disagree. It seems to me that breaking the abstraction like that is just asking for trouble.
There was a problem hiding this comment.
@dholmes-ora - OS_HEADER and CPU_HEADER instead of explicit knowledge of what platforms exist and
which ones share some code in a "_posix" file and which ones have some unshared code. IMO the includer
shouldn't need to know that kind of implementation detail.
But oh well, all y'all seem to really hate the simple forwarding files and would prefer to skip that in favor of
hard-coding the file organization. In the interest of making progress, I'll do that.
There was a problem hiding this comment.
@coleenp The copyrights on the new files being 2024-2025 is intentional. They were first published (in this PR)
in 2024, so I think are supposed to have that starting year.
There was a problem hiding this comment.
@kimbarrett I understand. I also don't like the includer needing to know about this, and maybe it is time to add OS_FAMILY_HEADER to deal with it. But in the absence of the new macro I prefer this break of abstraction to the creation of a bunch of tiny forwarding files.
There was a problem hiding this comment.
I prefer the opposite. If you break these up into a bunch of forwarding files, do all the rest of the VM the same way.
It may be my problem but I don't have a good IDE to have to navigate through a slew of similarly named files to find the real implementation.
There was a problem hiding this comment.
@coleenp I thought you and I were on the same page not liking the creation of all these little files that just forward to the posix one. ???
Thanks for testing that port. I restructured the implementation of FORBID_C_FUNCTION, and added more commentary about the clang issue. |
Isn't the last paragraph of the PR description (beginning with "Some of the poisoned functions...") what your are
I named it as a collection of forbidden functions. But I agree it reads better without the plural, so I've removed the S. |
Thanks! This has solved one of the problems, but not all. The next one is: |
Per discussion in that bug, it's been closed as not an issue, and I'm changing the code here to add and use the |
|
@stefank Someday that long ago long discussion about include ordering and formatting needs to get cleaned up and |
Yes. The guide that system includes should come last and be separated from the rest of the HotSpot includes was never written down here: |
I ran GHA tests on at least one early version, but I'll do another run before integrating. |
|
Thanks for reviews. |
|
/integrate |
|
Going to push as commit e0f2f4b.
Your commit was automatically rebased without conflicts. |
openjdk
bot
removed
ready
|
@kimbarrett Pushed as commit e0f2f4b. 💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored. |
|
Curious question: Did we ever find out why the attribute warning didn't fire off at the sites of forbidden methods being called inside gtest? |
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:
... and more stuff about the declaration ...
clang:
... and more stuff about the declaration ...
Visual Studio:
Progress
Issue
Reviewers
Contributors
<mdoerr@openjdk.org>Reviewing
Using
gitCheckout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/22890/head:pull/22890$ git checkout pull/22890Update a local copy of the PR:
$ git checkout pull/22890$ git pull https://git.openjdk.org/jdk.git pull/22890/headUsing Skara CLI tools
Checkout this PR locally:
$ git pr checkout 22890View PR using the GUI difftool:
$ git pr show -t 22890Using diff file
Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/22890.diff
Using Webrev
Link to Webrev Comment