Re: [PATCH] Handle DW_AT_decl_file 0

[Aaron Merey <amerey@redhat.com>]

Hi Mark,

On Mon, Feb 12, 2024 at 12:31 PM Mark Wielaard <mark@klomp.org> wrote:
> >        (void) INTUSE(dwarf_getsrclines) (&CUDIE (cu), &lines, &nlines);
> > -      assert (cu->lines != NULL);
> >      }
>
> I see why would like to get rid of asserts in the code base.
> But I believe the assert is valid. dwarf_getsrclines will check whether
> cu->lines is NULL, in which case it tries to load the line table. It
> then sets cu->lines to the newly parsed line table, or to -1 to
> indicate there was an error parsing (or no) line table.
> >
> > -  if (cu->lines == (void *) -1l)
> > +  if (cu->lines == NULL || cu->lines == (void *) -1l)
> >      {
> > -      /* If the file index is not zero, there must be file information
> > -      available.  */
> > -      __libdw_seterrno (DWARF_E_INVALID_DWARF);
> > +      /* Line table could not be found.  */
> >        return NULL;
> >      }
>
> Which means this is a change in behavior. Now if there was no line
> table, or a problem parsing it, then no error is set, but NULL is
> returned anyway. Which means using dwarf_errno or dwarf_errmsg after
> dwarf_decl_file returns NULL doesn't work reliably anymore. Are you
> sure libdw errno shouldn't be set to DWARF_E_INVALID_DWARF?

My thinking was to rely on dwarf_getsrclines setting the libdw errno
if an error occurred.  If we always use DWARF_E_INVALID_DWARF then we
might overwrite an error code that describes the failure more specifically.

If we want to ensure that the libdw errno is set whenever we reach this
condition, we could check if dwarf_getsrclines set the errno. If it did,
then just leave that errno set. If it didn't, then set errno to
DWARF_E_INVALID_DWARF.

>> > -  assert (cu->files != NULL && cu->files != (void *) -1l);
> > + if (cu->files == NULL || cu->files == (void *) -1l)
> > +    {
> > +      /* If the line table was found then then the file table should
> > +      have also been found.  */
> > +      __libdw_seterrno (DWARF_E_UNKNOWN_ERROR);
> > +      return NULL;
> > +    }
> >
>
> I think if you are going to replace the assert here, then it should
> (also) be DWARF_E_INVALID_DWARF. It means a decl_file was given, but
> there is no file table. Which IMHO is invalid. Just like in the case
> below:
>
> >
> >    if (idx >= cu->files->nfiles)
> >      {
>
> Here we also set DWARF_E_INVALID_DWARF because the decl_file number is
> larger than the number of files in the file table.

Ok, fixed.

> > --- a/tests/run-allfcts.sh
> > +++ b/tests/run-allfcts.sh
> > @@ -170,4 +170,21 @@ testrun_compare ${abs_builddir}/allfcts testfile-lto-gcc9 <<\EOF
> >  /home/mark/src/tests/testfile-lto-main.c:6:main
> >  EOF
> >
> > +# = dwarf5-line.c =
> > +# int
> > +# main (int argc, char ** argv)
> > +# {
> > +#   return 0;
> > +# }
> > +
> > +# Using clang version 17.0.4 (Fedora 17.0.4-1.fc39)
> > +# clang -gdwarf-5 -O0 -o testfile-dwarf5-line-clang dwarf5-line.c
>
> Does it need to be -O0? Not that I really object. Just hoping to get a
> slightly smaller binary/testfile with -O1 or -O2.

It doesn't need to be -O0.  I'll just leave out -O altogether.

Aaron