Re: [PATCH] PowerPC 32 with Secure PLT

Michael Eager
2012-01-18T15:33:43Z

On 01/18/2012 04:08 AM, Joel Brobecker wrote:
>> 2012-01-17 Michael Eager<eager@eagercon.com>
>>
>> * configure.tgt (powerpc-*-linux*): Add glibc-tdep.o.
>> * ppc-linux-tdep.c: Include glibc-tdep.h.
>> (powerpc32_plt_stub, powerpc32_plt_stub_so): Add PLT stub templates.
>> (powerpc_linux_in_plt_stub): New function.
>> (powerpc_linux_in_dynsym_resolve_code): New function.
>> (ppc_skip_trampoline_code): New function.
>> (ppc_linux_init_abi): Use PPC specific functions rather than generic.
>> Use glibc_skip_solib_resolver.
>
> Overall, this looks good to me, but I am not a Linux specialist,
> so please give it another week before checking in to give the other
> maintainers a chance to comment as well.
>
> Below are some comments.
>
>> +#define POWERPC32_PLT_STUB_LEN \
>> + (sizeof powerpc32_plt_stub / sizeof (powerpc32_plt_stub[0]))
>
> There is a macro, ARRAY_SIZE, that does this for you.
>
>> +/* Check if PC is in PLT stub. For non-secure PLT, stub is in .plt
>
> Trailing space at the end of this line...
>
>> +/* Follow PLT stub to actual routine. */
>> +
>> +static CORE_ADDR
>> +ppc_skip_trampoline_code (struct frame_info *frame, CORE_ADDR pc)
>
> There are several lines in the code and the comments of this funtion
> that have trailing spaces. Can you get rid of them?
>
> Also, several lines are too long. The absolute limit is 80 characters,
> but the recommended limit is 70 characters. Can you reformat some of
> the lines where it doesn't make the code uglier?
>
>> + target = (CORE_ADDR) read_memory_unsigned_integer (target, 4, byte_order);
>
> This is an area where I am not sure, but I think it would be better
> to use read_memory_typed_address. I think the address type that you
> want is: builtin_type (gdbarch)->builtin_func_ptr.

Thanks. I'll fix all of these.