"Thierry [email protected]" <[email protected]> writes:
> @@ -34,7 +34,29 @@
> #ifdef __powerpc64__ // Says 'ltrace' is 64 bits, says nothing about target.
> #define LT_ELFCLASS2 ELFCLASS64
> #define LT_ELF_MACHINE2 EM_PPC64
> -#define ARCH_SUPPORTS_OPD
> +
> +#ifdef __LITTLE_ENDIAN__
> +# define BREAKPOINT_VALUE { 0x08, 0x00, 0xe0, 0x7f }
> +# define ARCH_ENDIAN_LITTLE
> +#else
> +# define BREAKPOINT_VALUE { 0x7f, 0xe0, 0x00, 0x08 }
> +# define ARCH_SUPPORTS_OPD
> +# define ARCH_ENDIAN_BIG
> +#ifndef PPC64_LOCAL_ENTRY_OFFSET
> +#define PPC64_LOCAL_ENTRY_OFFSET(other) other
> +#endif
This has wrong indentation. Please indent a space for each nested
preprocessor level. (Though I suppose the include guard ifdefs don't
count.)
> @@ -55,14 +57,19 @@ struct fetch_context {
> arch_addr_t stack_pointer;
> int greg;
> int freg;
> - int ret_struct;
> + bool ret_struct:1;
>
> union {
> gregs32_t r32;
> gregs64_t r64;
> } regs;
> - struct fpregs_t fpregs;
>
> + struct fpregs_t fpregs;
> + bool heterog:1;
> + bool hfa_type:1;
According to the code below, heterog and hfa_type are not part of
_state_. You don't need to keep their values between calls. Why are
they in context?
Also, hfa_type is a good name for something that is a type (in ltrace
typically a variable of type enum arg_type or possibly struct
arg_type_info). A predicate might be called is_hfa_type, or hfa_type_p,
or some such.
> @@ -124,9 +132,36 @@ arch_fetch_arg_init(enum tof type, struct process *proc,
[...]
> + if (context->ret_struct) {
> +#if _CALL_ELF != 2
> context->greg++;
> +#else
> + /* if R3 points to stack, parameters will be in R4 */
> + long pstack_end = ptrace(PTRACE_PEEKTEXT, proc->pid,
> + proc->stack_pointer,0);
There should be a space after comma.
> + if (( (long)context->regs.r64[3] > (long)proc->stack_pointer )
> + && ( (long)context->regs.r64[3] < pstack_end ) ) {
> + context->greg++;
> + context->stack_pointer += 8;
> + }
Don't put spaces around parenthesized arguments.
Also, the first cast should be to arch_addr_t, the second should be
absent altogether. If you make pstack_end unsigned, I suspect you will
be able to avoid the bottom cast as well.
> @@ -216,18 +258,34 @@ align_small_int(unsigned char *buf, size_t w, size_t sz)
[...]
> +
> +#if _CALL_ELF == 2
> + /* Consume the stack slot corresponding to this arg */
> + if ( (int)(sz + off ) >= 8 ) {
Why do you cast to int here?
There should be a period and two spaces before */
There shouldn't be spaces around the parenthesized argument.
> + ctx->greg++;
> + }
> + if ( ctx->hfa_type )
> + ctx->stack_pointer += sz ;
> + else
> + ctx->stack_pointer += 8 ;
There should be no space before a semicolon. The if has wrong spacing
as well.
> @@ -258,7 +317,10 @@ allocate_float(struct fetch_context *ctx, struct process
> *proc,
>
> ctx->freg++;
> if (proc->e_machine == EM_PPC64)
> - allocate_gpr(ctx, proc, info, NULL);
> + allocate_gpr(ctx, proc, info, NULL, off);
> +
> + if ( ! ctx->hfa_type )
> + ctx->vgreg++;
Again, no spaces inside parentheses.
> @@ -276,9 +338,140 @@ allocate_float(struct fetch_context *ctx, struct
> process *proc,
> }
>
> static int
> +allocate_hfa(struct fetch_context *ctx, struct process *proc,
> + struct arg_type_info *info, struct value *valuep,
> + enum arg_type hfa_type, size_t hfa_count)
> + {
> + size_t sz = type_sizeof(proc, info);
> + if (sz == (size_t)-1)
> + return -1;
> +
> + ctx->hfa_size += sz;;
No double semicolons please.
> + unsigned char *buf = value_reserve(valuep, sz);
> + if (buf == NULL)
> + return -1;
Up here, you have a tab followed by a space. That's probably wrong.
It's OK down here:
> +
> + ctx->hfa_type = 1 ;
true, not 1. Also, no space before semicolon.
> + ctx->heterog = 0 ;
> + if ( hfa_count > 8 ) {
> + ctx->heterog = 1 ;
> + }
Again, too much spacing throughout.
> +
> +
> + struct arg_type_info *hfa_info = type_get_simple(hfa_type);
> + size_t hfa_sz = type_sizeof(proc, hfa_info);
> +
> +
The following while has again wrong indentation:
> + while ( hfa_count > 0 && ctx->freg <= 13 ) {
> + int rc;
> + struct value tmp;
> +
> + value_init(&tmp, proc, NULL, hfa_info, 0);
> +
> + /* Hetereogeneous struct - get value on GPR or stack */
You keep saying hetereogeneous and I keep wondering whether you mean
homogeneous, or whether heterogeneous structs really are something in
context of ELFv2 ABI. Like, I could imagine mixed float/double structs
(i.e. heterogeneous) getting the sort of optimization that homogeneous
normally do. But I don't know. Could you explain this to me, please?
> + if ( ((hfa_type == ARGTYPE_FLOAT
> + || hfa_type == ARGTYPE_DOUBLE )
> + && hfa_count <= 8 ) )
> + rc = allocate_float(ctx, proc, hfa_info, &tmp,
> + slot_off);
> + else
> + rc = allocate_gpr(ctx, proc, hfa_info, &tmp,
> + slot_off );
> +
> + memcpy(buf, value_get_data(&tmp, NULL), hfa_sz);
> +
> + slot_off += hfa_sz;
> + buf += hfa_sz;
> + hfa_count--;
> + if ( slot_off == 8 ) {
> + slot_off = 0;
> + ctx->vgreg++;
> + }
Spacing.
> +
> +
> + value_destroy(&tmp);
> + if (rc < 0)
> + return -1;
> + }
> + if ( ! hfa_count ) {
> + return 0;
> + }
Spacing.
> + if ( ! hfa_count )
> + return 0;
hfa_count is not a boolean, say if (hfa_count == 0).
> +
> + /* Remaining values are on stack */
> + while ( hfa_count ) {
> + /* allocate_stack_slot(ctx, proc, info, valuep); */
Why this comment? Also, spacing at while is wrong.
> + struct value tmp;
> + value_init(&tmp, proc, NULL, hfa_info, 0);
This has one extra space of indentation.
> +
> + value_in_inferior(&tmp, ctx->stack_pointer);
> + memcpy(buf, value_get_data(&tmp, NULL), hfa_sz);
> + ctx->stack_pointer += hfa_sz ;
> + buf += hfa_sz ;
> + hfa_count--;
No spaces before semicolons.
> + }
> + return 0;
> + }
> +
> +static int
> allocate_argument(struct fetch_context *ctx, struct process *proc,
> struct arg_type_info *info, struct value *valuep)
> {
> + ctx->hfa_type = 0;
false, not 0.
> @@ -287,13 +480,26 @@ allocate_argument(struct fetch_context *ctx, struct
> process *proc,
>
> case ARGTYPE_FLOAT:
> case ARGTYPE_DOUBLE:
> - return allocate_float(ctx, proc, info, valuep);
> + return allocate_float(ctx, proc, info, valuep,
> + 8 - type_sizeof(proc,info));
>
> case ARGTYPE_STRUCT:
> if (proc->e_machine == EM_PPC) {
> if (value_pass_by_reference(valuep) < 0)
> return -1;
> } else {
> +#if _CALL_ELF != 2
> + break;
> +#endif
> + struct arg_type_info *hfa_info;
> + size_t hfa_size;
> + hfa_info = type_get_hfa_type(info, &hfa_size);
> + if (hfa_info != NULL ) {
> + size_t sz = type_sizeof(proc, info);
> + ctx->struct_size += sz;
> + return allocate_hfa(ctx, proc, info, valuep,
> + hfa_info->type, hfa_size);
> + }
It would be better to wrap the new code in #if _CALL_ELF == 2 / #endif.
If you think the break makes things clear (there's a fall-through below,
but it just falls through to a break), then place it explicitly after
the endif, and possibly after the below comment as well (instead of the
fall-through).
> /* PPC64: Fixed size aggregates and unions passed by
> * value are mapped to as many doublewords of the
> * parameter save area as the value uses in memory.
> @@ -326,6 +532,10 @@ allocate_argument(struct fetch_context *ctx, struct
> process *proc,
> size_t sz = type_sizeof(proc, valuep->type);
> if (sz == (size_t)-1)
> return -1;
> +
> + if ( ctx->ret_struct )
> + ctx->struct_size += sz;
Spacing.
> @@ -346,9 +556,11 @@ allocate_argument(struct fetch_context *ctx, struct
> process *proc,
> struct arg_type_info *fp_info
> = type_get_fp_equivalent(valuep->type);
> if (fp_info != NULL)
> - rc = allocate_float(ctx, proc, fp_info, &val);
> + rc = allocate_float(ctx, proc, fp_info, &val,
> + 8-type_sizeof(proc,info));
> else
> - rc = allocate_gpr(ctx, proc, long_info, &val);
> + rc = allocate_gpr(ctx, proc, long_info, &val,
> + 0);
The 0 would fit to the previous line.
> @@ -411,7 +625,22 @@ arch_fetch_retval(struct fetch_context *ctx, enum tof
> type,
> struct process *proc, struct arg_type_info *info,
> struct value *valuep)
> {
> + if (fetch_context_init(proc, ctx) < 0)
> + return -1;
> +
> +
> +#if _CALL_ELF != 2
> if (ctx->ret_struct) {
> +#else
> + void *ptr=(void *) (ctx->regs.r64[1]+32);
There should be spaces around the =.
> + long val = ptrace(PTRACE_PEEKTEXT, proc->pid, ptr , 0);
> + if ( ctx->ret_struct &&
> + ( ( ( ctx->struct_size > 64 || ctx->heterog ) )
> + || (ctx->struct_size > 56 && ! ctx->hfa_type )
> + || ( (void *)ctx->regs.r64[3] == (void *)(ctx->regs.r64[1]+32) )
Why is the void* necessary?
> + || ( ctx->regs.r64[3] == (uint64_t)val) )) {
Also, please fix spacing in this whole expression.
> diff --git a/sysdeps/linux-gnu/ppc/plt.c b/sysdeps/linux-gnu/ppc/plt.c
> @@ -430,7 +450,10 @@ reloc_copy_if_irelative(GElf_Rela *rela, void *data)
> int
> arch_elf_init(struct ltelf *lte, struct library *lib)
> {
> + lte->arch.elfv2_abi=((lte->ehdr.e_flags & 3) & 2);
Uhh, do these things have symbolic names by any chance?
Also, why & 3 & 2? That's the same as & 2...
> @@ -541,6 +564,10 @@ arch_elf_init(struct ltelf *lte, struct library *lib)
>
> const char *name = lte->strtab + sym.st_name;
>
> +/* if ( ! strlen(name) )
> + continue;
> +*/
Just drop it instead of commenting out.
> @@ -616,21 +643,58 @@ unresolve_plt_slot(struct process *proc, GElf_Addr
> addr, GElf_Addr value)
[...]
> +extern void delete_symbol_chain(struct library_symbol *);
> +
This belongs to ltrace-elf.h.
> enum plt_status
> arch_elf_add_func_entry(struct process *proc, struct ltelf *lte,
> const GElf_Sym *sym,
> arch_addr_t addr, const char *name,
> struct library_symbol **ret)
> {
> - if (lte->ehdr.e_machine != EM_PPC || lte->ehdr.e_type == ET_DYN)
> + /* With ABIv2 st_other field contains an offset */
> + if ( lte->arch.elfv2_abi )
> + addr += PPC64_LOCAL_ENTRY_OFFSET(sym->st_other);
> +
> + int st_info=GELF_ST_TYPE(sym->st_info);
Spaces around =.
> +
> + if ( (lte->ehdr.e_machine != EM_PPC && ! sym->st_other )
st_other is not a boolean, please use sym->st_other == 0.
> diff --git a/sysdeps/linux-gnu/ppc/trace.c b/sysdeps/linux-gnu/ppc/trace.c
> index ee9a6b5..03d9d51 100644
> --- a/sysdeps/linux-gnu/ppc/trace.c
> +++ b/sysdeps/linux-gnu/ppc/trace.c
> @@ -66,7 +66,11 @@ syscall_p(struct process *proc, int status, int *sysnum)
> && WSTOPSIG(status) == (SIGTRAP | proc->tracesysgood)) {
> long pc = (long)get_instruction_pointer(proc);
> int insn =
> +#ifndef __LITTLE_ENDIAN__
> (int)ptrace(PTRACE_PEEKTEXT, proc->pid, pc - sizeof(long),
> +#else
> + (int)ptrace(PTRACE_PEEKTEXT, proc->pid, pc - sizeof(int),
> +#endif
> 0);
Please wrap syntactically meaningful units in preprocessor directives.
At least bring inside the 0, possibly even the declarator.
Thank you,
PM
_______________________________________________
Ltrace-devel mailing list
[email protected]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel
