[PATCH] Correctly handle DW_LLE_start_end

[PATCH] Correctly handle DW_LLE_start_end

[Tom Tromey]

When the code to handle DW_LLE_start_end was added (as part of some
DWARF 5 work), it was written to add the base address.  However, this
seems incorrect -- the DWARF standard describes this as an address,
not an offset from the base address.

This patch changes a couple of spots in dwarf2/loc.c to fix this
problem.  It then changes decode_debug_loc_addresses to return
DEBUG_LOC_OFFSET_PAIR instead, which preserves the previous semantics.

This only showed up on the RISC-V target internally, due to the
combination of DWARF 5 and a newer version of GCC.  I've updated a
couple of existing loclists test cases to demonstrate the bug.
---
 gdb/dwarf2/loc.c                                | 17 ++++++++++++-----
 .../gdb.dwarf2/loclists-multiple-cus.exp        |  5 ++++-
 gdb/testsuite/gdb.dwarf2/loclists-start-end.exp |  3 +++
 gdb/testsuite/lib/dwarf.exp                     | 12 +++++++++++-
 4 files changed, 30 insertions(+), 7 deletions(-)

diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index eb128fa5fc6..b5936e13eee 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -139,7 +139,9 @@ decode_debug_loc_addresses (const gdb_byte *loc_ptr, const gdb_byte *buf_end,
   if (*low == 0 && *high == 0)
     return DEBUG_LOC_END_OF_LIST;
 
-  return DEBUG_LOC_START_END;
+  /* We want the caller to apply the base address, so we must return
+     DEBUG_LOC_OFFSET_PAIR here.  */
+  return DEBUG_LOC_OFFSET_PAIR;
 }
 
 /* Decode the addresses in .debug_loclists entry.
@@ -416,13 +418,15 @@ dwarf2_find_location_expression (struct dwarf2_loclist_baton *baton,
 	 .debug_addr which already has the DWARF "base address". We still add
 	 base_offset in case we're debugging a PIE executable. However, if the
 	 entry is DW_LLE_offset_pair from a DWO, add the base address as the
-	 operands are offsets relative to the applicable base address.  */
+	 operands are offsets relative to the applicable base address.
+	 If the entry is DW_LLE_start_end or DW_LLE_start_length, then
+	 it already is an address, and we don't need to add the base.  */
       if (baton->from_dwo && kind != DEBUG_LOC_OFFSET_PAIR)
 	{
 	  low += base_offset;
 	  high += base_offset;
 	}
