[Bug c++/108893] New: attribute access read_only

[Bug c++/108893] New: attribute access read_only

[jg at jguk dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108893

            Bug ID: 108893
           Summary: attribute access read_only
           Product: gcc
           Version: 12.2.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: c++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: jg at jguk dot org
  Target Milestone: ---

could attribute access read_only give a build warning about this nullptr
dereference? This example compiles, and then gives SEGV when run.

Following on from gcc-help mailing list discussion I tried this out
https://gcc.gnu.org/pipermail/gcc-help/2023-February/142280.html


void f(const char * const str) __attribute__((access(read_only, 1)));
void f(const char * const str)
{
    __builtin_puts(str);
}

int main()
{
    const char * a = nullptr;
    f(a);
}


https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html

Tested on trunk in C++
https://godbolt.org/z/rc8G6bePq

[Bug c++/108893] attribute access read_only

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108893

--- Comment #1 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Isn't this the same as PR 108871 ?


Also, the access attribute does not imply the attribute nonnull; it may be
appropriate to add both attributes at the declaration of a function that
unconditionally manipulates a buffer via a pointer argument. See the nonnull
attribute for more information and caveats.

[Bug c++/108893] attribute access read_only

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108893

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |RESOLVED
         Resolution|---                         |DUPLICATE

--- Comment #2 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
Yes this is a dup of bug 108871 which was specifically about that gcc-help
thread too.

*** This bug has been marked as a duplicate of bug 108871 ***

[Bug c++/108893] attribute access read_only

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108893

--- Comment #3 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Jonny Grant from comment #0) 
> 
> void f(const char * const str) __attribute__((access(read_only, 1)));
> void f(const char * const str)
> {
>     __builtin_puts(str);
> }
> 
> int main()
> {
>     const char * a = nullptr;
>     f(a);
> }
> 
> Tested on trunk in C++
> https://godbolt.org/z/rc8G6bePq

The goldbolt testcase does not match the above testcase. In fact using the
above testcase works and gives us the warning for both -fanalyzer and
-Wnonnull:
```
In function 'void f(const char*)',
    inlined from 'int main()' at <source>:10:6:
<source>:4:19: warning: use of NULL where non-null expected [CWE-476]
[-Wanalyzer-null-argument]
    4 |     __builtin_puts(str);
      |     ~~~~~~~~~~~~~~^~~~~
  'int main()': event 1
    |
    |   10 |     f(a);
    |      |      ^
    |      |      |
    |      |      (1) inlined call to 'f' from 'main'
    |
    +--> 'void f(const char*)': event 2
           |
           |    4 |     __builtin_puts(str);
           |      |     ~~~~~~~~~~~~~~^~~~~
           |      |                   |
           |      |                   (2) argument 1 NULL where non-null
expected
           |
<built-in>: In function 'int main()':
<built-in>: note: argument 1 of 'int __builtin_puts(const char*)' must be
non-null
In function 'void f(const char*)',
    inlined from 'int main()' at <source>:10:6:
<source>:4:19: warning: argument 1 null where non-null expected [-Wnonnull]
<source>:4:19: note: in a call to built-in function 'int __builtin_puts(const
char*)'
```

[Bug c++/108893] attribute access read_only

[jg at jguk dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108893

--- Comment #4 from Jonny Grant <jg at jguk dot org> ---
My apologies, I had understood attribute access read_only was different from
the attribute nonnull. So I filed a different report for this.

I didn't want to use __attribute__((nonnull)) because the optimizer may use
that knowledge to remove nullptr checks. access read_only I hoped would just
warn about the dereference.

Unfortunately I pasted the wrong example link in the report.

[Bug c++/108893] attribute access read_only

[jg at jguk dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108893

--- Comment #5 from Jonny Grant <jg at jguk dot org> ---
Here is an example, no warnings during compilation.

https://godbolt.org/z/h8E7r3Wf8

#include <cstdlib>
// Try get a build warning for nullptr dereference
__attribute__ ((access (read_only, 1, 2))) void f(char * s, size_t size);

void f(char * s, size_t size)
{
        *s = 'a';
}

int main()
{
    char * s = nullptr;
    f(s, 1);
}

[Bug c++/108893] attribute access read_only

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108893

Andrew Pinski <pinskia at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|DUPLICATE                   |INVALID

--- Comment #6 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
From the manual:
Also, the access attribute does not imply the attribute nonnull; it may be
appropriate to add both attributes at the declaration of a function that
unconditionally manipulates a buffer via a pointer argument. See the nonnull
attribute for more information and caveats.

[Bug c++/108893] attribute access read_only

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108893

--- Comment #7 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
access attribute says if it is access, then it will be that. It does not say it
MUST be accessed. That is what nonnull is for.

>I didn't want to use __attribute__((nonnull)) because the optimizer may use that knowledge to remove nullptr checks. 

It only uses it afterwards or inside the function.
It cannot use nonnull attribute to optimize before the access has happened.

[Bug c++/108893] attribute access read_only

[jg at jguk dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108893

--- Comment #8 from Jonny Grant <jg at jguk dot org> ---
(In reply to Andrew Pinski from comment #7)
> access attribute says if it is access, then it will be that. It does not say
> it MUST be accessed. That is what nonnull is for.
> 
> >I didn't want to use __attribute__((nonnull)) because the optimizer may use that knowledge to remove nullptr checks. 
> 
> It only uses it afterwards or inside the function.
> It cannot use nonnull attribute to optimize before the access has happened.

Thank you for your reply.

https://gcc.gnu.org/onlinedocs/gcc/Common-Function-Attributes.html

The manual does state:

quote 1:
"The compiler may also perform optimizations based on the knowledge that
certain function arguments cannot be null. These optimizations can be disabled
by the -fno-delete-null-pointer-checks option. See Optimize Options."

quote 2:
"The compiler may also perform optimizations based on the knowledge that nonnul
parameters cannot be null. This can currently not be disabled other than by
removing the nonnull attribute."

So the caveat is this issue (2). I can't use attribute nonnull due to these
optimizations that cannot be disabled.

BTW, there is a "nonnul" typo there, if you have a moment and commit access to
change (the patches I sent to gcc-patches for documentation didn't get any
replies)

Thank you again for your reply.

[Bug c++/108893] attribute access read_only

[pinskia at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108893

--- Comment #9 from Andrew Pinski <pinskia at gcc dot gnu.org> ---
(In reply to Jonny Grant from comment #8)
> So the caveat is this issue (2). I can't use attribute nonnull due to these
> optimizations that cannot be disabled.

But you declare that argument cannot be null. So why keep around a check for it
being null. The nonnull attribute is basically saying there is a requirement
for it being nonnull no matter what. It basically says if a null is passed
undefined behavior happens.

[Bug c++/108893] attribute access read_only

[jg at jguk dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108893

--- Comment #10 from Jonny Grant <jg at jguk dot org> ---
(In reply to Andrew Pinski from comment #9)
> (In reply to Jonny Grant from comment #8)
> > So the caveat is this issue (2). I can't use attribute nonnull due to these
> > optimizations that cannot be disabled.
> 
> But you declare that argument cannot be null. So why keep around a check for
> it being null. The nonnull attribute is basically saying there is a
> requirement for it being nonnull no matter what. It basically says if a null
> is passed undefined behavior happens.

My concern would be when building a library, the nullptr checks might be
removed by the optimizer, and then when linked a nullptr could slip through if
they called functions indirectly (ie. not via a header with the same attribute
nonnull)

I would rather avoid undefined behavior nullptr dereference SEGV, as safety
critical software. For instance such functions could check parameters and
return -1 if a nullptr is present. So application can handle it, log an issue
etc

So, I'm adding -fno-delete-null-pointer-checks to my builds as I didn't realise
that was on already in -O3.

[Bug c++/108893] attribute access read_only

[jg at jguk dot org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108893

--- Comment #11 from Jonny Grant <jg at jguk dot org> ---
As you say, in your quote from the manual, the access attribute read_only
doesn't mean there will be any access at all.
However, it doesn't seem to generate any warnings itself, maybe it is only for
the optimizer to utilize this knowledge.


Quoting the manual:
"The read_only access mode specifies that the pointer to which it applies is
used to read the referenced object but not write to it. Unless the argument
specifying the size of the access denoted by size-index is zero, the referenced
object must be initialized."


I tried size-index of 0, and 1, but it doesn't make any difference.

Updated example:
https://godbolt.org/z/Ms3zWThjT


So if I need such nullptr warnings where it is passed and the optimizer can see
it, I should use -Wnonnull and attribute nonnull on those functions. Perhaps
via a macro, so only in a special build run occasionally to check for such
issues.