[Ltrace-devel] ltrace update for ppc64el arch (Petr Machata)
Petr Machata
pmachata at redhat.com
Thu Feb 27 11:22:41 UTC 2014
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
More information about the Ltrace-devel
mailing list