[Ltrace-devel] ltrace update for ppc64el arch (Petr Machata)

Thierry@vnet thierry at linux.vnet.ibm.com
Thu Feb 27 16:22:41 UTC 2014


On 02/27/2014 12:22 PM, Petr Machata wrote:
> Thanks, this will save me quite a bit of work figuring out how the new
> ABI works.
>
> "Thierry at vnet" <thierry at linux.vnet.ibm.com> writes:
>
>> diff --git a/ltrace-0.7.3.orig/sysdeps/linux-gnu/ppc/arch.h b/ltrace-0.7.3/sysdeps/linux-gnu/ppc/arch.h
>> index fb8768a..edabe2e 100644
>> --- a/ltrace-0.7.3.orig/sysdeps/linux-gnu/ppc/arch.h
>> +++ b/ltrace-0.7.3/sysdeps/linux-gnu/ppc/arch.h
>> @@ -27,6 +27,7 @@
>>  #define BREAKPOINT_VALUE { 0x7f, 0xe0, 0x00, 0x08 }
>>  #define BREAKPOINT_LENGTH 4
>>  #define DECR_PC_AFTER_BREAK 0
>> +#define ARCH_ENDIAN_BIG
>>  
>>  #define LT_ELFCLASS	ELFCLASS32
>>  #define LT_ELF_MACHINE	EM_PPC
>> @@ -35,6 +36,12 @@
>>  #define LT_ELFCLASS2	ELFCLASS64
>>  #define LT_ELF_MACHINE2	EM_PPC64
>>  #define ARCH_SUPPORTS_OPD
>> +#ifdef __LITTLE_ENDIAN__
>> +#undef BREAKPOINT_VALUE
>> +#define BREAKPOINT_VALUE { 0x08, 0x00, 0xe0, 0x7f }
>> +#define ARCH_ENDIAN_LITTLE
>> +#undef ARCH_ENDIAN_BIG
>> +#endif
> Please don't #undef, instead conditionally #define one or the other.
Ok - thought it was safer to avoid special case. But definitely setting
the BREAKPOINT_VALUE upon arch is easy.
>> diff --git a/ltrace.0.7.3-v1/ltrace-elf.c b/ltrace-0.7.3/ltrace-elf.c
>> index c571d2a..4b8b0bc 100644
>> --- a/ltrace.0.7.3-v1/ltrace-elf.c
>> +++ b/ltrace-0.7.3/ltrace-elf.c
>> @@ -714,6 +714,10 @@ populate_this_symtab(struct Process *proc, const char *filename,
>>  			continue;
>>  		}
>>  
>> +#if defined(__powerpc64__) && _CALL_ELF == 2
>> +		naddr += PPC64_LOCAL_ENTRY_OFFSET (sym.st_other);
>> +#endif
>> +
> Where does _CALL_ELF come from?  What is PPC64_LOCAL_ENTRY_OFFSET?  In
> any case, this should be folded into arch_translate_address in PPC
> backend.
The PPC64_LOCAL_ENTRY_OFFSE() is a macro in elf.h
* PowerPC64 specific values for the Elf64_Sym st_other field.  */
#define STO_PPC64_LOCAL_BIT     5
#define STO_PPC64_LOCAL_MASK    (7 << STO_PPC64_LOCAL_BIT)
#define PPC64_LOCAL_ENTRY_OFFSET(other)                         \
 (((1 << (((other) & STO_PPC64_LOCAL_MASK) >> STO_PPC64_LOCAL_BIT)) >>
2) << 2)

