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

public inbox for dwz@sourceware.org
 help / color / mirror / Atom feed

* [PATCH] Don't handle blocks as exprlocs for DWARF version 4 or higher.
@ 2021-02-13 22:46 Mark Wielaard
  2021-02-18 13:40 ` Mark Wielaard
  0 siblings, 1 reply; 9+ messages in thread
From: Mark Wielaard @ 2021-02-13 22:46 UTC (permalink / raw)
  To: dwz; +Cc: Mark Wielaard

Since DWARF version 4 blocks just contain bytes, trying to interpret
them as exprlocs will most likely fail.

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

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

diff --git a/dwz.c b/dwz.c
index d6b9df0..02fcd8c 100644
--- a/dwz.c
+++ b/dwz.c
@@ -2913,43 +2913,44 @@ add_locexpr_dummy_dies (DSO *dso, dw_cu_ref cu, dw_die_ref die,
   if (form == DW_FORM_block1)
     {
       /* Old DWARF uses blocks instead of exprlocs.  */
-      switch (attr)
-	{
-	case DW_AT_frame_base:
-	case DW_AT_location:
-	case DW_AT_data_member_location:
-	case DW_AT_vtable_elem_location:
-	case DW_AT_byte_size:
-	case DW_AT_bit_offset:
-	case DW_AT_bit_size:
-	case DW_AT_string_length:
-	case DW_AT_lower_bound:
-	case DW_AT_return_addr:
-	case DW_AT_bit_stride:
-	case DW_AT_upper_bound:
-	case DW_AT_count:
-	case DW_AT_segment:
-	case DW_AT_static_link:
-	case DW_AT_use_location:
-	case DW_AT_allocated:
-	case DW_AT_associated:
-	case DW_AT_data_location:
-	case DW_AT_byte_stride:
-	case DW_AT_rank:
-	case DW_AT_call_value:
-	case DW_AT_call_target:
-	case DW_AT_call_target_clobbered:
-	case DW_AT_call_data_location:
-	case DW_AT_call_data_value:
-	case DW_AT_GNU_call_site_value:
-	case DW_AT_GNU_call_site_data_value:
-	case DW_AT_GNU_call_site_target:
-	case DW_AT_GNU_call_site_target_clobbered:
-	  if (read_exprloc_low_mem_phase1 (dso, die, ptr, len))
-	    return 1;
-	default:
-	  break;
-	}
+      if (cu->cu_version < 4)
+	switch (attr)
+	  {
+	  case DW_AT_frame_base:
+	  case DW_AT_location:
+	  case DW_AT_data_member_location:
+	  case DW_AT_vtable_elem_location:
+	  case DW_AT_byte_size:
+	  case DW_AT_bit_offset:
+	  case DW_AT_bit_size:
+	  case DW_AT_string_length:
+	  case DW_AT_lower_bound:
+	  case DW_AT_return_addr:
+	  case DW_AT_bit_stride:
+	  case DW_AT_upper_bound:
+	  case DW_AT_count:
+	  case DW_AT_segment:
+	  case DW_AT_static_link:
+	  case DW_AT_use_location:
+	  case DW_AT_allocated:
+	  case DW_AT_associated:
+	  case DW_AT_data_location:
+	  case DW_AT_byte_stride:
+	  case DW_AT_rank:
+	  case DW_AT_call_value:
+	  case DW_AT_call_target:
+	  case DW_AT_call_target_clobbered:
+	  case DW_AT_call_data_location:
+	  case DW_AT_call_data_value:
+	  case DW_AT_GNU_call_site_value:
+	  case DW_AT_GNU_call_site_data_value:
+	  case DW_AT_GNU_call_site_target:
+	  case DW_AT_GNU_call_site_target_clobbered:
+	    if (read_exprloc_low_mem_phase1 (dso, die, ptr, len))
+	      return 1;
+	  default:
+	    break;
+	  }
 
       return 0;
     }
@@ -3736,50 +3737,51 @@ checksum_die (DSO *dso, dw_cu_ref cu, dw_die_ref top_die, dw_die_ref die)
       if (form == DW_FORM_block1)
 	{
 	  /* Old DWARF uses blocks instead of exprlocs.  */
-	  switch (t->attr[i].attr)
-	    {
-	    case DW_AT_frame_base:
-	    case DW_AT_location:
-	    case DW_AT_data_member_location:
-	    case DW_AT_vtable_elem_location:
-	    case DW_AT_byte_size:
-	    case DW_AT_bit_offset:
-	    case DW_AT_bit_size:
-	    case DW_AT_string_length:
-	    case DW_AT_lower_bound:
-	    case DW_AT_return_addr:
-	    case DW_AT_bit_stride:
-	    case DW_AT_upper_bound:
-	    case DW_AT_count:
-	    case DW_AT_segment:
-	    case DW_AT_static_link:
-	    case DW_AT_use_location:
-	    case DW_AT_allocated:
-	    case DW_AT_associated:
-	    case DW_AT_data_location:
-	    case DW_AT_byte_stride:
-	    case DW_AT_rank:
-	    case DW_AT_call_value:
-	    case DW_AT_call_target:
-	    case DW_AT_call_target_clobbered:
-	    case DW_AT_call_data_location:
-	    case DW_AT_call_data_value:
-	    case DW_AT_GNU_call_site_value:
-	    case DW_AT_GNU_call_site_data_value:
-	    case DW_AT_GNU_call_site_target:
-	    case DW_AT_GNU_call_site_target_clobbered:
-	      if (die->die_ck_state != CK_BAD)
-		{
-		  s = t->attr[i].attr;
-		  die->u.p1.die_hash
-		    = iterative_hash_object (s, die->u.p1.die_hash);
-		}
-	      if (read_exprloc (dso, die, ptr, len, NULL))
-		return 1;
-	      handled = true;
-	    default:
-	      break;
-	    }
+	  if (cu->cu_version < 4)
+	    switch (t->attr[i].attr)
+	      {
+	      case DW_AT_frame_base:
+	      case DW_AT_location:
+	      case DW_AT_data_member_location:
+	      case DW_AT_vtable_elem_location:
+	      case DW_AT_byte_size:
+	      case DW_AT_bit_offset:
+	      case DW_AT_bit_size:
+	      case DW_AT_string_length:
+	      case DW_AT_lower_bound:
+	      case DW_AT_return_addr:
+	      case DW_AT_bit_stride:
+	      case DW_AT_upper_bound:
+	      case DW_AT_count:
+	      case DW_AT_segment:
+	      case DW_AT_static_link:
+	      case DW_AT_use_location:
+	      case DW_AT_allocated:
+	      case DW_AT_associated:
+	      case DW_AT_data_location:
+	      case DW_AT_byte_stride:
+	      case DW_AT_rank:
+	      case DW_AT_call_value:
+	      case DW_AT_call_target:
+	      case DW_AT_call_target_clobbered:
+	      case DW_AT_call_data_location:
+	      case DW_AT_call_data_value:
+	      case DW_AT_GNU_call_site_value:
+	      case DW_AT_GNU_call_site_data_value:
+	      case DW_AT_GNU_call_site_target:
+	      case DW_AT_GNU_call_site_target_clobbered:
+		if (die->die_ck_state != CK_BAD)
+		  {
+		    s = t->attr[i].attr;
+		    die->u.p1.die_hash
+		      = iterative_hash_object (s, die->u.p1.die_hash);
+		  }
+		if (read_exprloc (dso, die, ptr, len, NULL))
+		  return 1;
+		handled = true;
+	      default:
+		break;
+	      }
 	  ptr += len;
 	}
       else if (form == DW_FORM_exprloc)
@@ -12392,7 +12394,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.20.1


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Don't handle blocks as exprlocs for DWARF version 4 or higher.
  2021-02-13 22:46 [PATCH] Don't handle blocks as exprlocs for DWARF version 4 or higher Mark Wielaard
@ 2021-02-18 13:40 ` Mark Wielaard
  2021-02-18 14:09   ` Jakub Jelinek
  0 siblings, 1 reply; 9+ messages in thread
From: Mark Wielaard @ 2021-02-18 13:40 UTC (permalink / raw)
  To: dwz

On Sat, 2021-02-13 at 23:46 +0100, Mark Wielaard wrote:
> Since DWARF version 4 blocks just contain bytes, trying to interpret
> them as exprlocs will most likely fail.
> 
>      * dwz.c (add_locexpr_dummy_dies): Only handle block as exprloc
>      for cu_version < 4.
>      (checksum_die): Likewise.
>      (write_die): Likewise.
> 
> https://sourceware.org/bugzilla/show_bug.cgi?id=26987

Ping. Any comments?

>  dwz.c | 166 +++++++++++++++++++++++++++++---------------------------
> --
>  1 file changed, 84 insertions(+), 82 deletions(-)
> 
> diff --git a/dwz.c b/dwz.c
> index d6b9df0..02fcd8c 100644
> --- a/dwz.c
> +++ b/dwz.c
> @@ -2913,43 +2913,44 @@ add_locexpr_dummy_dies (DSO *dso, dw_cu_ref
> cu, dw_die_ref die,
>    if (form == DW_FORM_block1)
>      {
>        /* Old DWARF uses blocks instead of exprlocs.  */
> -      switch (attr)
> -	{
> -	case DW_AT_frame_base:
> -	case DW_AT_location:
> -	case DW_AT_data_member_location:
> -	case DW_AT_vtable_elem_location:
> -	case DW_AT_byte_size:
> -	case DW_AT_bit_offset:
> -	case DW_AT_bit_size:
> -	case DW_AT_string_length:
> -	case DW_AT_lower_bound:
> -	case DW_AT_return_addr:
> -	case DW_AT_bit_stride:
> -	case DW_AT_upper_bound:
> -	case DW_AT_count:
> -	case DW_AT_segment:
> -	case DW_AT_static_link:
> -	case DW_AT_use_location:
> -	case DW_AT_allocated:
> -	case DW_AT_associated:
> -	case DW_AT_data_location:
> -	case DW_AT_byte_stride:
> -	case DW_AT_rank:
> -	case DW_AT_call_value:
> -	case DW_AT_call_target:
> -	case DW_AT_call_target_clobbered:
> -	case DW_AT_call_data_location:
> -	case DW_AT_call_data_value:
> -	case DW_AT_GNU_call_site_value:
> -	case DW_AT_GNU_call_site_data_value:
> -	case DW_AT_GNU_call_site_target:
> -	case DW_AT_GNU_call_site_target_clobbered:
> -	  if (read_exprloc_low_mem_phase1 (dso, die, ptr, len))
> -	    return 1;
> -	default:
> -	  break;
> -	}
> +      if (cu->cu_version < 4)
> +	switch (attr)
> +	  {
> +	  case DW_AT_frame_base:
> +	  case DW_AT_location:
> +	  case DW_AT_data_member_location:
> +	  case DW_AT_vtable_elem_location:
> +	  case DW_AT_byte_size:
> +	  case DW_AT_bit_offset:
> +	  case DW_AT_bit_size:
> +	  case DW_AT_string_length:
> +	  case DW_AT_lower_bound:
> +	  case DW_AT_return_addr:
> +	  case DW_AT_bit_stride:
> +	  case DW_AT_upper_bound:
> +	  case DW_AT_count:
> +	  case DW_AT_segment:
> +	  case DW_AT_static_link:
> +	  case DW_AT_use_location:
> +	  case DW_AT_allocated:
> +	  case DW_AT_associated:
> +	  case DW_AT_data_location:
> +	  case DW_AT_byte_stride:
> +	  case DW_AT_rank:
> +	  case DW_AT_call_value:
> +	  case DW_AT_call_target:
> +	  case DW_AT_call_target_clobbered:
> +	  case DW_AT_call_data_location:
> +	  case DW_AT_call_data_value:
> +	  case DW_AT_GNU_call_site_value:
> +	  case DW_AT_GNU_call_site_data_value:
> +	  case DW_AT_GNU_call_site_target:
> +	  case DW_AT_GNU_call_site_target_clobbered:
> +	    if (read_exprloc_low_mem_phase1 (dso, die, ptr, len))
> +	      return 1;
> +	  default:
> +	    break;
> +	  }
>  
>        return 0;
>      }
> @@ -3736,50 +3737,51 @@ checksum_die (DSO *dso, dw_cu_ref cu,
> dw_die_ref top_die, dw_die_ref die)
>        if (form == DW_FORM_block1)
>  	{
>  	  /* Old DWARF uses blocks instead of exprlocs.  */
> -	  switch (t->attr[i].attr)
> -	    {
> -	    case DW_AT_frame_base:
> -	    case DW_AT_location:
> -	    case DW_AT_data_member_location:
> -	    case DW_AT_vtable_elem_location:
> -	    case DW_AT_byte_size:
> -	    case DW_AT_bit_offset:
> -	    case DW_AT_bit_size:
> -	    case DW_AT_string_length:
> -	    case DW_AT_lower_bound:
> -	    case DW_AT_return_addr:
> -	    case DW_AT_bit_stride:
> -	    case DW_AT_upper_bound:
> -	    case DW_AT_count:
> -	    case DW_AT_segment:
> -	    case DW_AT_static_link:
> -	    case DW_AT_use_location:
> -	    case DW_AT_allocated:
> -	    case DW_AT_associated:
> -	    case DW_AT_data_location:
> -	    case DW_AT_byte_stride:
> -	    case DW_AT_rank:
> -	    case DW_AT_call_value:
> -	    case DW_AT_call_target:
> -	    case DW_AT_call_target_clobbered:
> -	    case DW_AT_call_data_location:
> -	    case DW_AT_call_data_value:
> -	    case DW_AT_GNU_call_site_value:
> -	    case DW_AT_GNU_call_site_data_value:
> -	    case DW_AT_GNU_call_site_target:
> -	    case DW_AT_GNU_call_site_target_clobbered:
> -	      if (die->die_ck_state != CK_BAD)
> -		{
> -		  s = t->attr[i].attr;
> -		  die->u.p1.die_hash
> -		    = iterative_hash_object (s, die->u.p1.die_hash);
> -		}
> -	      if (read_exprloc (dso, die, ptr, len, NULL))
> -		return 1;
> -	      handled = true;
> -	    default:
> -	      break;
> -	    }
> +	  if (cu->cu_version < 4)
> +	    switch (t->attr[i].attr)
> +	      {
> +	      case DW_AT_frame_base:
> +	      case DW_AT_location:
> +	      case DW_AT_data_member_location:
> +	      case DW_AT_vtable_elem_location:
> +	      case DW_AT_byte_size:
> +	      case DW_AT_bit_offset:
> +	      case DW_AT_bit_size:
> +	      case DW_AT_string_length:
> +	      case DW_AT_lower_bound:
> +	      case DW_AT_return_addr:
> +	      case DW_AT_bit_stride:
> +	      case DW_AT_upper_bound:
> +	      case DW_AT_count:
> +	      case DW_AT_segment:
> +	      case DW_AT_static_link:
> +	      case DW_AT_use_location:
> +	      case DW_AT_allocated:
> +	      case DW_AT_associated:
> +	      case DW_AT_data_location:
> +	      case DW_AT_byte_stride:
> +	      case DW_AT_rank:
> +	      case DW_AT_call_value:
> +	      case DW_AT_call_target:
> +	      case DW_AT_call_target_clobbered:
> +	      case DW_AT_call_data_location:
> +	      case DW_AT_call_data_value:
> +	      case DW_AT_GNU_call_site_value:
> +	      case DW_AT_GNU_call_site_data_value:
> +	      case DW_AT_GNU_call_site_target:
> +	      case DW_AT_GNU_call_site_target_clobbered:
> +		if (die->die_ck_state != CK_BAD)
> +		  {
> +		    s = t->attr[i].attr;
> +		    die->u.p1.die_hash
> +		      = iterative_hash_object (s, die->u.p1.die_hash);
> +		  }
> +		if (read_exprloc (dso, die, ptr, len, NULL))
> +		  return 1;
> +		handled = true;
> +	      default:
> +		break;
> +	      }
>  	  ptr += len;
>  	}
>        else if (form == DW_FORM_exprloc)
> @@ -12392,7 +12394,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:

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Don't handle blocks as exprlocs for DWARF version 4 or higher.
  2021-02-18 13:40 ` Mark Wielaard
