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@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
>
--
Thierry on vnet.ibm
_______________________________________________
Ltrace-devel mailing list
[email protected]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel
