Thanks, this will save me quite a bit of work figuring out how the new ABI works.
"Thierry@vnet" <[email protected]> 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 _______________________________________________ Ltrace-devel mailing list [email protected] http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel
