[PATCH][gcc/doc] Improve nonnull attribute documentation

[Tom de Vries]

On 7/30/21 9:25 AM, Richard Biener wrote:
> On Wed, 28 Jul 2021, Tom de Vries wrote:
> 
>> Hi,
>>
>> Improve nonnull attribute documentation in a number of ways:
>>
>> Reorganize discussion of effects into:
>> - effects for calls to functions with nonnull-marked parameters, and
>> - effects for function definitions with nonnull-marked parameters.
>> This makes it clear that -fno-delete-null-pointer-checks has no effect for
>> optimizations based on nonnull-marked parameters in function definitions
>> (see PR100404).
>>
>> Mention -Wnonnull-compare.
>>
>> Mention workaround from PR100404 comment 7.
>>
>> The workaround can be used for this scenario.  Say we have a test.c:
>> ...
>>  #include <stdlib.h>
>>
>>  extern int isnull (char *ptr) __attribute__ ((nonnull));
>>  int isnull (char *ptr)
>>  {
>>    if (ptr == 0)
>>      return 1;
>>    return 0;
>>  }
>>
>>  int
>>  main (void)
>>  {
>>    char *ptr = NULL;
>>    if (isnull (ptr)) __builtin_abort ();
>>    return 0;
>>  }
>> ...
>>
>> The test-case contains a mistake: ptr == NULL, and we want to detect the
>> mistake using an abort:
>> ...
>> $ gcc test.c
>> $ ./a.out
>> Aborted (core dumped)
>> ...
>>
>> At -O2 however, the mistake is not detected:
>> ...
>> $ gcc test.c -O2
>> $ ./a.out
>> ...
>> which is what -Wnonnull-compare (not show here) warns about.
>>
>> The easiest way to fix this is by dropping the nonnull attribute.  But that
>> also disables -Wnonnull, which would detect something like:
>> ...
>>   if (isnull (NULL)) __builtin_abort ();
>> ...
>> at compile time.
>>
>> Using this workaround:
>> ...
>>  int isnull (char *ptr)
>>  {
>> +  asm ("" : "+r"(ptr));
>>    if (ptr == 0)
>>      return 1;
>>    return 0;
>>  }
>> ...
>> we still manage to detect the problem at runtime with -O2:
>> ...
>> $ ~/gcc_versions/devel/install/bin/gcc test.c -O2
>> $ ./a.out
>> Aborted (core dumped)
>> ...
>> while keeping the possibility to detect "isnull (NULL)" at compile time.
>>
>> OK for trunk?
> 
> I think it's an improvement over the current situation but the
> inline-assembler suggestion to "fix" definition side optimizations
> are IMHO a hint at that we need a better solution here.

Agreed.

[ If you want I can resubmit without that bit, if it's not acceptable. I
merely included it because it's the only solution I found advertised (in
bugzilla). ]

> Splitting
> the attribute into a caller and a calle side one for example,
> or making -fno-delete-null-pointer-checks do what it suggests.
> 

I think the problem is that in fact there are two attributes:
- assume_nonnull
- verify_nonnull (or, want_nonnull or some such)
which got conflated in the nonnull attribute.  [ Which still could be
fine if you got an -fnonnull=assume/verify to switch between the two
interpretations. ]

Anyway, so more concretely my idea is that this should generate a -Wnonnull:
...
extern void foo (void *ptr) __attribute__((verify_nonnull (1)));
void bar (void) {
  foo (nullptr);
}
...
and the same with assume_nonnull (after all, the assumption is violated).

And this assert could be optimized away (with -Wnonnull-compare):
...
extern void foo (void *ptr) __attribute__((assume_nonnull (1)));
void foo (void *ptr) {
  assert (ptr != nullptr);
}
...
while the assert shouldn't be optimized away with verify_nonnull.

And likewise, this if could be optimized away by
fdelete-null-pointer-checks:
...	
extern void foo (void *ptr) __attribute__((assume_nonnull (1)));
void bar (void *ptr) {
  foo (ptr);
  if (ptr != nullptr)
    { ... }
}
...
while the if shouldn't be optimized away with verify_nonnull.

I think this way of splitting up the functionality conforms to the
principle of least surprise and does not require thinking about caller /
callee distinction, nor does it require extension of
-fno-delete-null-pointer-checks.

Thanks,
- Tom

> And as suggested elsewhere the effect of -fno-delete-null-pointer-checks
> making objects at NULL address valid should be a target hook based
> on the address-space with the default implementation considering
> only the default address-space having no objects at NULL.
>