Re: [PATCH] Update auxv cache when there is no auxv cached data

[John Baldwin <jhb@FreeBSD.org>]

On 8/2/22 8:05 AM, Luis Machado wrote:
> Here's the sequence of events for each invocation mode:
> 
> 1 - Loading core file only (OK)
> 
> Invocations:
> gdb -c exec.core
> gdb -ex "core exec.core
> 
> In this mode, GDB will load the core file and eventually invoke the constructor in core_target_open:
> 
>     core_target *target = new core_target ();
> 
> This constructor will attempt to retrieve the target description, and it won't find a XML note in the core file because this
> was generated by the Linux Kernel. So eventually it will invoke aarch64_linux_core_read_description, which *will* try to
> retrieve the hwcap bits.
> 
> In this case, it works because we don't have any auxv entries cached for inferior with pid 0, so we proceed to fetch the corefile's
> auxv section data.
> 
> Eventually we will figure out the real pid for the core file inferior and we will fetch the target description again and
> things just work.
> 
> 
> 2 - Loading exec file and core file (OK)
> 
> Invocations:
> gdb exec -c exec.core
> gdb -ex "file exec" -c exec.core
> gdb -ex "core exec.core" -ex "file exec"
> 
> Loading the exec file through the command line (not using the file command) doesn't force the svr4 relocation
> functions to be called, and thus doesn't attempt to fetch the auxv (none available) and cache it (empty).
> 
> When using the file command to load the exec file, we eventually attempt to fetch the auxv (none available) and
> cache it (empty).
> 
> With that in mind, these invocations will always cause the corefile handling routines to be the first ones to attempt
> to fetch the auxv data, so this will in fact cache valid data and things just work normally.
> 
> 
> 3 - Loading exec file and core file (Broken)
> 
> Invocation:
> gdb -ex "file exec" -ex "core exec.core"
> 
> Having in mind the explanation from (2), this invocation will first attempt to fetch auxv data for the exec target and will
> cache empty data, because the exec target doesn't have auxv entries. This will be cached for inferior pid 0.
> 
> Afterwards, we process the corefile and during the construction of the core_target, the aarch64 code will attempt to
> fetch the auxv data, but at this point we still have inferior pid 0, since we didn't read the registers yet.
> 
> Given we cached the empty auxv data from the exec target earlier, we will get that when we attempt to fetch the auxv data
> for the core file, which is incorrect.
> 
> This will throw off the target feature detection for aarch64, and so we won't register the required hooks (in particular report_signal_info).
> 
> Loading the corefile proceeds as usual and we will eventually find the pid for the corefile inferior, which causes the cached auxv entry to be
> invalidated and we fetch the auxv again, but with a valid inferior pid.
> 
> What actually goes wrong now is the fact that core_gdbarch was not calculated correctly due to the auxv situation. But the gdbarch (not core_gdbarch) is
> correct though.
> 
> There are some odd things that I noticed:
> 
> - The use of pid 0 for both the exec and the core file at the same time.
> - The use of core_gdbarch as opposed to using the regular gdbarch. It feels like we have a couple gdbarch's when we should have only one.
> - The handling of non-existent / empty auxv is not great. Should we cache an empty auxv? Should we cache a non-existent auxv value?
> 
> An easy fix would be to not use hwcap bits for detecting corefile features, instead relying on dumped register sections. But we should be able to use
> information that is available through the auxv.

FWIW, for FreeBSD so far I have relied on the register sets present to determine
the target description (both for ARM and x86 architectures), in part because
the presence of register set notes is what ensures the core file can supply the
relevant registers.  This is also true for Linux/x86 though part of the reason for
this is that on x86 we need the value of the xcr0 register to determine what
register sets are contained in the XSAVE note and that isn't described by
HWCAP bits but instead a copy of xcr0 is saved in the XSAVE note at a fixed
offset.  (Actually, 32-bit arm on FreeBSD does use AT_HWCAP as well to determine
the type of VFP registers.)

That said, I wonder if what might make sense is to not cache auxv data for pid 0.
pid 0 is the special "not yet fully ready" inferior.  (In fact, the check I had
envisioned for guarding svr4_relocate_file could possibly be expressed as the
pid != 0.)  This would cache auxv data once an inferior was fully "live" but would
work permit case 3) to work.  Something like:

diff --git a/gdb/auxv.c b/gdb/auxv.c
index 8e175138f5d..53fd66dd3af 100644
--- a/gdb/auxv.c
+++ b/gdb/auxv.c
@@ -361,7 +361,7 @@ get_auxv_inferior_data (struct target_ops *ops)
    struct inferior *inf = current_inferior ();
  
    info = auxv_inferior_data.get (inf);
-  if (info == NULL)
+  if (info == NULL || inf->pid == 0)
      {
        info = auxv_inferior_data.emplace (inf);
        info->data = target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL);

I know previously I said I didn't like overwriting the cached info as your
original patch did, but looking at the existing uses of get_auxv_inferior_data
trying to teach them cleanly to discard the returned data for pid 0 and not
otherwise seemed ugly.  There is still a downside that this will return the
"last" cached auxv data for pid 0 once the pid changes, but the pid changing
should trigger cache invalidation via one of the observers so I think it's ok.

-- 
John Baldwin