Re: [PATCH] Don't handle blocks as exprlocs for DWARF version 4 or higher.

[Mark Wielaard <mark@klomp.org>]

[-- Attachment #1: Type: text/plain, Size: 2385 bytes --]

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

[-- Attachment #2: 0001-Don-t-handle-blocks-as-exprlocs-for-DWARF-version-4-.patch --]
[-- Type: text/x-patch, Size: 3951 bytes --]

From f5803d41dbddd59e2e0267806826dd3503dd2abd Mon Sep 17 00:00:00 2001
From: Mark Wielaard <mark@klomp.org>
Date: Sat, 13 Feb 2021 23:34:55 +0100
Subject: [PATCH] Don't handle blocks as exprlocs for DWARF version 4 or
 higher.

Since DWARF version 4 blocks just contain bytes, trying to interpret
them as exprlocs will most likely fail. Also make sure block1 form
and len are correctly passed to add_locexpr_dummy_dies.

     * dwz.c (add_locexpr_dummy_dies): Only handle block as exprloc
     for cu_version < 4.
     (checksum_die): Likewise.
     (write_die): Likewise.
     (read_debug_info): Get block length before calling
     add_locexpr_dummy_dies.

https://sourceware.org/bugzilla/show_bug.cgi?id=26987
---
 dwz.c | 50 ++++++++++++++++++++++++++++++++++----------------
 1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/dwz.c b/dwz.c
index d6b9df0..5e7003a 100644
--- a/dwz.c
+++ b/dwz.c
@@ -2910,7 +2910,7 @@ add_locexpr_dummy_dies (DSO *dso, dw_cu_ref cu, dw_die_ref die,
 			unsigned char *ptr, uint32_t form, unsigned int attr,
 			size_t len)
 {
-  if (form == DW_FORM_block1)
+  if (form == DW_FORM_block1 && cu->cu_version < 4)
     {
       /* Old DWARF uses blocks instead of exprlocs.  */
       switch (attr)
@@ -3733,7 +3733,7 @@ checksum_die (DSO *dso, dw_cu_ref cu, dw_die_ref top_die, dw_die_ref die)
 	  abort ();
 	}
 
-      if (form == DW_FORM_block1)
+      if (form == DW_FORM_block1 && cu->cu_version < 4)
 	{
 	  /* Old DWARF uses blocks instead of exprlocs.  */
 	  switch (t->attr[i].attr)
@@ -3780,7 +3780,6 @@ checksum_die (DSO *dso, dw_cu_ref cu, dw_die_ref top_die, dw_die_ref die)
 	    default:
 	      break;
 	    }
-	  ptr += len;
 	}
       else if (form == DW_FORM_exprloc)
 	{
@@ -3793,8 +3792,8 @@ checksum_die (DSO *dso, dw_cu_ref cu, dw_die_ref top_die, dw_die_ref die)
 	  if (read_exprloc (dso, die, ptr, len, NULL))
 	    return 1;
 	  handled = true;
-	  ptr += len;
 	}
+      ptr += len; /* Skip expr/blocks.  */
       if (!handled && die->die_ck_state != CK_BAD)
 	{
 	  s = t->attr[i].attr;
@@ -6875,6 +6874,30 @@ read_debug_info (DSO *dso, int kind, unsigned int *die_count)
 		    }
 		}
 
+	      /* Get length of expr/blocks first.  Canonicalize all,
+		 except exprloc, to DW_FORM_block1.  */
+	      switch (form)
+		{
+		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;
+		}
+
 	      if (unlikely (low_mem_phase1)
 		  && add_locexpr_dummy_dies (dso, cu, die, ptr, form,
 					     t->attr[i].attr, len))
@@ -6999,22 +7022,17 @@ read_debug_info (DSO *dso, int kind, unsigned int *die_count)
 		  break;
 		case DW_FORM_indirect:
 		  abort ();
+		/* All expr/blocks lengths already handled above.
+		   Just canonicalize exprloc to block1 too.  */
+		case DW_FORM_exprloc:
+		  form = DW_FORM_block1;
+		  break;
 		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:
-		case DW_FORM_exprloc:
-		  len = read_uleb128 (ptr);
-		  form = DW_FORM_block1;
-		  break;
+		  abort ();
 		default:
 		  error (0, 0, "%s: Unknown DWARF %s",
 			 dso->filename, get_DW_FORM_str (form));
@@ -12392,7 +12410,7 @@ write_die (unsigned char *ptr, dw_cu_ref cu, dw_die_ref die,
 	  ptr += inptr - orig_ptr;
 
 	  /* Old DWARF uses blocks instead of exprlocs.  */
-	  if (form == DW_FORM_block1)
+	  if (form == DW_FORM_block1 && cu->cu_version < 4)
 	    switch (reft->attr[i].attr)
 	      {
 	      case DW_AT_frame_base:
-- 
2.18.4