[1/2] testsuite: Fix analyzer errors for newlib-errno

  Ok to commit?
-- >8 --
Investigating analyzer tesstsuite errors for cris-elf.  The same are
seen for pru-elf according to posts to gcc-testresults@.

For glibc, errno is #defined as:
 extern int *__errno_location (void) __THROW __attribute_const__;
 # define errno (*__errno_location ())
while for newlib in its default configuration, it's:
 #define errno (*__errno())
 extern int *__errno (void);

The critical difference is that __attribute__ ((__const__)),
where glibc says that the caller will see the same value on
all calls (from the same context; read: same thread).  I'm
not sure the absence of __attribute__ ((__const__)) for the
newlib definition is deliberate, but I guess it can.
Either way, without the "const" attribute, it can't be known
that the same location will be returned the next time, so
analyzer-tests that depend the value being known it should
see UNKNOWN rather than TRUE, that's why the deliberate
check for UNKNOWN rather than xfailing the test.

For isatty-1.c, it's the same problem, but here it'd be
unweildy with the extra dg-lines, so better just skip it for
newlib targets.

testsuite:
	* gcc.dg/analyzer/call-summaries-errno.c: Expect UNKNOWN
	for newlib after having set errno.
	* gcc.dg/analyzer/errno-1.c: Ditto.
	* gcc.dg/analyzer/isatty-1.c: Skip for newlib targets.
---
 gcc/testsuite/gcc.dg/analyzer/call-summaries-errno.c | 3 ++-
 gcc/testsuite/gcc.dg/analyzer/errno-1.c              | 3 ++-
 gcc/testsuite/gcc.dg/analyzer/isatty-1.c             | 2 +-
 3 files changed, 5 insertions(+), 3 deletions(-)
  

On Tue, 2023-02-28 at 19:47 +0100, Hans-Peter Nilsson wrote:
> Ok to commit?
> -- >8 --
> Investigating analyzer tesstsuite errors for cris-elf.  The same are
> seen for pru-elf according to posts to gcc-testresults@.
> 
> For glibc, errno is #defined as:
>  extern int *__errno_location (void) __THROW __attribute_const__;
>  # define errno (*__errno_location ())
> while for newlib in its default configuration, it's:
>  #define errno (*__errno())
>  extern int *__errno (void);

We're already handling ___errno (three underscores) for Solaris as of
7c9717fcb5cf94ce1e7ef5c903058adf9980ff28; does it fix the issue if you 
add __errno (two underscores) to analyzer/kf.cc's 
register_known_functions in an analogous way to that commit?  (i.e.
wiring it up to kf_errno_location, "teaching" the analyzer that that
function returns a pointer to the "errno region")

Dave

