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

Christian Biesinger
2020-05-28T16:24:50Z

On Thu, May 28, 2020 at 10:33 AM Simon Marchi <simark@simark.ca> wrote:
>
> 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.

Maybe add a comment describing this case, so that future developers
looking at the code know why the two objfiles can be different?

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