Hi,

On Thu, 2021-02-18 at 17:59 +0100, Jakub Jelinek wrote:
> On Thu, Feb 18, 2021 at 05:18:10PM +0100, Mark Wielaard wrote:
> > > > >        if (form == DW_FORM_block1)
> > > 
> > > And likewise here:
> > > -      if (form == DW_FORM_block1)
> > > +      if (form == DW_FORM_block1 && cu->cu_version < 4)
> > 
> > But here we do need to handle the DW_FORM_block && cu->cu_version
> > >=4
> > version separately. But that can be done by not indention the large
> > block and adding an small else if block.
> 
> Ah, I got confused by DW_FORM_block{2,4,} cases changing form to
> DW_FORM_block1, indeed, for all of DW_FORM_{block{1,2,4,},exprloc} we
> need
> to do ptr += len;
> But perhaps we could do instead do:
> -      if (form == DW_FORM_block1)
> +      if (form == DW_FORM_block1 && cu->cu_version < 4)
> ...
> -	  ptr += len;
> ...
> -	  ptr += len;
>  	}
> +      ptr += len;
> ?
> len is only set to non-0 for:
>         case DW_FORM_block1:
>           len = *ptr++;
>           break;
>         case DW_FORM_block2:
>           len = read_16 (ptr);
>           form = DW_FORM_block1;
>           break;
>         case DW_FORM_block4:
>           len = read_32 (ptr);
>           form = DW_FORM_block1;
>           break;
>         case DW_FORM_block:
>           len = read_uleb128 (ptr);
>           form = DW_FORM_block1;
>           break;
>         case DW_FORM_exprloc:
>           len = read_uleb128 (ptr);
>           break;
> i.e. exactly the cases we want to move.

OK. That would make the code even simpler. You are right, we set len to
zero at the start and only set it for the block/exprlocs, so we can
unconditionally do a ptr += len at the end.

> Anyway, looking around some more,
>               if (unlikely (low_mem_phase1)
>                   && add_locexpr_dummy_dies (dso, cu, die, ptr, form,
>                                              t->attr[i].attr, len))
>                   goto fail;
> looks incorrect to me, form in that case will be DW_FORM_block{2,4,}
> and won't be canonicalized to DW_FORM_block1.  And furthermore
> len will be always 0.  It is preceded only by
>               size_t len = 0;
> and a loop handling DW_FORM_indirect.  So, ptr will always be
> the pointer to the block count too.

I had missed that. Also fixed in the attached patch.

Cheers,

Mark