> 
> The critical difference is that __attribute__ ((__const__)),
> where glibc says that the caller will see the same value on
> all calls (from the same context; read: same thread).  I'm
> not sure the absence of __attribute__ ((__const__)) for the
> newlib definition is deliberate, but I guess it can.
> Either way, without the "const" attribute, it can't be known
> that the same location will be returned the next time, so
> analyzer-tests that depend the value being known it should
> see UNKNOWN rather than TRUE, that's why the deliberate
> check for UNKNOWN rather than xfailing the test.
> 
> For isatty-1.c, it's the same problem, but here it'd be
> unweildy with the extra dg-lines, so better just skip it for
> newlib targets.
> 
> testsuite:
>         * gcc.dg/analyzer/call-summaries-errno.c: Expect UNKNOWN
>         for newlib after having set errno.
>         * gcc.dg/analyzer/errno-1.c: Ditto.
>         * gcc.dg/analyzer/isatty-1.c: Skip for newlib targets.
> ---
>  gcc/testsuite/gcc.dg/analyzer/call-summaries-errno.c | 3 ++-
>  gcc/testsuite/gcc.dg/analyzer/errno-1.c              | 3 ++-
>  gcc/testsuite/gcc.dg/analyzer/isatty-1.c             | 2 +-
>  3 files changed, 5 insertions(+), 3 deletions(-)
> 
> diff --git a/gcc/testsuite/gcc.dg/analyzer/call-summaries-errno.c
> b/gcc/testsuite/gcc.dg/analyzer/call-summaries-errno.c
> index e4333b30bb77..cf4d9f7141e4 100644
> --- a/gcc/testsuite/gcc.dg/analyzer/call-summaries-errno.c
> +++ b/gcc/testsuite/gcc.dg/analyzer/call-summaries-errno.c
> @@ -13,5 +13,6 @@ void test_sets_errno (int y)
>    sets_errno (y);
>    sets_errno (y);
>  
> -  __analyzer_eval (errno == y); /* { dg-warning "TRUE" } */  
> +  __analyzer_eval (errno == y); /* { dg-warning "TRUE" "errno is at
> a constant location" { target { ! newlib } } } */
> +  /* { dg-warning "UNKNOWN" "errno is not known to be at a constant
> location" { target { newlib } } .-1 } */
>  }
> diff --git a/gcc/testsuite/gcc.dg/analyzer/errno-1.c
> b/gcc/testsuite/gcc.dg/analyzer/errno-1.c
> index 6b9d28c10799..af0cc3d52a36 100644
> --- a/gcc/testsuite/gcc.dg/analyzer/errno-1.c
> +++ b/gcc/testsuite/gcc.dg/analyzer/errno-1.c
> @@ -17,7 +17,8 @@ void test_storing_to_errno (int val)
>  {
>    __analyzer_eval (errno == val); /* { dg-warning "UNKNOWN" } */
>    errno = val;
> -  __analyzer_eval (errno == val); /* { dg-warning "TRUE" } */
> +  __analyzer_eval (errno == val); /* { dg-warning "TRUE" "errno is
> at a constant location" { target { ! newlib } } } */
> +  /* { dg-warning "UNKNOWN" "errno is not known to be at a constant
> location" { target { newlib } } .-1 } */
>    external_fn ();
>    __analyzer_eval (errno == val); /* { dg-warning "UNKNOWN" } */  
>  }
> diff --git a/gcc/testsuite/gcc.dg/analyzer/isatty-1.c
> b/gcc/testsuite/gcc.dg/analyzer/isatty-1.c
> index 389d2cdf3f18..450a7d71990d 100644
> --- a/gcc/testsuite/gcc.dg/analyzer/isatty-1.c
> +++ b/gcc/testsuite/gcc.dg/analyzer/isatty-1.c
> @@ -1,4 +1,4 @@
> -/* { dg-skip-if "" { powerpc*-*-aix* } } */
> +/* { dg-skip-if "" { powerpc*-*-aix* || newlib } } */
>  
>  #include <errno.h>
>  #include "analyzer-decls.h"
  

> From: David Malcolm <dmalcolm@redhat.com>
> Date: Tue, 28 Feb 2023 14:12:47 -0500

> On Tue, 2023-02-28 at 19:47 +0100, Hans-Peter Nilsson wrote:
> > Ok to commit?
> > -- >8 --
> > Investigating analyzer tesstsuite errors for cris-elf.  The same are
> > seen for pru-elf according to posts to gcc-testresults@.
> > 
> > For glibc, errno is #defined as:
> >  extern int *__errno_location (void) __THROW __attribute_const__;
> >  # define errno (*__errno_location ())
> > while for newlib in its default configuration, it's:
> >  #define errno (*__errno())
> >  extern int *__errno (void);
> 
> We're already handling ___errno (three underscores) for Solaris as of
> 7c9717fcb5cf94ce1e7ef5c903058adf9980ff28; does it fix the issue if you 
> add __errno (two underscores) to analyzer/kf.cc's 
> register_known_functions in an analogous way to that commit?  (i.e.
> wiring it up to kf_errno_location, "teaching" the analyzer that that
> function returns a pointer to the "errno region")

But...there's already "support" for two underscores since
the commit you quote, so I guess not.  I strongly believe
"the critical difference is that __attribute__
((__const__))" because I indeed proved it by adding it to
the newlib definition.

I doubt these tests actually *pass* for OS X, because that
one looks identical to the newlib definition as quoted in
that commit (compare to the newlib one I pasted in the quote
above).

