Re: [PATCH] gdb: fix auxv caching

[John Baldwin <jhb@FreeBSD.org>]

On 10/7/22 1:44 PM, Simon Marchi wrote:
> There's a flaw in the interaction of the auxv caching and the fact that
> target_auxv_search allows reading auxv from an arbitrary target_ops
> (passed in as a parameter).  This has consequences as explained in this
> thread:
> 
>    https://inbox.sourceware.org/gdb-patches/20220719144542.1478037-1-luis.machado@arm.com/
> 
> In summary, when loading an AArch64 core file with MTE support by
> passing the executable and core file names directly to GDB, we see the
> MTE info:
> 
>      $ ./gdb -nx --data-directory=data-directory -q aarch64-mte-gcore aarch64-mte-gcore.core
>      ...
>      Program terminated with signal SIGSEGV, Segmentation fault
>      Memory tag violation while accessing address 0x0000ffff8ef5e000
>      Allocation tag 0x1
>      Logical tag 0x0.
>      #0  0x0000aaaade3d0b4c in ?? ()
>      (gdb)
> 
> But if we do it as two separate commands (file and core) we don't:
> 
>      $ ./gdb -nx --data-directory=data-directory -q -ex "file aarch64-mte-gcore" -ex "core aarch64-mte-gcore.core"
>      ...
>      Program terminated with signal SIGSEGV, Segmentation fault.
>      #0  0x0000aaaade3d0b4c in ?? ()
>      (gdb)
> 
> The problem with the latter is that auxv data gets improperly cached
> between the two commands.  When executing the file command, auxv gets
> first queried here, when loading the executable:
> 
>      #0  target_auxv_search (ops=0x55555b842400 <exec_ops>, match=0x9, valp=0x7fffffffc5d0) at /home/simark/src/binutils-gdb/gdb/auxv.c:383
>      #1  0x0000555557e576f2 in svr4_exec_displacement (displacementp=0x7fffffffc8c0) at /home/simark/src/binutils-gdb/gdb/solib-svr4.c:2482
>      #2  0x0000555557e594d1 in svr4_relocate_main_executable () at /home/simark/src/binutils-gdb/gdb/solib-svr4.c:2878
>      #3  0x0000555557e5989e in svr4_solib_create_inferior_hook (from_tty=1) at /home/simark/src/binutils-gdb/gdb/solib-svr4.c:2933
>      #4  0x0000555557e6e49f in solib_create_inferior_hook (from_tty=1) at /home/simark/src/binutils-gdb/gdb/solib.c:1253
>      #5  0x0000555557f33e29 in symbol_file_command (args=0x7fffffffe01c "aarch64-mte-gcore", from_tty=1) at /home/simark/src/binutils-gdb/gdb/symfile.c:1655
>      #6  0x00005555573319c3 in file_command (arg=0x7fffffffe01c "aarch64-mte-gcore", from_tty=1) at /home/simark/src/binutils-gdb/gdb/exec.c:555
>      #7  0x0000555556e47185 in do_simple_func (args=0x7fffffffe01c "aarch64-mte-gcore", from_tty=1, c=0x612000047740) at /home/simark/src/binutils-gdb/gdb/cli/cli-decode.c:95
>      #8  0x0000555556e551c9 in cmd_func (cmd=0x612000047740, args=0x7fffffffe01c "aarch64-mte-gcore", from_tty=1) at /home/simark/src/binutils-gdb/gdb/cli/cli-decode.c:2543
>      #9  0x00005555580e63fd in execute_command (p=0x7fffffffe02c "e", from_tty=1) at /home/simark/src/binutils-gdb/gdb/top.c:692
>      #10 0x0000555557771913 in catch_command_errors (command=0x5555580e55ad <execute_command(char const*, int)>, arg=0x7fffffffe017 "file aarch64-mte-gcore", from_tty=1, do_bp_actions=true) at /home/simark/src/binutils-gdb/gdb/main.c:513
>      #11 0x0000555557771fba in execute_cmdargs (cmdarg_vec=0x7fffffffd570, file_type=CMDARG_FILE, cmd_type=CMDARG_COMMAND, ret=0x7fffffffd230) at /home/simark/src/binutils-gdb/gdb/main.c:608
>      #12 0x00005555577755ac in captured_main_1 (context=0x7fffffffda10) at /home/simark/src/binutils-gdb/gdb/main.c:1299
>      #13 0x0000555557775c2d in captured_main (data=0x7fffffffda10) at /home/simark/src/binutils-gdb/gdb/main.c:1320
>      #14 0x0000555557775cc2 in gdb_main (args=0x7fffffffda10) at /home/simark/src/binutils-gdb/gdb/main.c:1345
>      #15 0x00005555568bdcbe in main (argc=10, argv=0x7fffffffdba8) at /home/simark/src/binutils-gdb/gdb/gdb.c:32
> 
> Here, target_auxv_search is called on the inferior's target stack.  The
> target stack only contains the exec target, so the query returns empty
> auxv data.  This gets cached for that inferior in `auxv_inferior_data`.
> 
> In its constructor (before it is pushed to the inferior's target stack),
> the core_target needs to identify the right target description from the
> core, and for that asks the gdbarch to read a target description from
> the core file.  Because some implementations of
> gdbarch_core_read_description (such as AArch64's) need to read auxv data
> from the core in order to determine the right target description, the
> core_target passes a pointer to itself, allowing implementations to call
> target_auxv_search it.  However, because we have previously cached
> (empty) auxv data for that inferior, target_auxv_search searched that
> cached (empty) auxv data, not auxv data read from the core.  Remember
> that this data was obtained by reading auxv on the inferior's target
> stack, which only contained an exec target.
> 
> The problem I see is that while target_auxv_search offers the
> flexibility of reading from an arbitrary (passed as an argument) target,
> the caching doesn't do the distinction of which target is being queried,
> and where the cached data came from.  So, you could read auxv from a
> target A, it gets cached, then you try to read auxv from a target B, and
> it returns the cached data from target A.  That sounds wrong.  In our
> case, we expect to read different auxv data from the core target than
> what we have read from the target stack earlier, so it doesn't make
> sense to hit the cache in this case.
> 
> To fix this, I propose splitting the code paths that read auxv data from
> an inferior's target stack and those that read from a passed-in target.
> The code path that reads from the target stack will keep caching,
> whereas the one that reads from a passed-in target won't.  And since,
> searching in auxv data is independent from where this data came from,
> split the "read" part from the "search" part.
> 
>  From what I understand, auxv caching was introduced mostly to reduce
> latency on remote connections, when doing many queries.  With the change
> I propose, only the queries done while constructing the core_target
> end up not using cached auxv data.  This is fine, because there are just
> a handful of queries max, done at this point, and reading core files is
> local.

I think this approach is fine.  Having two variants of target_read_auxv is
a bit verbose, and I'm not sure it's abundantly clear to a new person when
to use one vs the other.  That said, these are used rarely, so probably
will intuit the right thing by looking at existing uses.  I agree with the
idea that the auxv reads during gdbarch_core_read_description should
effectively all be "raw" and uncached.

-- 
John Baldwin