@ 2021-02-18 14:09   ` Jakub Jelinek
  2021-02-18 16:18     ` Mark Wielaard
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2021-02-18 14:09 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: dwz

On Thu, Feb 18, 2021 at 02:40:36PM +0100, Mark Wielaard wrote:
> On Sat, 2021-02-13 at 23:46 +0100, Mark Wielaard wrote:
> > Since DWARF version 4 blocks just contain bytes, trying to interpret
> > them as exprlocs will most likely fail.
> > 
> >      * dwz.c (add_locexpr_dummy_dies): Only handle block as exprloc
> >      for cu_version < 4.
> >      (checksum_die): Likewise.
> >      (write_die): Likewise.
> > 
> > https://sourceware.org/bugzilla/show_bug.cgi?id=26987
> 
> Ping. Any comments?

Doing some GCC archeology if it is safe, I think it principially ok, but I'd
like slightly different patch, see below.
While -gdwarf-4 support has been added in
https://gcc.gnu.org/r0-96109-g15b3fbeb7e97f2ca3731881bf3a0f899ec56ebbf
during GCC 4.5 development, it was still emitting DWARF 3 .debug_info
headers and not using DW_FORM_exprloc.
DW_FORM_exprloc came with:
https://gcc.gnu.org/r0-99103-g290d8971e6e3b784a88b5c4b6b91b8d77552cb3a
and DWARF version .debug_info header changed from 3 to 4 for -gdwarf-4
a day after that:
https://gcc.gnu.org/r0-99139-g2f43d500a6769e563fb8f645e7530c2f144d7023