It looks like that definition was added for good measure
along with the Solaris definition (that has the
attribute-const) but never tested.  Sorry, I don't have an
OS X to test it myself and according to a popular search
engine(...) nobody has posted gcc-testresults@ for anything
"apple" since gcc-4.7 era. :(

brgds, H-P

> 
> Dave
> 
> > 
> > The critical difference is that __attribute__ ((__const__)),
> > where glibc says that the caller will see the same value on
> > all calls (from the same context; read: same thread).  I'm
> > not sure the absence of __attribute__ ((__const__)) for the
> > newlib definition is deliberate, but I guess it can.
> > Either way, without the "const" attribute, it can't be known
> > that the same location will be returned the next time, so
> > analyzer-tests that depend the value being known it should
> > see UNKNOWN rather than TRUE, that's why the deliberate
> > check for UNKNOWN rather than xfailing the test.
> > 
> > For isatty-1.c, it's the same problem, but here it'd be
> > unweildy with the extra dg-lines, so better just skip it for
> > newlib targets.
> > 
> > testsuite:
> >         * gcc.dg/analyzer/call-summaries-errno.c: Expect UNKNOWN
> >         for newlib after having set errno.
> >         * gcc.dg/analyzer/errno-1.c: Ditto.
> >         * gcc.dg/analyzer/isatty-1.c: Skip for newlib targets.
> > ---
> >  gcc/testsuite/gcc.dg/analyzer/call-summaries-errno.c | 3 ++-
> >  gcc/testsuite/gcc.dg/analyzer/errno-1.c              | 3 ++-
> >  gcc/testsuite/gcc.dg/analyzer/isatty-1.c             | 2 +-
> >  3 files changed, 5 insertions(+), 3 deletions(-)
> > 
> > diff --git a/gcc/testsuite/gcc.dg/analyzer/call-summaries-errno.c
> > b/gcc/testsuite/gcc.dg/analyzer/call-summaries-errno.c
> > index e4333b30bb77..cf4d9f7141e4 100644
> > --- a/gcc/testsuite/gcc.dg/analyzer/call-summaries-errno.c
> > +++ b/gcc/testsuite/gcc.dg/analyzer/call-summaries-errno.c
> > @@ -13,5 +13,6 @@ void test_sets_errno (int y)
> >    sets_errno (y);
> >    sets_errno (y);
> >  
> > -  __analyzer_eval (errno == y); /* { dg-warning "TRUE" } */  
> > +  __analyzer_eval (errno == y); /* { dg-warning "TRUE" "errno is at
> > a constant location" { target { ! newlib } } } */
> > +  /* { dg-warning "UNKNOWN" "errno is not known to be at a constant
> > location" { target { newlib } } .-1 } */
> >  }
> > diff --git a/gcc/testsuite/gcc.dg/analyzer/errno-1.c
> > b/gcc/testsuite/gcc.dg/analyzer/errno-1.c
> > index 6b9d28c10799..af0cc3d52a36 100644
> > --- a/gcc/testsuite/gcc.dg/analyzer/errno-1.c
> > +++ b/gcc/testsuite/gcc.dg/analyzer/errno-1.c
> > @@ -17,7 +17,8 @@ void test_storing_to_errno (int val)
> >  {
> >    __analyzer_eval (errno == val); /* { dg-warning "UNKNOWN" } */
> >    errno = val;
> > -  __analyzer_eval (errno == val); /* { dg-warning "TRUE" } */
> > +  __analyzer_eval (errno == val); /* { dg-warning "TRUE" "errno is
> > at a constant location" { target { ! newlib } } } */
> > +  /* { dg-warning "UNKNOWN" "errno is not known to be at a constant
> > location" { target { newlib } } .-1 } */
> >    external_fn ();
> >    __analyzer_eval (errno == val); /* { dg-warning "UNKNOWN" } */  
> >  }
> > diff --git a/gcc/testsuite/gcc.dg/analyzer/isatty-1.c
> > b/gcc/testsuite/gcc.dg/analyzer/isatty-1.c
> > index 389d2cdf3f18..450a7d71990d 100644
> > --- a/gcc/testsuite/gcc.dg/analyzer/isatty-1.c
> > +++ b/gcc/testsuite/gcc.dg/analyzer/isatty-1.c
> > @@ -1,4 +1,4 @@
> > -/* { dg-skip-if "" { powerpc*-*-aix* } } */
> > +/* { dg-skip-if "" { powerpc*-*-aix* || newlib } } */
> >  
> >  #include <errno.h>
> >  #include "analyzer-decls.h"
>
  

