Re: [PATCH 2/2] gdbserver: Add linux_get_hwcap

[Pedro Franco de Carvalho <pedromfc at linux dot ibm dot com>]

Hi Alan,

Thank you for the comments.

Alan Hayward <Alan.Hayward@arm.com> writes:

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

The client version doesn't do this (target_auxv_search). Should I be
consistent with that or change it here?

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

I'm probably nitpicking, but I tried using memory at 0x0 with mmap and
it seems to work, even if it is unusual, and gdb can read from that
address, so apparently ptrace can read memory at 0x0, which should mean
that read_inferior_memory can do that too.

Still, I don't think the program headers could really be at 0x0, since
there would be no room for the ELF header before that.  However,
wouldn't it make sense to return in error separately, so that the next
person who reads that function doesn't have to go through this
reasoning?

Besides, other auxv values that might be retrieved in the future with
linux_get_auxv could have a real meaning for 0.

Other than that, I agree with your other comments.

> Thanks,
> Alan.

Thanks!

--
Pedro Franco de Carvalho