Re: [PATCH] gdb: ensure bp_location::section is set correct to avoid an assert

[Andrew Burgess <aburgess@redhat.com>]

Sébastien Darche <sdarche@efficios.com> writes:

> Hi Andrew,
>
> We've noticed a regression caused by this patch on one of the amdgpu
> tests (namely gdb.rocm/displaced-stepping.exp). This test attempts
> to insert a breakpoint at the beginning of a GPU kernel, step, then
> proceed to the end of the test. It would appear the breakpoint is
> inserted but not hit by the GPU.
>
>
> Upon investigating why the breakpoint is not set the way it should be,
> I've found that code_breakpoint::add_location relies on the section
> being set to deduce the proper gdbarch, by calling get_sal_arch
> (breakpoint.c:8615). If the sal section is not set, then the gdbarch is
> set by default to the breakpoint gdbarch, which in our case is the host's
> gdbarch. This causes problem down the road leading to the breakpoint not
> being hit.
>
>
> A quick fix to the problem would be to call find_pc_section() to assign
> the proper section to the pc, *after* checking for find_pc_overlay :
>
>
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index e634bf22f3c..4a4d178085c 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -4120,6 +4120,10 @@ minsym_found (struct linespec_state *self, struct 
> objfile *objfile,
>        debugging, it should reflect the SAL's pc value.  */
>     sal.section = find_pc_overlay (sal.pc);
>
> +  if (sal.section == nullptr) {
> +    sal.section = find_pc_section (sal.pc);
> +  }
> +
>     if (self->maybe_add_address (objfile->pspace (), sal.pc))
>       add_sal_to_sals (self, result, &sal, msymbol->natural_name (), 
> false);
>   }
>
>
> This ensures that minsym_found() finds the proper pc (instead of the
> resolver, for ifuncs, just like your patch intended), then assigns
> the section corresponding to that pc. Ensuring that sal.section is
> consistent to the pc is, in my opinion, a more appropriate solution
> than leaving its contents empty for the time being. Modifying the
> other places where the section is needed (or expected to be null) may
> require some further investigation.
>
>> I think what we need to do is update minsym_found to just use
>> find_pc_overlay, which is how the symtab_and_line::section is set in
>> most other cases.  What this actually means in practise is that the
>> section field will be set to NULL (see find_pc_overlay in symfile.c).
>> But given that this is how the section is computed in most other
>> cases, I don't see why it should be especially problematic for this
>> case.  In reality, I think this just means that the section is
>> calculated via a call to find_pc_section when it's needed, as an
>> example, see lookup_minimal_symbol_by_pc_section (minsyms.c).
>>
>> I do wonder if we should be doing better when creating the
>> symtab_and_line, and insist that the section be calculated correctly
>> at that point, but I really don't want to open that can of worms right
>> now, so I think just changing minsym_found to "do it just like
>> everyone else" should be good enough.
>
> Admittedly, this means thinking about *when* sections are stored
> in the sal. Let me know what you think about this problem and if
> you think of another approach.

First, I don't really object to the fix you propose.  There's already
code like this in GDB, see the find_pc_overlay call in blockframe.c.

However.

I think the real fix here would be to fold the find_pc_section call into
find_pc_overlay, like:

  struct obj_section *
  find_pc_overlay (CORE_ADDR pc)
  {
    struct obj_section *best_match = NULL;
  
    if (overlay_debugging)
      {
        ... existing code here ...
      }
    else
      best_match = find_pc_section (pc);
  
    return best_match;
  }

Note that the function names might not be ideal after this change, but
they'll do for now.

Almost all calls to find_pc_overlay are related to setting the section
within a SAL, so I think we can be comfortable in those cases that this
change is OK (I say that without testing this at all).

Then there's a find_pc_overlay call in blockframe.c where we already
have a find_pc_section call, so I think that case is fine.

There's a call in find_pc_line, this one appears at a glance to be
interesting as we'd now be passing a non NULL section to
find_pc_sect_line in more cases, which, following the code through,
would appear as if we might trigger some additional cases.

And there's a call inside memory_xfer_partial_1, but this is already
guarded by an overlay_debugging check, so in this case the change I
propose would have no impact.

I'd definitely be interested in seeing if the above change throws up any
problems in testing.

Part of the reason I'd push for a wider fix is that there are lots
different linespec formats, and I don't think they all pass through
minsym_found (or maybe they do?).  If they don't then it feels like you
should be able to adjust your original test such that minsym_found isn't
called, and you'll have the same incorrect gdbarch problem.  So then
you'll have to add a find_pc_section in _another_ place....

Anyway, the above isn't a requirement, just my thoughts.

Thanks,
Andrew