On Wed, 2023-03-01 at 01:59 +0100, Hans-Peter Nilsson wrote:
> > From: David Malcolm <dmalcolm@redhat.com>
> > Date: Tue, 28 Feb 2023 14:12:47 -0500
> 
> > On Tue, 2023-02-28 at 19:47 +0100, Hans-Peter Nilsson wrote:
> > > Ok to commit?
> > > -- >8 --
> > > Investigating analyzer tesstsuite errors for cris-elf.  The same
> > > are
> > > seen for pru-elf according to posts to gcc-testresults@.
> > > 
> > > For glibc, errno is #defined as:
> > >  extern int *__errno_location (void) __THROW __attribute_const__;
> > >  # define errno (*__errno_location ())
> > > while for newlib in its default configuration, it's:
> > >  #define errno (*__errno())
> > >  extern int *__errno (void);
> > 
> > We're already handling ___errno (three underscores) for Solaris as
> > of
> > 7c9717fcb5cf94ce1e7ef5c903058adf9980ff28; does it fix the issue if
> > you 
> > add __errno (two underscores) to analyzer/kf.cc's 
> > register_known_functions in an analogous way to that commit?  (i.e.
> > wiring it up to kf_errno_location, "teaching" the analyzer that
> > that
> > function returns a pointer to the "errno region")
> 
> But...there's already "support" for two underscores since
> the commit you quote, so I guess not.  

Is there?  That commit adds support for:
  __error
but not for:
  __errno
unless there's something I'm missing.

> I strongly believe
> "the critical difference is that __attribute__
> ((__const__))" because I indeed proved it by adding it to
> the newlib definition.
> 
> I doubt these tests actually *pass* for OS X, because that
> one looks identical to the newlib definition as quoted in
> that commit (compare to the newlib one I pasted in the quote
> above).
> 
> It looks like that definition was added for good measure
> along with the Solaris definition (that has the
> attribute-const) but never tested.  Sorry, I don't have an
> OS X to test it myself and according to a popular search
> engine(...) nobody has posted gcc-testresults@ for anything
> "apple" since gcc-4.7 era. :(

The comments by Rainer in
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=107807
suggests that it did fix stuff on Solaris and OS X.

Sorry if I'm missing something here, I'm not very familiar with newlib.

Dave

> 
> brgds, H-P
> 
> > 
> > Dave
> > 
> > > 
> > > The critical difference is that __attribute__ ((__const__)),
> > > where glibc says that the caller will see the same value on
> > > all calls (from the same context; read: same thread).  I'm
> > > not sure the absence of __attribute__ ((__const__)) for the
> > > newlib definition is deliberate, but I guess it can.
> > > Either way, without the "const" attribute, it can't be known
> > > that the same location will be returned the next time, so
> > > analyzer-tests that depend the value being known it should
> > > see UNKNOWN rather than TRUE, that's why the deliberate
> > > check for UNKNOWN rather than xfailing the test.
> > > 
> > > For isatty-1.c, it's the same problem, but here it'd be
> > > unweildy with the extra dg-lines, so better just skip it for
> > > newlib targets.
> > > 
> > > testsuite:
> > >         * gcc.dg/analyzer/call-summaries-errno.c: Expect UNKNOWN
> > >         for newlib after having set errno.
> > >         * gcc.dg/analyzer/errno-1.c: Ditto.
> > >         * gcc.dg/analyzer/isatty-1.c: Skip for newlib targets.
> > > ---
> > >  gcc/testsuite/gcc.dg/analyzer/call-summaries-errno.c | 3 ++-
> > >  gcc/testsuite/gcc.dg/analyzer/errno-1.c              | 3 ++-
> > >  gcc/testsuite/gcc.dg/analyzer/isatty-1.c             | 2 +-
> > >  3 files changed, 5 insertions(+), 3 deletions(-)
> > > 
> > > diff --git a/gcc/testsuite/gcc.dg/analyzer/call-summaries-errno.c
> > > b/gcc/testsuite/gcc.dg/analyzer/call-summaries-errno.c
> > > index e4333b30bb77..cf4d9f7141e4 100644
> > > --- a/gcc/testsuite/gcc.dg/analyzer/call-summaries-errno.c
> > > +++ b/gcc/testsuite/gcc.dg/analyzer/call-summaries-errno.c
> > > @@ -13,5 +13,6 @@ void test_sets_errno (int y)
> > >    sets_errno (y);
> > >    sets_errno (y);
> > >  
> > > -  __analyzer_eval (errno == y); /* { dg-warning "TRUE" } */  
> > > +  __analyzer_eval (errno == y); /* { dg-warning "TRUE" "errno is
> > > at
> > > a constant location" { target { ! newlib } } } */
> > > +  /* { dg-warning "UNKNOWN" "errno is not known to be at a
> > > constant
> > > location" { target { newlib } } .-1 } */
> > >  }
> > > diff --git a/gcc/testsuite/gcc.dg/analyzer/errno-1.c
> > > b/gcc/testsuite/gcc.dg/analyzer/errno-1.c
> > > index 6b9d28c10799..af0cc3d52a36 100644
> > > --- a/gcc/testsuite/gcc.dg/analyzer/errno-1.c
> > > +++ b/gcc/testsuite/gcc.dg/analyzer/errno-1.c
> > > @@ -17,7 +17,8 @@ void test_storing_to_errno (int val)
> > >  {
> > >    __analyzer_eval (errno == val); /* { dg-warning "UNKNOWN" } */
> > >    errno = val;
> > > -  __analyzer_eval (errno == val); /* { dg-warning "TRUE" } */
> > > +  __analyzer_eval (errno == val); /* { dg-warning "TRUE" "errno
> > > is
> > > at a constant location" { target { ! newlib } } } */
> > > +  /* { dg-warning "UNKNOWN" "errno is not known to be at a
> > > constant
> > > location" { target { newlib } } .-1 } */
> > >    external_fn ();
> > >    __analyzer_eval (errno == val); /* { dg-warning "UNKNOWN" }
> > > */  
> > >  }
> > > diff --git a/gcc/testsuite/gcc.dg/analyzer/isatty-1.c
> > > b/gcc/testsuite/gcc.dg/analyzer/isatty-1.c
> > > index 389d2cdf3f18..450a7d71990d 100644
> > > --- a/gcc/testsuite/gcc.dg/analyzer/isatty-1.c
> > > +++ b/gcc/testsuite/gcc.dg/analyzer/isatty-1.c
> > > @@ -1,4 +1,4 @@
> > > -/* { dg-skip-if "" { powerpc*-*-aix* } } */
> > > +/* { dg-skip-if "" { powerpc*-*-aix* || newlib } } */
> > >  
> > >  #include <errno.h>
> > >  #include "analyzer-decls.h"
> > 
>
  

