public inbox for gdb-patches@sourceware.org
 help / color / mirror / Atom feed
* [PATCH] Update auxv cache when there is no auxv cached data
@ 2022-07-19 14:45 Luis Machado
  2022-07-25  9:42 ` [PING][PATCH] " Luis Machado
                   ` (3 more replies)
  0 siblings, 4 replies; 29+ messages in thread
From: Luis Machado @ 2022-07-19 14:45 UTC (permalink / raw)
  To: gdb-patches

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.

My guess is that these two modes of opening a symbol file + core file take
slightly different paths in GDB, and in the latter case where we issue
separate "file" and "core" commands, we first cache empty auxv data (no core
file data yet) and later we just return that empty data as if it were valid.

The following patch checks for an empty info->data field, and forces a re-fetch
of auxv data if that is the case.

With this patch, I see full passes for the gdb.arch/aarch64-mte-core testcase.
---
 gdb/auxv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/auxv.c b/gdb/auxv.c
index 8e175138f5d..0d6a860e5f4 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 || !info->data.has_value ())
     {
       info = auxv_inferior_data.emplace (inf);
       info->data = target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PING][PATCH] Update auxv cache when there is no auxv cached data
  2022-07-19 14:45 [PATCH] Update auxv cache when there is no auxv cached data Luis Machado
@ 2022-07-25  9:42 ` Luis Machado
  2022-07-25 16:05 ` [PATCH] " John Baldwin
                   ` (2 subsequent siblings)
  3 siblings, 0 replies; 29+ messages in thread
From: Luis Machado @ 2022-07-25  9:42 UTC (permalink / raw)
  To: gdb-patches

On 7/19/22 15:45, 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.
> 
> My guess is that these two modes of opening a symbol file + core file take
> slightly different paths in GDB, and in the latter case where we issue
> separate "file" and "core" commands, we first cache empty auxv data (no core
> file data yet) and later we just return that empty data as if it were valid.
> 
> The following patch checks for an empty info->data field, and forces a re-fetch
> of auxv data if that is the case.
> 
> With this patch, I see full passes for the gdb.arch/aarch64-mte-core testcase.
> ---
>   gdb/auxv.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/auxv.c b/gdb/auxv.c
> index 8e175138f5d..0d6a860e5f4 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 || !info->data.has_value ())
>       {
>         info = auxv_inferior_data.emplace (inf);
>         info->data = target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL);


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] Update auxv cache when there is no auxv cached data
  2022-07-19 14:45 [PATCH] Update auxv cache when there is no auxv cached data Luis Machado
  2022-07-25  9:42 ` [PING][PATCH] " Luis Machado
@ 2022-07-25 16:05 ` John Baldwin
  2022-07-25 18:03   ` Luis Machado
  2022-08-05 15:46 ` [PATCH] Update auxv cache when inferior pid is 0 (no inferior) Luis Machado
  2022-09-20 12:28 ` [PATCH] Invalidate auxv cache before creating a core target Luis Machado
  3 siblings, 1 reply; 29+ messages in thread
From: John Baldwin @ 2022-07-25 16:05 UTC (permalink / raw)
  To: Luis Machado, gdb-patches

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:

@@ -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?

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?

-- 
John Baldwin

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] Update auxv cache when there is no auxv cached data
  2022-07-25 16:05 ` [PATCH] " John Baldwin
@ 2022-07-25 18:03   ` Luis Machado
  2022-07-25 19:13     ` John Baldwin
  0 siblings, 1 reply; 29+ messages in thread
From: Luis Machado @ 2022-07-25 18:03 UTC (permalink / raw)
  To: John Baldwin, gdb-patches

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.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] Update auxv cache when there is no auxv cached data
  2022-07-25 18:03   ` Luis Machado
@ 2022-07-25 19:13     ` John Baldwin
  2022-08-02 15:05       ` Luis Machado
  0 siblings, 1 reply; 29+ messages in thread
From: John Baldwin @ 2022-07-25 19:13 UTC (permalink / raw)
  To: Luis Machado, gdb-patches

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] Update auxv cache when there is no auxv cached data
  2022-07-25 19:13     ` John Baldwin
@ 2022-08-02 15:05       ` Luis Machado
  2022-08-02 16:05         ` John Baldwin
  0 siblings, 1 reply; 29+ messages in thread
From: Luis Machado @ 2022-08-02 15:05 UTC (permalink / raw)
  To: John Baldwin, gdb-patches

On 7/25/22 20:13, John Baldwin wrote:
> 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.

I did some more investigation on this one, and there are a few more details here.

First, we try to fetch auxv for the exec target, which either shouldn't happen or should return something that
indicates we don't have an auxv entry.

Second, the initial inferior pid is 0, a dummy value.

> 
> 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.

Turns out we do call inferior_appeared and it invalidates the auxv cache, but there is something else going on.

> 
> 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.
> 

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.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] Update auxv cache when there is no auxv cached data
  2022-08-02 15:05       ` Luis Machado
@ 2022-08-02 16:05         ` John Baldwin
  0 siblings, 0 replies; 29+ messages in thread
From: John Baldwin @ 2022-08-02 16:05 UTC (permalink / raw)
  To: Luis Machado, gdb-patches

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH] Update auxv cache when inferior pid is 0 (no inferior)
  2022-07-19 14:45 [PATCH] Update auxv cache when there is no auxv cached data Luis Machado
  2022-07-25  9:42 ` [PING][PATCH] " Luis Machado
  2022-07-25 16:05 ` [PATCH] " John Baldwin
@ 2022-08-05 15:46 ` Luis Machado
  2022-08-11  9:05   ` [PING][PATCH] " Luis Machado
                     ` (5 more replies)
  2022-09-20 12:28 ` [PATCH] Invalidate auxv cache before creating a core target Luis Machado
  3 siblings, 6 replies; 29+ messages in thread
From: Luis Machado @ 2022-08-05 15:46 UTC (permalink / raw)
  To: gdb-patches

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 empty
auxv data for pid 0, which gets cached. This is triggered by attempting to
read auxv data for the exec target.

In the early stages of reading the core file, we're still using inferior pid
0, so when we attempt to read auxv to determine corefile features, we get the
cached empty data vector again. This breaks core_gdbarch setup.

The fix, suggested by John Baldwin, prevents caching auxv data for pid 0.
---
 gdb/auxv.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/gdb/auxv.c b/gdb/auxv.c
index 6154988f6dd..33a829a7573 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);
-- 
2.25.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PING][PATCH] Update auxv cache when inferior pid is 0 (no inferior)
  2022-08-05 15:46 ` [PATCH] Update auxv cache when inferior pid is 0 (no inferior) Luis Machado
@ 2022-08-11  9:05   ` Luis Machado
  2022-08-18 15:48   ` Luis Machado
                     ` (4 subsequent siblings)
  5 siblings, 0 replies; 29+ messages in thread
From: Luis Machado @ 2022-08-11  9:05 UTC (permalink / raw)
  To: gdb-patches

On 8/5/22 16:46, 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 empty
> auxv data for pid 0, which gets cached. This is triggered by attempting to
> read auxv data for the exec target.
> 
> In the early stages of reading the core file, we're still using inferior pid
> 0, so when we attempt to read auxv to determine corefile features, we get the
> cached empty data vector again. This breaks core_gdbarch setup.
> 
> The fix, suggested by John Baldwin, prevents caching auxv data for pid 0.
> ---
>   gdb/auxv.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/auxv.c b/gdb/auxv.c
> index 6154988f6dd..33a829a7573 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);


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PING][PATCH] Update auxv cache when inferior pid is 0 (no inferior)
  2022-08-05 15:46 ` [PATCH] Update auxv cache when inferior pid is 0 (no inferior) Luis Machado
  2022-08-11  9:05   ` [PING][PATCH] " Luis Machado
@ 2022-08-18 15:48   ` Luis Machado
  2022-09-01  9:29   ` Luis Machado
                     ` (3 subsequent siblings)
  5 siblings, 0 replies; 29+ messages in thread
From: Luis Machado @ 2022-08-18 15:48 UTC (permalink / raw)
  To: gdb-patches

On 8/5/22 16:46, 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 empty
> auxv data for pid 0, which gets cached. This is triggered by attempting to
> read auxv data for the exec target.
> 
> In the early stages of reading the core file, we're still using inferior pid
> 0, so when we attempt to read auxv to determine corefile features, we get the
> cached empty data vector again. This breaks core_gdbarch setup.
> 
> The fix, suggested by John Baldwin, prevents caching auxv data for pid 0.
> ---
>   gdb/auxv.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/auxv.c b/gdb/auxv.c
> index 6154988f6dd..33a829a7573 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);


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PING][PATCH] Update auxv cache when inferior pid is 0 (no inferior)
  2022-08-05 15:46 ` [PATCH] Update auxv cache when inferior pid is 0 (no inferior) Luis Machado
  2022-08-11  9:05   ` [PING][PATCH] " Luis Machado
  2022-08-18 15:48   ` Luis Machado
@ 2022-09-01  9:29   ` Luis Machado
  2022-09-07  8:20   ` Luis Machado
                     ` (2 subsequent siblings)
  5 siblings, 0 replies; 29+ messages in thread
From: Luis Machado @ 2022-09-01  9:29 UTC (permalink / raw)
  To: gdb-patches

On 8/5/22 16:46, 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 empty
> auxv data for pid 0, which gets cached. This is triggered by attempting to
> read auxv data for the exec target.
> 
> In the early stages of reading the core file, we're still using inferior pid
> 0, so when we attempt to read auxv to determine corefile features, we get the
> cached empty data vector again. This breaks core_gdbarch setup.
> 
> The fix, suggested by John Baldwin, prevents caching auxv data for pid 0.
> ---
>   gdb/auxv.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/auxv.c b/gdb/auxv.c
> index 6154988f6dd..33a829a7573 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);


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PING][PATCH] Update auxv cache when inferior pid is 0 (no inferior)
  2022-08-05 15:46 ` [PATCH] Update auxv cache when inferior pid is 0 (no inferior) Luis Machado
                     ` (2 preceding siblings ...)
  2022-09-01  9:29   ` Luis Machado
@ 2022-09-07  8:20   ` Luis Machado
  2022-09-12 12:48   ` Luis Machado
  2022-09-12 13:30   ` [PATCH] " Simon Marchi
  5 siblings, 0 replies; 29+ messages in thread
From: Luis Machado @ 2022-09-07  8:20 UTC (permalink / raw)
  To: gdb-patches

On 8/5/22 16:46, 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 empty
> auxv data for pid 0, which gets cached. This is triggered by attempting to
> read auxv data for the exec target.
> 
> In the early stages of reading the core file, we're still using inferior pid
> 0, so when we attempt to read auxv to determine corefile features, we get the
> cached empty data vector again. This breaks core_gdbarch setup.
> 
> The fix, suggested by John Baldwin, prevents caching auxv data for pid 0.
> ---
>   gdb/auxv.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/auxv.c b/gdb/auxv.c
> index 6154988f6dd..33a829a7573 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);


^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PING][PATCH] Update auxv cache when inferior pid is 0 (no inferior)
  2022-08-05 15:46 ` [PATCH] Update auxv cache when inferior pid is 0 (no inferior) Luis Machado
                     ` (3 preceding siblings ...)
  2022-09-07  8:20   ` Luis Machado
@ 2022-09-12 12:48   ` Luis Machado
  2022-09-12 13:30   ` [PATCH] " Simon Marchi
  5 siblings, 0 replies; 29+ messages in thread
From: Luis Machado @ 2022-09-12 12:48 UTC (permalink / raw)
  To: gdb-patches

On 8/5/22 16:46, 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 empty
> auxv data for pid 0, which gets cached. This is triggered by attempting to
> read auxv data for the exec target.
> 
> In the early stages of reading the core file, we're still using inferior pid
> 0, so when we attempt to read auxv to determine corefile features, we get the
> cached empty data vector again. This breaks core_gdbarch setup.
> 
> The fix, suggested by John Baldwin, prevents caching auxv data for pid 0.
> ---
>   gdb/auxv.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/gdb/auxv.c b/gdb/auxv.c
> index 6154988f6dd..33a829a7573 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);


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] Update auxv cache when inferior pid is 0 (no inferior)
  2022-08-05 15:46 ` [PATCH] Update auxv cache when inferior pid is 0 (no inferior) Luis Machado
                     ` (4 preceding siblings ...)
  2022-09-12 12:48   ` Luis Machado
@ 2022-09-12 13:30   ` Simon Marchi
  2022-09-12 13:53     ` John Baldwin
  5 siblings, 1 reply; 29+ messages in thread
From: Simon Marchi @ 2022-09-12 13:30 UTC (permalink / raw)
  To: Luis Machado, gdb-patches



On 2022-08-05 11:46, 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 empty
> auxv data for pid 0, which gets cached. This is triggered by attempting to
> read auxv data for the exec target.
> 
> In the early stages of reading the core file, we're still using inferior pid
> 0, so when we attempt to read auxv to determine corefile features, we get the
> cached empty data vector again. This breaks core_gdbarch setup.
> 
> The fix, suggested by John Baldwin, prevents caching auxv data for pid 0.

I read the thread where you discussed this with John, I'm not sure I
completely grasp the problem yet, but this doesn't feel like the right
fix.  It should be fine to cache the auxv data for an inferior with pid
0.  If the inferior's memory and the inferior's target stack don't
change between two invocations of get_auxv_inferior_data, there's no
reason for the auxv data to be different for both calls.  I think the
problem is more that we don't invalidate the data at the right time.

The first call is done when only the exec target is pushed.  The second
call is done when the core target is pushed on top of that.  It's
expected that the returned auxv data can be different for the two calls,
so the cache should be invalidated somewhere between them.

