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

[Christian Biesinger <cbiesinger@google.com>]

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
>