Bug 91554 - if (!__builtin_constant_p (fn_arg)) warning_function() works in inline when fn_arg is int, not when it is void *
Summary: if (!__builtin_constant_p (fn_arg)) warning_function() works in inline when f...
Status: NEW
Alias: None
Product: gcc
Classification: Unclassified
Component: c (show other bugs)
Version: 9.2.1
: P3 normal
Target Milestone: ---
Assignee: Not yet assigned to anyone
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-08-26 20:43 UTC by Zack Weinberg
Modified: 2019-08-27 13:46 UTC (History)
0 users

See Also:
Host:
Target:
Build:
Known to work:
Known to fail:
Last reconfirmed: 2019-08-27 00:00:00


Attachments

Note You need to log in before you can comment on or make changes to this bug.
Description Zack Weinberg 2019-08-26 20:43:46 UTC
This snippet

```
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);
}
```

generates warnings from _both_ `warning_expected` and `warning_not_expected`, on all versions of GCC I can conveniently test (see https://godbolt.org/z/V-FHtZ ).  If I change the type of `b` to be `int` throughout, or if I convert the static inline to a macro

```
#define thefun(a, b) \
  (((!__builtin_constant_p(b) || (b) != 0) \
    ? thefun_called_with_nonnull_arg()     \
    : (void) 0),                           \
   real_thefun(a, b))
```

then I get a warning only for `warning_expected`, which is the behavior I want.

It seems to me that whether or not you can use `__builtin_constant_p` to guard a call to a function declared with attribute warning, should not depend on the type of __builtin_constant_p's argument.
Comment 1 Richard Biener 2019-08-27 10:21:14 UTC
Already the frontend (or GENERIC expression simplification) generates

;; Function thefun (null)
;; enabled by -tree-original


{
  if (1)
    {
      thefun_called_with_nonnull_arg ();
    }
  return real_thefun (a, b);

I couldn't quickly spot who does this though...
Comment 2 Zack Weinberg 2019-08-27 13:22:52 UTC
Additional fun detail:

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

still warns for any use of `thefun`, but

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

works as intended!  `(int)(__UINTPTR_TYPE__)` also works as intended on targets where __UINTPTR_TYPE__ is bigger than int.

https://gcc.gnu.org/git/?p=gcc.git;a=blob;f=gcc/builtins.c;h=f902e246f1f347be4d4dc04e339fa865393039fe#l8462 looks suspicious to me.  Note also the STRIP_NOPS shortly above, which might explain why it matters whether the pointer is cast to a different-sized integer type.
Comment 3 Richard Biener 2019-08-27 13:34:46 UTC
It's obviously

8462   /* If this expression has side effects, show we don't know it to be a
8463      constant.  Likewise if it's a pointer or aggregate type since in
8464      those case we only want literals, since those are only optimized
8465      when generating RTL, not later.
8466      And finally, if we are compiling an initializer, not code, we
8467      need to return a definite result now; there's not going to be any
8468      more optimization done.  */
8469   if (TREE_SIDE_EFFECTS (arg)
8470       || AGGREGATE_TYPE_P (TREE_TYPE (arg))
8471       || POINTER_TYPE_P (TREE_TYPE (arg))
8472       || cfun == 0
8473       || folding_initializer
8474       || force_folding_builtin_constant_p)
8475     return integer_zero_node;

that causes this, I guess you want to use

__builtin_constant_p (b != 0)

instead.  The docs don't explain what a "constant at compile time" is
so whether for example the address of a global or the address of an
automatic var would be "constant".  But I'd say the above incorrectly
disregards the NULL-pointer case.
Comment 4 Zack Weinberg 2019-08-27 13:46:04 UTC
(In reply to Richard Biener from comment #3)
> I guess you want to use
> 
> __builtin_constant_p (b != 0)
> 
> instead.

That wouldn't do what I want.  The goal is to warn for any argument _other than_ a compile-time null pointer.  `!__builtin_constant_p(b) || b != 0` does just that (it might be easier to understand the De Morgan equivalent `!(__builtin_constant_p(b) && b == 0)`.  This is in aid of deprecating the second argument to gettimeofday (see https://sourceware.org/git/?p=glibc.git;a=blob;f=NEWS;h=75453bbda865c7d51df39177caef40b16e086dcf#l53 and https://sourceware.org/git/?p=glibc.git;a=blob;f=manual/time.texi;h=cb234bd08fae1841034a2bdccf4e1d246be23034#l557 ).

> The docs don't explain what a "constant at compile time" is
> so whether for example the address of a global or the address of an
> automatic var would be "constant".  But I'd say the above incorrectly
> disregards the NULL-pointer case.

It seems like this code pre-dates tree optimizations, I would suggest removing these lines

8470       || AGGREGATE_TYPE_P (TREE_TYPE (arg))
8471       || POINTER_TYPE_P (TREE_TYPE (arg))

(and fixing the comment above to match) and seeing if that breaks anything.