Simon

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] Update auxv cache when inferior pid is 0 (no inferior)
  2022-09-12 13:30   ` [PATCH] " Simon Marchi
@ 2022-09-12 13:53     ` John Baldwin
  2022-09-12 13:59       ` Luis Machado
  0 siblings, 1 reply; 29+ messages in thread
From: John Baldwin @ 2022-09-12 13:53 UTC (permalink / raw)
  To: Simon Marchi, Luis Machado, gdb-patches

On 9/12/22 2:30 PM, Simon Marchi via Gdb-patches wrote:
> 
> 
> On 2022-08-05 11:46, 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 empty
>> auxv data for pid 0, which gets cached. This is triggered by attempting to
>> read auxv data for the exec target.
>>
>> In the early stages of reading the core file, we're still using inferior pid
>> 0, so when we attempt to read auxv to determine corefile features, we get the
>> cached empty data vector again. This breaks core_gdbarch setup.
>>
>> The fix, suggested by John Baldwin, prevents caching auxv data for pid 0.
> 
> I read the thread where you discussed this with John, I'm not sure I
> completely grasp the problem yet, but this doesn't feel like the right
> fix.  It should be fine to cache the auxv data for an inferior with pid
> 0.  If the inferior's memory and the inferior's target stack don't
> change between two invocations of get_auxv_inferior_data, there's no
> reason for the auxv data to be different for both calls.  I think the
> problem is more that we don't invalidate the data at the right time.
> 
> The first call is done when only the exec target is pushed.  The second
> call is done when the core target is pushed on top of that.  It's
> expected that the returned auxv data can be different for the two calls,
> so the cache should be invalidated somewhere between them.

The problem is that the core target attaching is a multi-step process.
The auxv cache gets invalidated when the pid is changed from 0 to the
"real" value after reading the registers near the "end" of the core
target attach process.  However, in order to read the registers from the
core dump for some architectures (like Linux/AArch64), the
read_description_from_core gdbarch hook needs to be able to fetch auxv
data (specifically the AT_HWCAP bits).  This occurs while the pid is still
zero, so the old value from the exec target is still cached.

This complexity is already present in the way that we fetch an "initial"
gdbarch from the core file and then ask that gdbarch for a more detailed
target description that is used to then instantiate a second gdbarch
(the "actual" gdbarch to use for the core file).  The place to possibly
flush the auxv cache again would perhaps be just before invoking the
core read_description method.  This would need a new observer hook though
that the auxv code could hook into.

OTOH, pid 0 is rather special and short-lived, so caching auxv data for
it seems less important than caching it once a target is fully attached
to a core or running process, etc.

-- 
John Baldwin

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] Update auxv cache when inferior pid is 0 (no inferior)
  2022-09-12 13:53     ` John Baldwin
@ 2022-09-12 13:59       ` Luis Machado
  0 siblings, 0 replies; 29+ messages in thread
From: Luis Machado @ 2022-09-12 13:59 UTC (permalink / raw)
  To: John Baldwin, Simon Marchi, gdb-patches

On 9/12/22 14:53, John Baldwin wrote:
> On 9/12/22 2:30 PM, Simon Marchi via Gdb-patches wrote:
>>
>>
>> On 2022-08-05 11:46, 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 empty
>>> auxv data for pid 0, which gets cached. This is triggered by attempting to
>>> read auxv data for the exec target.
>>>
>>> In the early stages of reading the core file, we're still using inferior pid
>>> 0, so when we attempt to read auxv to determine corefile features, we get the
>>> cached empty data vector again. This breaks core_gdbarch setup.
>>>
>>> The fix, suggested by John Baldwin, prevents caching auxv data for pid 0.
>>
>> I read the thread where you discussed this with John, I'm not sure I
>> completely grasp the problem yet, but this doesn't feel like the right
>> fix.  It should be fine to cache the auxv data for an inferior with pid
>> 0.  If the inferior's memory and the inferior's target stack don't
>> change between two invocations of get_auxv_inferior_data, there's no
>> reason for the auxv data to be different for both calls.  I think the
>> problem is more that we don't invalidate the data at the right time.
>>
>> The first call is done when only the exec target is pushed.  The second
>> call is done when the core target is pushed on top of that.  It's
>> expected that the returned auxv data can be different for the two calls,
>> so the cache should be invalidated somewhere between them.
> 
> The problem is that the core target attaching is a multi-step process.
> The auxv cache gets invalidated when the pid is changed from 0 to the
> "real" value after reading the registers near the "end" of the core
> target attach process.  However, in order to read the registers from the
> core dump for some architectures (like Linux/AArch64), the
> read_description_from_core gdbarch hook needs to be able to fetch auxv
> data (specifically the AT_HWCAP bits).  This occurs while the pid is still
> zero, so the old value from the exec target is still cached.
> 
> This complexity is already present in the way that we fetch an "initial"
> gdbarch from the core file and then ask that gdbarch for a more detailed
> target description that is used to then instantiate a second gdbarch
> (the "actual" gdbarch to use for the core file).  The place to possibly
> flush the auxv cache again would perhaps be just before invoking the
> core read_description method.  This would need a new observer hook though
> that the auxv code could hook into.

That's what I was thinking about. If we need to invalidate it, it would be during
opening of the core file target.

> 
> OTOH, pid 0 is rather special and short-lived, so caching auxv data for
> it seems less important than caching it once a target is fully attached
> to a core or running process, etc.
> 

Also the fact that pid 0 (although a reasonable pid number) is really meant to be
"no pid", as opposed to a real pid number 0.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH] Invalidate auxv cache before creating a core target
  2022-07-19 14:45 [PATCH] Update auxv cache when there is no auxv cached data Luis Machado
                   ` (2 preceding siblings ...)
  2022-08-05 15:46 ` [PATCH] Update auxv cache when inferior pid is 0 (no inferior) Luis Machado
@ 2022-09-20 12:28 ` Luis Machado
  2022-09-20 17:49   ` John Baldwin
  2022-10-07 20:44   ` [PATCH] gdb: fix auxv caching Simon Marchi
  3 siblings, 2 replies; 29+ messages in thread
From: Luis Machado @ 2022-09-20 12:28 UTC (permalink / raw)
  To: gdb-patches

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 empty
auxv data for pid 0, which gets cached. This is triggered by attempting to
read auxv data for the exec target.

In the early stages of reading the core file, we're still using inferior pid
0, so when we attempt to read auxv to determine corefile features, we get the
cached empty data vector again. This breaks core_gdbarch setup.

To overcome this, make sure we invalidate the auxv data when creating a new core
target.
---
 gdb/auxv.c    |  4 ++--
 gdb/auxv.h    |  2 ++
 gdb/corelow.c | 15 +++++++++++++++
 3 files changed, 19 insertions(+), 2 deletions(-)

diff --git a/gdb/auxv.c b/gdb/auxv.c
index 6154988f6dd..2423f51f224 100644
--- a/gdb/auxv.c
+++ b/gdb/auxv.c
@@ -342,9 +342,9 @@ invalidate_auxv_cache_inf (struct inferior *inf)
   auxv_inferior_data.clear (inf);
 }
 
-/* Invalidate current inferior's auxv cache.  */
+/* See auxv.h */
 
-static void
+void
 invalidate_auxv_cache (void)
 {
   invalidate_auxv_cache_inf (current_inferior ());
diff --git a/gdb/auxv.h b/gdb/auxv.h
index a4801c34d2f..242bc2eff00 100644
--- a/gdb/auxv.h
+++ b/gdb/auxv.h
@@ -79,5 +79,7 @@ extern int fprint_target_auxv (struct ui_file *file, struct target_ops *ops);
 
 extern target_xfer_partial_ftype memory_xfer_auxv;
 
+/* Invalidate current inferior's auxv cache.  */
+void invalidate_auxv_cache (void);
 
 #endif
diff --git a/gdb/corelow.c b/gdb/corelow.c
index 293bc8d4f59..1c30c7d011c 100644
--- a/gdb/corelow.c
+++ b/gdb/corelow.c
@@ -53,6 +53,7 @@
 #include "gdbcmd.h"
 #include "xml-tdesc.h"
 #include "memtag.h"
+#include "auxv.h"
 
 #ifndef O_LARGEFILE
 #define O_LARGEFILE 0
@@ -515,6 +516,20 @@ core_target_open (const char *arg, int from_tty)
 
   current_program_space->cbfd = std::move (temp_bfd);
 
+  /* Some architectures identify features using the hwcap bits that
+     come from the auxv.  Before creating a new core target, make sure we
+     invalidate the auxv cache so we can get fresh data.
+
+     This is required when we have an exec target before loading the core
+     file.  Under such conditions the pid is 0, and the exec target may
+     attempt to fetch and cache the auxv.  This results in GDB caching
+     empty contents as the exec target doesn't have auxv data.
+
+     Given we haven't determined the pid yet (we will read it later), it can
+     still be 0 and simply fetching the auxv may return the cached empty
+     data.  This ultimately results in problems coming up with the proper
+     gdbarch data.  */
+  invalidate_auxv_cache ();
   core_target *target = new core_target ();
 
   /* Own the target until it is successfully pushed.  */
-- 
2.25.1


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] Invalidate auxv cache before creating a core target
  2022-09-20 12:28 ` [PATCH] Invalidate auxv cache before creating a core target Luis Machado
@ 2022-09-20 17:49   ` John Baldwin
  2022-10-07 20:44   ` [PATCH] gdb: fix auxv caching Simon Marchi
  1 sibling, 0 replies; 29+ messages in thread
From: John Baldwin @ 2022-09-20 17:49 UTC (permalink / raw)
  To: Luis Machado, gdb-patches

On 9/20/22 1:28 PM, Luis Machado 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 empty
> auxv data for pid 0, which gets cached. This is triggered by attempting to
> read auxv data for the exec target.
> 
> In the early stages of reading the core file, we're still using inferior pid
> 0, so when we attempt to read auxv to determine corefile features, we get the
> cached empty data vector again. This breaks core_gdbarch setup.
> 
> To overcome this, make sure we invalidate the auxv data when creating a new core
> target.
> ---
>   gdb/auxv.c    |  4 ++--
>   gdb/auxv.h    |  2 ++
>   gdb/corelow.c | 15 +++++++++++++++
>   3 files changed, 19 insertions(+), 2 deletions(-)
> 
> diff --git a/gdb/auxv.c b/gdb/auxv.c
> index 6154988f6dd..2423f51f224 100644
> --- a/gdb/auxv.c
> +++ b/gdb/auxv.c
> @@ -342,9 +342,9 @@ invalidate_auxv_cache_inf (struct inferior *inf)
>     auxv_inferior_data.clear (inf);
>   }
>   
> -/* Invalidate current inferior's auxv cache.  */
> +/* See auxv.h */
>   
> -static void
> +void
>   invalidate_auxv_cache (void)
>   {
>     invalidate_auxv_cache_inf (current_inferior ());
> diff --git a/gdb/auxv.h b/gdb/auxv.h
> index a4801c34d2f..242bc2eff00 100644
> --- a/gdb/auxv.h
> +++ b/gdb/auxv.h
> @@ -79,5 +79,7 @@ extern int fprint_target_auxv (struct ui_file *file, struct target_ops *ops);
>   
>   extern target_xfer_partial_ftype memory_xfer_auxv;
>   
> +/* Invalidate current inferior's auxv cache.  */
> +void invalidate_auxv_cache (void);
>   
>   #endif
> diff --git a/gdb/corelow.c b/gdb/corelow.c
> index 293bc8d4f59..1c30c7d011c 100644
> --- a/gdb/corelow.c
> +++ b/gdb/corelow.c
> @@ -53,6 +53,7 @@
>   #include "gdbcmd.h"
>   #include "xml-tdesc.h"
>   #include "memtag.h"
> +#include "auxv.h"
>   
>   #ifndef O_LARGEFILE
>   #define O_LARGEFILE 0
> @@ -515,6 +516,20 @@ core_target_open (const char *arg, int from_tty)
>   
>     current_program_space->cbfd = std::move (temp_bfd);
>   
> +  /* Some architectures identify features using the hwcap bits that
> +     come from the auxv.  Before creating a new core target, make sure we
> +     invalidate the auxv cache so we can get fresh data.
> +
> +     This is required when we have an exec target before loading the core
> +     file.  Under such conditions the pid is 0, and the exec target may
> +     attempt to fetch and cache the auxv.  This results in GDB caching
> +     empty contents as the exec target doesn't have auxv data.
> +
> +     Given we haven't determined the pid yet (we will read it later), it can
> +     still be 0 and simply fetching the auxv may return the cached empty
> +     data.  This ultimately results in problems coming up with the proper
> +     gdbarch data.  */
> +  invalidate_auxv_cache ();
>     core_target *target = new core_target ();
>   
>     /* Own the target until it is successfully pushed.  */

This looks good to me.

-- 
John Baldwin

^ permalink raw reply	[flat|nested] 29+ messages in thread

* [PATCH] gdb: fix auxv caching
  2022-09-20 12:28 ` [PATCH] Invalidate auxv cache before creating a core target Luis Machado
  2022-09-20 17:49   ` John Baldwin
@ 2022-10-07 20:44   ` Simon Marchi
  2022-10-07 21:43     ` John Baldwin
  2022-10-10  9:33     ` Luis Machado
  1 sibling, 2 replies; 29+ messages in thread
From: Simon Marchi @ 2022-10-07 20:44 UTC (permalink / raw)
  To: gdb-patches

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.

The changes to auxv functions are:

 - Introduce 2 target_read_auxv functions.  One reads from an explicit
   target_ops and doesn't do caching (to be used in
   gdbarch_core_read_description context).  The other takes no argument,
   reads from the current inferior's target stack (it looks just like a
   standard target function wrapper) and does caching.

   The first target_read_auxv actually replaces get_auxv_inferior_data,
   since it became a trivial wrapper around it.

 - Change the existing target_auxv_search to not read auxv data from the
   target, but to accept it as a parameter (a gdb::byte_vector).  This
   function doesn't care where the data came from, it just searches in
   it.  It still needs to take a target_ops and gdbarch to know how to
   parse auxv entries.

 - Add a convenience target_auxv_search overload that reads auxv
   data from the inferior's target stack and searches in it.  This
   overload is useful to replace the exist target_auxv_search calls that
   passed the `current_inferior ()->top_target ()` target and keep the
   call sites short.

 - Modify parse_auxv to accept a target_ops and gdbarch to use for
   parsing entries.  Not strictly related to the rest of this change,
   but it seems like a good change in the context.

Changes in architecture-specific files (tdep and nat):

 - In linux-tdep, linux_get_hwcap and linux_get_hwcap2 get split in two,
   similar to target_auxv_search.  One version receives auxv data,
   target and arch as parameters.  The other gets everything from the
   current inferior.  The latter is for convenience, to avoid making
   call sites too ugly.

 - Call sites of linux_get_hwcap and linux_get_hwcap2 are adjusted to
   use either of the new versions.  The call sites in
   gdbarch_core_read_description context explicitly read auxv data from
   the passed-in target and call the linux_get_hwcap{,2} function with
   parameters.  Other call sites use the versions without parameters.

 - Same idea for arm_fbsd_read_description_auxv.

 - Call sites of target_auxv_search that passed
   `current_inferior ()->top_target ()` are changed to use the
   target_auxv_search overload that works in the current inferior.

Change-Id: Ib775a220cf1e76443fb7da2fdff8fc631128fe66
---
 gdb/aarch64-linux-nat.c  |  6 +--
 gdb/aarch64-linux-tdep.c |  6 ++-
 gdb/arm-fbsd-nat.c       |  2 +-
 gdb/arm-fbsd-tdep.c      | 25 ++++++++---
 gdb/arm-fbsd-tdep.h      | 12 ++++-
 gdb/arm-linux-nat.c      |  2 +-
 gdb/arm-linux-tdep.c     |  3 +-
 gdb/auxv.c               | 94 ++++++++++++++++++++++------------------
 gdb/auxv.h               | 22 +++++++++-
 gdb/elfread.c            |  2 +-
 gdb/fbsd-tdep.c          |  7 ++-
 gdb/linux-tdep.c         | 58 ++++++++++++++++++-------
 gdb/linux-tdep.h         | 26 ++++++++---
 gdb/ppc-linux-nat.c      | 19 +++-----
 gdb/ppc-linux-tdep.c     |  3 +-
 gdb/rs6000-tdep.c        |  3 +-
 gdb/s390-linux-nat.c     |  2 +-
 gdb/s390-linux-tdep.c    |  3 +-
 gdb/solib-svr4.c         | 15 +++----
 gdb/sparc64-tdep.c       |  6 +--
 20 files changed, 200 insertions(+), 116 deletions(-)

diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
index eda79ec6d35c..caefcb364852 100644
--- a/gdb/aarch64-linux-nat.c
+++ b/gdb/aarch64-linux-nat.c
@@ -781,8 +781,8 @@ aarch64_linux_nat_target::read_description ()
   if (ret == 0)
     return aarch32_read_description ();
 
-  CORE_ADDR hwcap = linux_get_hwcap (this);
-  CORE_ADDR hwcap2 = linux_get_hwcap2 (this);
+  CORE_ADDR hwcap = linux_get_hwcap ();
+  CORE_ADDR hwcap2 = linux_get_hwcap2 ();
 
   aarch64_features features;
   features.vq = aarch64_sve_get_vq (tid);
@@ -918,7 +918,7 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
 bool
 aarch64_linux_nat_target::supports_memory_tagging ()
 {
-  return (linux_get_hwcap2 (this) & HWCAP2_MTE) != 0;
+  return (linux_get_hwcap2 () & HWCAP2_MTE) != 0;
 }
 
 /* Implement the "fetch_memtags" target_ops method.  */
diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index 15773c75da83..9ce3569a277c 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -33,6 +33,7 @@
 #include "target.h"
 #include "target/target.h"
 #include "expop.h"
+#include "auxv.h"
 
 #include "regcache.h"
 #include "regset.h"
@@ -779,8 +780,9 @@ aarch64_linux_core_read_description (struct gdbarch *gdbarch,
 				     struct target_ops *target, bfd *abfd)
 {
   asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
-  CORE_ADDR hwcap = linux_get_hwcap (target);
-  CORE_ADDR hwcap2 = linux_get_hwcap2 (target);
+  gdb::optional<gdb::byte_vector> auxv = target_read_auxv (target);
+  CORE_ADDR hwcap = linux_get_hwcap (auxv, target, gdbarch);
+  CORE_ADDR hwcap2 = linux_get_hwcap2 (auxv, target, gdbarch);
 
   aarch64_features features;
   features.vq = aarch64_linux_core_read_vq (gdbarch, abfd);
diff --git a/gdb/arm-fbsd-nat.c b/gdb/arm-fbsd-nat.c
index b161b7ed9080..bbd722ed9230 100644
--- a/gdb/arm-fbsd-nat.c
+++ b/gdb/arm-fbsd-nat.c
@@ -122,7 +122,7 @@ arm_fbsd_nat_target::read_description ()
 #ifdef PT_GETREGSET
   tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
 #endif
-  desc = arm_fbsd_read_description_auxv (this, tls);
+  desc = arm_fbsd_read_description_auxv (tls);
   if (desc == NULL)
     desc = this->beneath ()->read_description ();
   return desc;
diff --git a/gdb/arm-fbsd-tdep.c b/gdb/arm-fbsd-tdep.c
index 61c8f0cecad4..8944f7e9b758 100644
--- a/gdb/arm-fbsd-tdep.c
+++ b/gdb/arm-fbsd-tdep.c
@@ -190,15 +190,17 @@ arm_fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
 	&arm_fbsd_vfpregset, "VFP floating-point", cb_data);
 }
 
-/* Lookup a target description from a target's AT_HWCAP auxiliary
-   vector.  */
+/* See arm-fbsd-tdep.h.  */
 
 const struct target_desc *
-arm_fbsd_read_description_auxv (struct target_ops *target, bool tls)
+arm_fbsd_read_description_auxv (const gdb::optional<gdb::byte_vector> &auxv,
+				target_ops *target, gdbarch *gdbarch, bool tls)
 {
   CORE_ADDR arm_hwcap = 0;
 
-  if (target_auxv_search (target, AT_FREEBSD_HWCAP, &arm_hwcap) != 1)
+  if (!auxv.has_value ()
+      || target_auxv_search (*auxv, target, gdbarch, AT_FREEBSD_HWCAP,
+			     &arm_hwcap) != 1)
     return arm_read_description (ARM_FP_TYPE_NONE, tls);
 
   if (arm_hwcap & HWCAP_VFP)
@@ -215,6 +217,18 @@ arm_fbsd_read_description_auxv (struct target_ops *target, bool tls)
   return arm_read_description (ARM_FP_TYPE_NONE, tls);
 }
 
+/* See arm-fbsd-tdep.h.  */
+
+const struct target_desc *
+arm_fbsd_read_description_auxv (bool tls)
+{
+  gdb::optional<gdb::byte_vector> auxv = target_read_auxv ();
+  return arm_fbsd_read_description_auxv (auxv,
+					 current_inferior ()->top_target (),
+					 current_inferior ()->gdbarch,
+					 tls);
+}
+
 /* Implement the "core_read_description" gdbarch method.  */
 
 static const struct target_desc *
