Re: [PATCH] Support updating DWARF5 .debug_loclists.

[Mark Wielaard <mark@klomp.org>]

Hi Jakub,

On Tue, 2020-09-29 at 10:42 +0200, Jakub Jelinek wrote:
> On Mon, Sep 28, 2020 at 10:07:59AM +0200, Mark Wielaard wrote:
> >    ptr += offset;
> >    while (ptr < endsec)
> >      {
> > -      low = read_size (ptr, ptr_size);
> > -      high = read_size (ptr + ptr_size, ptr_size);
> > -      ptr += 2 * ptr_size;
> > -      if (low == 0 && high == 0)
> > -	break;
> > +      if (cu->cu_version < 5)
> 
> I wonder if it wouldn't be clearer to use sec == DEBUG_LOC
> in this spot (i.e. leave the decision based on cu_version just to the
> start of the function).

Yes, it would be clearer. Changed.

> > +	    case DW_LLE_start_length:
> > +	      ptr += ptr_size;
> > +	      skip_leb128 (ptr);
> > +	      len = read_uleb128 (ptr);
> > +	      break;
> 
> Wonder if we shouldn't handle case DW_LLE_GNU_view_part here, but perhaps
> only for cu->cu_version == 5 because it isn't in a separate vendor code
> space and thus in DWARF 6 it could mean something different.

Yes, it would. gcc doesn't emit it by default at the moment. But
handling it is easy:

            case DW_LLE_GNU_view_pair:                                          
              if (cu->cu_version != 5)                                          
                error (0, 0,                                                    
                       "%s: DW_LLE_GNU_view_pair used with DWARF version %u\n", 
                       dso->filename, cu->cu_version);                          
              skip_leb128 (ptr);                                                
              skip_leb128 (ptr);                                                
              continue;

It will warn, but still try to handle it as is.
We probably never hit
this because dwz doesn't actually accept
DWARFv6 CUs, because they don't
exist yet :)

> Otherwise LGTM.

Thanks. Pushed with those changes. Tested against a
gcc -gvariable-location-views=incompat5 build.

BTW. I fixed binutils readelf so that it now also correctly shows
.debug_loclists:
https://sourceware.org/pipermail/binutils/2020-September/113510.html

Cheers,

Mark