Re: [PATCH v2 28/42] Remove dwarf2_per_cu_data::objfile ()

[Simon Marchi <simark@simark.ca>]

On 2020-05-28 9:05 a.m., Tom de Vries wrote:
> Hi,
> 
> thanks for looking into this.
> 
> I tried out the patch, and it fixes all regressions for me.
> 
> Thanks,
> - Tom

Thanks, I pushed it with the following commit message.

Simon


From 44486dcf19b62708ad49bbb6094e065a223dea99 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@efficios.com>
Date: Thu, 28 May 2020 11:30:11 -0400
Subject: [PATCH] gdb: use caller objfile in
 dwarf_evaluate_loc_desc::push_dwarf_reg_entry_value

In commit

    89b07335fe ("Add dwarf2_per_objfile to dwarf_expr_context and dwarf2_frame_cache")

I replaced the offset property of dwarf_expr_context by a per_objfile
property (since we can get the text offset from the objfile).  The
previous code in dwarf_evaluate_loc_desc::push_dwarf_reg_entry_value
(dwarf_evaluate_loc_desc derives from dwarf_expr_context) did
temporarily override the offset property while evaluating a DWARF
sub-expression.  I speculated that this sub-expression always came from
the same objfile as the outer expression, so I didn't see the need to
temporarily override the per_objfile property in the new code.  A later
commit:

    9f47c70716 ("Remove dwarf2_per_cu_data::objfile ()")

added the following assertion to verify this:

    gdb_assert (this->per_objfile == caller_per_objfile);

It turns out that this is not true.  Call sites can refer to function in
another objfile, and therefore the caller's objfile can be different
from the callee's objfile.  This can happen when the call site DIE in the
DWARF represents a function call done through a function pointer.  The
DIE can't describe statically which function is being called, since it's
variable and not known at compile time.  Instead, it provides an
expression that evaluates to the address of the function being called.
In this case, the called function can very well be in a separate
objfile.

Fix this by overriding the per_objfile property while evaluating the
sub-expression.

This was exposed by the gdb.base/catch-load.exp test failing on openSUSE
Tumbleweed with the glibc debug info installed.  It was also reported to
fail on Fedora.

When I investigated the problem, the particular call site on which we
did hit the assert was coming from this DIE, in
/usr/lib/debug/lib64/libc-2.31.so-2.31-5.1.x86_64.debug on openSUSE
Tumbleweed:

    0x0091aa10:     DW_TAG_GNU_call_site
                      DW_AT_low_pc [DW_FORM_addr]   (0x00000000001398e0)
                      DW_AT_GNU_call_site_target [DW_FORM_exprloc]  (DW_OP_fbreg -272, DW_OP_deref)
                      DW_AT_sibling [DW_FORM_ref4]  (0x0091aa2b)

And for you curious out there, this call site is found in this function:

    0x0091a91d:   DW_TAG_subprogram
                    DW_AT_external [DW_FORM_flag_present]   (true)
                    DW_AT_name [DW_FORM_strp]       ("_dl_catch_exception")
                    DW_AT_decl_file [DW_FORM_data1] ("/usr/src/debug/glibc-2.31-5.1.x86_64/elf/dl-error-skeleton.c")
                    ...

Which is a function that indeed uses a function pointer.

gdb/ChangeLog:

	* dwarf2/loc.c (class dwarf_evaluate_loc_desc)
	<push_dwarf_reg_entry_value>: Remove assert.  Override
	per_objfile with caller_per_objfile.

Change-Id: Ib227d767ce525c10607ab6621a373aaae982c67a
---
 gdb/ChangeLog    | 6 ++++++
 gdb/dwarf2/loc.c | 6 +++---
 2 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/gdb/ChangeLog b/gdb/ChangeLog
index 593ff01cc9d0..e5b4019dd646 100644
--- a/gdb/ChangeLog
+++ b/gdb/ChangeLog
@@ -1,3 +1,9 @@
+2020-05-28  Simon Marchi  <simon.marchi@efficios.com>
+
+	* dwarf2/loc.c (class dwarf_evaluate_loc_desc)
+	<push_dwarf_reg_entry_value>: Remove assert.  Override
+	per_objfile with caller_per_objfile.
+
 2020-05-28  Tom de Vries  <tdevries@suse.de>

 	* dwarf2/read.c	(dw2_symtab_iter_next, dw2_expand_marked_cus): Limit
diff --git a/gdb/dwarf2/loc.c b/gdb/dwarf2/loc.c
index 7953361adeed..1aab1a4f51bc 100644
--- a/gdb/dwarf2/loc.c
+++ b/gdb/dwarf2/loc.c
@@ -726,8 +726,6 @@ class dwarf_evaluate_loc_desc : public dwarf_expr_context
     data_src = deref_size == -1 ? parameter->value : parameter->data_value;
     size = deref_size == -1 ? parameter->value_size : parameter->data_value_size;

-    gdb_assert (this->per_objfile == caller_per_objfile);
-
     /* DEREF_SIZE size is not verified here.  */
     if (data_src == NULL)
       throw_error (NO_ENTRY_VALUE_ERROR,
@@ -739,11 +737,13 @@ class dwarf_evaluate_loc_desc : public dwarf_expr_context
 						      caller_per_cu);
     scoped_restore save_obj_addr = make_scoped_restore (&this->obj_address,
 							(CORE_ADDR) 0);
+    scoped_restore save_per_objfile = make_scoped_restore (&this->per_objfile,
+							   caller_per_objfile);

     scoped_restore save_arch = make_scoped_restore (&this->gdbarch);
     this->gdbarch = this->per_objfile->objfile->arch ();
     scoped_restore save_addr_size = make_scoped_restore (&this->addr_size);
-    this->addr_size = per_cu->addr_size ();
+    this->addr_size = this->per_cu->addr_size ();

     this->eval (data_src, size);
   }
-- 
2.26.2