@@ -224,7 +238,8 @@ arm_fbsd_core_read_description (struct gdbarch *gdbarch,
 {
   asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
 
-  return arm_fbsd_read_description_auxv (target, tls != nullptr);
+  gdb::optional<gdb::byte_vector> auxv = target_read_auxv (target);
+  return arm_fbsd_read_description_auxv (auxv, target, gdbarch, tls != nullptr);
 }
 
 /* Implement the get_thread_local_address gdbarch method.  */
diff --git a/gdb/arm-fbsd-tdep.h b/gdb/arm-fbsd-tdep.h
index 193eb76df3c9..85d7b59d1362 100644
--- a/gdb/arm-fbsd-tdep.h
+++ b/gdb/arm-fbsd-tdep.h
@@ -42,7 +42,17 @@ extern const struct regset arm_fbsd_vfpregset;
 #define	HWCAP_VFPv3		0x00002000
 #define	HWCAP_VFPD32		0x00080000
 
+/* Lookup a target description based on the AT_HWCAP value in the auxv data
+   AUXV.  */
+
+extern const struct target_desc *
+  arm_fbsd_read_description_auxv (const gdb::optional<gdb::byte_vector> &auxv,
+				  target_ops *target, gdbarch *gdbarch,
+				  bool tls);
+
+/* Same as the above, but read the auxv data from the current inferior.  */
+
 extern const struct target_desc *
-arm_fbsd_read_description_auxv (struct target_ops *target, bool tls);
+  arm_fbsd_read_description_auxv (bool tls);
 
 #endif /* ARM_FBSD_TDEP_H */
diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
index 0188c78fe7a0..a8b582fbef32 100644
--- a/gdb/arm-linux-nat.c
+++ b/gdb/arm-linux-nat.c
@@ -531,7 +531,7 @@ ps_get_thread_area (struct ps_prochandle *ph,
 const struct target_desc *
 arm_linux_nat_target::read_description ()
 {
-  CORE_ADDR arm_hwcap = linux_get_hwcap (this);
+  CORE_ADDR arm_hwcap = linux_get_hwcap ();
 
   if (have_ptrace_getregset == TRIBOOL_UNKNOWN)
     {
diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
index 1feb69fe6dd1..9db52de1a5f5 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -732,7 +732,8 @@ arm_linux_core_read_description (struct gdbarch *gdbarch,
 				 struct target_ops *target,
 				 bfd *abfd)
 {
-  CORE_ADDR arm_hwcap = linux_get_hwcap (target);
+  gdb::optional<gdb::byte_vector> auxv = target_read_auxv (target);
+  CORE_ADDR arm_hwcap = linux_get_hwcap (auxv, target, gdbarch);
 
   if (arm_hwcap & HWCAP_VFP)
     {
diff --git a/gdb/auxv.c b/gdb/auxv.c
index 76fc821c07c3..5853437b0f24 100644
--- a/gdb/auxv.c
+++ b/gdb/auxv.c
@@ -307,23 +307,21 @@ svr4_auxv_parse (struct gdbarch *gdbarch, const gdb_byte **readptr,
 
 /* Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
 
-   Use the auxv_parse method from the current inferior's gdbarch, if defined,
-   else use the current inferior's target stack's auxv_parse.
+   Use the auxv_parse method from GDBARCH, if defined, else use the auxv_parse
+   method of OPS.
 
    Return 0 if *READPTR is already at the end of the buffer.
    Return -1 if there is insufficient buffer for a whole entry.
    Return 1 if an entry was read into *TYPEP and *VALP.  */
+
 static int
-parse_auxv (const gdb_byte **readptr, const gdb_byte *endptr, CORE_ADDR *typep,
-	    CORE_ADDR *valp)
+parse_auxv (target_ops *ops, gdbarch *gdbarch, const gdb_byte **readptr,
+	    const gdb_byte *endptr, CORE_ADDR *typep, CORE_ADDR *valp)
 {
-  struct gdbarch *gdbarch = target_gdbarch();
-
   if (gdbarch_auxv_parse_p (gdbarch))
     return gdbarch_auxv_parse (gdbarch, readptr, endptr, typep, valp);
 
-  return current_inferior ()->top_target ()->auxv_parse (readptr, endptr,
-							 typep, valp);
+  return ops->auxv_parse (readptr, endptr, typep, valp);
 }
 
 
@@ -354,45 +352,45 @@ invalidate_auxv_cache (void)
   invalidate_auxv_cache_inf (current_inferior ());
 }
 
-/* Fetch the auxv object from inferior INF.  If auxv is cached already,
-   return a pointer to the cache.  If not, fetch the auxv object from the
-   target and cache it.  This function always returns a valid INFO pointer.  */
+/* See auxv.h.  */
 
-static struct auxv_info *
-get_auxv_inferior_data (struct target_ops *ops)
+gdb::optional<gdb::byte_vector>
+target_read_auxv ()
 {
-  struct auxv_info *info;
-  struct inferior *inf = current_inferior ();
+  inferior *inf = current_inferior ();
+  auxv_info *info = auxv_inferior_data.get (inf);
 
-  info = auxv_inferior_data.get (inf);
-  if (info == NULL)
+  if (info == nullptr)
     {
       info = auxv_inferior_data.emplace (inf);
-      info->data = target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL);
+      info->data = target_read_alloc (inf->top_target (), TARGET_OBJECT_AUXV,
+				      nullptr);
     }
 
-  return info;
+  return info->data;
 }
 
-/* Extract the auxiliary vector entry with a_type matching MATCH.
-   Return zero if no such entry was found, or -1 if there was
-   an error getting the information.  On success, return 1 after
-   storing the entry's value field in *VALP.  */
-int
-target_auxv_search (struct target_ops *ops, CORE_ADDR match, CORE_ADDR *valp)
+/* See auxv.h.  */
+
+gdb::optional<gdb::byte_vector>
+target_read_auxv (target_ops *ops)
 {
-  CORE_ADDR type, val;
-  auxv_info *info = get_auxv_inferior_data (ops);
+  return target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL);
+}
 
-  if (!info->data)
-    return -1;
+/* See auxv.h.  */
 
-  const gdb_byte *data = info->data->data ();
+int
+target_auxv_search (const gdb::byte_vector &auxv, target_ops *ops,
+		    gdbarch *gdbarch, CORE_ADDR match, CORE_ADDR *valp)
+{
+  CORE_ADDR type, val;
+  const gdb_byte *data = auxv.data ();
   const gdb_byte *ptr = data;
-  size_t len = info->data->size ();
+  size_t len = auxv.size ();
 
   while (1)
-    switch (parse_auxv (&ptr, data + len, &type, &val))
+    switch (parse_auxv (ops, gdbarch, &ptr, data + len, &type, &val))
       {
       case 1:			/* Here's an entry, check it.  */
 	if (type == match)
@@ -406,10 +404,21 @@ target_auxv_search (struct target_ops *ops, CORE_ADDR match, CORE_ADDR *valp)
       default:			/* Bogosity.  */
 	return -1;
       }
-
-  /*NOTREACHED*/
 }
 
+/* See auxv.h.  */
+
+int
+target_auxv_search (CORE_ADDR match, CORE_ADDR *valp)
+{
+  gdb::optional<gdb::byte_vector> auxv = target_read_auxv ();
+
+  if (!auxv.has_value ())
+    return -1;
+
+  return target_auxv_search (*auxv, current_inferior ()->top_target (),
+			     current_inferior ()->gdbarch, match, valp);
+}
 
 /* Print the description of a single AUXV entry on the specified file.  */
 
@@ -551,21 +560,23 @@ default_print_auxv_entry (struct gdbarch *gdbarch, struct ui_file *file,
 /* Print the contents of the target's AUXV on the specified file.  */
 
 static int
-fprint_target_auxv (struct ui_file *file, struct target_ops *ops)
+fprint_target_auxv (struct ui_file *file)
 {
   struct gdbarch *gdbarch = target_gdbarch ();
   CORE_ADDR type, val;
   int ents = 0;
-  auxv_info *info = get_auxv_inferior_data (ops);
+  gdb::optional<gdb::byte_vector> auxv = target_read_auxv ();
 
-  if (!info->data)
+  if (!auxv.has_value ())
     return -1;
 
-  const gdb_byte *data = info->data->data ();
+  const gdb_byte *data = auxv->data ();
   const gdb_byte *ptr = data;
-  size_t len = info->data->size ();
+  size_t len = auxv->size ();
 
-  while (parse_auxv (&ptr, data + len, &type, &val) > 0)
+  while (parse_auxv (current_inferior ()->top_target (),
+		     current_inferior ()->gdbarch,
+		     &ptr, data + len, &type, &val) > 0)
     {
       gdbarch_print_auxv_entry (gdbarch, file, type, val);
       ++ents;
@@ -583,8 +594,7 @@ info_auxv_command (const char *cmd, int from_tty)
     error (_("The program has no auxiliary information now."));
   else
     {
-      int ents = fprint_target_auxv (gdb_stdout,
-				     current_inferior ()->top_target ());
+      int ents = fprint_target_auxv (gdb_stdout);
 
       if (ents < 0)
 	error (_("No auxiliary vector found, or failed reading it."));
diff --git a/gdb/auxv.h b/gdb/auxv.h
index ab2a5dee5f74..983e3bc9b0d9 100644
--- a/gdb/auxv.h
+++ b/gdb/auxv.h
@@ -46,13 +46,31 @@ extern int svr4_auxv_parse (struct gdbarch *gdbarch, const gdb_byte **readptr,
 			    const gdb_byte *endptr, CORE_ADDR *typep,
 			    CORE_ADDR *valp);
 
-/* Extract the auxiliary vector entry with a_type matching MATCH.
+/* Read auxv data from the current inferior's target stack.  */
+
+extern gdb::optional<gdb::byte_vector> target_read_auxv ();
+
+/* Read auxv data from OPS.  */
+
+extern gdb::optional<gdb::byte_vector> target_read_auxv (target_ops *ops);
+
+/* Search AUXV for an entry with a_type matching MATCH.
+
+   OPS and GDBARCH are the target and architecture to use to parse auxv entries.
+
    Return zero if no such entry was found, or -1 if there was
    an error getting the information.  On success, return 1 after
    storing the entry's value field in *VALP.  */
-extern int target_auxv_search (struct target_ops *ops,
+
+extern int target_auxv_search (const gdb::byte_vector &auxv,
+			       target_ops *ops, gdbarch *gdbarch,
 			       CORE_ADDR match, CORE_ADDR *valp);
 
+/* Same as the above, but read the auxv data from the current inferior.  Use
+   the current inferior's top target and arch to parse auxv entries.  */
+
+extern int target_auxv_search (CORE_ADDR match, CORE_ADDR *valp);
+
 /* Print a description of a single AUXV entry on the specified file.  */
 enum auxv_format { AUXV_FORMAT_DEC, AUXV_FORMAT_HEX, AUXV_FORMAT_STR };
 
diff --git a/gdb/elfread.c b/gdb/elfread.c
index 8aee634b44b5..1e8935302e8f 100644
--- a/gdb/elfread.c
+++ b/gdb/elfread.c
@@ -909,7 +909,7 @@ elf_gnu_ifunc_resolve_addr (struct gdbarch *gdbarch, CORE_ADDR pc)
      parameter.  FUNCTION is the function entry address.  ADDRESS may be a
      function descriptor.  */
 
-  target_auxv_search (current_inferior ()->top_target (), AT_HWCAP, &hwcap);
+  target_auxv_search (AT_HWCAP, &hwcap);
   hwcap_val = value_from_longest (builtin_type (gdbarch)
 				  ->builtin_unsigned_long, hwcap);
   address_val = call_function_by_hand (function, NULL, hwcap_val);
diff --git a/gdb/fbsd-tdep.c b/gdb/fbsd-tdep.c
index 309777c55f28..8431caf8f597 100644
--- a/gdb/fbsd-tdep.c
+++ b/gdb/fbsd-tdep.c
@@ -2308,9 +2308,7 @@ fbsd_vmmap_length (struct gdbarch *gdbarch, unsigned char *entries, size_t len,
 static bool
 fbsd_vdso_range (struct gdbarch *gdbarch, struct mem_range *range)
 {
-  struct target_ops *ops = current_inferior ()->top_target ();
-
-  if (target_auxv_search (ops, AT_FREEBSD_KPRELOAD, &range->start) <= 0)
+  if (target_auxv_search (AT_FREEBSD_KPRELOAD, &range->start) <= 0)
     return false;
 
   if (!target_has_execution ())
@@ -2337,7 +2335,8 @@ fbsd_vdso_range (struct gdbarch *gdbarch, struct mem_range *range)
     {
       /* Fetch the list of address space entries from the running target. */
       gdb::optional<gdb::byte_vector> buf =
-	target_read_alloc (ops, TARGET_OBJECT_FREEBSD_VMMAP, nullptr);
+	target_read_alloc (current_inferior ()->top_target (),
+			   TARGET_OBJECT_FREEBSD_VMMAP, nullptr);
       if (!buf || buf->empty ())
 	return false;
 
diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
index adf518023bbe..dccb45d73a8d 100644
--- a/gdb/linux-tdep.c
+++ b/gdb/linux-tdep.c
@@ -417,10 +417,9 @@ int
 linux_is_uclinux (void)
 {
   CORE_ADDR dummy;
-  target_ops *target = current_inferior ()->top_target ();
 
-  return (target_auxv_search (target, AT_NULL, &dummy) > 0
-	  && target_auxv_search (target, AT_PAGESZ, &dummy) == 0);
+  return (target_auxv_search (AT_NULL, &dummy) > 0
+	  && target_auxv_search (AT_PAGESZ, &dummy) == 0);
 }
 
 static int
@@ -2379,8 +2378,7 @@ linux_vsyscall_range_raw (struct gdbarch *gdbarch, struct mem_range *range)
   char filename[100];
   long pid;
 
-  if (target_auxv_search (current_inferior ()->top_target (),
-			  AT_SYSINFO_EHDR, &range->start) <= 0)
+  if (target_auxv_search (AT_SYSINFO_EHDR, &range->start) <= 0)
     return 0;
 
   /* It doesn't make sense to access the host's /proc when debugging a
@@ -2570,8 +2568,7 @@ linux_displaced_step_location (struct gdbarch *gdbarch)
      local-store address and is thus not usable as displaced stepping
      location.  The auxiliary vector gets us the PowerPC-side entry
      point address instead.  */
-  if (target_auxv_search (current_inferior ()->top_target (),
-			  AT_ENTRY, &addr) <= 0)
+  if (target_auxv_search (AT_ENTRY, &addr) <= 0)
     throw_error (NOT_SUPPORTED_ERROR,
 		 _("Cannot find AT_ENTRY auxiliary vector entry."));
 
@@ -2658,13 +2655,15 @@ linux_displaced_step_restore_all_in_ptid (inferior *parent_inf, ptid_t ptid)
   per_inferior->disp_step_bufs->restore_in_ptid (ptid);
 }
 
-/* See linux-tdep.h.  */
+/* Helper for linux_get_hwcap and linux_get_hwcap2.  */
 
-CORE_ADDR
-linux_get_hwcap (struct target_ops *target)
+static CORE_ADDR
+linux_get_hwcap_helper (const gdb::optional<gdb::byte_vector> &auxv,
+			target_ops *target, gdbarch *gdbarch, CORE_ADDR match)
 {
   CORE_ADDR field;
-  if (target_auxv_search (target, AT_HWCAP, &field) != 1)
+  if (!auxv.has_value ()
+      || target_auxv_search (*auxv, target, gdbarch, match, &field) != 1)
     return 0;
   return field;
 }
@@ -2672,12 +2671,39 @@ linux_get_hwcap (struct target_ops *target)
 /* See linux-tdep.h.  */
 
 CORE_ADDR
-linux_get_hwcap2 (struct target_ops *target)
+linux_get_hwcap (const gdb::optional<gdb::byte_vector> &auxv,
+		 target_ops *target, gdbarch *gdbarch)
 {
-  CORE_ADDR field;
-  if (target_auxv_search (target, AT_HWCAP2, &field) != 1)
-    return 0;
-  return field;
+  return linux_get_hwcap_helper (auxv, target, gdbarch, AT_HWCAP);
+}
+
+/* See linux-tdep.h.  */
+
+CORE_ADDR
+linux_get_hwcap ()
+{
+  return linux_get_hwcap (target_read_auxv (),
+			  current_inferior ()->top_target (),
+			  current_inferior ()->gdbarch);
+}
+
+/* See linux-tdep.h.  */
+
+CORE_ADDR
+linux_get_hwcap2 (const gdb::optional<gdb::byte_vector> &auxv,
+		  target_ops *target, gdbarch *gdbarch)
+{
+  return linux_get_hwcap_helper (auxv, target, gdbarch, AT_HWCAP2);
+}
+
+/* See linux-tdep.h.  */
+
+CORE_ADDR
+linux_get_hwcap2 ()
+{
+  return linux_get_hwcap2 (target_read_auxv (),
+			   current_inferior ()->top_target (),
+			   current_inferior ()->gdbarch);
 }
 
 /* Display whether the gcore command is using the
diff --git a/gdb/linux-tdep.h b/gdb/linux-tdep.h
index bb907f2c8f35..95cc29c828c2 100644
--- a/gdb/linux-tdep.h
+++ b/gdb/linux-tdep.h
@@ -90,13 +90,27 @@ extern void linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch,
 
 extern int linux_is_uclinux (void);
 
-/* Fetch the AT_HWCAP entry from the auxv vector for the given TARGET.  On
-   error, 0 is returned.  */
-extern CORE_ADDR linux_get_hwcap (struct target_ops *target);
+/* Fetch the AT_HWCAP entry from auxv data AUXV.  Use TARGET and GDBARCH to
+   parse auxv entries.
 
-/* Fetch the AT_HWCAP2 entry from the auxv vector for the given TARGET.  On
-   error, 0 is returned.  */
-extern CORE_ADDR linux_get_hwcap2 (struct target_ops *target);
+   On error, 0 is returned.  */
+extern CORE_ADDR linux_get_hwcap (const gdb::optional<gdb::byte_vector> &auxv,
+				  struct target_ops *target, gdbarch *gdbarch);
+
+/* Same as the above, but obtain all the inputs from the current inferior.  */
+
+extern CORE_ADDR linux_get_hwcap ();
+
+/* Fetch the AT_HWCAP2 entry from auxv data AUXV.  Use TARGET and GDBARCH to
+   parse auxv entries.
+
+   On error, 0 is returned.  */
+extern CORE_ADDR linux_get_hwcap2 (const gdb::optional<gdb::byte_vector> &auxv,
+				   struct target_ops *target, gdbarch *gdbarch);
+
+/* Same as the above, but obtain all the inputs from the current inferior.  */
+
+extern CORE_ADDR linux_get_hwcap2 ();
 
 /* Fetch (and possibly build) an appropriate `struct link_map_offsets'
    for ILP32 and LP64 Linux systems.  */
diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
index dfa81e19a79f..795bb298955f 100644
--- a/gdb/ppc-linux-nat.c
+++ b/gdb/ppc-linux-nat.c
@@ -1965,8 +1965,8 @@ ppc_linux_nat_target::read_description ()
 
   features.wordsize = ppc_linux_target_wordsize (tid);
 
-  CORE_ADDR hwcap = linux_get_hwcap (current_inferior ()->top_target ());
-  CORE_ADDR hwcap2 = linux_get_hwcap2 (current_inferior ()->top_target ());
+  CORE_ADDR hwcap = linux_get_hwcap ();
+  CORE_ADDR hwcap2 = linux_get_hwcap2 ();
 
   if (have_ptrace_getsetvsxregs
       && (hwcap & PPC_FEATURE_HAS_VSX))
@@ -2123,8 +2123,7 @@ ppc_linux_nat_target::region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
 	 takes two hardware watchpoints though.  */
       if (len > 1
 	  && hwdebug_info.features & PPC_DEBUG_FEATURE_DATA_BP_RANGE
-	  && (linux_get_hwcap (current_inferior ()->top_target ())
-	      & PPC_FEATURE_BOOKE))
+	  && (linux_get_hwcap () & PPC_FEATURE_BOOKE))
 	return 2;
       /* Check if the processor provides DAWR interface.  */
       if (hwdebug_info.features & PPC_DEBUG_FEATURE_DATA_BP_DAWR)
@@ -2152,8 +2151,7 @@ ppc_linux_nat_target::region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
     {
       gdb_assert (m_dreg_interface.debugreg_p ());
 
-      if (((linux_get_hwcap (current_inferior ()->top_target ())
-	    & PPC_FEATURE_BOOKE)
+      if (((linux_get_hwcap () & PPC_FEATURE_BOOKE)
 	   && (addr + len) > (addr & ~3) + 4)
 	  || (addr + len) > (addr & ~7) + 8)
 	return 0;
@@ -2640,8 +2638,7 @@ ppc_linux_nat_target::insert_watchpoint (CORE_ADDR addr, int len,
       long wp_value;
       long read_mode, write_mode;
 
-      if (linux_get_hwcap (current_inferior ()->top_target ())
-	  & PPC_FEATURE_BOOKE)
+      if (linux_get_hwcap () & PPC_FEATURE_BOOKE)
 	{
 	  /* PowerPC 440 requires only the read/write flags to be passed
 	     to the kernel.  */
@@ -3014,11 +3011,9 @@ ppc_linux_nat_target::watchpoint_addr_within_range (CORE_ADDR addr,
   int mask;
 
   if (m_dreg_interface.hwdebug_p ()
-      && (linux_get_hwcap (current_inferior ()->top_target ())
-	  & PPC_FEATURE_BOOKE))
+      && (linux_get_hwcap () & PPC_FEATURE_BOOKE))
     return start <= addr && start + length >= addr;
-  else if (linux_get_hwcap (current_inferior ()->top_target ())
-	   & PPC_FEATURE_BOOKE)
+  else if (linux_get_hwcap () & PPC_FEATURE_BOOKE)
     mask = 3;
   else
     mask = 7;
diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
index 96eb931743fd..3bc972dfc9aa 100644
--- a/gdb/ppc-linux-tdep.c
+++ b/gdb/ppc-linux-tdep.c
@@ -1599,7 +1599,8 @@ ppc_linux_core_read_description (struct gdbarch *gdbarch,
   if (vsx)
     features.vsx = true;
 
-  CORE_ADDR hwcap = linux_get_hwcap (target);
+  gdb::optional<gdb::byte_vector> auxv = target_read_auxv (target);
+  CORE_ADDR hwcap = linux_get_hwcap (auxv, target, gdbarch);
 
   features.isa205 = ppc_linux_has_isa205 (hwcap);
 
diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
index 2a396eff4c49..bbdc6f6e5555 100644
--- a/gdb/rs6000-tdep.c
+++ b/gdb/rs6000-tdep.c
@@ -5503,8 +5503,7 @@ ppc_process_record_op31 (struct gdbarch *gdbarch, struct regcache *regcache,
       return 0;
 
     case 1014:		/* Data Cache Block set to Zero */
-      if (target_auxv_search (current_inferior ()->top_target (),
-			      AT_DCACHEBSIZE, &at_dcsz) <= 0
+      if (target_auxv_search (AT_DCACHEBSIZE, &at_dcsz) <= 0
 	  || at_dcsz == 0)
 	at_dcsz = 128; /* Assume 128-byte cache line size (POWER8)  */
 
diff --git a/gdb/s390-linux-nat.c b/gdb/s390-linux-nat.c
index 2b21e0822362..96833e804e9a 100644
--- a/gdb/s390-linux-nat.c
+++ b/gdb/s390-linux-nat.c
@@ -1002,7 +1002,7 @@ s390_linux_nat_target::read_description ()
      that mode, report s390 architecture with 64-bit GPRs.  */
 #ifdef __s390x__
   {
-    CORE_ADDR hwcap = linux_get_hwcap (current_inferior ()->top_target ());
+    CORE_ADDR hwcap = linux_get_hwcap ();
 
     have_regset_tdb = (hwcap & HWCAP_S390_TE)
       && check_regset (tid, NT_S390_TDB, s390_sizeof_tdbregset);
diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
index 05bf03973fc3..b8f4362c0d9e 100644
--- a/gdb/s390-linux-tdep.c
+++ b/gdb/s390-linux-tdep.c
@@ -332,7 +332,8 @@ s390_core_read_description (struct gdbarch *gdbarch,
 			    struct target_ops *target, bfd *abfd)
 {
   asection *section = bfd_get_section_by_name (abfd, ".reg");
-  CORE_ADDR hwcap = linux_get_hwcap (target);
+  gdb::optional<gdb::byte_vector> auxv = target_read_auxv (target);
+  CORE_ADDR hwcap = linux_get_hwcap (auxv, target, gdbarch);
   bool high_gprs, v1, v2, te, vx, gs;
 
   if (!section)
diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
index a6a9ec5c86b3..db71cd31d33f 100644
--- a/gdb/solib-svr4.c
+++ b/gdb/solib-svr4.c
@@ -428,14 +428,11 @@ read_program_header (int type, int *p_arch_size, CORE_ADDR *base_addr)
   int pt_phdr_p = 0;
 
   /* Get required auxv elements from target.  */
-  if (target_auxv_search (current_inferior ()->top_target (),
-			  AT_PHDR, &at_phdr) <= 0)
+  if (target_auxv_search (AT_PHDR, &at_phdr) <= 0)
     return {};
-  if (target_auxv_search (current_inferior ()->top_target (),
-			  AT_PHENT, &at_phent) <= 0)
+  if (target_auxv_search (AT_PHENT, &at_phent) <= 0)
     return {};
-  if (target_auxv_search (current_inferior ()->top_target (),
-			  AT_PHNUM, &at_phnum) <= 0)
+  if (target_auxv_search (AT_PHNUM, &at_phnum) <= 0)
     return {};
   if (!at_phdr || !at_phnum)
     return {};
@@ -2250,8 +2247,7 @@ enable_break (struct svr4_info *info, int from_tty)
       /* If we were not able to find the base address of the loader
 	 from our so_list, then try using the AT_BASE auxilliary entry.  */
       if (!load_addr_found)
-	if (target_auxv_search (current_inferior ()->top_target (),
-				AT_BASE, &load_addr) > 0)
+	if (target_auxv_search (AT_BASE, &load_addr) > 0)
 	  {
 	    int addr_bit = gdbarch_addr_bit (target_gdbarch ());
 
@@ -2479,8 +2475,7 @@ svr4_exec_displacement (CORE_ADDR *displacementp)
   if ((bfd_get_file_flags (current_program_space->exec_bfd ()) & DYNAMIC) == 0)
     return 0;
 
-  if (target_auxv_search (current_inferior ()->top_target (),
-			  AT_ENTRY, &entry_point) <= 0)
+  if (target_auxv_search (AT_ENTRY, &entry_point) <= 0)
     return 0;
 
   exec_displacement
diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c
index 6b9d9eaa9575..3122c62a1c38 100644
--- a/gdb/sparc64-tdep.c
+++ b/gdb/sparc64-tdep.c
@@ -214,12 +214,10 @@ adi_available (void)
     return proc->stat.is_avail;
 
   proc->stat.checked_avail = true;
-  if (target_auxv_search (current_inferior ()->top_target (),
-			  AT_ADI_BLKSZ, &value) <= 0)
+  if (target_auxv_search (AT_ADI_BLKSZ, &value) <= 0)
     return false;
   proc->stat.blksize = value;
-  target_auxv_search (current_inferior ()->top_target (),
-		      AT_ADI_NBITS, &value);
+  target_auxv_search (AT_ADI_NBITS, &value);
   proc->stat.nbits = value;
   proc->stat.max_version = (1 << proc->stat.nbits) - 2;
   proc->stat.is_avail = true;

base-commit: ae17d05a4a58baf42f297dfd40ed29256f4bc44d
-- 
2.37.3


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] gdb: fix auxv caching
  2022-10-07 20:44   ` [PATCH] gdb: fix auxv caching Simon Marchi
@ 2022-10-07 21:43     ` John Baldwin
  2022-10-09  0:39       ` Simon Marchi
  2022-10-10  9:33     ` Luis Machado
  1 sibling, 1 reply; 29+ messages in thread
From: John Baldwin @ 2022-10-07 21:43 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches

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

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] gdb: fix auxv caching
  2022-10-07 21:43     ` John Baldwin
@ 2022-10-09  0:39       ` Simon Marchi
  2022-10-10 18:32         ` John Baldwin
  2022-10-11 20:31         ` Pedro Alves
  0 siblings, 2 replies; 29+ messages in thread
From: Simon Marchi @ 2022-10-09  0:39 UTC (permalink / raw)
  To: John Baldwin, gdb-patches


> 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.

The second one is perhaps not essential, call sites could call

  target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL)

themselves.  But I find it convenient to have this little wrapper.

As to how to know which overload to call, perhaps that can be improved
with better documentation and comments.  I'm not sure what to add
though, the problem is so fresh in my mind that it's obvious to me.  So
I'm open to suggestions.

Simon

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] gdb: fix auxv caching
  2022-10-07 20:44   ` [PATCH] gdb: fix auxv caching Simon Marchi
  2022-10-07 21:43     ` John Baldwin
@ 2022-10-10  9:33     ` Luis Machado
  2022-10-11 17:53       ` Simon Marchi
  1 sibling, 1 reply; 29+ messages in thread
From: Luis Machado @ 2022-10-10  9:33 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches

On 10/7/22 21:44, 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.
> 
> The changes to auxv functions are:
> 
>   - Introduce 2 target_read_auxv functions.  One reads from an explicit
>     target_ops and doesn't do caching (to be used in
>     gdbarch_core_read_description context).  The other takes no argument,
>     reads from the current inferior's target stack (it looks just like a
>     standard target function wrapper) and does caching.
> 
>     The first target_read_auxv actually replaces get_auxv_inferior_data,
>     since it became a trivial wrapper around it.
> 
>   - Change the existing target_auxv_search to not read auxv data from the
>     target, but to accept it as a parameter (a gdb::byte_vector).  This
>     function doesn't care where the data came from, it just searches in
>     it.  It still needs to take a target_ops and gdbarch to know how to
>     parse auxv entries.
> 
>   - Add a convenience target_auxv_search overload that reads auxv
>     data from the inferior's target stack and searches in it.  This
>     overload is useful to replace the exist target_auxv_search calls that
>     passed the `current_inferior ()->top_target ()` target and keep the
>     call sites short.
> 
>   - Modify parse_auxv to accept a target_ops and gdbarch to use for
>     parsing entries.  Not strictly related to the rest of this change,
>     but it seems like a good change in the context.
> 
> Changes in architecture-specific files (tdep and nat):
> 
>   - In linux-tdep, linux_get_hwcap and linux_get_hwcap2 get split in two,
>     similar to target_auxv_search.  One version receives auxv data,
>     target and arch as parameters.  The other gets everything from the
>     current inferior.  The latter is for convenience, to avoid making
>     call sites too ugly.
> 
>   - Call sites of linux_get_hwcap and linux_get_hwcap2 are adjusted to
>     use either of the new versions.  The call sites in
>     gdbarch_core_read_description context explicitly read auxv data from
>     the passed-in target and call the linux_get_hwcap{,2} function with
>     parameters.  Other call sites use the versions without parameters.
> 
>   - Same idea for arm_fbsd_read_description_auxv.
> 
>   - Call sites of target_auxv_search that passed
>     `current_inferior ()->top_target ()` are changed to use the
>     target_auxv_search overload that works in the current inferior.
> 
> Change-Id: Ib775a220cf1e76443fb7da2fdff8fc631128fe66
> ---
>   gdb/aarch64-linux-nat.c  |  6 +--
>   gdb/aarch64-linux-tdep.c |  6 ++-
>   gdb/arm-fbsd-nat.c       |  2 +-
>   gdb/arm-fbsd-tdep.c      | 25 ++++++++---
>   gdb/arm-fbsd-tdep.h      | 12 ++++-
>   gdb/arm-linux-nat.c      |  2 +-
>   gdb/arm-linux-tdep.c     |  3 +-
>   gdb/auxv.c               | 94 ++++++++++++++++++++++------------------
>   gdb/auxv.h               | 22 +++++++++-
>   gdb/elfread.c            |  2 +-
>   gdb/fbsd-tdep.c          |  7 ++-
>   gdb/linux-tdep.c         | 58 ++++++++++++++++++-------
>   gdb/linux-tdep.h         | 26 ++++++++---
>   gdb/ppc-linux-nat.c      | 19 +++-----
>   gdb/ppc-linux-tdep.c     |  3 +-
>   gdb/rs6000-tdep.c        |  3 +-
>   gdb/s390-linux-nat.c     |  2 +-
>   gdb/s390-linux-tdep.c    |  3 +-
>   gdb/solib-svr4.c         | 15 +++----
>   gdb/sparc64-tdep.c       |  6 +--
>   20 files changed, 200 insertions(+), 116 deletions(-)
> 
> diff --git a/gdb/aarch64-linux-nat.c b/gdb/aarch64-linux-nat.c
> index eda79ec6d35c..caefcb364852 100644
> --- a/gdb/aarch64-linux-nat.c
> +++ b/gdb/aarch64-linux-nat.c
> @@ -781,8 +781,8 @@ aarch64_linux_nat_target::read_description ()
>     if (ret == 0)
>       return aarch32_read_description ();
>   
> -  CORE_ADDR hwcap = linux_get_hwcap (this);
> -  CORE_ADDR hwcap2 = linux_get_hwcap2 (this);
> +  CORE_ADDR hwcap = linux_get_hwcap ();
> +  CORE_ADDR hwcap2 = linux_get_hwcap2 ();
>   
>     aarch64_features features;
>     features.vq = aarch64_sve_get_vq (tid);
> @@ -918,7 +918,7 @@ aarch64_linux_nat_target::thread_architecture (ptid_t ptid)
>   bool
>   aarch64_linux_nat_target::supports_memory_tagging ()
>   {
> -  return (linux_get_hwcap2 (this) & HWCAP2_MTE) != 0;
> +  return (linux_get_hwcap2 () & HWCAP2_MTE) != 0;
>   }
>   
>   /* Implement the "fetch_memtags" target_ops method.  */
> diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
> index 15773c75da83..9ce3569a277c 100644
> --- a/gdb/aarch64-linux-tdep.c
> +++ b/gdb/aarch64-linux-tdep.c
> @@ -33,6 +33,7 @@
>   #include "target.h"
>   #include "target/target.h"
>   #include "expop.h"
> +#include "auxv.h"
>   
>   #include "regcache.h"
>   #include "regset.h"
> @@ -779,8 +780,9 @@ aarch64_linux_core_read_description (struct gdbarch *gdbarch,
>   				     struct target_ops *target, bfd *abfd)
>   {
>     asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
> -  CORE_ADDR hwcap = linux_get_hwcap (target);
> -  CORE_ADDR hwcap2 = linux_get_hwcap2 (target);
> +  gdb::optional<gdb::byte_vector> auxv = target_read_auxv (target);
> +  CORE_ADDR hwcap = linux_get_hwcap (auxv, target, gdbarch);
> +  CORE_ADDR hwcap2 = linux_get_hwcap2 (auxv, target, gdbarch);
>   
>     aarch64_features features;
>     features.vq = aarch64_linux_core_read_vq (gdbarch, abfd);
> diff --git a/gdb/arm-fbsd-nat.c b/gdb/arm-fbsd-nat.c
> index b161b7ed9080..bbd722ed9230 100644
> --- a/gdb/arm-fbsd-nat.c
> +++ b/gdb/arm-fbsd-nat.c
> @@ -122,7 +122,7 @@ arm_fbsd_nat_target::read_description ()
>   #ifdef PT_GETREGSET
>     tls = have_regset (inferior_ptid, NT_ARM_TLS) != 0;
>   #endif
> -  desc = arm_fbsd_read_description_auxv (this, tls);
> +  desc = arm_fbsd_read_description_auxv (tls);
>     if (desc == NULL)
>       desc = this->beneath ()->read_description ();
>     return desc;
> diff --git a/gdb/arm-fbsd-tdep.c b/gdb/arm-fbsd-tdep.c
> index 61c8f0cecad4..8944f7e9b758 100644
> --- a/gdb/arm-fbsd-tdep.c
> +++ b/gdb/arm-fbsd-tdep.c
> @@ -190,15 +190,17 @@ arm_fbsd_iterate_over_regset_sections (struct gdbarch *gdbarch,
>   	&arm_fbsd_vfpregset, "VFP floating-point", cb_data);
>   }
>   
> -/* Lookup a target description from a target's AT_HWCAP auxiliary
> -   vector.  */
> +/* See arm-fbsd-tdep.h.  */
>   
>   const struct target_desc *
> -arm_fbsd_read_description_auxv (struct target_ops *target, bool tls)
> +arm_fbsd_read_description_auxv (const gdb::optional<gdb::byte_vector> &auxv,
> +				target_ops *target, gdbarch *gdbarch, bool tls)
>   {
>     CORE_ADDR arm_hwcap = 0;
>   
> -  if (target_auxv_search (target, AT_FREEBSD_HWCAP, &arm_hwcap) != 1)
> +  if (!auxv.has_value ()
> +      || target_auxv_search (*auxv, target, gdbarch, AT_FREEBSD_HWCAP,
> +			     &arm_hwcap) != 1)
>       return arm_read_description (ARM_FP_TYPE_NONE, tls);
>   
>     if (arm_hwcap & HWCAP_VFP)
> @@ -215,6 +217,18 @@ arm_fbsd_read_description_auxv (struct target_ops *target, bool tls)
>     return arm_read_description (ARM_FP_TYPE_NONE, tls);
>   }
>   
> +/* See arm-fbsd-tdep.h.  */
> +
> +const struct target_desc *
> +arm_fbsd_read_description_auxv (bool tls)
> +{
> +  gdb::optional<gdb::byte_vector> auxv = target_read_auxv ();
> +  return arm_fbsd_read_description_auxv (auxv,
> +					 current_inferior ()->top_target (),
> +					 current_inferior ()->gdbarch,
> +					 tls);
> +}
> +
>   /* Implement the "core_read_description" gdbarch method.  */
>   
>   static const struct target_desc *
> @@ -224,7 +238,8 @@ arm_fbsd_core_read_description (struct gdbarch *gdbarch,
>   {
>     asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
>   
> -  return arm_fbsd_read_description_auxv (target, tls != nullptr);
> +  gdb::optional<gdb::byte_vector> auxv = target_read_auxv (target);
> +  return arm_fbsd_read_description_auxv (auxv, target, gdbarch, tls != nullptr);
>   }
>   
>   /* Implement the get_thread_local_address gdbarch method.  */
> diff --git a/gdb/arm-fbsd-tdep.h b/gdb/arm-fbsd-tdep.h
> index 193eb76df3c9..85d7b59d1362 100644
> --- a/gdb/arm-fbsd-tdep.h
> +++ b/gdb/arm-fbsd-tdep.h
> @@ -42,7 +42,17 @@ extern const struct regset arm_fbsd_vfpregset;
>   #define	HWCAP_VFPv3		0x00002000
>   #define	HWCAP_VFPD32		0x00080000
>   
> +/* Lookup a target description based on the AT_HWCAP value in the auxv data
> +   AUXV.  */
> +
> +extern const struct target_desc *
> +  arm_fbsd_read_description_auxv (const gdb::optional<gdb::byte_vector> &auxv,
> +				  target_ops *target, gdbarch *gdbarch,
> +				  bool tls);
> +
> +/* Same as the above, but read the auxv data from the current inferior.  */
> +
>   extern const struct target_desc *
> -arm_fbsd_read_description_auxv (struct target_ops *target, bool tls);
> +  arm_fbsd_read_description_auxv (bool tls);
>   
>   #endif /* ARM_FBSD_TDEP_H */
> diff --git a/gdb/arm-linux-nat.c b/gdb/arm-linux-nat.c
> index 0188c78fe7a0..a8b582fbef32 100644
> --- a/gdb/arm-linux-nat.c
> +++ b/gdb/arm-linux-nat.c
> @@ -531,7 +531,7 @@ ps_get_thread_area (struct ps_prochandle *ph,
>   const struct target_desc *
>   arm_linux_nat_target::read_description ()
>   {
> -  CORE_ADDR arm_hwcap = linux_get_hwcap (this);
> +  CORE_ADDR arm_hwcap = linux_get_hwcap ();
>   
>     if (have_ptrace_getregset == TRIBOOL_UNKNOWN)
>       {
> diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
> index 1feb69fe6dd1..9db52de1a5f5 100644
> --- a/gdb/arm-linux-tdep.c
> +++ b/gdb/arm-linux-tdep.c
> @@ -732,7 +732,8 @@ arm_linux_core_read_description (struct gdbarch *gdbarch,
>   				 struct target_ops *target,
>   				 bfd *abfd)
>   {
> -  CORE_ADDR arm_hwcap = linux_get_hwcap (target);
> +  gdb::optional<gdb::byte_vector> auxv = target_read_auxv (target);
> +  CORE_ADDR arm_hwcap = linux_get_hwcap (auxv, target, gdbarch);
>   
>     if (arm_hwcap & HWCAP_VFP)
>       {
> diff --git a/gdb/auxv.c b/gdb/auxv.c
> index 76fc821c07c3..5853437b0f24 100644
> --- a/gdb/auxv.c
> +++ b/gdb/auxv.c
> @@ -307,23 +307,21 @@ svr4_auxv_parse (struct gdbarch *gdbarch, const gdb_byte **readptr,
>   
>   /* Read one auxv entry from *READPTR, not reading locations >= ENDPTR.
>   
> -   Use the auxv_parse method from the current inferior's gdbarch, if defined,
> -   else use the current inferior's target stack's auxv_parse.
> +   Use the auxv_parse method from GDBARCH, if defined, else use the auxv_parse
> +   method of OPS.
>   
>      Return 0 if *READPTR is already at the end of the buffer.
>      Return -1 if there is insufficient buffer for a whole entry.
>      Return 1 if an entry was read into *TYPEP and *VALP.  */
> +
>   static int
> -parse_auxv (const gdb_byte **readptr, const gdb_byte *endptr, CORE_ADDR *typep,
> -	    CORE_ADDR *valp)
> +parse_auxv (target_ops *ops, gdbarch *gdbarch, const gdb_byte **readptr,
> +	    const gdb_byte *endptr, CORE_ADDR *typep, CORE_ADDR *valp)
>   {
> -  struct gdbarch *gdbarch = target_gdbarch();
> -
>     if (gdbarch_auxv_parse_p (gdbarch))
>       return gdbarch_auxv_parse (gdbarch, readptr, endptr, typep, valp);
>   
> -  return current_inferior ()->top_target ()->auxv_parse (readptr, endptr,
> -							 typep, valp);
> +  return ops->auxv_parse (readptr, endptr, typep, valp);
>   }
>   
>   
> @@ -354,45 +352,45 @@ invalidate_auxv_cache (void)
>     invalidate_auxv_cache_inf (current_inferior ());
>   }
>   
> -/* Fetch the auxv object from inferior INF.  If auxv is cached already,
> -   return a pointer to the cache.  If not, fetch the auxv object from the
> -   target and cache it.  This function always returns a valid INFO pointer.  */
> +/* See auxv.h.  */
>   
> -static struct auxv_info *
> -get_auxv_inferior_data (struct target_ops *ops)
> +gdb::optional<gdb::byte_vector>
> +target_read_auxv ()
>   {
> -  struct auxv_info *info;
> -  struct inferior *inf = current_inferior ();
> +  inferior *inf = current_inferior ();
> +  auxv_info *info = auxv_inferior_data.get (inf);
>   
> -  info = auxv_inferior_data.get (inf);
> -  if (info == NULL)
> +  if (info == nullptr)
>       {
>         info = auxv_inferior_data.emplace (inf);
> -      info->data = target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL);
> +      info->data = target_read_alloc (inf->top_target (), TARGET_OBJECT_AUXV,
> +				      nullptr);
>       }
>   
> -  return info;
> +  return info->data;
>   }
>   
> -/* Extract the auxiliary vector entry with a_type matching MATCH.
> -   Return zero if no such entry was found, or -1 if there was
> -   an error getting the information.  On success, return 1 after
> -   storing the entry's value field in *VALP.  */
> -int
> -target_auxv_search (struct target_ops *ops, CORE_ADDR match, CORE_ADDR *valp)
> +/* See auxv.h.  */
> +
> +gdb::optional<gdb::byte_vector>
> +target_read_auxv (target_ops *ops)
>   {
> -  CORE_ADDR type, val;
> -  auxv_info *info = get_auxv_inferior_data (ops);
> +  return target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL);
> +}
>   
> -  if (!info->data)
> -    return -1;
> +/* See auxv.h.  */
>   
> -  const gdb_byte *data = info->data->data ();
> +int
> +target_auxv_search (const gdb::byte_vector &auxv, target_ops *ops,
> +		    gdbarch *gdbarch, CORE_ADDR match, CORE_ADDR *valp)
> +{
> +  CORE_ADDR type, val;
> +  const gdb_byte *data = auxv.data ();
>     const gdb_byte *ptr = data;
> -  size_t len = info->data->size ();
> +  size_t len = auxv.size ();
>   
>     while (1)
> -    switch (parse_auxv (&ptr, data + len, &type, &val))
> +    switch (parse_auxv (ops, gdbarch, &ptr, data + len, &type, &val))
>         {
>         case 1:			/* Here's an entry, check it.  */
>   	if (type == match)
> @@ -406,10 +404,21 @@ target_auxv_search (struct target_ops *ops, CORE_ADDR match, CORE_ADDR *valp)
>         default:			/* Bogosity.  */
>   	return -1;
>         }
> -
> -  /*NOTREACHED*/
>   }
>   
> +/* See auxv.h.  */
> +
> +int
> +target_auxv_search (CORE_ADDR match, CORE_ADDR *valp)
> +{
> +  gdb::optional<gdb::byte_vector> auxv = target_read_auxv ();
> +
> +  if (!auxv.has_value ())
> +    return -1;
> +
> +  return target_auxv_search (*auxv, current_inferior ()->top_target (),
> +			     current_inferior ()->gdbarch, match, valp);
> +}
>   
>   /* Print the description of a single AUXV entry on the specified file.  */
>   
> @@ -551,21 +560,23 @@ default_print_auxv_entry (struct gdbarch *gdbarch, struct ui_file *file,
>   /* Print the contents of the target's AUXV on the specified file.  */
>   
>   static int
> -fprint_target_auxv (struct ui_file *file, struct target_ops *ops)
> +fprint_target_auxv (struct ui_file *file)
>   {
>     struct gdbarch *gdbarch = target_gdbarch ();
>     CORE_ADDR type, val;
>     int ents = 0;
> -  auxv_info *info = get_auxv_inferior_data (ops);
> +  gdb::optional<gdb::byte_vector> auxv = target_read_auxv ();
>   
> -  if (!info->data)
> +  if (!auxv.has_value ())
>       return -1;
>   
> -  const gdb_byte *data = info->data->data ();
> +  const gdb_byte *data = auxv->data ();
>     const gdb_byte *ptr = data;
> -  size_t len = info->data->size ();
> +  size_t len = auxv->size ();
>   
> -  while (parse_auxv (&ptr, data + len, &type, &val) > 0)
> +  while (parse_auxv (current_inferior ()->top_target (),
> +		     current_inferior ()->gdbarch,
> +		     &ptr, data + len, &type, &val) > 0)
>       {
>         gdbarch_print_auxv_entry (gdbarch, file, type, val);
>         ++ents;
> @@ -583,8 +594,7 @@ info_auxv_command (const char *cmd, int from_tty)
>       error (_("The program has no auxiliary information now."));
>     else
>       {
> -      int ents = fprint_target_auxv (gdb_stdout,
> -				     current_inferior ()->top_target ());
> +      int ents = fprint_target_auxv (gdb_stdout);
>   
>         if (ents < 0)
>   	error (_("No auxiliary vector found, or failed reading it."));
> diff --git a/gdb/auxv.h b/gdb/auxv.h
> index ab2a5dee5f74..983e3bc9b0d9 100644
> --- a/gdb/auxv.h
> +++ b/gdb/auxv.h
> @@ -46,13 +46,31 @@ extern int svr4_auxv_parse (struct gdbarch *gdbarch, const gdb_byte **readptr,
>   			    const gdb_byte *endptr, CORE_ADDR *typep,
>   			    CORE_ADDR *valp);
>   
> -/* Extract the auxiliary vector entry with a_type matching MATCH.
> +/* Read auxv data from the current inferior's target stack.  */
> +
> +extern gdb::optional<gdb::byte_vector> target_read_auxv ();
> +
> +/* Read auxv data from OPS.  */
> +
> +extern gdb::optional<gdb::byte_vector> target_read_auxv (target_ops *ops);
> +
> +/* Search AUXV for an entry with a_type matching MATCH.
> +
> +   OPS and GDBARCH are the target and architecture to use to parse auxv entries.
> +
>      Return zero if no such entry was found, or -1 if there was
>      an error getting the information.  On success, return 1 after
>      storing the entry's value field in *VALP.  */
> -extern int target_auxv_search (struct target_ops *ops,
> +
> +extern int target_auxv_search (const gdb::byte_vector &auxv,
> +			       target_ops *ops, gdbarch *gdbarch,
>   			       CORE_ADDR match, CORE_ADDR *valp);
>   
> +/* Same as the above, but read the auxv data from the current inferior.  Use
> +   the current inferior's top target and arch to parse auxv entries.  */
> +
> +extern int target_auxv_search (CORE_ADDR match, CORE_ADDR *valp);
> +
>   /* Print a description of a single AUXV entry on the specified file.  */
>   enum auxv_format { AUXV_FORMAT_DEC, AUXV_FORMAT_HEX, AUXV_FORMAT_STR };
>   
> diff --git a/gdb/elfread.c b/gdb/elfread.c
> index 8aee634b44b5..1e8935302e8f 100644
> --- a/gdb/elfread.c
> +++ b/gdb/elfread.c
> @@ -909,7 +909,7 @@ elf_gnu_ifunc_resolve_addr (struct gdbarch *gdbarch, CORE_ADDR pc)
>        parameter.  FUNCTION is the function entry address.  ADDRESS may be a
>        function descriptor.  */
>   
> -  target_auxv_search (current_inferior ()->top_target (), AT_HWCAP, &hwcap);
> +  target_auxv_search (AT_HWCAP, &hwcap);
>     hwcap_val = value_from_longest (builtin_type (gdbarch)
>   				  ->builtin_unsigned_long, hwcap);
>     address_val = call_function_by_hand (function, NULL, hwcap_val);
> diff --git a/gdb/fbsd-tdep.c b/gdb/fbsd-tdep.c
> index 309777c55f28..8431caf8f597 100644
> --- a/gdb/fbsd-tdep.c
> +++ b/gdb/fbsd-tdep.c
> @@ -2308,9 +2308,7 @@ fbsd_vmmap_length (struct gdbarch *gdbarch, unsigned char *entries, size_t len,
>   static bool
>   fbsd_vdso_range (struct gdbarch *gdbarch, struct mem_range *range)
>   {
> -  struct target_ops *ops = current_inferior ()->top_target ();
> -
> -  if (target_auxv_search (ops, AT_FREEBSD_KPRELOAD, &range->start) <= 0)
> +  if (target_auxv_search (AT_FREEBSD_KPRELOAD, &range->start) <= 0)
>       return false;
>   
>     if (!target_has_execution ())
> @@ -2337,7 +2335,8 @@ fbsd_vdso_range (struct gdbarch *gdbarch, struct mem_range *range)
>       {
>         /* Fetch the list of address space entries from the running target. */
>         gdb::optional<gdb::byte_vector> buf =
> -	target_read_alloc (ops, TARGET_OBJECT_FREEBSD_VMMAP, nullptr);
> +	target_read_alloc (current_inferior ()->top_target (),
> +			   TARGET_OBJECT_FREEBSD_VMMAP, nullptr);
>         if (!buf || buf->empty ())
>   	return false;
>   
> diff --git a/gdb/linux-tdep.c b/gdb/linux-tdep.c
> index adf518023bbe..dccb45d73a8d 100644
> --- a/gdb/linux-tdep.c
> +++ b/gdb/linux-tdep.c
> @@ -417,10 +417,9 @@ int
>   linux_is_uclinux (void)
>   {
>     CORE_ADDR dummy;
> -  target_ops *target = current_inferior ()->top_target ();
>   
> -  return (target_auxv_search (target, AT_NULL, &dummy) > 0
> -	  && target_auxv_search (target, AT_PAGESZ, &dummy) == 0);
> +  return (target_auxv_search (AT_NULL, &dummy) > 0
> +	  && target_auxv_search (AT_PAGESZ, &dummy) == 0);
>   }
>   
>   static int
> @@ -2379,8 +2378,7 @@ linux_vsyscall_range_raw (struct gdbarch *gdbarch, struct mem_range *range)
>     char filename[100];
>     long pid;
>   
> -  if (target_auxv_search (current_inferior ()->top_target (),
> -			  AT_SYSINFO_EHDR, &range->start) <= 0)
> +  if (target_auxv_search (AT_SYSINFO_EHDR, &range->start) <= 0)
>       return 0;
>   
>     /* It doesn't make sense to access the host's /proc when debugging a
> @@ -2570,8 +2568,7 @@ linux_displaced_step_location (struct gdbarch *gdbarch)
>        local-store address and is thus not usable as displaced stepping
>        location.  The auxiliary vector gets us the PowerPC-side entry
>        point address instead.  */
> -  if (target_auxv_search (current_inferior ()->top_target (),
> -			  AT_ENTRY, &addr) <= 0)
> +  if (target_auxv_search (AT_ENTRY, &addr) <= 0)
>       throw_error (NOT_SUPPORTED_ERROR,
>   		 _("Cannot find AT_ENTRY auxiliary vector entry."));
>   
> @@ -2658,13 +2655,15 @@ linux_displaced_step_restore_all_in_ptid (inferior *parent_inf, ptid_t ptid)
>     per_inferior->disp_step_bufs->restore_in_ptid (ptid);
>   }
>   
> -/* See linux-tdep.h.  */
> +/* Helper for linux_get_hwcap and linux_get_hwcap2.  */
>   
> -CORE_ADDR
> -linux_get_hwcap (struct target_ops *target)
> +static CORE_ADDR
> +linux_get_hwcap_helper (const gdb::optional<gdb::byte_vector> &auxv,
> +			target_ops *target, gdbarch *gdbarch, CORE_ADDR match)
>   {
>     CORE_ADDR field;
> -  if (target_auxv_search (target, AT_HWCAP, &field) != 1)
> +  if (!auxv.has_value ()
> +      || target_auxv_search (*auxv, target, gdbarch, match, &field) != 1)
>       return 0;
>     return field;
>   }
> @@ -2672,12 +2671,39 @@ linux_get_hwcap (struct target_ops *target)
>   /* See linux-tdep.h.  */
>   
>   CORE_ADDR
> -linux_get_hwcap2 (struct target_ops *target)
> +linux_get_hwcap (const gdb::optional<gdb::byte_vector> &auxv,
> +		 target_ops *target, gdbarch *gdbarch)
>   {
> -  CORE_ADDR field;
> -  if (target_auxv_search (target, AT_HWCAP2, &field) != 1)
> -    return 0;
> -  return field;
> +  return linux_get_hwcap_helper (auxv, target, gdbarch, AT_HWCAP);
> +}
> +
> +/* See linux-tdep.h.  */
> +
> +CORE_ADDR
> +linux_get_hwcap ()
> +{
> +  return linux_get_hwcap (target_read_auxv (),
> +			  current_inferior ()->top_target (),
> +			  current_inferior ()->gdbarch);
> +}
> +
> +/* See linux-tdep.h.  */
> +
> +CORE_ADDR
> +linux_get_hwcap2 (const gdb::optional<gdb::byte_vector> &auxv,
> +		  target_ops *target, gdbarch *gdbarch)
> +{
> +  return linux_get_hwcap_helper (auxv, target, gdbarch, AT_HWCAP2);
> +}
> +
> +/* See linux-tdep.h.  */
> +
> +CORE_ADDR
> +linux_get_hwcap2 ()
> +{
> +  return linux_get_hwcap2 (target_read_auxv (),
> +			   current_inferior ()->top_target (),
> +			   current_inferior ()->gdbarch);
>   }
>   
>   /* Display whether the gcore command is using the
> diff --git a/gdb/linux-tdep.h b/gdb/linux-tdep.h
> index bb907f2c8f35..95cc29c828c2 100644
> --- a/gdb/linux-tdep.h
> +++ b/gdb/linux-tdep.h
> @@ -90,13 +90,27 @@ extern void linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch,
>   
>   extern int linux_is_uclinux (void);
>   
> -/* Fetch the AT_HWCAP entry from the auxv vector for the given TARGET.  On
> -   error, 0 is returned.  */
> -extern CORE_ADDR linux_get_hwcap (struct target_ops *target);
> +/* Fetch the AT_HWCAP entry from auxv data AUXV.  Use TARGET and GDBARCH to
> +   parse auxv entries.
>   
> -/* Fetch the AT_HWCAP2 entry from the auxv vector for the given TARGET.  On
> -   error, 0 is returned.  */
> -extern CORE_ADDR linux_get_hwcap2 (struct target_ops *target);
> +   On error, 0 is returned.  */
> +extern CORE_ADDR linux_get_hwcap (const gdb::optional<gdb::byte_vector> &auxv,
> +				  struct target_ops *target, gdbarch *gdbarch);
> +
> +/* Same as the above, but obtain all the inputs from the current inferior.  */
> +
> +extern CORE_ADDR linux_get_hwcap ();
> +
> +/* Fetch the AT_HWCAP2 entry from auxv data AUXV.  Use TARGET and GDBARCH to
> +   parse auxv entries.
> +
> +   On error, 0 is returned.  */
> +extern CORE_ADDR linux_get_hwcap2 (const gdb::optional<gdb::byte_vector> &auxv,
> +				   struct target_ops *target, gdbarch *gdbarch);
> +
> +/* Same as the above, but obtain all the inputs from the current inferior.  */
> +
> +extern CORE_ADDR linux_get_hwcap2 ();
>   
>   /* Fetch (and possibly build) an appropriate `struct link_map_offsets'
>      for ILP32 and LP64 Linux systems.  */
> diff --git a/gdb/ppc-linux-nat.c b/gdb/ppc-linux-nat.c
> index dfa81e19a79f..795bb298955f 100644
> --- a/gdb/ppc-linux-nat.c
> +++ b/gdb/ppc-linux-nat.c
> @@ -1965,8 +1965,8 @@ ppc_linux_nat_target::read_description ()
>   
>     features.wordsize = ppc_linux_target_wordsize (tid);
>   
> -  CORE_ADDR hwcap = linux_get_hwcap (current_inferior ()->top_target ());
> -  CORE_ADDR hwcap2 = linux_get_hwcap2 (current_inferior ()->top_target ());
> +  CORE_ADDR hwcap = linux_get_hwcap ();
> +  CORE_ADDR hwcap2 = linux_get_hwcap2 ();
>   
>     if (have_ptrace_getsetvsxregs
>         && (hwcap & PPC_FEATURE_HAS_VSX))
> @@ -2123,8 +2123,7 @@ ppc_linux_nat_target::region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
>   	 takes two hardware watchpoints though.  */
>         if (len > 1
>   	  && hwdebug_info.features & PPC_DEBUG_FEATURE_DATA_BP_RANGE
> -	  && (linux_get_hwcap (current_inferior ()->top_target ())
> -	      & PPC_FEATURE_BOOKE))
> +	  && (linux_get_hwcap () & PPC_FEATURE_BOOKE))
>   	return 2;
>         /* Check if the processor provides DAWR interface.  */
>         if (hwdebug_info.features & PPC_DEBUG_FEATURE_DATA_BP_DAWR)
> @@ -2152,8 +2151,7 @@ ppc_linux_nat_target::region_ok_for_hw_watchpoint (CORE_ADDR addr, int len)
>       {
>         gdb_assert (m_dreg_interface.debugreg_p ());
>   
> -      if (((linux_get_hwcap (current_inferior ()->top_target ())
> -	    & PPC_FEATURE_BOOKE)
> +      if (((linux_get_hwcap () & PPC_FEATURE_BOOKE)
>   	   && (addr + len) > (addr & ~3) + 4)
>   	  || (addr + len) > (addr & ~7) + 8)
>   	return 0;
> @@ -2640,8 +2638,7 @@ ppc_linux_nat_target::insert_watchpoint (CORE_ADDR addr, int len,
>         long wp_value;
>         long read_mode, write_mode;
>   
> -      if (linux_get_hwcap (current_inferior ()->top_target ())
> -	  & PPC_FEATURE_BOOKE)
> +      if (linux_get_hwcap () & PPC_FEATURE_BOOKE)
>   	{
>   	  /* PowerPC 440 requires only the read/write flags to be passed
>   	     to the kernel.  */
> @@ -3014,11 +3011,9 @@ ppc_linux_nat_target::watchpoint_addr_within_range (CORE_ADDR addr,
>     int mask;
>   
>     if (m_dreg_interface.hwdebug_p ()
> -      && (linux_get_hwcap (current_inferior ()->top_target ())
> -	  & PPC_FEATURE_BOOKE))
> +      && (linux_get_hwcap () & PPC_FEATURE_BOOKE))
>       return start <= addr && start + length >= addr;
> -  else if (linux_get_hwcap (current_inferior ()->top_target ())
> -	   & PPC_FEATURE_BOOKE)
> +  else if (linux_get_hwcap () & PPC_FEATURE_BOOKE)
>       mask = 3;
>     else
>       mask = 7;
> diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
> index 96eb931743fd..3bc972dfc9aa 100644
> --- a/gdb/ppc-linux-tdep.c
> +++ b/gdb/ppc-linux-tdep.c
> @@ -1599,7 +1599,8 @@ ppc_linux_core_read_description (struct gdbarch *gdbarch,
>     if (vsx)
>       features.vsx = true;
>   
> -  CORE_ADDR hwcap = linux_get_hwcap (target);
> +  gdb::optional<gdb::byte_vector> auxv = target_read_auxv (target);
> +  CORE_ADDR hwcap = linux_get_hwcap (auxv, target, gdbarch);
>   
>     features.isa205 = ppc_linux_has_isa205 (hwcap);
>   
> diff --git a/gdb/rs6000-tdep.c b/gdb/rs6000-tdep.c
> index 2a396eff4c49..bbdc6f6e5555 100644
> --- a/gdb/rs6000-tdep.c
> +++ b/gdb/rs6000-tdep.c
> @@ -5503,8 +5503,7 @@ ppc_process_record_op31 (struct gdbarch *gdbarch, struct regcache *regcache,
>         return 0;
>   
>       case 1014:		/* Data Cache Block set to Zero */
> -      if (target_auxv_search (current_inferior ()->top_target (),
> -			      AT_DCACHEBSIZE, &at_dcsz) <= 0
> +      if (target_auxv_search (AT_DCACHEBSIZE, &at_dcsz) <= 0
>   	  || at_dcsz == 0)
>   	at_dcsz = 128; /* Assume 128-byte cache line size (POWER8)  */
>   
> diff --git a/gdb/s390-linux-nat.c b/gdb/s390-linux-nat.c
> index 2b21e0822362..96833e804e9a 100644
> --- a/gdb/s390-linux-nat.c
> +++ b/gdb/s390-linux-nat.c
> @@ -1002,7 +1002,7 @@ s390_linux_nat_target::read_description ()
>        that mode, report s390 architecture with 64-bit GPRs.  */
>   #ifdef __s390x__
>     {
> -    CORE_ADDR hwcap = linux_get_hwcap (current_inferior ()->top_target ());
> +    CORE_ADDR hwcap = linux_get_hwcap ();
>   
>       have_regset_tdb = (hwcap & HWCAP_S390_TE)
>         && check_regset (tid, NT_S390_TDB, s390_sizeof_tdbregset);
> diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
> index 05bf03973fc3..b8f4362c0d9e 100644
> --- a/gdb/s390-linux-tdep.c
> +++ b/gdb/s390-linux-tdep.c
> @@ -332,7 +332,8 @@ s390_core_read_description (struct gdbarch *gdbarch,
>   			    struct target_ops *target, bfd *abfd)
>   {
>     asection *section = bfd_get_section_by_name (abfd, ".reg");
> -  CORE_ADDR hwcap = linux_get_hwcap (target);
> +  gdb::optional<gdb::byte_vector> auxv = target_read_auxv (target);
> +  CORE_ADDR hwcap = linux_get_hwcap (auxv, target, gdbarch);
>     bool high_gprs, v1, v2, te, vx, gs;
>   
>     if (!section)
> diff --git a/gdb/solib-svr4.c b/gdb/solib-svr4.c
> index a6a9ec5c86b3..db71cd31d33f 100644
> --- a/gdb/solib-svr4.c
> +++ b/gdb/solib-svr4.c
> @@ -428,14 +428,11 @@ read_program_header (int type, int *p_arch_size, CORE_ADDR *base_addr)
>     int pt_phdr_p = 0;
>   
>     /* Get required auxv elements from target.  */
> -  if (target_auxv_search (current_inferior ()->top_target (),
> -			  AT_PHDR, &at_phdr) <= 0)
> +  if (target_auxv_search (AT_PHDR, &at_phdr) <= 0)
>       return {};
> -  if (target_auxv_search (current_inferior ()->top_target (),
> -			  AT_PHENT, &at_phent) <= 0)
> +  if (target_auxv_search (AT_PHENT, &at_phent) <= 0)
>       return {};
> -  if (target_auxv_search (current_inferior ()->top_target (),
> -			  AT_PHNUM, &at_phnum) <= 0)
> +  if (target_auxv_search (AT_PHNUM, &at_phnum) <= 0)
>       return {};
>     if (!at_phdr || !at_phnum)
>       return {};
> @@ -2250,8 +2247,7 @@ enable_break (struct svr4_info *info, int from_tty)
>         /* If we were not able to find the base address of the loader
>   	 from our so_list, then try using the AT_BASE auxilliary entry.  */
>         if (!load_addr_found)
> -	if (target_auxv_search (current_inferior ()->top_target (),
> -				AT_BASE, &load_addr) > 0)
> +	if (target_auxv_search (AT_BASE, &load_addr) > 0)
>   	  {
>   	    int addr_bit = gdbarch_addr_bit (target_gdbarch ());
>   
> @@ -2479,8 +2475,7 @@ svr4_exec_displacement (CORE_ADDR *displacementp)
>     if ((bfd_get_file_flags (current_program_space->exec_bfd ()) & DYNAMIC) == 0)
>       return 0;
>   
> -  if (target_auxv_search (current_inferior ()->top_target (),
> -			  AT_ENTRY, &entry_point) <= 0)
> +  if (target_auxv_search (AT_ENTRY, &entry_point) <= 0)
>       return 0;
>   
>     exec_displacement
> diff --git a/gdb/sparc64-tdep.c b/gdb/sparc64-tdep.c
> index 6b9d9eaa9575..3122c62a1c38 100644
> --- a/gdb/sparc64-tdep.c
> +++ b/gdb/sparc64-tdep.c
> @@ -214,12 +214,10 @@ adi_available (void)
>       return proc->stat.is_avail;
>   
>     proc->stat.checked_avail = true;
> -  if (target_auxv_search (current_inferior ()->top_target (),
> -			  AT_ADI_BLKSZ, &value) <= 0)
> +  if (target_auxv_search (AT_ADI_BLKSZ, &value) <= 0)
>       return false;
>     proc->stat.blksize = value;
> -  target_auxv_search (current_inferior ()->top_target (),
> -		      AT_ADI_NBITS, &value);
> +  target_auxv_search (AT_ADI_NBITS, &value);
>     proc->stat.nbits = value;
>     proc->stat.max_version = (1 << proc->stat.nbits) - 2;
>     proc->stat.is_avail = true;
> 
> base-commit: ae17d05a4a58baf42f297dfd40ed29256f4bc44d

Thanks for the patch.

The code is a bit more verbose, but the reasoning makes sense to me. So LGTM.

I verified this on my end with mte corefiles and it works correctly.

As a general comment, it feels to me the auxv-handling code could use some C++-ification
to handle things in a better way.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] gdb: fix auxv caching
  2022-10-09  0:39       ` Simon Marchi
@ 2022-10-10 18:32         ` John Baldwin
  2022-10-11 17:52           ` Simon Marchi
  2022-10-11 20:31         ` Pedro Alves
  1 sibling, 1 reply; 29+ messages in thread
From: John Baldwin @ 2022-10-10 18:32 UTC (permalink / raw)
  To: Simon Marchi, gdb-patches

On 10/8/22 5:39 PM, Simon Marchi wrote:
> 
>> 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.
> 
> The second one is perhaps not essential, call sites could call
> 
>    target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL)
> 
> themselves.  But I find it convenient to have this little wrapper.
> 
> As to how to know which overload to call, perhaps that can be improved
> with better documentation and comments.  I'm not sure what to add
> though, the problem is so fresh in my mind that it's obvious to me.  So
> I'm open to suggestions.

I don't have any good suggestions.  Also, FWIW, the BSD bits are all
fine with me.

-- 
John Baldwin

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] gdb: fix auxv caching
  2022-10-10 18:32         ` John Baldwin
@ 2022-10-11 17:52           ` Simon Marchi
  0 siblings, 0 replies; 29+ messages in thread
From: Simon Marchi @ 2022-10-11 17:52 UTC (permalink / raw)
  To: John Baldwin, gdb-patches

On 10/10/22 14:32, John Baldwin wrote:
> On 10/8/22 5:39 PM, Simon Marchi wrote:
>>
>>> 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.
>>
>> The second one is perhaps not essential, call sites could call
>>
>>    target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL)
>>
>> themselves.  But I find it convenient to have this little wrapper.
>>
>> As to how to know which overload to call, perhaps that can be improved
>> with better documentation and comments.  I'm not sure what to add
>> though, the problem is so fresh in my mind that it's obvious to me.  So
>> I'm open to suggestions.
> 
> I don't have any good suggestions.  Also, FWIW, the BSD bits are all
> fine with me.
> 
> -- 
> John Baldwin

Ok, thanks.

Simon

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] gdb: fix auxv caching
  2022-10-10  9:33     ` Luis Machado
@ 2022-10-11 17:53       ` Simon Marchi
  0 siblings, 0 replies; 29+ messages in thread
From: Simon Marchi @ 2022-10-11 17:53 UTC (permalink / raw)
  To: Luis Machado, gdb-patches

> Thanks for the patch.
> 
> The code is a bit more verbose, but the reasoning makes sense to me. So LGTM.
> 
> I verified this on my end with mte corefiles and it works correctly.
Ok, thanks, I will push.

> As a general comment, it feels to me the auxv-handling code could use some C++-ification
> to handle things in a better way.

Indeed, a lot of things are :)

Simon


^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] gdb: fix auxv caching
  2022-10-09  0:39       ` Simon Marchi
  2022-10-10 18:32         ` John Baldwin
@ 2022-10-11 20:31         ` Pedro Alves
  2022-10-11 20:34           ` Pedro Alves
  1 sibling, 1 reply; 29+ messages in thread
From: Pedro Alves @ 2022-10-11 20:31 UTC (permalink / raw)
  To: Simon Marchi, John Baldwin, gdb-patches

On 2022-10-09 1:39 a.m., Simon Marchi via Gdb-patches wrote:
> 
>> 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.
> 
> The second one is perhaps not essential, call sites could call
> 
>   target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL)
> 
> themselves.  But I find it convenient to have this little wrapper.
> 
> As to how to know which overload to call, perhaps that can be improved
> with better documentation and comments.  I'm not sure what to add
> though, the problem is so fresh in my mind that it's obvious to me.  So
> I'm open to suggestions.

Maybe call the one that works with the cache, target_read_auxv_cached ?

Two overloads that do different things is a sign that they shouldn't be
overloads, to me.

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] gdb: fix auxv caching
  2022-10-11 20:31         ` Pedro Alves
@ 2022-10-11 20:34           ` Pedro Alves
  2022-10-11 20:42             ` John Baldwin
  0 siblings, 1 reply; 29+ messages in thread
From: Pedro Alves @ 2022-10-11 20:34 UTC (permalink / raw)
  To: Simon Marchi, John Baldwin, gdb-patches


On 2022-10-11 9:31 p.m., Pedro Alves wrote:
> On 2022-10-09 1:39 a.m., Simon Marchi via Gdb-patches wrote:
>>
>>> 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.
>>
>> The second one is perhaps not essential, call sites could call
>>
>>   target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL)
>>
>> themselves.  But I find it convenient to have this little wrapper.
>>
>> As to how to know which overload to call, perhaps that can be improved
>> with better documentation and comments.  I'm not sure what to add
>> though, the problem is so fresh in my mind that it's obvious to me.  So
>> I'm open to suggestions.
> 
> Maybe call the one that works with the cache, target_read_auxv_cached ?
> 

Or the other one target_read_auxv_raw, of course.

> Two overloads that do different things is a sign that they shouldn't be
> overloads, to me.
> 

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] gdb: fix auxv caching
  2022-10-11 20:34           ` Pedro Alves
@ 2022-10-11 20:42             ` John Baldwin
  2022-10-12  1:11               ` Simon Marchi
  0 siblings, 1 reply; 29+ messages in thread
From: John Baldwin @ 2022-10-11 20:42 UTC (permalink / raw)
  To: Pedro Alves, Simon Marchi, gdb-patches

On 10/11/22 1:34 PM, Pedro Alves wrote:
> 
> On 2022-10-11 9:31 p.m., Pedro Alves wrote:
>> On 2022-10-09 1:39 a.m., Simon Marchi via Gdb-patches wrote:
>>>
>>>> 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.
>>>
>>> The second one is perhaps not essential, call sites could call
>>>
>>>    target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL)
>>>
>>> themselves.  But I find it convenient to have this little wrapper.
>>>
>>> As to how to know which overload to call, perhaps that can be improved
>>> with better documentation and comments.  I'm not sure what to add
>>> though, the problem is so fresh in my mind that it's obvious to me.  So
>>> I'm open to suggestions.
>>
>> Maybe call the one that works with the cache, target_read_auxv_cached ?
>>
> 
> Or the other one target_read_auxv_raw, of course.

I think "_raw" would be better here as using the cached values should be the
default.

>> Two overloads that do different things is a sign that they shouldn't be
>> overloads, to me.
>>


-- 
John Baldwin

^ permalink raw reply	[flat|nested] 29+ messages in thread

* Re: [PATCH] gdb: fix auxv caching
  2022-10-11 20:42             ` John Baldwin
@ 2022-10-12  1:11               ` Simon Marchi
  0 siblings, 0 replies; 29+ messages in thread
From: Simon Marchi @ 2022-10-12  1:11 UTC (permalink / raw)
  To: John Baldwin, Pedro Alves, gdb-patches



On 2022-10-11 16:42, John Baldwin wrote:
> 
> I think "_raw" would be better here as using the cached values should be the
> default.

Indeed, and the fact that target_read_auxv does caching is an
implementation detail.

Here's what I pushed:


From 1639fab33b5932e1a5e88e29273996f70047da85 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Tue, 11 Oct 2022 20:53:39 -0400
Subject: [PATCH] gdb: rename target_read_auxv(target_ops *) to
 target_read_auxv_raw

Having two overloads of target_read_auxv that don't have the same goals
is confusing.  Rename the one that reads from an explicit target_ops to
target_read_auxv_raw.  Also, it occured to me that the non-raw version
could use the raw version, that reduces duplication a bit.

Change-Id: I28e5f7cecbfcacd0174d4686efb3e4a23b4ad491
---
 gdb/aarch64-linux-tdep.c | 2 +-
 gdb/arm-fbsd-tdep.c      | 2 +-
 gdb/arm-linux-tdep.c     | 2 +-
 gdb/auxv.c               | 5 ++---
 gdb/auxv.h               | 2 +-
 gdb/ppc-linux-tdep.c     | 2 +-
 gdb/s390-linux-tdep.c    | 2 +-
 7 files changed, 8 insertions(+), 9 deletions(-)

diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index 476db5aa3b88..a321aee036a0 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -780,7 +780,7 @@ aarch64_linux_core_read_description (struct gdbarch *gdbarch,
 				     struct target_ops *target, bfd *abfd)
 {
   asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
-  gdb::optional<gdb::byte_vector> auxv = target_read_auxv (target);
+  gdb::optional<gdb::byte_vector> auxv = target_read_auxv_raw (target);
   CORE_ADDR hwcap = linux_get_hwcap (auxv, target, gdbarch);
   CORE_ADDR hwcap2 = linux_get_hwcap2 (auxv, target, gdbarch);
 
diff --git a/gdb/arm-fbsd-tdep.c b/gdb/arm-fbsd-tdep.c
index 28fc73d694e6..4395136b83ce 100644
--- a/gdb/arm-fbsd-tdep.c
+++ b/gdb/arm-fbsd-tdep.c
@@ -238,7 +238,7 @@ arm_fbsd_core_read_description (struct gdbarch *gdbarch,
 {
   asection *tls = bfd_get_section_by_name (abfd, ".reg-aarch-tls");
 
-  gdb::optional<gdb::byte_vector> auxv = target_read_auxv (target);
+  gdb::optional<gdb::byte_vector> auxv = target_read_auxv_raw (target);
   return arm_fbsd_read_description_auxv (auxv, target, gdbarch, tls != nullptr);
 }
 
diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
index 65343f6c0758..7fa8a67ae42c 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -732,7 +732,7 @@ arm_linux_core_read_description (struct gdbarch *gdbarch,
 				 struct target_ops *target,
 				 bfd *abfd)
 {
-  gdb::optional<gdb::byte_vector> auxv = target_read_auxv (target);
+  gdb::optional<gdb::byte_vector> auxv = target_read_auxv_raw (target);
   CORE_ADDR arm_hwcap = linux_get_hwcap (auxv, target, gdbarch);
 
   if (arm_hwcap & HWCAP_VFP)
diff --git a/gdb/auxv.c b/gdb/auxv.c
index 5853437b0f24..51723f9b17cb 100644
--- a/gdb/auxv.c
+++ b/gdb/auxv.c
@@ -363,8 +363,7 @@ target_read_auxv ()
   if (info == nullptr)
     {
       info = auxv_inferior_data.emplace (inf);
-      info->data = target_read_alloc (inf->top_target (), TARGET_OBJECT_AUXV,
-				      nullptr);
+      info->data = target_read_auxv_raw (inf->top_target ());
     }
 
   return info->data;
@@ -373,7 +372,7 @@ target_read_auxv ()
 /* See auxv.h.  */
 
 gdb::optional<gdb::byte_vector>
-target_read_auxv (target_ops *ops)
+target_read_auxv_raw (target_ops *ops)
 {
   return target_read_alloc (ops, TARGET_OBJECT_AUXV, NULL);
 }
diff --git a/gdb/auxv.h b/gdb/auxv.h
index 983e3bc9b0d9..788d187b27a0 100644
--- a/gdb/auxv.h
+++ b/gdb/auxv.h
@@ -52,7 +52,7 @@ extern gdb::optional<gdb::byte_vector> target_read_auxv ();
 
 /* Read auxv data from OPS.  */
 
-extern gdb::optional<gdb::byte_vector> target_read_auxv (target_ops *ops);
+extern gdb::optional<gdb::byte_vector> target_read_auxv_raw (target_ops *ops);
 
 /* Search AUXV for an entry with a_type matching MATCH.
 
diff --git a/gdb/ppc-linux-tdep.c b/gdb/ppc-linux-tdep.c
index 12f418fb5ac6..f7d13bac8a3e 100644
--- a/gdb/ppc-linux-tdep.c
+++ b/gdb/ppc-linux-tdep.c
@@ -1599,7 +1599,7 @@ ppc_linux_core_read_description (struct gdbarch *gdbarch,
   if (vsx)
     features.vsx = true;
 
-  gdb::optional<gdb::byte_vector> auxv = target_read_auxv (target);
+  gdb::optional<gdb::byte_vector> auxv = target_read_auxv_raw (target);
   CORE_ADDR hwcap = linux_get_hwcap (auxv, target, gdbarch);
 
   features.isa205 = ppc_linux_has_isa205 (hwcap);
diff --git a/gdb/s390-linux-tdep.c b/gdb/s390-linux-tdep.c
index ef2ed8510a64..14d71134e0cd 100644
--- a/gdb/s390-linux-tdep.c
+++ b/gdb/s390-linux-tdep.c
@@ -332,7 +332,7 @@ s390_core_read_description (struct gdbarch *gdbarch,
 			    struct target_ops *target, bfd *abfd)
 {
   asection *section = bfd_get_section_by_name (abfd, ".reg");
-  gdb::optional<gdb::byte_vector> auxv = target_read_auxv (target);
+  gdb::optional<gdb::byte_vector> auxv = target_read_auxv_raw (target);
   CORE_ADDR hwcap = linux_get_hwcap (auxv, target, gdbarch);
   bool high_gprs, v1, v2, te, vx, gs;
 

base-commit: 8652404e813a895dfebe8591b30e90328b6e6898
-- 
2.38.0


^ permalink raw reply	[flat|nested] 29+ messages in thread

end of thread, other threads:[~2022-10-12  1:11 UTC | newest]

Thread overview: 29+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-19 14:45 [PATCH] Update auxv cache when there is no auxv cached data Luis Machado
2022-07-25  9:42 ` [PING][PATCH] " Luis Machado
2022-07-25 16:05 ` [PATCH] " John Baldwin
2022-07-25 18:03   ` Luis Machado
2022-07-25 19:13     ` John Baldwin
2022-08-02 15:05       ` Luis Machado
2022-08-02 16:05         ` John Baldwin
2022-08-05 15:46 ` [PATCH] Update auxv cache when inferior pid is 0 (no inferior) Luis Machado
2022-08-11  9:05   ` [PING][PATCH] " Luis Machado
2022-08-18 15:48   ` Luis Machado
2022-09-01  9:29   ` Luis Machado
2022-09-07  8:20   ` Luis Machado
2022-09-12 12:48   ` Luis Machado
2022-09-12 13:30   ` [PATCH] " Simon Marchi
2022-09-12 13:53     ` John Baldwin
2022-09-12 13:59       ` Luis Machado
2022-09-20 12:28 ` [PATCH] Invalidate auxv cache before creating a core target Luis Machado
2022-09-20 17:49   ` John Baldwin
2022-10-07 20:44   ` [PATCH] gdb: fix auxv caching Simon Marchi
2022-10-07 21:43     ` John Baldwin
2022-10-09  0:39       ` Simon Marchi
2022-10-10 18:32         ` John Baldwin
2022-10-11 17:52           ` Simon Marchi
2022-10-11 20:31         ` Pedro Alves
2022-10-11 20:34           ` Pedro Alves
2022-10-11 20:42             ` John Baldwin
2022-10-12  1:11               ` Simon Marchi
2022-10-10  9:33     ` Luis Machado
2022-10-11 17:53       ` Simon Marchi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).