> > +++ b/dwz.c
> > @@ -2913,43 +2913,44 @@ add_locexpr_dummy_dies (DSO *dso, dw_cu_ref
> > cu, dw_die_ref die,
> >    if (form == DW_FORM_block1)
> >      {
> >        /* Old DWARF uses blocks instead of exprlocs.  */

Instead of reindenting everything, can't you simply change
-  if (form == DW_FORM_block1)
+  if (form == DW_FORM_block1 && cu->cu_version < 4)

> >        if (form == DW_FORM_block1)

And likewise here:
-      if (form == DW_FORM_block1)
+      if (form == DW_FORM_block1 && cu->cu_version < 4)

> > @@ -12392,7 +12394,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)

Like you've done it here?

> >  	    switch (reft->attr[i].attr)
> >  	      {
> >  	      case DW_AT_frame_base:

	Jakub


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Don't handle blocks as exprlocs for DWARF version 4 or higher.
  2021-02-18 14:09   ` Jakub Jelinek
@ 2021-02-18 16:18     ` Mark Wielaard
  2021-02-18 16:59       ` Jakub Jelinek
  0 siblings, 1 reply; 9+ messages in thread
From: Mark Wielaard @ 2021-02-18 16:18 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: dwz

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

On Thu, 2021-02-18 at 15:09 +0100, Jakub Jelinek wrote:
> On Thu, Feb 18, 2021 at 02:40:36PM +0100, Mark Wielaard wrote:
> > On Sat, 2021-02-13 at 23:46 +0100, Mark Wielaard wrote:
> > > Since DWARF version 4 blocks just contain bytes, trying to interpret
> > > them as exprlocs will most likely fail.
> > > 
> > >      * dwz.c (add_locexpr_dummy_dies): Only handle block as exprloc
> > >      for cu_version < 4.
> > >      (checksum_die): Likewise.
> > >      (write_die): Likewise.
> > > 
> > > https://sourceware.org/bugzilla/show_bug.cgi?id=26987
> > 
> > Ping. Any comments?
> 
> Doing some GCC archeology if it is safe, I think it principially ok, but I'd
> like slightly different patch, see below.
> [...]
> > > +++ b/dwz.c
> > > @@ -2913,43 +2913,44 @@ add_locexpr_dummy_dies (DSO *dso,
> > > dw_cu_ref
> > > cu, dw_die_ref die,
> > >    if (form == DW_FORM_block1)
> > >      {
> > >        /* Old DWARF uses blocks instead of exprlocs.  */
> 
> Instead of reindenting everything, can't you simply change
> -  if (form == DW_FORM_block1)
> +  if (form == DW_FORM_block1 && cu->cu_version < 4)

Yes, I was under the impression that the return 0; in the block was
special, but it isn't, the rest of the function does explicitly check
the form (isn't DW_FORM_block1) and so if it simply falls-through it
will also end up at return 0; at the end.

> > >        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.

Does the attached variant look better?

Thanks,

Mark

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

From 3c23d7075af603d1cb8ed5e66f1659c46ec85dc7 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.

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

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

diff --git a/dwz.c b/dwz.c
index d6b9df0..5f0cc1d 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)
@@ -3782,6 +3782,11 @@ checksum_die (DSO *dso, dw_cu_ref cu, dw_die_ref top_die, dw_die_ref die)
 	    }
 	  ptr += len;
 	}
+      else if (form == DW_FORM_block1)
+	{
+	  /* DWARF4 or higher, handle block as an opaque block of bytes.  */
+	  ptr += len;
+	}
       else if (form == DW_FORM_exprloc)
 	{
 	  if (die->die_ck_state != CK_BAD)
@@ -12392,7 +12397,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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Don't handle blocks as exprlocs for DWARF version 4 or higher.
  2021-02-18 16:18     ` Mark Wielaard
@ 2021-02-18 16:59       ` Jakub Jelinek
  2021-02-18 17:32         ` Jakub Jelinek
  2021-02-18 20:01         ` Mark Wielaard
  0 siblings, 2 replies; 9+ messages in thread
From: Jakub Jelinek @ 2021-02-18 16:59 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: dwz

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.

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.
This has been added for PR dwz/24204 by Tom, Tom, can you please comment on
that?
That function handles the DW_FORM_block1 (it wants canonicalization of
DW_FORM_block{2,4,} to DW_FORM_block1) and DW_FORM_exprloc but wants
ptr to be the start of those blocks and len to be the block length, or
it handles DW_FORM_data{4,8} and DW_FORM_sec_offset for which it wants
ptr to stay before the bump.

So, I bet we need something like:
	      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;
		default:
		  break;
		}
added before the
              if (unlikely (low_mem_phase1)
                  && add_locexpr_dummy_dies (dso, cu, die, ptr, form,
                                             t->attr[i].attr, len))
                  goto fail;
and then the DW_FORM_{block*,exprloc} handling later on be changed to:
                case DW_FORM_block1:
		  break;
		case DW_FORM_exprloc:
		  form = DW_FORM_block1;
		  break;
and remove the DW_FORM_block{2,4,} cases.

	Jakub


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Don't handle blocks as exprlocs for DWARF version 4 or higher.
  2021-02-18 16:59       ` Jakub Jelinek
@ 2021-02-18 17:32         ` Jakub Jelinek
  2021-02-18 20:01         ` Mark Wielaard
  1 sibling, 0 replies; 9+ messages in thread
From: Jakub Jelinek @ 2021-02-18 17:32 UTC (permalink / raw)
  To: Mark Wielaard, dwz

On Thu, Feb 18, 2021 at 05:59:40PM +0100, Jakub Jelinek via Dwz wrote:
> 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.

Another possibility would be (if we don't want to slow down non-low mem
case) to just repeat that
> 	      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;
> 		default:
> 		  break;
> 		}
at the start of add_locexpr_dummy_dies (and not pass len to that function
and have it instead as a local var.  Perhaps even by doing:
  size_t len = 0;
  if (cu->cu_version < 4)
    {
      switch (form)
	{
	... // the above switch without DW_FORM_exprloc in there
	}
      if (form == DW_FORM_block1)
	...
    }

  if (form == DW_FORM_exprloc)
    {
      len = read_uleb128 (ptr);
      return read_exprloc_low_mem_phase1 (dso, die, ptr, len);
    }

...

	Jakub


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Don't handle blocks as exprlocs for DWARF version 4 or higher.
  2021-02-18 16:59       ` Jakub Jelinek
  2021-02-18 17:32         ` Jakub Jelinek
@ 2021-02-18 20:01         ` Mark Wielaard
  2021-02-18 20:06           ` Jakub Jelinek
  1 sibling, 1 reply; 9+ messages in thread
From: Mark Wielaard @ 2021-02-18 20:01 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: dwz

[-- 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


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Don't handle blocks as exprlocs for DWARF version 4 or higher.
  2021-02-18 20:01         ` Mark Wielaard
@ 2021-02-18 20:06           ` Jakub Jelinek
  2021-02-18 21:17             ` Mark Wielaard
  0 siblings, 1 reply; 9+ messages in thread
From: Jakub Jelinek @ 2021-02-18 20:06 UTC (permalink / raw)
  To: Mark Wielaard; +Cc: dwz

On Thu, Feb 18, 2021 at 09:01:43PM +0100, Mark Wielaard wrote:
> @@ -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;

Can you please add
		default:
		  break;
here?

Otherwise LGTM, thanks.

	Jakub


^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] Don't handle blocks as exprlocs for DWARF version 4 or higher.
  2021-02-18 20:06           ` Jakub Jelinek
@ 2021-02-18 21:17             ` Mark Wielaard
  0 siblings, 0 replies; 9+ messages in thread
From: Mark Wielaard @ 2021-02-18 21:17 UTC (permalink / raw)
  To: Jakub Jelinek; +Cc: dwz

On Thu, Feb 18, 2021 at 09:06:35PM +0100, Jakub Jelinek via Dwz wrote:
> Can you please add
> 		default:
> 		  break;
> here?
> 
> Otherwise LGTM, thanks.

Added and pushed.

Thanks,

Mark

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-02-18 21:18 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-02-13 22:46 [PATCH] Don't handle blocks as exprlocs for DWARF version 4 or higher Mark Wielaard
2021-02-18 13:40 ` Mark Wielaard
2021-02-18 14:09   ` Jakub Jelinek
2021-02-18 16:18     ` Mark Wielaard
2021-02-18 16:59       ` Jakub Jelinek
2021-02-18 17:32         ` Jakub Jelinek
2021-02-18 20:01         ` Mark Wielaard
2021-02-18 20:06           ` Jakub Jelinek
2021-02-18 21:17             ` Mark Wielaard

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).