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

[John Baldwin <jhb@FreeBSD.org>]

On 7/25/22 11:03 AM, Luis Machado wrote:
> On 7/25/22 17:05, John Baldwin wrote:
>> On 7/19/22 7:45 AM, Luis Machado via Gdb-patches wrote:
>>> While adding support for MTE corefiles and running the MTE corefile tests,
>>> I noticed a strange situation where loading the symbol file + core file
>>> through the command line has a different behavior compared to firing up
>>> GDB, loading the symbol file with the "file" command and then loading the
>>> core file with the "core" command.
>>>
>>> I tracked this down to gdb/auxv.c:get_auxv_inferior_data returning a valid
>>> info struct, but with no auxv data.
>>>
>>> We've been doing the auxv caching for a while now, but sometime between
>>> enabling auxv data caching and now, we turned the auxv data into an optional
>>> value.
>>
>> The commit to use gdb::optional<> was this one in 2018:
>>
>> commit 9018be22e022e6db2ba07c4e407c7244022bc69a
>> Author: Simon Marchi <simon.marchi@ericsson.com>
>> Date:   Sat Apr 7 13:19:12 2018 -0400
>>
>>       Make target_read_alloc & al return vectors
>>       This patch started by changing target_read_alloc_1 to return a
>>       byte_vector, to avoid manual memory management (in target_read_alloc_1
>>       and in the callers).  To communicate failures to the callers, it
>>       actually returns a gdb::optional<gdb::byte_vector>.
>>       Adjusting target_read_stralloc was a bit more tricky, since it wants to
>>       return a buffer of char, and not gdb_byte.  Since you can't just cast a
>>       gdb::byte_vector into a gdb::def_vector<char>, I made
>>       target_read_alloc_1 templated, so both versions (that return vectors of
>>       gdb_byte and char) are generated.  Since target_read_stralloc now
>>       returns a gdb::char_vector instead of a gdb::unique_xmalloc_ptr<char>, a
>>       few callers need to be adjusted.
>>
>> But looking at it's diff, it was always caching negative results even
>> before this change.  This is the relevant hunk of that commit:
> 
> True. I think my analysis went bad when I mixed up data before (a pointer) the optional conversion and after (an optional vector).
> 
> So we used to access the contents of auxv through a data pointer, but now we access it through data->data ().
>>
>> @@ -358,9 +356,8 @@ get_auxv_inferior_data (struct target_ops *ops)
>>      info = (struct auxv_info *) inferior_data (inf, auxv_inferior_data);
>>      if (info == NULL)
>>        {
>> -      info = XCNEW (struct auxv_info);
>> -      info->length = target_read_alloc (ops, TARGET_OBJECT_AUXV,
>> -                    NULL, &info->data);
>> +      info = new auxv_info;
>> +      info->data = target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL);
>>          set_inferior_data (inf, auxv_inferior_data, info);
>>        }
>>
>> So even before it seems that we would have cached a "negative" result.
>>
>> I believe your change will mean that we will keep asking over and over
>> for the auxv data if a target doesn't support it and that it will only
>> support positive caching.  I wonder if a recent change means that in
>> your test case something is trying to fetch the AUXV vector before the
>> target can read it, e.g. trying to fetch AT_HWCAP* when determining the
>> target description for a core file or the like?  It seems like for
>> the core target in corelow.c it should be fine to fetch AUXV data in
>> the gdbarch_core_read_description hook though.  Did you try setting a
>> breakpoint for when this function was called to see when it is being
>> cached?
> 
> Yes, we do try to fetch data from auxv before it exists. I think it's always been this way for this particular case.
> 
>>
>> I wonder if the issue is that parsing the symbol file without a core
>> is trying to fetch hwcap actually.  Presumably a breakpoint would let
>> you see that case?  Maybe the fetch of hwcap needs to be guarded by
>> something like "target_has_execution" or the like?
>>
> 
> No, it is trying to fetch AT_ENTRY to figure out the relocation that should be applied to the executable (if any).
> 
> When we finally load the (native) corefile, then we have access to the AUXV data through the corefile data.
> 
> This doesn't reproduce with a gcore corefile due to the fact that we now include the target description GDB was using when
> the corefile was generated.
> 
> Here's the backtrace:
> 
> --
> 
> #0  get_auxv_inferior_data (ops=0xaaaaad3b0760 <exec_ops>) at ../../../repos/binutils-gdb/gdb/auxv.c:359
> #1  0x0000aaaaab28cdc8 in target_auxv_search (ops=0xaaaaad3b0760 <exec_ops>, match=0x9, valp=0xffffffffe988)
>       at ../../../repos/binutils-gdb/gdb/auxv.c:381
> #2  0x0000aaaaab920110 in svr4_exec_displacement (displacementp=0xffffffffebc0) at ../../../repos/binutils-gdb/gdb/solib-svr4.c:2482
> #3  0x0000aaaaab920e4c in svr4_relocate_main_executable () at ../../../repos/binutils-gdb/gdb/solib-svr4.c:2878
> #4  0x0000aaaaab921008 in svr4_solib_create_inferior_hook (from_tty=1) at ../../../repos/binutils-gdb/gdb/solib-svr4.c:2933
> #5  0x0000aaaaab9288a0 in solib_create_inferior_hook (from_tty=1) at ../../../repos/binutils-gdb/gdb/solib.c:1285
> #6  0x0000aaaaab9747cc in symbol_file_command (args=0xfffffffff6c6 "~/aarch64-mte-gcore", from_tty=1)
>       at ../../../repos/binutils-gdb/gdb/symfile.c:1655
> #7  0x0000aaaaab500fc4 in file_command (arg=0xfffffffff6c6 "~/aarch64-mte-gcore", from_tty=1) at ../../../repos/binutils-gdb/gdb/exec.c:555
> #8  0x0000aaaaab3519f0 in do_simple_func (args=0xfffffffff6c6 "~/aarch64-mte-gcore", from_tty=1, c=0xaaaaad786a60)
>       at ../../../repos/binutils-gdb/gdb/cli/cli-decode.c:95
> #9  0x0000aaaaab3572c8 in cmd_func (cmd=0xaaaaad786a60, args=0xfffffffff6c6 "~/aarch64-mte-gcore", from_tty=1)
>       at ../../../repos/binutils-gdb/gdb/cli/cli-decode.c:2516
> #10 0x0000aaaaab9f0ed4 in execute_command (p=0xfffffffff6d8 "e", from_tty=1) at ../../../repos/binutils-gdb/gdb/top.c:699
> #11 0x0000aaaaab68c75c in catch_command_errors (command=0xaaaaab9f0984 <execute_command(char const*, int)>,
>       arg=0xfffffffff6c1 "file ~/aarch64-mte-gcore", from_tty=1, do_bp_actions=true) at ../../../repos/binutils-gdb/gdb/main.c:513
> #12 0x0000aaaaab68c9ac in execute_cmdargs (cmdarg_vec=0xfffffffff080, file_type=CMDARG_FILE, cmd_type=CMDARG_COMMAND, ret=0xffffffffefd4)
>       at ../../../repos/binutils-gdb/gdb/main.c:608
> #13 0x0000aaaaab68df18 in captured_main_1 (context=0xfffffffff280) at ../../../repos/binutils-gdb/gdb/main.c:1298
> #14 0x0000aaaaab68e0c0 in captured_main (data=0xfffffffff280) at ../../../repos/binutils-gdb/gdb/main.c:1319
> #15 0x0000aaaaab68e11c in gdb_main (During symbol reading: const value length mismatch for 'sigall_set', got 128, expected 0
> args=0xfffffffff280) at ../../../repos/binutils-gdb/gdb/main.c:1344
> #16 0x0000aaaaab1655ec in main (argc=5, argv=0xfffffffff418) at ../../../repos/binutils-gdb/gdb/gdb.c:32
> 
> --
> 
> I think this was a latent bug and I just happened to run into it. It doesn't make sense to cache the output of the auxv from a target that
> doesn't have auxv data. But we don't want to keep requesting things over and over again.

