14

Suppose I am maintaining a library function that takes two arguments, both pointers. The second argument exists only for backward compatibility; callers should always pass a NULL. I would like to put something into my header file that makes the compiler issue warnings if the second argument is not a compile-time constant NULL. I thought I would be able to do this using GCC's __builtin_constant_p and __attribute__((warning)) extensions:

extern void thefun_called_with_nonnull_arg (void)
    __attribute__((__warning__(
        "'thefun' called with second argument not NULL")));

extern int real_thefun (void *, void *);

static inline int
thefun (void *a, void *b)
{
   if (!__builtin_constant_p(b) || b != 0)
       thefun_called_with_nonnull_arg();
   return real_thefun(a, b);
}

int warning_expected (void *a, void *b)
{
    return thefun(a, b);
}
int warning_not_expected (void *a)
{
    return thefun(a, 0);
}

But this doesn't work with any version of GCC I have tested. I get warnings for both calls to thefun. (Compiler Explorer demo.)

Can anyone suggest an alternative construct that will produce a warning for warning_expected, and not for warning_not_expected ?

Notes:

  • Curiously, the above does work if b is an int.
  • The above uses GCC-specific extensions, however a solution that works on a broader variety of compilers would be welcome. (In particular, clang does not implement attribute((warning)) and I haven't had any luck finding an alternative.)
  • A solution that still works when optimization is turned off would be preferable to one that doesn't. (The above does not work with optimization turned off, even if b is an int and thefun is marked always-inline.)
  • A solution that doesn't involve defining thefun as a macro would be preferable to one that does.
  • The header has to work when included from C programs and from C++ programs. A modest amount of ifdeffage is acceptable.
  • It must be a warning, not a hard error, unless -Werror or equivalent is active.

EDIT: Based on Kamil Cuk's discovery that the unwanted warning can be suppressed by casting the pointer to an integer of a different size, I have determined that this is an oversight in the implementation of __builtin_constant_p and filed GCC bug report #91554. I'd still be interested in answers that provide ways to do this with clang, icc, or any other compiler that's commonly used together with GNU libc.

zwol
  • 135,547
  • 38
  • 252
  • 361
  • `gettimeofday()`? – Jonathan Leffler Aug 26 '19 at 16:18
  • @JonathanLeffler not sure about that: https://godbolt.org/z/TbCwpb – Marco Bonelli Aug 26 '19 at 16:30
  • Have you tried `b!=(void *)0`? – Paul Ogilvie Aug 26 '19 at 16:31
  • I read: _"...does not return 1 when you pass a constant numeric value to the inline function unless you specify the -O option."_ (https://gcc.gnu.org/onlinedocs/gcc-9.1.0/gcc/Other-Builtins.html) – Paul Ogilvie Aug 26 '19 at 16:36
  • @JonathanLeffler You have correctly guessed my larger goal; this is in aid of finding a version of https://sourceware.org/git/?p=glibc.git;a=commitdiff;h=53df1cd2811b71aa4193cb250b95fc14b7f310a3 that actually works. – zwol Aug 26 '19 at 20:10
  • @zwol, are you by chance a glibc developer? – S.S. Anne Aug 26 '19 at 20:28
  • 1
    @JL2210 Innn my copious free time, yes. – zwol Aug 26 '19 at 20:37
  • Related: https://stackoverflow.com/questions/8936063/does-there-exist-a-static-warning – S.S. Anne Aug 26 '19 at 20:40
  • This is so similar to FORTIFY_SOURCE implementation ex. [memset check](https://github.com/kraj/glibc/blob/master/string/bits/string_fortified.h#L59). `I get warnings for both calls` - well because your condition is `!__builtin_constant_p(b) || b != 0`. So you get a warning when `b` is not(!) a constant expresion or when `b` is not zero. Did you mean to `__builtin_constant_p(b) && b != (void*)0`? – KamilCuk Aug 26 '19 at 20:54
  • @KamilCuk The OP wants to warn when either `b` is not a constant or `b` is a constant but is not zero. – S.S. Anne Aug 26 '19 at 21:05
  • @KamilCuk Heh, yes, I _was_ cribbing from `string_fortified.h` when I originally wrote this code. – zwol Aug 27 '19 at 13:31

2 Answers2

4

I finally managed to get it to work:

if (!__builtin_constant_p((int)(uintptr_t)b) || b != 0) {

With this you get only one warning.

It seems that gcc can't do __builtin_constant_p on a pointer type. The __builtin_constant_p(b) always returns 0, so the warn function is always linked. Casting b to int strangely works. Although it looses precision in the pointer value, we don't care about it, cause we only check if it's a constant.

KamilCuk
  • 120,984
  • 8
  • 59
  • 111
  • Why not just cast to `uintptr_t`? – ssbssa Aug 27 '19 at 10:24
  • Because it doesn't work. Casting to `long` or `long long` and `unsigned` fashions doesn't work either. Casting to `short`, `int` or `char` works. You can try it `__builtin_constant_p((long long)b)` will always be zero and gcc thinkgs it's a constant expression, you can even `static_assert(__builtin_constant_p((long long)b) == 0, "");`. – KamilCuk Aug 27 '19 at 10:55
  • Oh, this is fun. Your suggestion works correctly when GCC is "targeting" a CPU and ABI where `uintptr_t` is not the same size as `int`. But if `uintptr_t` and `int` _are_ the same size, it doesn't work. But `(short)(uintptr_t)` still works in that case. Based on this I have managed to identify the function in the guts of GCC that isn't handling this case properly, and have filed a bug report (see edits to question). I'm going to accept this answer after a little further testing. Is "Kamil Cuk" the name you would like to be credited by in the commit history? – zwol Aug 27 '19 at 13:25
  • @zwol Wow, good job. "Kamil Cukrowski " would be nicer ; ) – KamilCuk Aug 27 '19 at 16:12
  • @JL2210 Yeah, if you can find an alternative to calling a function declared with `__attribute__((warning))`, that works on Clang, I'd be interested to see an alternative answer demonstrating it. (Your existing answer is no good to me because this really cannot be a hard error.) – zwol Aug 28 '19 at 15:40
  • @zwol Try... `__attribute__((deprecated(message)))`. – S.S. Anne Aug 28 '19 at 15:58
  • @zwol Found it! See answer. – S.S. Anne Aug 28 '19 at 16:04
3

There is no way to do what you describe without GNU extensions.

This portable approach gives a hard error (because _Static_assert requires a constant expression):

#define thefun(a, b) \
({ \
   _Static_assert(b == 0, \
       "'thefun' called with second argument not NULL"); \
   real_thefun(a, b); \
})

However, there is one fortified-style approach that works on both GCC and Clang:

extern void thefun_called_with_nonnull_arg (void)
    __attribute__((__deprecated__(
        "'thefun' called with second argument not NULL")));

extern int real_thefun (void *, void *);

static inline int
thefun (void *a, void *b)
{
   if (!__builtin_constant_p((unsigned short)(unsigned long)b) || b != 0)
       thefun_called_with_nonnull_arg();
   return real_thefun(a, b);
}

int warning_expected (void *a, void *b)
{
    return thefun(a, b);
}
int warning_not_expected (void *a)
{
    return thefun(a, 0);
}

Tested with GCC 8.3.0 and Clang 8.0.0.

See GCC bug report #91554 for more information about the need for the casts.

S.S. Anne
  • 15,171
  • 8
  • 38
  • 76