RFR: 8313396: Portable implementation of FORBID_C_FUNCTION and ALLOW_C_FUNCTION

[Kim Barrett]

On Sun, 29 Dec 2024 08:34:27 GMT, Julian Waters <jwaters at openjdk.org> wrote:

>> 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 poison...
>
> src/hotspot/os/windows/permitForbiddenFunctions_windows.hpp line 41:
> 
>> 39:   return ::_fullpath(absPath, relPath, maxLength);
>> 40: }
>> 41: 
> 
> Could we forbid the non Standard _snprintf too?

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).

> src/hotspot/share/utilities/compilerWarnings_gcc.hpp line 85:
> 
>> 83: #ifdef __clang__
>> 84: #define FORBID_NORETURN_C_FUNCTION(Signature, Alternative) \
>> 85:   FORBID_C_FUNCTION(__attribute__((__noreturn__)) Signature, Alternative)
> 
> 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

I don't know what `[[gnu::noreturn]]` has to do with this?  And in what way does gcc treat them differently?

> src/hotspot/share/utilities/permitForbiddenFunctions.hpp line 48:
> 
>> 46: // for one reason or another.
>> 47: 
>> 48: namespace permit_forbidden_functions {
> 
> 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

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.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/22890#discussion_r1899110603
PR Review Comment: https://git.openjdk.org/jdk/pull/22890#discussion_r1899109858
PR Review Comment: https://git.openjdk.org/jdk/pull/22890#discussion_r1899110095