> From: David Malcolm <dmalcolm@redhat.com>
> Date: Tue, 28 Feb 2023 21:25:34 -0500

> On Wed, 2023-03-01 at 01:59 +0100, Hans-Peter Nilsson wrote:
> > > From: David Malcolm <dmalcolm@redhat.com>
> > > Date: Tue, 28 Feb 2023 14:12:47 -0500
> > 
> > > On Tue, 2023-02-28 at 19:47 +0100, Hans-Peter Nilsson wrote:
> > > > Ok to commit?
> > > > -- >8 --
> > > > Investigating analyzer tesstsuite errors for cris-elf.  The same
> > > > are
> > > > seen for pru-elf according to posts to gcc-testresults@.
> > > > 
> > > > For glibc, errno is #defined as:
> > > >  extern int *__errno_location (void) __THROW __attribute_const__;
> > > >  # define errno (*__errno_location ())
> > > > while for newlib in its default configuration, it's:
> > > >  #define errno (*__errno())
> > > >  extern int *__errno (void);
> > > 
> > > We're already handling ___errno (three underscores) for Solaris as
> > > of
> > > 7c9717fcb5cf94ce1e7ef5c903058adf9980ff28; does it fix the issue if
> > > you 
> > > add __errno (two underscores) to analyzer/kf.cc's 
> > > register_known_functions in an analogous way to that commit?  (i.e.
> > > wiring it up to kf_errno_location, "teaching" the analyzer that
> > > that
> > > function returns a pointer to the "errno region")
> > 
> > But...there's already "support" for two underscores since
> > the commit you quote, so I guess not.  
> 
> Is there?  That commit adds support for:
>   __error
> but not for:
>   __errno
> unless there's something I'm missing.

No, it's me.  Apparently I can't spot the difference between
error and errno.  Sorry.

It's nice to know that the const-attribute (to signal that
the pointer points to a constant location) works
automatically, i.e. the Solaris and glibc definitions should
not (no longer?) be needed - unless there's other
errno-analyzing magic elsewhere too.  Either way, since
there's specific support for this errno kind of thing, that
certainly works for me.  The patch gets smaller too. :)

So, how's this instead, pending full testing (only
analyzer.exp spot-tested)?

-- >8 --
analyzer: Support errno for newlib

Without this definition, the errno definition for newlib
isn't recognized as such, and these tests fail for newlib
targets:

