This is the mail archive of the
gdb-patches@sourceware.org
mailing list for the GDB project.
Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap
- From: Alan Hayward <Alan dot Hayward at arm dot com>
- To: Ulrich Weigand <uweigand at de dot ibm dot com>, Pedro Franco de Carvalho <pedromfc at linux dot ibm dot com>
- Cc: Simon Marchi <simark at simark dot ca>, "gdb-patches\\@sourceware.org" <gdb-patches at sourceware dot org>, nd <nd at arm dot com>, "anton at linux dot ibm dot com" <anton at linux dot ibm dot com>
- Date: Mon, 8 Apr 2019 09:38:51 +0000
- Subject: Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap
- References: <20190405163946.2DB32D802DA@oc3748833570.ibm.com>
Apologies, I’ve been away for the last week, only just saw this whole thread.
Thanks for fixing this without me.
Pedro: Some comments/nits below. Given this is pushed, I’m happy if you don’t
raise a whole new patch for it.
> On 5 Apr 2019, at 17:39, Ulrich Weigand <uweigand@de.ibm.com> wrote:
>
> Pedro Franco de Carvalho wrote:
>
>> Note that there is a difference in the interface between linux_get_auxv
>> and linux_get_hwcap(2), the latter still return both the status and the
>> entry value, but I didn't want to change all their users.
>
> Actually, I think this is fine -- for hwcap, 0 is a natural default
> value to use if the entry is not present.
Yes, all the targets in GDB treated an error condition the same as a hwcap
value of 0. Which is sensible given it’s for checking features present in
the hardware/cpu.
>
>> Also, contrary to the gdb client version (target_auxv_search
>> gdb/auxv.c), this one doesn't differentiate between an error and the
>> entry not being found.
>
> That also seems reasonable. If anybody requires more specific error
> handling, that can always be added later.
Originally I was going to keep gdbserver linux_get_auxv with the same returns
as the gdb version. However, given it is a static function and the hwap
functions were not checking the error value, I removed it.
>
>> gdb/gdbserver/ChangeLog:
>> 2019-04-DD Pedro Franco de Carvalho <pedromfc@linux.ibm.com>
>>
>> * linux-low.c (linux_get_auxv): Remove static. Return auxv entry
>> value in argument pointer, return 1 if the entry is found and 0
>> otherwise. Move comment.
Returning true/false would have been better than 1/0. The build requires C++ support.
>> (linux_get_hwcap, linux_get_hwcap2): Use modified linux_get_auxv.
>> * linux-low.h (linux_get_auxv): Declare.
>> * linux-ppc-low.c (is_elfv2_inferior): Use linux_get_auxv.
>
> index d825184835..d5d074efc5 100644
> --- a/gdb/gdbserver/linux-low.h
> +++ b/gdb/gdbserver/linux-low.h
> @@ -435,6 +435,14 @@ bool thread_db_thread_handle (ptid_t ptid, gdb_byte **handle, int *handle_len);
>
> extern int have_ptrace_getregset;
>
> +/* Search for the value with type MATCH in the auxv vector with
> + entries of length WORDSIZE bytes. If found, store the value in
> + *VALP and return 1. If not found or if there is an error, return
> + 0. */
Comment should state that VALP is not checked for nullptr.
> +
> +int linux_get_auxv (int wordsize, CORE_ADDR match,
> + CORE_ADDR *valp);
> +
>
> diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
> index 265043f97e..65919c3262 100644
> --- a/gdb/gdbserver/linux-low.c
> +++ b/gdb/gdbserver/linux-low.c
> @@ -7427,11 +7427,10 @@ linux_get_pc_64bit (struct regcache *regcache)
> return pc;
> }
>
> -/* Fetch the entry MATCH from the auxv vector, where entries are length
> - WORDSIZE. If no entry was found, return zero. */
> +/* See linux-low.h. */
>
> -static CORE_ADDR
> -linux_get_auxv (int wordsize, CORE_ADDR match)
> +int
> +linux_get_auxv (int wordsize, CORE_ADDR match, CORE_ADDR *valp)
> {
> gdb_byte *data = (gdb_byte *) alloca (2 * wordsize);
> int offset = 0;
Would be useful to set valp to zero here. (Not sure if the GDB version
does this either).
> @@ -7442,15 +7441,21 @@ linux_get_auxv (int wordsize, CORE_ADDR match)
> {
> if (wordsize == 4)
> {
> - uint32_t *data_p = (uint32_t *)data;
> + uint32_t *data_p = (uint32_t *) data;
> if (data_p[0] == match)
> - return data_p[1];
> + {
> + *valp = data_p[1];
> + return 1;
> + }
> }
> else
> {
> - uint64_t *data_p = (uint64_t *)data;
> + uint64_t *data_p = (uint64_t *) data;
> if (data_p[0] == match)
> - return data_p[1];
> + {
> + *valp = data_p[1];
> + return 1;
> + }
> }
> diff --git a/gdb/gdbserver/linux-ppc-low.c b/gdb/gdbserver/linux-ppc-low.c
> index 8deb0ce068..f17f05a0a3 100644
> --- a/gdb/gdbserver/linux-ppc-low.c
> +++ b/gdb/gdbserver/linux-ppc-low.c
> @@ -1107,10 +1107,13 @@ is_elfv2_inferior (void)
> #else
> const int def_res = 0;
> #endif
> - unsigned long phdr;
> + CORE_ADDR phdr;
> Elf64_Ehdr ehdr;
>
> - if (!ppc_get_auxv (AT_PHDR, &phdr))
> + const struct target_desc *tdesc = current_process ()->tdesc;
> + int wordsize = register_size (tdesc, 0);
> +
> + if (!linux_get_auxv (wordsize, AT_PHDR, &phdr))
> return def_res;
I don’t think there is a requirement here for the error to be return separately
to the phdr. With the my version of linux_get_auxv, on error you would get 0
for phdr. Given that it is an address, 0 should never be a valid value.
With the code pre my patch and this patch, I’m not sure what will happen if the
PHDR value is 0 - will read_inferior_memory then the memcmp deal with that? (To
be fair, I suspect there are bigger issues to deal with if phdr is 0).
Therefore I’d suggest it’d be better to have:
CORE_ADDR phdr = linux_get_auxv (wordsize, AT_PHDR);
if (phdr == nullptr)
return def_res;
Thanks,
Alan.