Re: _Nullable and _Nonnull in GCC's analyzer (was: [PATCH v5] libio: Add nonnull attribute for most FILE * arguments in stdio.h)

[Martin Uecker]

Am Dienstag, dem 08.08.2023 um 17:14 -0700 schrieb enh:
> (bionic maintainer here, mostly by accident...)
> 
> yeah, we tried the GCC attributes once before with _disastrous_
> results (because GCC saw it as an excuse to delete explicit null
> checks, it broke all kinds of things).

Thanks! I am currently exploring different options and what
could be done to improve the situation from the language
as well as compile side.  It looks we have some partial
solution but they do not work quite right or do not
fit together perfectly.


>  the clang attributes are
> "better" in that they don't confuse two entirely separate notions ...
> but they're not "good" because the analysis is so limited. i think we
> were hoping for something more like the "used but not set" kind of
> diagnostic, but afaict it really only catches _direct_ use of a
> literal nullptr. so:
> 
>   foo(nullptr); // caught
> 
>   void* p = nullptr;
>   foo(p); // not caught
> 
> without getting on to anything fancy like:
> 
>   void* p;
>   if (a) p = bar();
>   foo(p);
> 
> we've done all the annotations anyway, but we're not expecting to
> actually catch many bugs with them, and in fact did not catch any real
> bugs in AOSP while adding the annotations. (though we did find several
> "you're not _wrong_, but ..." bits of code :-) )
> 
> what i really want for christmas is the annotation that lets me say
> "this size_t argument tells you the size of this array argument" and
> actually does something usefully fortify-like with that.
> 

What is your opinion about the access attribute?

https://godbolt.org/z/PPfajefvP

it is a bit cumbersome to use, but one can use [static]
instead, which gives you the same static warnings.

[static] does not work with __builtin_dynamic_object_size,
but maybe this could be changed (there is a bug filed.)

I am not sure whether [static] should imply [[gnu::nonnull]]
which would then also trigger the optimization. I think
clang uses it for optimization.

Martin


>  i've seen
> proposals like 
> https://discourse.llvm.org/t/rfc-enforcing-bounds-safety-in-c-fbounds-safety/70854/
> but i haven't seen anything i can use yet, so you -- who do use GCC
> which aiui has something along these lines already -- will find out
> what's good/bad about it before i do...



