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

Thierry@vnet thierry at linux.vnet.ibm.com
Wed Mar 12 10:01:01 UTC 2014


Hello,

1) Working on it, can I plan to enhance type.h with the following definition

enum arg_type {
        ARGTYPE_VOID,
        ARGTYPE_INT,
        ARGTYPE_UINT,
        ARGTYPE_LONG,
        ARGTYPE_ULONG,
        ARGTYPE_CHAR,
        ARGTYPE_SHORT,
        ARGTYPE_USHORT,
        ARGTYPE_FLOAT,
        ARGTYPE_DOUBLE,
        ARGTYPE_ARRAY,          /* Series of values in memory */
        ARGTYPE_STRUCT,         /* Structure of values */
        ARGTYPE_POINTER,        /* Pointer to some other type */
        ARGTYPE_HETEROGENEOUS,
        ARGTYPE_HOMOGENEOUS,
        ARGTYPE_HOMOGENEOUS_NESTED_FLOAT,
};

struct arg_type_info {
        enum arg_type type;
        union {
                struct vect entries;

                /* HETEROGENEOUS STRUCT (>16Bytes) */
                struct {
                        struct arg_type_info *info;
                        enum arg_type type;
                       size_t nb_elements;
                        enum arg_type homogeneous;
                } struct_info;


                /* ARGTYPE_ARRAY */
                struct {
                        struct arg_type_info *elt_type;
                        struct expr_node *length;
                        int own_info:1;
                        int own_length:1;
                } array_info;

                /* ARGTYPE_POINTER */
                struct {
                        struct arg_type_info *info;
                        int own_info:1;
                } ptr_info;
        } u;

        struct lens *lens;
        int own_lens;
};


Thanks

On 02/27/14 12:22, 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.
>
>> 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.
>
>> +#if _CALL_ELF == 2
>> +#undef ARCH_SUPPORTS_OPD
>> +#endif
> Again, either #define or don't #define, but don't #undef.
>
>> +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?
>
>> +	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.
>
>> +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.
>
>> +#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.
>
>>  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?
>
>>  		/* 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.
>
>> @@ -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.
>
> Thanks,
> PM
>


-- 
Thierry on vnet.ibm




More information about the Ltrace-devel mailing list