FAIL: gcc.dg/analyzer/call-summaries-errno.c  (test for warnings, line 16)
FAIL: gcc.dg/analyzer/call-summaries-errno.c (test for excess errors)
FAIL: gcc.dg/analyzer/errno-1.c  (test for warnings, line 20)
FAIL: gcc.dg/analyzer/errno-1.c (test for excess errors)
FAIL: gcc.dg/analyzer/isatty-1.c  (test for warnings, line 31)
FAIL: gcc.dg/analyzer/isatty-1.c  (test for warnings, line 35)
FAIL: gcc.dg/analyzer/isatty-1.c  (test for warnings, line 46)
FAIL: gcc.dg/analyzer/isatty-1.c  (test for warnings, line 56)
FAIL: gcc.dg/analyzer/isatty-1.c (test for excess errors)

gcc/analyzer:
	* kf.cc (register_known_functions): Add __errno function for newlib.
---
 gcc/analyzer/kf.cc | 1 +
 1 file changed, 1 insertion(+)

diff --git a/gcc/analyzer/kf.cc b/gcc/analyzer/kf.cc
index 3a91b6bd6ebb..bfb9148f9ae7 100644
--- a/gcc/analyzer/kf.cc
+++ b/gcc/analyzer/kf.cc
@@ -1036,6 +1036,7 @@ register_known_functions (known_function_manager &kfm)
        Add these as synonyms for "__errno_location".  */
     kfm.add ("___errno", make_unique<kf_errno_location> ());
     kfm.add ("__error", make_unique<kf_errno_location> ());
+    kfm.add ("__errno", make_unique<kf_errno_location> ());
   }
 
   /* Language-specific support functions.  */
  

On Wed, 2023-03-01 at 04:01 +0100, Hans-Peter Nilsson wrote:
> > From: David Malcolm <dmalcolm@redhat.com>
> > Date: Tue, 28 Feb 2023 21:25:34 -0500
> 
> > On Wed, 2023-03-01 at 01:59 +0100, Hans-Peter Nilsson wrote:
> > > > From: David Malcolm <dmalcolm@redhat.com>
> > > > Date: Tue, 28 Feb 2023 14:12:47 -0500
> > > 
> > > > On Tue, 2023-02-28 at 19:47 +0100, Hans-Peter Nilsson wrote:
> > > > > Ok to commit?
> > > > > -- >8 --
> > > > > Investigating analyzer tesstsuite errors for cris-elf.  The
> > > > > same
> > > > > are
> > > > > seen for pru-elf according to posts to gcc-testresults@.
> > > > > 
> > > > > For glibc, errno is #defined as:
> > > > >  extern int *__errno_location (void) __THROW
> > > > > __attribute_const__;
> > > > >  # define errno (*__errno_location ())
> > > > > while for newlib in its default configuration, it's:
> > > > >  #define errno (*__errno())
> > > > >  extern int *__errno (void);
> > > > 
> > > > We're already handling ___errno (three underscores) for Solaris
> > > > as
> > > > of
> > > > 7c9717fcb5cf94ce1e7ef5c903058adf9980ff28; does it fix the issue
> > > > if
> > > > you 
> > > > add __errno (two underscores) to analyzer/kf.cc's 
> > > > register_known_functions in an analogous way to that commit? 
> > > > (i.e.
> > > > wiring it up to kf_errno_location, "teaching" the analyzer that
> > > > that
> > > > function returns a pointer to the "errno region")
> > > 
> > > But...there's already "support" for two underscores since
> > > the commit you quote, so I guess not.  
> > 
> > Is there?  That commit adds support for:
> >   __error
> > but not for:
> >   __errno
> > unless there's something I'm missing.
> 
> No, it's me.  Apparently I can't spot the difference between
> error and errno.  Sorry.
> 
> It's nice to know that the const-attribute (to signal that
> the pointer points to a constant location) works
> automatically, i.e. the Solaris and glibc definitions should
> not (no longer?) be needed - unless there's other
> errno-analyzing magic elsewhere too.  

With the const-attribute, the analyzer "knows" that the pointer it gets
back always points to the same thing, but it doesn't know what it
points to, and so has to assume that any writes to errno could modify
anything that has escaped.

Also, the analyzer uses region_model::set_errno in various
known_function implementations, e.g. for functions that can succeed or
fail, to set errno on the "failure" execution path to a new symbolic
value that tests as > 0.

> Either way, since
> there's specific support for this errno kind of thing, that
> certainly works for me.  The patch gets smaller too. :)

:)

> 
> So, how's this instead, pending full testing (only
> analyzer.exp spot-tested)?

Looks good, though how about adding a mention of newlib to the comment
immediately above (the one that talks about Solaris and OS X)?