That entry in ltrace-elf.c is definitely not good and needs to be placed
in */*/ppc/plt.c , but currently we haven't find a proper place to have
it - my though was of course to use the arch_translate_addres() but I
have 2 issues there
    - we sometime are in the case we don't call it
    - As I don't know the context (saying it is a symbol) , I have to
check the address is in the good range - that make sense.
I will work on it .
>
>> +#if _CALL_ELF == 2
>> +#undef ARCH_SUPPORTS_OPD
>> +#endif
> Again, either #define or don't #define, but don't #undef.
yes
>> +enum homogeneous_type {
>> +	NOINIT = 0,
>> +	HETEROGENEOUS,
>> +	HOMOGENEOUS,
>> +	HOMOGENEOUS_NESTED_FLOAT,
>> +};
>> +
>> +struct struct_attributes {
>> +	struct arg_type_info *info;
>> +	enum arg_type type;
>> +	size_t nb_elements;
>> +	enum homogeneous_type homogeneous;
>> +};
>> +
>> +
> Why can't you use type_get_hfa_type?  Can we extend it so that it does
> what you need?
That code written by Guy (taken from other bits) is definitely a first
drop and needs enhancement.
I (We) will send proposal for enhancements.
>> +	if (ret_info->type == ARGTYPE_STRUCT) {
>> +		get_struct_attribut(ret_info, proc, &ret_size, &struct_attr);
>> +
>> +		if (((struct_attr.homogeneous == HOMOGENEOUS_NESTED_FLOAT) &&
>> +		   (struct_attr.nb_elements > 8)) ||
>> +		   (((struct_attr.homogeneous == HOMOGENEOUS) ||
>> +		     (struct_attr.homogeneous == HETEROGENEOUS)) &&
>> +		    (ret_size > 16)))
> Please don't leave && and || hanging on the end of a line.  Also indent
> properly (what follows after the first && should be two characters
> ahead).  Personally I'd write it thus:
>
> +		if ((struct_attr.homogeneous == HOMOGENEOUS_NESTED_FLOAT
> +		     && struct_attr.nb_elements > 8)
> +                   || ((struct_attr.homogeneous == HOMOGENEOUS
> +                        || struct_attr.homogeneous == HETEROGENEOUS)
> +                       && ret_size > 16))
>
> There are more instances of this problem, I won't cite them all.
Agree on that - I will transmit ... we are learning.
>> +struct arg_type_info *
>> +arch_type_get_fp_equivalent(struct arg_type_info *info, struct Process *proc)
>> +{
> What is this good for?
>
>>  	unsigned char *buf = value_reserve(valuep, slots * width);
>>  	if (buf == NULL)
>>  		return -1;
>> -	struct arg_type_info *long_info = type_get_simple(ARGTYPE_LONG);
>> +	struct arg_type_info *long_info = arch_type_get_simple(ARGTYPE_LONG,proc);
> I see you undid this change in a follow-up patch.  It would be better if
> you could fold the fixes into this one, so that each patch can be
> reviewed and tested separately.
Yes my mistake while reading the patch ...bit tired when I double
checked it . will provide the clean patch
>> +#if _CALL_ELF != 2
>>  #define PPC64_PLT_STUB_SIZE 8 //xxx
>> +#else
>> +#define PPC64_PLT_STUB_SIZE 4 //xxx
>> +#endif
> Let's drop both xxx's.  Cleary that's what it is at this moment.
ok
>>  arch_translate_address(struct ltelf *lte,
>>  		       arch_addr_t addr, arch_addr_t *ret)
>>  {
>> -	if (lte->ehdr.e_machine == EM_PPC64) {
>> +	if (lte->ehdr.e_machine == EM_PPC64
>> +	   && (lte->ehdr.e_flags & 3) != 2 ) {
> Do these numbers have symbolic names by any chance?
Not as far as I know ( not in all *elf.h) on system - I will check the
asm bits since its coming from there.
>>  		/* XXX The double cast should be removed when
>>  		 * arch_addr_t becomes integral type.  */
>>  		GElf_Xword offset
>> @@ -433,6 +444,7 @@ int
>>  arch_elf_init(struct ltelf *lte, struct library *lib)
>>  {
>>  	if (lte->ehdr.e_machine == EM_PPC64
>> +	    && (lte->ehdr.e_flags & 3) != 2
> And again here... clearly we need to name this and refer back to it
> instead of testing the same thing again and again.  It seems as if the
> following is necessary:
>
> diff --git a/sysdeps/linux-gnu/ppc/arch.h b/sysdeps/linux-gnu/ppc/arch.h
> index bf9b5dc..664a670 100644
> --- a/sysdeps/linux-gnu/ppc/arch.h
> +++ b/sysdeps/linux-gnu/ppc/arch.h
> @@ -56,7 +56,8 @@ struct arch_ltelf_data {
>  	Elf_Data *opd_data;
>  	GElf_Addr opd_base;
>  	GElf_Xword opd_size;
> -	int secure_plt;
> +	bool secure_plt : 1;
> +	bool elfv2_abi : 1;
>
>  	Elf_Data *reladyn;
>  	size_t reladyn_count;
> @@ -65,7 +66,8 @@ struct arch_ltelf_data {
>  #define ARCH_HAVE_LIBRARY_DATA
>  struct arch_library_data {
>  	GElf_Addr pltgot_addr;
> -	int bss_plt_prelinked;
> +	bool bss_plt_prelinked : 1;
> +	bool elfv2_abi : 1;
>  };
>
>  enum ppc64_plt_type {
>
> (Likely just one of these will be necessary.)
>
> This should be initialized in arch_elf_init and later used in lieu of
> all the #ifdef stuff.  Since the ELFv2 is purely a userspace thing, this
> would allow the theoretical case of tracing ELFv2 process with ELFv1
> ltrace.  More importantly, it would expose all the code to the compiler,
> which prevents bitrot somewhat.
Agree on using that implementation. Thanks for pointing out.
>
>> @@ -862,9 +872,13 @@ ppc_plt_bp_continue(struct breakpoint *bp, struct Process *proc)
>>  			(struct process_stopping_handler *);
>>  
>>  	case PPC_DEFAULT:
>> +#if _CALL_ELF != 2
>>  		assert(proc->e_machine == EM_PPC);
>>  		assert(bp->libsym != NULL);
>>  		assert(bp->libsym->lib->arch.bss_plt_prelinked == 0);
>> +#else
>> +		/* ABIv2 uses plt and may set the flag */
>> +#endif
> PPC_DEFAULT is for non-PLT or PPC32 symbols.  Is the PLT scheme similar
> on ELFv2 to what ELFv1 does, or is it "plain old PLT" like other arches
> do?  I.e. does the comment on top of plt.c apply?
>
> If it's plain old PLT, then it seems we shouldn't fall through to
> PPC_PLT_UNRESOLVED.  If it's similar to what PPC64 ELFv1 does, then
> PPC_DEFAULT is not the proper type for this breakpoint.
Since .opd disapeared, PLT is more like what we have on other systems - and

    In ELFv2, the PLT
    is an array of plain (function) pointers, with the first pointing
    to _dl_runtime_resolve and the second holding the map address.

As result of your comment, I realized that the new test trace-irelative.exp is failing twice since the xyz entry is not added to the PLT after arch_elf_init() and such not initialized as a breakpoint. I need to review the arch_elf_init() code then. 

We found also few (6) problems with the new hfa tests and this is under analysis.
Great thanks for your help.

Thierry



-- 
Thierry on vnet.ibm

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


More information about the Ltrace-devel mailing list