Re: [PATCH] PowerPC 32 with Secure PLT

[Joel Brobecker <brobecker at adacore dot com>]

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

Same below.

-- 
Joel