Thanks for fixing this
Dave
  

> From: David Malcolm <dmalcolm@redhat.com>
> Cc: gcc-patches@gcc.gnu.org
> Date: Wed, 01 Mar 2023 08:32:13 -0500

> Also, the analyzer uses region_model::set_errno in various
> known_function implementations, e.g. for functions that can succeed or
> fail, to set errno on the "failure" execution path to a new symbolic
> value that tests as > 0.

Ha, there's indeed errno-analyzing magic. :)

(That's the level you get when your first impression of
source code is by tracing through failing test-cases...)

> > So, how's this instead, pending full testing (only
> > analyzer.exp spot-tested)?
> 
> Looks good, though how about adding a mention of newlib to the comment
> immediately above (the one that talks about Solaris and OS X)?

I deliberately didn't, as the format of that comment seemed
to ask for redundantly repeating the definition for each
system (telling exactly how the #define looks etc.), and I
thought the heading "Other implementations of C standard
library" should have been sufficient, assuming access to the
git log when there's interest in more than the first lines.

But I hear you so this is what I intend to commit later
today, just keeping the added comment as brief as
reasonable:

"analyzer: Support errno for newlib

Without this definition, the errno definition for newlib
isn't recognized as such, and these tests fail for newlib
targets:

FAIL: gcc.dg/analyzer/call-summaries-errno.c  (test for warnings, line 16)
FAIL: gcc.dg/analyzer/call-summaries-errno.c (test for excess errors)
FAIL: gcc.dg/analyzer/errno-1.c  (test for warnings, line 20)
FAIL: gcc.dg/analyzer/errno-1.c (test for excess errors)
FAIL: gcc.dg/analyzer/isatty-1.c  (test for warnings, line 31)
FAIL: gcc.dg/analyzer/isatty-1.c  (test for warnings, line 35)
FAIL: gcc.dg/analyzer/isatty-1.c  (test for warnings, line 46)
FAIL: gcc.dg/analyzer/isatty-1.c  (test for warnings, line 56)
FAIL: gcc.dg/analyzer/isatty-1.c (test for excess errors)

gcc/analyzer:
	* kf.cc (register_known_functions): Add __errno function for newlib.
---
 gcc/analyzer/kf.cc | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/gcc/analyzer/kf.cc b/gcc/analyzer/kf.cc
index 3a91b6bd6ebb..ed5f70398e1d 100644
--- a/gcc/analyzer/kf.cc
+++ b/gcc/analyzer/kf.cc
@@ -1033,9 +1033,11 @@ register_known_functions (known_function_manager &kfm)
        and OS X like this:
 	 extern int * __error(void);
 	 #define errno (*__error())
+       and similarly __errno for newlib.
        Add these as synonyms for "__errno_location".  */
     kfm.add ("___errno", make_unique<kf_errno_location> ());
     kfm.add ("__error", make_unique<kf_errno_location> ());
+    kfm.add ("__errno", make_unique<kf_errno_location> ());
   }
 
   /* Language-specific support functions.  */
  

> From: Hans-Peter Nilsson <hp@axis.com>
> Date: Wed, 1 Mar 2023 16:36:46 +0100

> ... this is what I intend to commit later
> today, just keeping the added comment as brief as
> reasonable:

Except I see the hook for errno magic took care of
gcc.dg/analyzer/flex-without-call-summaries.c so I'll add
that to the list of handled "FAIL"s in the commit log.  Yay.

brgds, H-P
  

On Wed, 1 Mar 2023 17:02:31 +0100
Hans-Peter Nilsson via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:

> > From: Hans-Peter Nilsson <hp@axis.com>
> > Date: Wed, 1 Mar 2023 16:36:46 +0100  
> 
> > ... this is what I intend to commit later
> > today, just keeping the added comment as brief as
> > reasonable:  
> 
> Except I see the hook for errno magic took care of
> gcc.dg/analyzer/flex-without-call-summaries.c so I'll add
> that to the list of handled "FAIL"s in the commit log.  Yay.

But in the end it means we'll have to keep _[_]+errno{,_location} 'til
we bump requirements or 10, 20 years or the end of the universe,
doesn't it.
Way fancy.
  

> Date: Thu, 2 Mar 2023 00:23:36 +0100
> From: Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>

