[Ltrace-devel] Support for ppc64el patch

[Thierry Fauck@linux.vnet.ibm.com]

Hi,

	I appreciate a so full documented review - I will try my best to apply your comments. It's definitely good.

  * For indentation problems, will care about it.

  * I badly said hetereogenous instead of not homogeneous (it is not
    really clear if hetereogeneous means mixed float struct and/or big
    homogeneous struct >64 bytes ...) . Will change that.

  * Regarding "According to the code below, heterog and hfa_type are not
    part of _state_. You don't need to keep their values between calls.
    Why are they in context?".

I agree, these 2 boolean must be renamed like is_... and doesn't have to be in context.
Size of structure and hfa structure being enough. 

  * " This has wrong indentation. Please indent a space for each nested
    preprocessor level. (Though I suppose the include guard ifdefs don't
    count.)"
     #ifdef __powerpc64__ // Says 'ltrace' is 64 bits, says nothing
    about target.
     #define LT_ELFCLASS2   ELFCLASS64
     #define LT_ELF_MACHINE2        EM_PPC64
    -#define ARCH_SUPPORTS_OPD
    -#endif
    +
    +# ifdef __LITTLE_ENDIAN__
    +# define BREAKPOINT_VALUE { 0x08, 0x00, 0xe0, 0x7f }
    +# define ARCH_ENDIAN_LITTLE
    +# else
    +# define BREAKPOINT_VALUE { 0x7f, 0xe0, 0x00, 0x08 }
    +# define ARCH_SUPPORTS_OPD
    +# define ARCH_ENDIAN_BIG
    +# endif
    +
    +# if _CALL_ELF != 2
    +# define ARCH_SUPPORTS_OPD
    +# define STACK_FRAME_OVERHEAD 112
    +# else /* _CALL_ELF == 2 ABIv2 */
    +# define STACK_FRAME_OVERHEAD 32
    +# endif /* CALL_ELF */
    +
    +#else
    +#define BREAKPOINT_VALUE { 0x7f, 0xe0, 0x00, 0x08 }
    +#define ARCH_ENDIAN_BIG
    +#endif         /* __powerpc64__ */
  *

     is this better ?

>  *
>     +	lte->arch.elfv2_abi=((lte->ehdr.e_flags & 3) & 2);
>
> Uhh, do these things have symbolic names by any chance?
> Also, why & 3 & 2?  That's the same as & 2...
I finaly found it :
/* e_flags bits specifying ABI.
   1 for original function descriptor using ABI,
   2 for revised ABI without function descriptors,
   0 for unspecified or not using any features affected by the
differences.  */
#define EF_PPC64_ABI    3

    would :                 lte->arch.elfv2_abi=((lte->ehdr.e_flags &
EF_PPC64_ABI ) == 2 )

be more appropriate
+/* if ( ! strlen(name) ) + continue; +*/
> Just drop it instead of commenting out.
sure - I forgot it.
> Thank you, PM 

Update coming soon

Best regards
Thierry

-- 
Thierry Fauck @ linux.vnet.ibm

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.alioth.debian.org/pipermail/ltrace-devel/attachments/20140410/b8b951c1/attachment-0001.html>