Re: [PATCH] PowerPC 32 with Secure PLT

[Michael Eager <eager@eagerm.com>]

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.


-- 
Michael Eager	 eager@eagercon.com
1960 Park Blvd., Palo Alto, CA 94306  650-325-8077