-      else
+      else if (kind == DEBUG_LOC_OFFSET_PAIR)
 	{
 	  low += base_address;
 	  high += base_address;
@@ -3983,8 +3987,11 @@ loclist_describe_location (struct symbol *symbol, CORE_ADDR addr,
 	}
 
       /* Otherwise, a location expression entry.  */
-      low += base_address;
-      high += base_address;
+      if (kind == DEBUG_LOC_OFFSET_PAIR)
+	{
+	  low += base_address;
+	  high += base_address;
+	}
 
       low = gdbarch_adjust_dwarf2_addr (gdbarch, low);
       high = gdbarch_adjust_dwarf2_addr (gdbarch, high);
diff --git a/gdb/testsuite/gdb.dwarf2/loclists-multiple-cus.exp b/gdb/testsuite/gdb.dwarf2/loclists-multiple-cus.exp
index 7844628bc16..a7af7fd0537 100644
--- a/gdb/testsuite/gdb.dwarf2/loclists-multiple-cus.exp
+++ b/gdb/testsuite/gdb.dwarf2/loclists-multiple-cus.exp
@@ -1,4 +1,4 @@
-# Copyright 2020 Free Software Foundation, Inc.
+# Copyright 2020, 2021 Free Software Foundation, Inc.
 
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -105,6 +105,9 @@ foreach_with_prefix is_64 {false true} {
 
 		# For variable foo.
 		list_ {
+		    # This should not affect the following addresses.
+		    base_address 0xffff
+
 		    # When in func1.
 		    start_length $func1_addr $func1_len {
 			DW_OP_constu 0x123456
diff --git a/gdb/testsuite/gdb.dwarf2/loclists-start-end.exp b/gdb/testsuite/gdb.dwarf2/loclists-start-end.exp
index e3c12e5093f..18ef2bfaf90 100644
--- a/gdb/testsuite/gdb.dwarf2/loclists-start-end.exp
+++ b/gdb/testsuite/gdb.dwarf2/loclists-start-end.exp
@@ -96,6 +96,9 @@ foreach_with_prefix is_64 {false true} {
 
 		# For variable foo.
 		list_ {
+		    # This should not affect the following addresses.
+		    base_address 0xffff
+
 		    # When in func1.
 		    start_end $func1_addr "$func1_addr + $func1_len" {
 			DW_OP_constu 0x123456
diff --git a/gdb/testsuite/lib/dwarf.exp b/gdb/testsuite/lib/dwarf.exp
index b48cfad3b9e..774cac712a2 100644
--- a/gdb/testsuite/lib/dwarf.exp
+++ b/gdb/testsuite/lib/dwarf.exp
@@ -1894,9 +1894,10 @@ namespace eval Dwarf {
 	define_label $list_label
 
 	with_override Dwarf::start_length Dwarf::_loclists_start_length {
+	with_override Dwarf::base_address Dwarf::_loclists_base_address {
 	with_override Dwarf::start_end Dwarf::_loclists_start_end {
 	    uplevel $body
-	}}
+	}}}
 
 	# Emit end of list.
 	_op .byte 0x00 "DW_LLE_end_of_list"
@@ -1972,6 +1973,15 @@ namespace eval Dwarf {
 	incr _debug_loclists_locdesc_count
     }
 
+    # Emit a DW_LLE_base_address entry.
+    proc _loclists_base_address {addr} {
+	variable _debug_loclists_addr_size
+	variable _debug_loclists_locdesc_count
+	_op .byte 0x06 "DW_LLE_base_address"
+	_op .${_debug_loclists_addr_size}byte $addr "base_address"
+	incr _debug_loclists_locdesc_count
+    }
+
     # Emit a DWARF .debug_line unit.
     # OPTIONS is a list with an even number of elements containing
     # option-name and option-value pairs.
-- 
2.31.1

Re: [PATCH] Correctly handle DW_LLE_start_end

[Simon Marchi]

On 2021-11-08 1:01 p.m., Tom Tromey via Gdb-patches wrote:
> When the code to handle DW_LLE_start_end was added (as part of some
> DWARF 5 work), it was written to add the base address.  However, this
> seems incorrect -- the DWARF standard describes this as an address,
> not an offset from the base address.
>
> This patch changes a couple of spots in dwarf2/loc.c to fix this
> problem.  It then changes decode_debug_loc_addresses to return
> DEBUG_LOC_OFFSET_PAIR instead, which preserves the previous semantics.
>
> This only showed up on the RISC-V target internally, due to the
> combination of DWARF 5 and a newer version of GCC.  I've updated a
> couple of existing loclists test cases to demonstrate the bug.

IIUC, this fixes some bugs for both DWARF 4's .debug_loc and DWARF 5's
.debug_loclists.

The patch LGTM.

Simon

Re: [PATCH] Correctly handle DW_LLE_start_end

[Tom Tromey]

>> This only showed up on the RISC-V target internally, due to the
>> combination of DWARF 5 and a newer version of GCC.  I've updated a
>> couple of existing loclists test cases to demonstrate the bug.

Simon> IIUC, this fixes some bugs for both DWARF 4's .debug_loc and DWARF 5's
Simon> .debug_loclists.

Simon> The patch LGTM.

I plan to push it, but FAOD the fix is just for DWARF 5; the other
change is to avoid introducing a regression for DWARF 4 (which I briefly
did here...)

thanks,
Tom

Re: [PATCH] Correctly handle DW_LLE_start_end

[Simon Marchi]

On 2021-11-09 9:52 a.m., Tom Tromey via Gdb-patches wrote:
>>> This only showed up on the RISC-V target internally, due to the
>>> combination of DWARF 5 and a newer version of GCC.  I've updated a
>>> couple of existing loclists test cases to demonstrate the bug.
> 
> Simon> IIUC, this fixes some bugs for both DWARF 4's .debug_loc and DWARF 5's
> Simon> .debug_loclists.
> 
> Simon> The patch LGTM.
> 
> I plan to push it, but FAOD the fix is just for DWARF 5; the other
> change is to avoid introducing a regression for DWARF 4 (which I briefly
> did here...)
> 
> thanks,
> Tom
> 

Makes sense, thanks.

Simon

Re: [PATCH] Correctly handle DW_LLE_start_end

[Simon Marchi]

On 2021-11-09 09:52, Tom Tromey via Gdb-patches wrote:
>>> This only showed up on the RISC-V target internally, due to the
>>> combination of DWARF 5 and a newer version of GCC.  I've updated a
>>> couple of existing loclists test cases to demonstrate the bug.
> 
> Simon> IIUC, this fixes some bugs for both DWARF 4's .debug_loc and DWARF 5's
> Simon> .debug_loclists.
> 
> Simon> The patch LGTM.
> 
> I plan to push it, but FAOD the fix is just for DWARF 5; the other
> change is to avoid introducing a regression for DWARF 4 (which I briefly
> did here...)
> 
> thanks,
> Tom
> 

Hmm, this caused failures in:

 - gdb.dwarf2/loclists-multiple-cus.exp
 - gdb.dwarf2/loclists-sec-offset.exp
 - gdb.dwarf2/loclists-start-end.exp

These use DEBUG_LOC_START_END and DEBUG_LOC_START_LENGTH.  The resulting
addresses need to have base_offset added, which is the objfile's base
address.  Before your change, it would get added because base_address
was added.  And it worked by chance, because base_address is initialized to
base_offset (the objfile's base), so they ended up relocated properly.

I'm currently poking at dwarf2_find_location_expression, trying to find
the right conditions to decide when to apply what.

Simon

Re: [PATCH] Correctly handle DW_LLE_start_end

[Simon Marchi]

On 2021-11-09 20:32, Simon Marchi via Gdb-patches wrote:
> On 2021-11-09 09:52, Tom Tromey via Gdb-patches wrote:
>>>> This only showed up on the RISC-V target internally, due to the
>>>> combination of DWARF 5 and a newer version of GCC.  I've updated a
>>>> couple of existing loclists test cases to demonstrate the bug.
>>
>> Simon> IIUC, this fixes some bugs for both DWARF 4's .debug_loc and DWARF 5's
>> Simon> .debug_loclists.
>>
>> Simon> The patch LGTM.
>>
>> I plan to push it, but FAOD the fix is just for DWARF 5; the other
>> change is to avoid introducing a regression for DWARF 4 (which I briefly
>> did here...)
>>
>> thanks,
>> Tom
>>
> 
> Hmm, this caused failures in:
> 
>  - gdb.dwarf2/loclists-multiple-cus.exp
>  - gdb.dwarf2/loclists-sec-offset.exp
>  - gdb.dwarf2/loclists-start-end.exp
> 
> These use DEBUG_LOC_START_END and DEBUG_LOC_START_LENGTH.  The resulting
> addresses need to have base_offset added, which is the objfile's base
> address.  Before your change, it would get added because base_address
> was added.  And it worked by chance, because base_address is initialized to
> base_offset (the objfile's base), so they ended up relocated properly.
> 
> I'm currently poking at dwarf2_find_location_expression, trying to find
> the right conditions to decide when to apply what.

When I change the condition to:

      if (kind == DEBUG_LOC_OFFSET_PAIR)
	{
	  low += base_address;
	  high += base_address;
	}
      else
	{
	  low += base_offset;
	  high += base_offset;
	}

the tests now pass, and I don't see any other failure in gdb.dwarf2.  I
don't understand why the current condition checks whether the location
list comes from a DWO, and I don't understand much of the comment just
above.  The commits that introduced this code did add two tests:

  - gdb.dwarf2/fission-loclists.exp
  - gdb.dwarf2/fission-loclists-pie.exp

... and they still pass with my change, I am not sure what to think
about that.

Simon

PS: I would suggest renaming "base_offset" to "obfile_base" or something
like that, because "base_offset" and "base_address" are really easy to
confuse.

Re: [PATCH] Correctly handle DW_LLE_start_end

[Tom Tromey]

Simon> Hmm, this caused failures in:

Simon>  - gdb.dwarf2/loclists-multiple-cus.exp
Simon>  - gdb.dwarf2/loclists-sec-offset.exp
Simon>  - gdb.dwarf2/loclists-start-end.exp

I don't see these.  I did "runtest --directory gdb.dwarf2"
and I get:

		=== gdb Summary ===

# of expected passes		1796
# of known failures		3
# of unsupported tests		3


Are you running in some special mode or something?
I'm wondering why I don't see this.

Tom

Re: [PATCH] Correctly handle DW_LLE_start_end

[Tom Tromey]

Simon> PS: I would suggest renaming "base_offset" to "obfile_base" or something
Simon> like that, because "base_offset" and "base_address" are really easy to
Simon> confuse.

Let me know what you think of the appended.

Tom

commit 0c7af29227028f58dfe8ac7e6f0be19f87b9fe22
Author: Tom Tromey <tromey@adacore.com>
Date:   Wed Nov 10 12:15:02 2021 -0700

    Handle PIE in .debug_loclists
    
    Simon pointed out that my recent patches to .debug_loclists caused
    some regressions.  After a brief discussion we realized it was because
    his system compiler defaults to PIE.
    
    This patch changes this code to unconditionally apply the text offset
    here.  It also changes loclist_describe_location to work more like
    dwarf2_find_location_expression.
    
    I tested this by running the gdb.dwarf2 tests both with and without
    -pie.

diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index b5936e13eee..182f15e7077 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -356,9 +356,9 @@ dwarf2_find_location_expression (struct dwarf2_loclist_baton *baton,
   enum bfd_endian byte_order = gdbarch_byte_order (gdbarch);
   unsigned int addr_size = baton->per_cu->addr_size ();
   int signed_addr_p = bfd_get_sign_extend_vma (objfile->obfd);
-  /* Adjust base_address for relocatable objects.  */
-  CORE_ADDR base_offset = baton->per_objfile->objfile->text_section_offset ();
-  CORE_ADDR base_address = baton->base_address + base_offset;
+  /* Adjustment for relocatable objects.  */
+  CORE_ADDR text_offset = baton->per_objfile->objfile->text_section_offset ();
+  CORE_ADDR base_address = baton->base_address;
   const gdb_byte *loc_ptr, *buf_end;
 
   loc_ptr = baton->data;
@@ -396,7 +396,7 @@ dwarf2_find_location_expression (struct dwarf2_loclist_baton *baton,
 	  return NULL;
 
 	case DEBUG_LOC_BASE_ADDRESS:
-	  base_address = high + base_offset;
+	  base_address = high;
 	  continue;
 
 	case DEBUG_LOC_START_END:
@@ -416,17 +416,14 @@ dwarf2_find_location_expression (struct dwarf2_loclist_baton *baton,
       /* Otherwise, a location expression entry.
 	 If the entry is from a DWO, don't add base address: the entry is from
 	 .debug_addr which already has the DWARF "base address". We still add
-	 base_offset in case we're debugging a PIE executable. However, if the
+	 text offset in case we're debugging a PIE executable. However, if the
 	 entry is DW_LLE_offset_pair from a DWO, add the base address as the
 	 operands are offsets relative to the applicable base address.
 	 If the entry is DW_LLE_start_end or DW_LLE_start_length, then
 	 it already is an address, and we don't need to add the base.  */
-      if (baton->from_dwo && kind != DEBUG_LOC_OFFSET_PAIR)
-	{
-	  low += base_offset;
-	  high += base_offset;
-	}
-      else if (kind == DEBUG_LOC_OFFSET_PAIR)
+      low += text_offset;
+      high += text_offset;
+      if (!baton->from_dwo && kind == DEBUG_LOC_OFFSET_PAIR)
 	{
 	  low += base_address;
 	  high += base_address;
@@ -3925,9 +3922,9 @@ loclist_describe_location (struct symbol *symbol, CORE_ADDR addr,
   unsigned int addr_size = dlbaton->per_cu->addr_size ();
   int offset_size = dlbaton->per_cu->offset_size ();
   int signed_addr_p = bfd_get_sign_extend_vma (objfile->obfd);
-  /* Adjust base_address for relocatable objects.  */
-  CORE_ADDR base_offset = objfile->text_section_offset ();
-  CORE_ADDR base_address = dlbaton->base_address + base_offset;
+  /* Adjustment for relocatable objects.  */
+  CORE_ADDR text_offset = objfile->text_section_offset ();
+  CORE_ADDR base_address = dlbaton->base_address;
   int done = 0;
 
   loc_ptr = dlbaton->data;
@@ -3967,7 +3964,7 @@ loclist_describe_location (struct symbol *symbol, CORE_ADDR addr,
 	  continue;
 
 	case DEBUG_LOC_BASE_ADDRESS:
-	  base_address = high + base_offset;
+	  base_address = high;
 	  fprintf_filtered (stream, _("  Base address %s"),
 			    paddress (gdbarch, base_address));
 	  continue;
@@ -3987,7 +3984,9 @@ loclist_describe_location (struct symbol *symbol, CORE_ADDR addr,
 	}
 
       /* Otherwise, a location expression entry.  */
-      if (kind == DEBUG_LOC_OFFSET_PAIR)
+      low += text_offset;
+      high += text_offset;
+      if (!dlbaton->from_dwo && kind == DEBUG_LOC_OFFSET_PAIR)
 	{
 	  low += base_address;
 	  high += base_address;

Re: [PATCH] Correctly handle DW_LLE_start_end

[Simon Marchi]

On 2021-11-10 14:19, Tom Tromey wrote:
> Simon> PS: I would suggest renaming "base_offset" to "obfile_base" or something
> Simon> like that, because "base_offset" and "base_address" are really easy to
> Simon> confuse.
> 
> Let me know what you think of the appended.
> 
> Tom

Looks good from my side, all tests pass, thanks!

Simon