Hmmm.  I think in some ways what we would like is something like:

if (!target_has_all_memory ())
    return;

before calling svr4_relocate_main_executable.  However, we don't yet have
a target_has_all_memory() wrapper function.  Remote, process_stratum (native),
and core file targets all have has_all_memory() overrides, but the exec
target does not.  We can't use target_has_memory() because the exec target
returns true for that.

It doesn't really make sense to relocate the main executable when we don't
have an address space.  Maybe that is the better question to ask if it's
askable?  That is, does the current inferior have a real address space, or
is it just a plain symbol file via the exec target without a real address
space yet.  Looking around though, there doesn't seem to be a good way to
ask that question.

Hmmm, I've dug a bit more though at this from a different angle in that
attaching the core file should be flushing the auxv cache and I'm curious
why it isn't.  The 'inferior_appeared' observer event (which clears the auxv
cache) should be firing when a core file is attached via 'inferior_attached'
from add_to_thread_list in corelow.c.

I wonder if the combined case on the command line doesn't trigger
inferior_appeared for some reason when opening the core file?  If so, fixing
that might fix a few other things as well.

In terms of the original patch, I find it a bit odd to replace the existing
info structure vs just replacing the data member and/or if we want to disable
negative caching we should perhaps avoid emplacing the object if the
fetch of the auxv data from the target fails.

-- 
John Baldwin