> 
> On Tue, Aug 8, 2023 at 3:01 AM Martin Uecker  wrote:
> > 
> > Am Montag, dem 10.07.2023 um 22:16 +0200 schrieb Alejandro Colomar via Gcc:
> > > On 7/10/23 22:14, Alejandro Colomar wrote:
> > > > [CC += Andrew]
> > > > 
> > > > Hi Xi, Andrew,
> > > > 
> > > > On 7/10/23 20:41, Xi Ruoyao wrote:
> > > > > Maybe we should have a weaker version of nonnull which only performs 
> > > > > the
> > > > > diagnostic, not the optimization.  But it looks like they hate the 
> > > > > idea:
> > > > > https://gcc.gnu.org/PR110617.
> > > > > 
> > > > This is the one thing that makes me use both Clang and GCC to compile,
> > > > because while any of them would be enough to build, I want as much
> > > > static analysis as I can get, and so I want -fanalyzer (so I need GCC),
> > > > but I also use _Nullable (so I need Clang).
> > > > 
> > > > If GCC had support for _Nullable, I would have in GCC the superset of
> > > > features that I need from both in a single vendor.  Moreover, Clang's
> > > > static analyzer is brain-damaged (sorry, but it doesn't have a simple
> > > > command line to run it, contrary to GCC's easy -fanalyzer), so having
> > > > GCC's analyzer get over those _Nullable qualifiers would be great.
> > > > 
> > > > Clang's _Nullable (and _Nonnull) are not very useful outside of analyzer
> > > > mode, as there are many cases where the compiler doesn't have enough
> > > > information, and the analyzer can get rid of false negatives and
> > > > positives.  See: 
> > > > 
> > > > I'll back the ask for the qualifiers in GCC, for compatibility with
> > > > Clang.
> > > 
> > > BTW, Bionic libc is adding those qualifiers:
> > > 
> > > 
> > > 
> > > 
> > > 
> > 
> > I am sceptical about these qualifiers because they do
> > not fit well with this language mechanism.   Qualifiers take
> > effect at accesses to objects and are discarded at lvalue
> > conversion.  So a qualifier should ideally describe a property
> > that is relevant for accessing objects but is not relevant
> > for values.
> > 
> > Also there are built-in conversion rules a qualifier should
> > conform to.  A pointer pointing to a type without qualifier
> > can implicitely convert to a pointer to a type with qualifier,
> > e.g.   int*  can be converted to const int*.
> > 
> > Together, this implies that a qualifier should add constraints
> > to a type that are relevant to how an object is accessed.
> > 
> > "const" and "volatile" or "_Atomic" are good examples.
> > ("restri

Please see the following post. Some of you may know the answer.

[Luís Carlos Carneiro Gonçalves via Gcc]

https://community.intel.com/t5/GPU-Compute-Software/Intel-GPU-OpenCL-with-g-O3-flag/m-p/1512130/emcs_t/S2h8ZW1haWx8dG9waWNfc3Vic2NyaXB0aW9ufExMMkkxSjhRTU1HMjZZfDE1MTIxMzB8U1VCU0NSSVBUSU9OU3xoSw

Re: Please see the following post. Some of you may know the answer.

[Jonathan Wakely via Gcc]

On Wed, 9 Aug 2023 at 10:00, Luís Carlos Carneiro Gonçalves via Gcc
 wrote:
>
> https://community.intel.com/t5/GPU-Compute-Software/Intel-GPU-OpenCL-with-g-O3-flag/m-p/1512130/emcs_t/S2h8ZW1haWx8dG9waWNfc3Vic2NyaXB0aW9ufExMMkkxSjhRTU1HMjZZfDE1MTIxMzB8U1VCU0NSSVBUSU9OU3xoSw

This is the wrong mailing list for this topic, please use
gcc-h...@gcc.gnu.org next time.

If you get different results for -O2 and -O3 then it probably means
your program has a bug.

"Before reporting that GCC compiles your code incorrectly, compile it
with gcc -Wall -Wextra and see whether this shows anything wrong with
your code. Similarly, if compiling with -fno-strict-aliasing -fwrapv
-fno-aggressive-loop-optimizations makes a difference, or if compiling
with -fsanitize=undefined produces any run-time errors, then your code
is probably not correct." -- https://gcc.gnu.org/bugs/

Re: Please see the following post. Some of you may know the answer.

[Luís Carlos Carneiro Gonçalves via Gcc]

Good morning (here),

Without -O3 and -O2 the OpenCL gave consistently good results without 
returning any error.


I think my code is Ok but I open to criticism.

I call the dynamic library from a Python script (Ctypes package).

If you are open to help me I can send all the sources.

Thanks,

Luís Gonçalves



On 09/08/2023 11:06, Jonathan Wakely wrote:

On Wed, 9 Aug 2023 at 10:00, Luís Carlos Carneiro Gonçalves via Gcc
 wrote:

https://community.intel.com/t5/GPU-Compute-Software/Intel-GPU-OpenCL-with-g-O3-flag/m-p/1512130/emcs_t/S2h8ZW1haWx8dG9waWNfc3Vic2NyaXB0aW9ufExMMkkxSjhRTU1HMjZZfDE1MTIxMzB8U1VCU0NSSVBUSU9OU3xoSw

This is the wrong mailing list for this topic, please use
gcc-h...@gcc.gnu.org next time.

If you get different results for -O2 and -O3 then it probably means
your program has a bug.

"Before reporting that GCC compiles your code incorrectly, compile it
with gcc -Wall -Wextra and see whether this shows anything wrong with
your code. Similarly, if compiling with -fno-strict-aliasing -fwrapv
-fno-aggressive-loop-optimizations makes a difference, or if compiling
with -fsanitize=undefined produces any run-time errors, then your code
is probably not correct." -- https://gcc.gnu.org/bugs/

ISO C's [static] (was: _Nullable and _Nonnull in GCC's analyzer)

[Alejandro Colomar via Gcc]

Hi Martin,

On 2023-08-09 09:26, Martin Uecker wrote:
> it is a bit cumbersome to use, but one can use [static]
> instead, which gives you the same static warnings.
> 
> [static] does not work with __builtin_dynamic_object_size,
> but maybe this could be changed (there is a bug filed.)
> 
> I am not sure whether [static] should imply [[gnu::nonnull]]

I have a gripe with ISO C's [static].  As you mention, ISO
conflated two functionalities in [static]:

-  The size of the array passed as argument must not be less
   than the size specified in the parameter's [].

-  The pointer must not be NULL.

And there are valid cases where you may want the first but
not the second.  Or the second but not the first (that's the
case for _Nonnull, of course).

In fact, it's so badly damaged, that it prompted a proposal
to ISO C of using [static 1] as an equivalent of _Nonnull in
the prototypes that accepted a pointer that should not be
NULL.  However, that proposal didn't include the functions
that actually take arrays as input (because they are taken
in the opposite order, so array syntax is not legal).  Don't
you find it ironic that ISO C could have used array syntax
for pointers and pointer syntax for arrays?  I do.

As for when one would want to mean the first (size of array)
but not _Nonnull: for a function where you may pass either
an array (which should not be smaller than the size), or a
sentinel NULL value.

Nevertheless, I floated the idea that [static] is completely
unnecessary, and nobody has yet been against it.

GCC could perfectly add a warning for the following case:

void foo(size_t n, int a[n]);

int
main(void)
{
int a[7];

foo(42, a);
}

Nobody in their right mind would specify a size of an array
in a parameter and expect that passing a smaller array than
that can produce a valid program.  So, why not make that a
Wall warning?

And so [static] would be irrelevant in GNU C, because well,
what does it add?  In fact, I like that [static] is so badly
designed, because then we can repurpose plain [size] to mean
the right thing, which would produce cleaner programs
([static] just adds noise to the source).

What do you think of giving [42] a meaning, instead of just
ignoring it?

Cheers,
Alex

> which would then also trigger the optimization. I think
> clang uses it for optimization.
> 
> Martin

-- 

GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5



OpenPGP_signature
Description: OpenPGP digital signature

LRA for avr: Arithmetic on stack pointer

[SenthilKumar.Selvaraj--- via Gcc]

Hi,

  After turning on FP -> SP elimination after Vlad fixed
  an elimination issue in 
https://gcc.gnu.org/git?p=gcc.git;a=commit;h=2971ff7b1d564ac04b537d907c70e6093af70832,
  I'm now running into reload failure if arithmetic is done on SP.

  For a call to a vararg functions, the avr target pushes args into the stack,
  calls the function, and then adjusts the SP back to where it was before the
  arg pushing occurred.

  So for code like

extern int foo(int, ...);
int bar(void) {
  long double l = 1.2345E6;
  foo(0, l);
  return 0;
}

  and 

$ avr-gcc -mmcu=avr51 -Os ../20031208-1.c
  
  Reload sees


(insn 5 2 6 2 (set (reg:QI 44)
(const_int 65 [0x41])) "../20031208-1.c":4:3 86 {movqi_insn_split}
 (expr_list:REG_EQUIV (const_int 65 [0x41])
(nil)))
(insn 6 5 7 2 (set (mem:QI (post_dec:HI (reg/f:HI 32 __SP_L__)) [0  S1 A8])
(reg:QI 44)) "../20031208-1.c":4:3 1 {pushqi1}
 (expr_list:REG_DEAD (reg:QI 44)
(expr_list:REG_ARGS_SIZE (const_int 1 [0x1])
(nil
...

...
(call_insn 19 18 21 2 (parallel [
(set (reg:HI 24 r24)
(call (mem:HI (symbol_ref:HI ("foo") [flags 0x41]  
) [0 foo S2 A8])
(const_int 10 [0xa])))
(use (const_int 0 [0]))
]) "../20031208-1.c":4:3 776 {call_value_insn}
 (expr_list:REG_UNUSED (reg:HI 24 r24)
(expr_list:REG_CALL_DECL (symbol_ref:HI ("foo") [flags 0x41]  
)
(nil)))
(nil))
(insn 21 19 25 2 (set (reg/f:HI 32 __SP_L__)
(plus:HI (reg/f:HI 32 __SP_L__)
(const_int 10 [0xa]))) "../20031208-1.c":5:10 discrim 1 165 
{*addhi3_split}
 (expr_list:REG_UNUSED (reg:QI 33 __SP_H__)
(expr_list:REG_ARGS_SIZE (const_int 0 [0])
(nil

  LRA doesn't pick any of the 4 alternatives for insn 21

Considering alt=0 of insn 21:   (0) =??r  (1) %0  (2) r
Staticly defined alt reject+=12
 Considering alt=1 of insn 21:   (0) d  (1) 0  (2) s
 Considering alt=2 of insn 21:   (0) !w  (1) 0  (2) IJYIJ
Staticly defined alt reject+=600
 Considering alt=3 of insn 21:   (0) d  (1) 0  (2) nYnn
 Considering alt=0 of insn 21:   (0) =??r  (1) %0  (2) r
Staticly defined alt reject+=12
 Considering alt=1 of insn 21:   (0) d  (1) 0  (2) s
 Considering alt=2 of insn 21:   (0) !w  (1) 0  (2) IJYIJ
Staticly defined alt reject+=600
 Considering alt=3 of insn 21:   (0) d  (1) 0  (2) nYnn

  whereas classic reload does.

Reloads for insn # 21
Reload 0: reload_in (HI) = (reg/f:HI 32 __SP_L__)
reload_out (HI) = (reg/f:HI 32 __SP_L__)
LD_REGS, RELOAD_OTHER (opnum = 0)
reload_in_reg: (reg/f:HI 32 __SP_L__)
reload_out_reg: (reg/f:HI 32 __SP_L__)
reload_reg_rtx: (reg:HI 24 r24)


  Digging through the code, I found lra-constraints.c:2687, which forbids
  output reload of SP, with a comment that says

  /* Never do output reload of stack pointer.  It makes
 impossible to do elimination when SP is changed in
 RTL.  */

   Just to check if that is indeed the problem, I commented out the check
   and goto fail; that follows that comment, and reload/code-gen worked fine.

Considering alt=0 of insn 21:   (0) =??r  (1) %0  (2) r
Staticly defined alt reject+=12
0 Non-pseudo reload: reject+=2
0 Small class reload: reject+=3
0 Non input pseudo reload: reject++
overall=24,losers=1 -- refuse
 Considering alt=1 of insn 21:   (0) d  (1) 0  (2) s
0 Non-pseudo reload: reject+=2
0 Small class reload: reject+=3
0 Non input pseudo reload: reject++
1 Matching alt: reject+=2
1 Non-pseudo reload: reject+=2
1 Small class reload: reject+=3
1 Non input pseudo reload: reject++
overall=26,losers=2 -- refuse
 Considering alt=2 of insn 21:   (0) !w  (1) 0  (2) IJYIJ
Staticly defined alt reject+=600
0 Non-pseudo reload: reject+=2
0 Non input pseudo reload: reject++
overall=609,losers=1 -- refuse
 Considering alt=3 of insn 21:   (0) d  (1) 0  (2) nYnn
0 Non-pseudo reload: reject+=2
0 Small class reload: reject+=3
0 Non input pseudo reload: reject++
1 Matching alt: reject+=2
1 Non-pseudo reload: reject+=2
1 Small class reload: reject+=3
1 Non input pseudo reload: reject++
overall=26,losers=2 -- refuse
  Choosing alt 3 in insn 21:  (0) d  (1) 0  (2) nYnn {*addhi3_split} 
(sp_off=-10)
  Creating newreg=50 from oldreg=32, assigning class LD_REGS to r50
   21: r50:HI=r50:HI+0xa
  REG_UNUSED __SP_H__:QI
  REG_ARGS_SIZE 0
Inserting insn reload before:
   29: r50:HI=__SP_L__:HI
Inserting insn reload after:
   30: __SP_L__:HI=r50:HI

  eventually generating

(

Re: ISO C's [static] (was: _Nullable and _Nonnull in GCC's analyzer)

[Martin Uecker]

Hi Alejandro!

Am Mittwoch, dem 09.08.2023 um 12:42 +0200 schrieb Alejandro Colomar:

...

> 
> As for when one would want to mean the first (size of array)
> but not _Nonnull: for a function where you may pass either
> an array (which should not be smaller than the size), or a
> sentinel NULL value.
> 
> Nevertheless, I floated the idea that [static] is completely
> unnecessary, and nobody has yet been against it.
> 
> GCC could perfectly add a warning for the following case:
> 
> void foo(size_t n, int a[n]);
> 
> int
> main(void)
> {
> int a[7];
> 
> foo(42, a);
> }
> 
> Nobody in their right mind would specify a size of an array
> in a parameter and expect that passing a smaller array than
> that can produce a valid program.  So, why not make that a
> Wall warning?

But we have this warning! is even activated by 
default without -Wall and already since GCC 11:





https://godbolt.org/z/sMbTon458

But this is for minimum required elements. How do 
we differentiate between null and non-null?

We have:

int[] or int* // no bound, nullable
int[N]// at least N, nullable
int[static N] // at least N, nonnull

The 'static' implies nonnull, so we could 
use 'static' to diffentiate between nonnull 
and nullable. 

What is missing something which implies bounds
also inside the callee.  You can use the "access"
attribute or we extend the meaning of int[N]
and int[static N] also imply a maximum bound.


Martin

Re: ISO C's [static] (was: _Nullable and _Nonnull in GCC's analyzer)

[Alejandro Colomar via Gcc]

Hi Martin!

On 2023-08-09 14:03, Martin Uecker wrote:
[...]

>> GCC could perfectly add a warning for the following case:
>>
>> void foo(size_t n, int a[n]);
>>
>> int
>> main(void)
>> {
>> int a[7];
>>
>> foo(42, a);
>> }
>>
>> Nobody in their right mind would specify a size of an array
>> in a parameter and expect that passing a smaller array than
>> that can produce a valid program.  So, why not make that a
>> Wall warning?
> 
> But we have this warning! is even activated by 
> default without -Wall and already since GCC 11:

Ahh, I hadn't realized that was added (relatievly) recently.
That's great news!  Thanks!

> https://godbolt.org/z/sMbTon458
> 
> But this is for minimum required elements. How do 
> we differentiate between null and non-null?
> 
> We have:
> 
> int[] or int* // no bound, nullable
> int[N]  // at least N, nullable
> int[static N] // at least N, nonnull
> 
> The 'static' implies nonnull, so we could 
> use 'static' to diffentiate between nonnull 
> and nullable. 

Since static is only for arrays (okay, they're all pointers, but
it's only usable with array syntax), and nullability is a thing
of pointers, I think static is not the right choice.

I oppose using array syntax for pointers, as it can confuse
programmers.

In any case, [static] is not that different from [_Nonnull], and
_Nonnull is more flexible, since you can also use it as
*_Nonnull.

Regarding the issues you have with _Nonnull being a qualifier,
I've been thinking about it for a long time and don't yet have
a concrete answer.  The more I think about it, the more I'm
convinced it is like `restrict`.  You can't have null correctness
as we can do with `const`.  With const, we know it's always bad
to store a const object in a non-const pointer.  With NULL, it
depends on the context: if you've checked for NULL in the lines
above, then it's fine to do the conversion.

There is a proposal for Clang to add `_Optional`, a qualifier to
the pointee, as a more correct version of _Nullable.  The
following would be equivalent:

int *_Nullable  i;
_Optional int   *j;

However, I don't believe it's a good choice, because of the above.
Instead, I think _Nullable or _Nonnull should be like `restrict`
and used only in function prototypes.  Let the analyzer do the
job.  I know `restrict` is hard to grasp, but I couldn't think of
anything better.

> 
> What is missing something which implies bounds
> also inside the callee.  You can use the "access"
> attribute or we extend the meaning of int[N]
> and int[static N] also imply a maximum bound.

Why is the upper bound important?  It's usually irrelevant.

In the case where one wants to make sure that an array is of an
exact size (instead of just "at least"), pointers to arrays can be
used.  They're just not often used, because you don't need that
so often.  I don't think we need a new addition to the language,
do we?

$ cat ap.c
#include 

void
foo(size_t n, int (*a)[n])
{
for (size_t i = 0; i < n; i++)
(*a)[i] = 42;
}
$ gcc-13 -S -Wall -Wextra ap.c 
$ 

In the function above, n is not a lower bound, but an exact bound.

GCC should add a warning for the following code:

$ cat ap2.c
#include 

void foo(size_t n, int (*a)[n]);

void
bar(void)
{
int x[7];

foo(3, &x);
foo(9, &x);
}
$ gcc-13 -S -Wall -Wextra ap2.c
$ 

Neither GCC nor Clang warn about that.  Not even with -fanalyzer.

Cheers,
Alex

-- 

GPG key fingerprint: A9348594CE31283A826FBDD8D57633D441E25BB5



OpenPGP_signature
Description: OpenPGP digital signature

Re: ISO C's [static] (was: _Nullable and _Nonnull in GCC's analyzer)

[Xi Ruoyao via Gcc]

On Wed, 2023-08-09 at 12:42 +0200, Alejandro Colomar wrote:
> I have a gripe with ISO C's [static].  As you mention, ISO
> conflated two functionalities in [static]:
> 
> -  The size of the array passed as argument must not be less
>    than the size specified in the parameter's [].
> 
> -  The pointer must not be NULL.

https://gcc.gnu.org/pipermail/gcc-patches/2023-July/625559.html

If this patch is merged, they'll just become __nonnull and have all
advantages and disadvantages as __nonnull.

-- 
Xi Ruoyao 
School of Aerospace Science and Technology, Xidian University

Re: Please see the following post. Some of you may know the answer.

[Paul Smith]

On Wed, 2023-08-09 at 11:14 +0200, Luís Carlos Carneiro Gonçalves via
Gcc wrote:
> Without -O3 and -O2 the OpenCL gave consistently good results without
> returning any error.
> 
> I think my code is Ok but I open to criticism.

By "has a bug" we don't necessarily mean something that is obviously
wrong and fails every time like trying to dereference a null pointer.

We mean, you are making an assumption about how the compiler will
compile your code, that is not guaranteed by the standard.  Because
it's not guaranteed, when you enable optimizations the compiler is free
to rearrange things in ways you maybe don't expect, but are allowed by
the language definition.

> If you are open to help me I can send all the sources.

I doubt anyone here has the time or energy to read and debug "all the
sources" for you.

If you reduce your code to a small example that shows the error, then
post that to the correct mailing list as Jonathan suggested.  Then
someone may be able to point out your mistake.

Or, maybe it really is a bug in the optimizer.  Such things are very
rare but have been known to happen.  In my long experience as a
programmer, however, "the compiler has a bug" has NEVER been a winning
bet.

Re: Please see the following post. Some of you may know the answer.

[Luís Carlos Carneiro Gonçalves via Gcc]

It was a bug. I already corrected. It was an array with bad dimensions.

It was like:

int a[2];

a[2]=1;

**

Corrected to:

int a[3];



I updated the post in the Intel Forum.

Sorry. Thanks.

Luís Gonçalves




On 09/08/2023 15:50, Paul Smith wrote:

On Wed, 2023-08-09 at 11:14 +0200, Luís Carlos Carneiro Gonçalves via
Gcc wrote:

Without -O3 and -O2 the OpenCL gave consistently good results without
returning any error.

I think my code is Ok but I open to criticism.

By "has a bug" we don't necessarily mean something that is obviously
wrong and fails every time like trying to dereference a null pointer.

We mean, you are making an assumption about how the compiler will
compile your code, that is not guaranteed by the standard. Because
it's not guaranteed, when you enable optimizations the compiler is free
to rearrange things in ways you maybe don't expect, but are allowed by
the language definition.


If you are open to help me I can send all the sources.

I doubt anyone here has the time or energy to read and debug "all the
sources" for you.

If you reduce your code to a small example that shows the error, then
post that to the correct mailing list as Jonathan suggested. Then
someone may be able to point out your mistake.

Or, maybe it really is a bug in the optimizer.  Such things are very
rare but have been known to happen.  In my long experience as a
programmer, however, "the compiler has a bug" has NEVER been a winning
bet.

Re: ISO C's [static] (was: _Nullable and _Nonnull in GCC's analyzer)

[Martin Uecker]

Am Mittwoch, dem 09.08.2023 um 14:37 +0200 schrieb Alejandro Colomar:
> Hi Martin!
> 
> On 2023-08-09 14:03, Martin Uecker wrote:


> 
> Regarding the issues you have with _Nonnull being a qualifier,
> I've been thinking about it for a long time and don't yet have
> a concrete answer.  The more I think about it, the more I'm
> convinced it is like `restrict`.  You can't have null correctness
> as we can do with `const`.  With const, we know it's always bad
> to store a const object in a non-const pointer.  With NULL, it
> depends on the context: if you've checked for NULL in the lines
> above, then it's fine to do the conversion.
> 
> There is a proposal for Clang to add `_Optional`, a qualifier to
> the pointee, as a more correct version of _Nullable.  The
> following would be equivalent:
> 
> int *_Nullable  i;
> _Optional int   *j;
> 
> However, I don't believe it's a good choice, because of the above.
> Instead, I think _Nullable or _Nonnull should be like `restrict`
> and used only in function prototypes.  Let the analyzer do the
> job.  I know `restrict` is hard to grasp, but I couldn't think of
> anything better.

I think this would be bad. More confusion is the least we need.
I would definitely prefer an attribute over a confusing qualifier 
which is not really a qualifier. 

_Optional is a something else and maybe not directly
comparable.  I think it is an interesting idea. First, it 
would fit well with rules the language.  One gets 
conversion errors exactly as with other qualifiers. So
one could add it to selected pointer in an existing code
base and incrementally improve the code by adding checks.

It would not be useful for existing interfaces that
need stay compatible.

> > 
> > What is missing something which implies bounds
> > also inside the callee.  You can use the "access"
> > attribute or we extend the meaning of int[N]
> > and int[static N] also imply a maximum bound.
> 
> Why is the upper bound important?  It's usually irrelevant.
> 
> In the case where one wants to make sure that an array is of an
> exact size (instead of just "at least"), pointers to arrays can be
> used.  They're just not often used, because you don't need that
> so often.  I don't think we need a new addition to the language,
> do we?
> 
> $ cat ap.c
> #include 
> 
> void
> foo(size_t n, int (*a)[n])
> {
> for (size_t i = 0; i < n; i++)
> (*a)[i] = 42;
> }
> $ gcc-13 -S -Wall -Wextra ap.c 
> $ 
> 
> In the function above, n is not a lower bound, but an exact bound.
> 
> GCC should add a warning for the following code:
> 
> $ cat ap2.c
> #include 
> 
> void foo(size_t n, int (*a)[n]);
> 
> void
> bar(void)
> {
> int x[7];
> 
> foo(3, &x);
> foo(9, &x);
> }
> $ gcc-13 -S -Wall -Wextra ap2.c
> $ 
> 
> Neither GCC nor Clang warn about that.  Not even with -fanalyzer.

I know. One can already get bounds checking side the callee with
this using UBSan. I have UBSan patch which would protect the
call itself and make sure the bounds are correct. Compile-time
warnings would also be good.

In any case, this does not work for existing interfaces.

Martin

Re: LRA for avr: Arithmetic on stack pointer

[Georg-Johann Lay]

Am 09.08.23 um 13:15 schrieb SenthilKumar.Selvaraj--- via Gcc:

[...]
   I guess the condition exists to ensure sp_off is always correct? Considering 
LRA already
   handles post_dec of SP just fine, perhaps it can allow RTX like

(set (reg/f:HI 32 __SP_L__)
  (plus:HI (reg/f:HI 32 __SP_L__)
   (const_int 10 [0xa]))) "../20031208-1.c":5:10 discrim 1 165 
{*addhi3_split}

   as long as the PLUS/MINUS is by a constant, and update sp_off accordingly?


It might be the case that non-const addends may also occur, e.g. with 
alloca like code?


Johann



   Or is there something the avr target has to do differently?

Regards
Senthil

[PATCH v2] analyzer: More features for CPython analyzer plugin [PR107646]

[Eric Feng via Gcc]

Thank you for your help in getting dg-require-python-h working! I can
confirm that the FAILs are related to differences between the --cflags
affecting the gimple seen by the analyzer. For this reason, I have
changed it to --includes for now. To be sure, I tested on Python 3.8 as
well and it works as expected. I have also addressed the following
comments on the WIP patch as you described.

-- Update Changelog entry to list new functions being simulated.
-- Update region_model::get_or_create_region_for_heap_alloc leading
comment.
-- Change register_alloc to update_state_machine.
-- Change move_ptr_sval_non_null to transition_ptr_sval_non_null.
-- Static helper functions for:
-- Initializing ob_refcnt field.
-- Setting ob_type field.
-- Getting ob_base field.
-- Initializing heap allocated region for PyObjects.
-- Incrementing a field by one.
-- Change arg_is_long_p to arg_is_integral_p.
-- Extract common failure scenario for reusability.

The initial WIP patch using 

/* { dg-options "-fanalyzer -I/usr/include/python3.9" }. */

have been bootstrapped and regtested on aarch64-unknown-linux-gnu. Since
we did not change any core logic in the revision and the only changes
within the analyzer core are changing variable names, is it OK for
trunk. In the mean time, the revised patch is currently going through
bootstrap and regtest process.

Best,
Eric

---
This patch adds known function subclasses for Python/C API functions
PyList_New, PyLong_FromLong, and PyList_Append. It also adds new
optional parameters for
region_model::get_or_create_region_for_heap_alloc, allowing for the
newly allocated region to immediately transition from the start state to
the assumed non-null state in the malloc state machine if desired.
Finally, it adds a new procedure, dg-require-python-h, intended as a
directive in Python-related analyzer tests, to append necessary Python
flags during the tests' build process.

The main warnings we gain in this patch with respect to the known function
subclasses mentioned are leak related. For example:

rc3.c: In function ‘create_py_object’:
│
rc3.c:21:10: warning: leak of ‘item’ [CWE-401] [-Wanalyzer-malloc-leak]
│
   21 |   return list;
  │
  |  ^~~~
│
  ‘create_py_object’: events 1-4
│
|
│
|4 |   PyObject* item = PyLong_FromLong(10);
│
|  |^~~
│
|  ||
│
|  |(1) allocated here
│
|  |(2) when ‘PyLong_FromLong’ succeeds
│
|5 |   PyObject* list = PyList_New(2);
│
|  |~
│
|  ||
│
|  |(3) when ‘PyList_New’ fails
│
|..
│
|   21 |   return list;
│
|  |  
│
|  |  |
│
|  |  (4) ‘item’ leaks here; was allocated at (1)
│

Some concessions were made to
simplify the analysis process when comparing kf_PyList_Append with the
real implementation. In particular, PyList_Append performs some
optimization internally to try and avoid calls to realloc if
possible. For simplicity, we assume that realloc is called every time.
Also, we grow the size by just 1 (to ensure enough space for adding a
new element) rather than abide by the heuristics that the actual implementation
follows.

gcc/analyzer/ChangeLog:
PR analyzer/107646
* region-model.cc (region_model::get_or_create_region_for_heap_alloc):
New optional parameters.
* region-model.h (class region_model): New optional parameters.
* sm-malloc.cc (on_realloc_with_move): New function.
(region_model::transition_ptr_sval_non_null): New function.

gcc/testsuite/ChangeLog:
PR analyzer/107646
* gcc.dg/plugin/analyzer_cpython_plugin.c: Analyzer support for
PyList_New, PyList_Append, PyLong_FromLong
* gcc.dg/plugin/plugin.exp: New test.
* lib/target-supports.exp: New procedure.
* gcc.dg/plugin/cpython-plugin-test-2.c: New test.

Signed-off-by: Eric Feng 
---
 gcc/analyzer/region-model.cc  |  20 +-
 gcc/analyzer/region-model.h   |  10 +-
 gcc/analyzer/sm-malloc.cc |  40 +
 .../gcc.dg/plugin/analyzer_cpython_plugin.c   | 711 ++
 .../gcc.dg/plugin/cpython-plugin-test-2.c |  78 ++
 gcc/testsuite/gcc.dg/plugin/plugin.exp|   3 +-
 gcc/testsuite/lib/target-supports.exp |  25 +
 7 files changed, 881 insertions(+), 6 deletions(-)
 create mode 100644 gcc/testsuite/gcc.dg/plugin/cpython-plugin-test-2.c

diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-model.cc
index e92b3f7b074..c338f045d92 100644
--- a/gcc/analyzer/region-model.cc
+++ b/gcc/analyzer/region-model.cc
@@ -5127,11 +5127,16 @@ region_model::check_dynamic_size_for_floats (const 
svalue *size_in_bytes,
Use CTXT to complain about tainted sizes.
 
Reuse an existing heap_al

Re: LRA for avr: Arithmetic on stack pointer

[Vladimir Makarov via Gcc]

On 8/9/23 07:15, senthilkumar.selva...@microchip.com wrote:

Hi,

   After turning on FP -> SP elimination after Vlad fixed
   an elimination issue in 
https://gcc.gnu.org/git?p=gcc.git;a=commit;h=2971ff7b1d564ac04b537d907c70e6093af70832,
   I'm now running into reload failure if arithmetic is done on SP.

   For a call to a vararg functions, the avr target pushes args into the stack,
   calls the function, and then adjusts the SP back to where it was before the
   arg pushing occurred.

   So for code like

extern int foo(int, ...);
int bar(void) {
   long double l = 1.2345E6;
   foo(0, l);
   return 0;
}

With some efforts, I reproduced this problem.

   and

$ avr-gcc -mmcu=avr51 -Os ../20031208-1.c
   

...



   I guess the condition exists to ensure sp_off is always correct? Considering 
LRA already
   handles post_dec of SP just fine, perhaps it can allow RTX like


It is a very old code when LRA elimination was pretty constraint.


(set (reg/f:HI 32 __SP_L__)
  (plus:HI (reg/f:HI 32 __SP_L__)
   (const_int 10 [0xa]))) "../20031208-1.c":5:10 discrim 1 165 
{*addhi3_split}

   as long as the PLUS/MINUS is by a constant, and update sp_off accordingly?

   Or is there something the avr target has to do differently?


I think we can permit to stack pointer output reloads.  The only thing 
we need to update sp offset accurately for the original and reload 
insns.  I'll try to make the patch on this week.

Re: [PATCH v2] analyzer: More features for CPython analyzer plugin [PR107646]

[David Malcolm via Gcc]

On Wed, 2023-08-09 at 15:22 -0400, Eric Feng wrote:
> Thank you for your help in getting dg-require-python-h working! I can
> confirm that the FAILs are related to differences between the --
> cflags
> affecting the gimple seen by the analyzer. For this reason, I have
> changed it to --includes for now. 

Sounds good.

Eventually we'll probably want to support --cflags, but given that
every distribution probably has its own set of flags, it's a recipe for
an unpleasantly large test matrix, so just using --includes is a good
compromise.

> To be sure, I tested on Python 3.8 as
> well and it works as expected. I have also addressed the following
> comments on the WIP patch as you described.
> 
> -- Update Changelog entry to list new functions being simulated.
> -- Update region_model::get_or_create_region_for_heap_alloc leading
> comment.
> -- Change register_alloc to update_state_machine.
> -- Change move_ptr_sval_non_null to transition_ptr_sval_non_null.
> -- Static helper functions for:
> -- Initializing ob_refcnt field.
> -- Setting ob_type field.
> -- Getting ob_base field.
> -- Initializing heap allocated region for PyObjects.
> -- Incrementing a field by one.
> -- Change arg_is_long_p to arg_is_integral_p.
> -- Extract common failure scenario for reusability.
> 
> The initial WIP patch using 
> 
> /* { dg-options "-fanalyzer -I/usr/include/python3.9" }. */
> 
> have been bootstrapped and regtested on aarch64-unknown-linux-gnu.
> Since
> we did not change any core logic in the revision and the only changes
> within the analyzer core are changing variable names, is it OK for
> trunk. In the mean time, the revised patch is currently going through
> bootstrap and regtest process.

Thanks for the updated patch.

Unfortunately I just pushed a largish analyzer patch (r14-3114-
g73da34a538ddc2) which may well conflict with your patch, so please
rebase to beyond that.  

Sorry about this.

In particular note that there's no longer a default assignment to the
LHS at a call-site in region_model::on_call_pre; known_function
subclasses are now responsible for assigning to the LHS of the
callsite.  But I suspect that all the known_function subclasses in the
cpython plugin already do that.

Some nits inline below...

[...snip...]

> Some concessions were made to
> simplify the analysis process when comparing kf_PyList_Append with
> the
> real implementation. In particular, PyList_Append performs some
> optimization internally to try and avoid calls to realloc if
> possible. For simplicity, we assume that realloc is called every
> time.
> Also, we grow the size by just 1 (to ensure enough space for adding a
> new element) rather than abide by the heuristics that the actual
> implementation
> follows.

Might be worth capturing these notes as comments in the source (for
class kf_PyList_Append), rather than just within the commit message.

[...snip...]
> 
> diff --git a/gcc/analyzer/region-model.cc b/gcc/analyzer/region-
> model.cc
> index e92b3f7b074..c338f045d92 100644
> --- a/gcc/analyzer/region-model.cc
> +++ b/gcc/analyzer/region-model.cc
> @@ -5127,11 +5127,16 @@ region_model::check_dynamic_size_for_floats
> (const svalue *size_in_bytes,
>     Use CTXT to complain about tainted sizes.
>  
>     Reuse an existing heap_allocated_region if it's not being
> referenced by
> -   this region_model; otherwise create a new one.  */
> +   this region_model; otherwise create a new one.
> +
> +   Optionally (update_state_machine) transitions the pointer
> pointing to the
> +   heap_allocated_region from start to assumed non-null.  */
>  
>  const region *
>  region_model::get_or_create_region_for_heap_alloc (const svalue
> *size_in_bytes,
> - 
> region_model_context *ctxt)
> +   region_model_context *ctxt,
> +   bool update_state_machine,
> +   const call_details *cd)
>  {
>    /* Determine which regions are referenced in this region_model, so
> that
>   we can reuse an existing heap_allocated_region if it's not in
> use on
> @@ -5153,6 +5158,17 @@
> region_model::get_or_create_region_for_heap_alloc (const svalue
> *size_in_bytes,
>    if (size_in_bytes)
>  if (compat_types_p (size_in_bytes->get_type (), size_type_node))
>    set_dynamic_extents (reg, size_in_bytes, ctxt);
> +
> +   if (update_state_machine && cd)
> +   {
> +   const svalue *ptr_sval = nullptr;
> +   if (cd->get_lhs_type ())
> +   ptr_sval = m_mgr->get_ptr_svalue (cd->get_lhs_type (), reg);
> +   else
> +   ptr_sval = m_mgr->get_ptr_svalue (NULL_TREE, reg);
> +   transition_ptr_sval_non_null (ctxt,
> ptr_sval);

This if/else is redundant: the "else" is only reached if cd-
>get_lhs_type () is null, in which case you pass in NULL_TREE, so it
works the same either way.  Or am I missing something?

Also, it looks like something weird's happening with