> On Wed, 1 Mar 2023 17:02:31 +0100
> Hans-Peter Nilsson via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> 
> > > From: Hans-Peter Nilsson <hp@axis.com>
> > > Date: Wed, 1 Mar 2023 16:36:46 +0100  
> > 
> > > ... this is what I intend to commit later
> > > today, just keeping the added comment as brief as
> > > reasonable:  
> > 
> > Except I see the hook for errno magic took care of
> > gcc.dg/analyzer/flex-without-call-summaries.c so I'll add
> > that to the list of handled "FAIL"s in the commit log.  Yay.
> 
> But in the end it means we'll have to keep _[_]+errno{,_location} 'til
> we bump requirements or 10, 20 years or the end of the universe,
> doesn't it.
> Way fancy.

Not sure I see your point?  The (other) identifiers are already there.

(And you do realize that this is in the analyzer part of gcc, right?)

brgds, H-P
  

On Thu, 2 Mar 2023 00:54:33 +0100
Hans-Peter Nilsson <hp@axis.com> wrote:

> > Date: Thu, 2 Mar 2023 00:23:36 +0100
> > From: Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>  
> 
> > On Wed, 1 Mar 2023 17:02:31 +0100
> > Hans-Peter Nilsson via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> >   
> > > > From: Hans-Peter Nilsson <hp@axis.com>
> > > > Date: Wed, 1 Mar 2023 16:36:46 +0100    
> > >   
> > > > ... this is what I intend to commit later
> > > > today, just keeping the added comment as brief as
> > > > reasonable:    
> > > 
> > > Except I see the hook for errno magic took care of
> > > gcc.dg/analyzer/flex-without-call-summaries.c so I'll add
> > > that to the list of handled "FAIL"s in the commit log.  Yay.  
> > 
> > But in the end it means we'll have to keep _[_]+errno{,_location} 'til
> > we bump requirements or 10, 20 years or the end of the universe,
> > doesn't it.
> > Way fancy.  
> 
> Not sure I see your point?  The (other) identifiers are already there.

I'm certainly not opposed to this partiular identifier, no.

> 
> (And you do realize that this is in the analyzer part of gcc, right?)

And yes, i'm well aware this is "just" the analyzer -- which is unfair
to state like that and does not mean to imply any inferiority --
particular in this spot.

Just let's ditch any specialcased identifier which was superseded
reliably ASAP?

thanks,
  

> Date: Thu, 2 Mar 2023 01:37:12 +0100
> From: Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>

> On Thu, 2 Mar 2023 00:54:33 +0100
> Hans-Peter Nilsson <hp@axis.com> wrote:
> 
> > > Date: Thu, 2 Mar 2023 00:23:36 +0100
> > > From: Bernhard Reutner-Fischer <rep.dot.nop@gmail.com>  
> > 
> > > On Wed, 1 Mar 2023 17:02:31 +0100
> > > Hans-Peter Nilsson via Gcc-patches <gcc-patches@gcc.gnu.org> wrote:
> > >   
> > > > > From: Hans-Peter Nilsson <hp@axis.com>
> > > > > Date: Wed, 1 Mar 2023 16:36:46 +0100    
> > > >   
> > > > > ... this is what I intend to commit later
> > > > > today, just keeping the added comment as brief as
> > > > > reasonable:    
> > > > 
> > > > Except I see the hook for errno magic took care of
> > > > gcc.dg/analyzer/flex-without-call-summaries.c so I'll add
> > > > that to the list of handled "FAIL"s in the commit log.  Yay.  
> > > 
> > > But in the end it means we'll have to keep _[_]+errno{,_location} 'til
> > > we bump requirements or 10, 20 years or the end of the universe,
> > > doesn't it.
> > > Way fancy.  
> > 
> > Not sure I see your point?  The (other) identifiers are already there.
> 
> I'm certainly not opposed to this partiular identifier, no.
> 
> > 
> > (And you do realize that this is in the analyzer part of gcc, right?)
> 
> And yes, i'm well aware this is "just" the analyzer -- which is unfair
> to state like that and does not mean to imply any inferiority --
> particular in this spot.

(Your statement and values; it can be read as you putting
that as mine, which I hope was not intended.)  My point is
that the presence of those identifiers does not affects an
ABI or code-generating parts of gcc.  All the identifiers
are present for all targets - for all invocation of the
analyzer.  If that passes a nuisance threshold, it seems it
can be changed easily, say by moving it to a target-hook by
someone who cares deeply enough.

> Just let's ditch any specialcased identifier which was superseded
> reliably ASAP?

I'm certainly not opposed to *that*
brgds, H-P