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

[David Malcolm <dmalcolm@redhat.com>]

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