* [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-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-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
* 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-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
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).