Poisoning in HotSpot

[Julian Waters]

This seems to change the forbidden interface, which by itself is ok,
but I have some questions

1. Changing from signature to a name might make the forbidden function
less specific, as information on exactly which function is forbidden
is lost. Would this create problems in the event that another function
with the same name but different signature which is not forbidden is
called?
2. ALLOW_C_FUNCTION used to take a statement, changing it to a call
expression would mean every use site would have to change as well.
Many uses of the macro also use qualified names as well. There's also
the VC specific warning disable in compilerWarnings, which I'm not
sure why it was there in the first place when FORBID_C_FUNCTION does
nothing on VC: https://github.com/openjdk/jdk/blob/28ae281b42cd00f471e275db544a5d23a42df59c/src/hotspot/share/utilities/compilerWarnings_visCPP.hpp#L64
Now that ALLOW_C_FUNCTION takes an expression, I don't think that same
warning disable can be done, would that cause issues for
ALLOW_C_FUNCTION when VC is the compiler?
3. None of the big 3 compilers inline lambdas by default, only when
optimizations are enabled. In practice this means slowdebug HotSpot
will have the wrapper lambda in the final assembly instead of a direct
call to the C library code. I don't know if this issue is a deal
breaker

In any case I tried again after seeing the new example, but using
separate macros to handle the permitted namespace and the actual
forbidding of the function itself. I used the older style signature
based forbidding rather than the new name based solution (Due to
concerns over #1), but I included a toggle to switch between name
based and signature based variants anyway, so a comparison between the
2 can easily be made. Rather than ALLOW_C_FUNCTION this example
directly does permitted::symbol, the meaning of which is also rather
clear

https://godbolt.org/z/hdhrh5eev

best regards,
Julian


On Sun, Dec 1, 2024 at 3:56 AM Kim Barrett <kim.barrett at oracle.com> wrote:
>
> On 11/30/24 2:31 PM, Julian Waters wrote:
> > VC returns:
> >
> > <source>(21): error C2668: 'malloc': ambiguous call to overloaded function
> > Z:/compilers/windows-kits-10/include/10.0.22621.0/ucrt\corecrt_malloc.h(101):
> > note: could be 'void *malloc(size_t)'
> > <source>(18): note: or       'void *forbidden::malloc(size_t) noexcept'
> > <source>(21): note: while trying to match the argument list '(size_t)'
> > Compiler returned: 2
> >
> > While clang returns:
> >
> >
> > <source>:21:17: error: call to 'malloc' is ambiguous
> >     21 |     void *ptr = malloc(size_t{1});
> >        |                 ^~~~~~
> > /usr/include/stdlib.h:540:14: note: candidate function
> >    540 | extern void *malloc (size_t __size) __THROW __attribute_malloc__
> >        |              ^
> > <source>:18:25: note: candidate function has been explicitly deleted
> >     18 | FORBID_C_FUNCTION(void *malloc(size_t), "use os::malloc")
> >        |                         ^
> > 1 error generated.
> > Compiler returned: 1
> >
> > https://urldefense.com/v3/__https://godbolt.org/z/zbfPzoz54__;!!ACWV5N9M2RV99hQ!PeGhd4cAnGh4EmOWTWIiolLTwAfE11a3ZrFkFy0zSVTsMbsdcWDRAgZH9kvoQS7zYkGsp7g_SNSVg3VUsO71JgIx$
> >
> > I managed to put in an override system to allow C Standard library
> > methods to be used when they are really needed, but unfortunately the
> > method body of the forwarding method that calls the actual C library
> > code is still defined in a rather clunky way
> I've been working on this too. Here's what I've come up with, which I
> think is
> significantly simpler, yet appears to work well.
>
> No clunky forwarding mechanism needed. And no metaprogramming.
>
> I think this might just be what we're looking for. The error messages are
> about ambiguity, but they all list the candidates, and with appropriate
> naming
> there's plenty of information provided.
>
> ----- begin example -----
>
> void* frob(unsigned);
>
> namespace forbidden_functions_support {
> struct Forbidden {};
> }
>
> // name must be unqualified.
> #define FORBID_C_FUNCTION(name)                         \
>    namespace forbidden_functions_support {               \
>    using ::name;                                         \
>    }                                                     \
>    inline namespace forbidden_functions {                \
>    forbidden_functions_support::Forbidden name{};        \
>    }
>
> // name and references in expression must be unqualified.
> #define ALLOW_C_FUNCTION(name, expression) \
>    ([&]() { using forbidden_functions_support::name; return
> (expression); }())
>
> FORBID_C_FUNCTION(frob)
>
> // no error, calls ::frob.
> // name and expr ref must be unqualified.
> void* good_frob(unsigned x) {
>    return ALLOW_C_FUNCTION(frob, frob(x));
> }
>
> // demonstrate error for unqualified call.
> #if 1 // change to 0 to avoid error
> void* bad_frob1(unsigned x) {
>    return frob(x);
> }
> #endif
>
> // demonstrate error for qualified call.
> #if 1 // change to 0 to avoid error
> void* bad_frob2(unsigned x) {
>    return ::frob(x);
> }
> #endif
>
> /*
>
> For good_frob, all 3 of gcc, clang, and msvc generate a direct call to frob.
>
> gcc error message:
>
> poison.cpp: In function ‘void* bad_frob(unsigned int)’:
> poison.cpp:25:10: error: reference to ‘frob’ is ambiguous
>     25 |   return frob(x);
>        |          ^~~~
> poison.cpp:16:19: note: candidates are:
> ‘forbidden_functions_support::Forbidden forbidden_functions::frob’
>     16 | FORBID_C_FUNCTION(frob)
>        |                   ^~~~
> poison.cpp:10:44: note: in definition of macro ‘FORBID_C_FUNCTION’
>     10 |     forbidden_functions_support::Forbidden name{}; \
>        |                                            ^~~~
> poison.cpp:1:7: note:                 ‘void* frob(unsigned int)’
>      1 | void* frob(unsigned);
>        |       ^~~~
>
> clang error message:
>
> <source>:31:10: error: reference to 'frob' is ambiguous
>     31 |   return frob(x);
>        |          ^
> <source>:1:7: note: candidate found by name lookup is 'frob'
>      1 | void* frob(unsigned);
>        |       ^
> <source>:20:19: note: candidate found by name lookup is
> 'forbidden_functions::frob'
>     20 | FORBID_C_FUNCTION(frob)
>        |                   ^
>
> msvc error message:
>
> <source>(31): error C2872: 'frob': ambiguous symbol
> <source>(1): note: could be 'void *frob(unsigned int)'
> <source>(20): note: or 'forbidden_functions_support::Forbidden
> forbidden_functions::frob'
>
> */
>
>