Faraz Shahbazker <[email protected]> writes:
> diff --git a/proc.h b/proc.h
> index a611456..00094e1 100644
> --- a/proc.h
> +++ b/proc.h
> @@ -117,6 +117,7 @@ struct process {
> * nauseam. */
> short e_machine;
> char e_class;
> + char e_abi;
So the part that the diff program snipped says this:
/* XXX We would like to replace this with a pointer to ABI
* object that would provide the relevant services, instead of
* checking the necessary flags in the back end ad
* nauseam. */
It seems like e_machine and e_class should be /replaced/ with some sort
of ABI identifier that the backend produces, given an ELF header. The
comment mentions a full-fledged object, but an integer or even a char
would very likely be enough. Having a triplet of machine-class-ABI
seems superfluous, the ABI itself subsumes the other two.
That's a more invasive change though and if you simply don't have
resources to pursue this, I'm not going to push it. The above is fine.
> --- a/sysdeps/linux-gnu/mips/plt.c
> +++ b/sysdeps/linux-gnu/mips/plt.c
> @@ -182,6 +183,11 @@ arch_find_dl_debug(struct process *proc, arch_addr_t
> dyn_addr,
> {
> arch_addr_t rld_addr;
> int r;
> +#ifdef __LP64__
> + size_t addrsize = proc->mask_32bit ? 4 : (sizeof *ret);
> +#else /* !__LP64__ */
> + size_t addrsize = sizeof *ret;
> +#endif /* !__LP64__ */
>
> /* MIPS puts the address of the r_debug structure into the
> * DT_MIPS_RLD_MAP entry instead of into the DT_DEBUG entry. */
> @@ -189,7 +195,7 @@ arch_find_dl_debug(struct process *proc, arch_addr_t
> dyn_addr,
> DT_MIPS_RLD_MAP, &rld_addr);
> if (r == 0) {
> if (umovebytes(proc, rld_addr,
> - ret, sizeof *ret) != sizeof *ret) {
> + ret, addrsize) != addrsize) {
I wonder if you can rely on the fact that *ret will be initialized to
zeroes. Otherwise, for 64-bit ltrace tracing a 32-bit process, you
should see garbage in the part that's not overwritten. You might want
to stash a *ret = 0 in front of the umovebytes.
This should also break if MIPS runs in big endian mode, unless I'm
missing something.
Also, you can meld the line into previous, it would be < 80 chars.
> @@ -295,14 +301,25 @@ arch_elf_init(struct ltelf *lte, struct library *lib)
>
> for (j = 0; j < data->d_size / 16; ++j) {
> uint32_t insn;
> + int got_size = 4;
> + uint32_t load_inst = 0x24180000U; /* addui t8,0,xx */
> +
> +#ifdef __LP64__
> + if (arch_get_abi(lte->ehdr) == ABI_N64
> + || arch_get_abi(lte->ehdr) == ABI_O64) {
> + got_size = 8;
> + load_inst = 0x64180000U; /* daddui t8,0,xx */
> + }
> +#endif /* __LP64__ */
Is there a reason that this needs to be between ifdefs? I'd like to
have as much code exposed to compilation all the time as possible.
> @@ -362,8 +379,17 @@ read_got_entry(struct process *proc, GElf_Addr addr,
> GElf_Addr *valp)
> {
> /* XXX double cast. */
> arch_addr_t a = (arch_addr_t) (uintptr_t) addr;
> - uint32_t l;
> - if (proc_read_32(proc, a, &l) < 0) {
> + uint64_t l = 0;
> + int result;
> +
> +#ifdef __LP64__
> + if (!proc->mask_32bit)
> + result = proc_read_64(proc, a, &l);
> + else
> +#endif /* __LP64__ */
> + result = proc_read_32(proc, a, (uint32_t *) &l);
Here as well. The above is essentially if (proc->e_class ==
ELFCLASS64).
> @@ -503,13 +529,27 @@ jump_to_entry_point(struct process *proc, struct
> breakpoint *bp)
[...]
> +#ifdef __LP64__
And here.
> diff --git a/sysdeps/linux-gnu/mips/trace.c b/sysdeps/linux-gnu/mips/trace.c
> index e81b374..d54818e 100644
> --- a/sysdeps/linux-gnu/mips/trace.c
> +++ b/sysdeps/linux-gnu/mips/trace.c
> +/**
> + \param abi ABI of current process, from mips_abi_type enum
> + \param list An array of 4 elements, each corresponding to an ABI, in
> + the order: o32, n32, n64, o64
> +
> + return value from array corresponding to requested ABI
> + */
> +static int
> +abi_select(const int abi, const int list[])
> +{
> + int retval;
> + switch (abi)
> + {
> + case ABI_N32:
> + retval = list[1];
Why not use the enumerators as indices?
> + const int rt_sigreturn_list[] = {193, 211, 211, 193};
> + const int sigreturn_list[] = {119, -1, -1, 119};
Use C99 designated initializers:
const int rt_sigreturn_list[] = {
[ABI_O32] = 193, /* etc. */
};
That makes the parameter passing convention a bit less magic.
> + /* get the user's pc (plus 8) */
You mean plus 4?
> + The above is not necessary in Linux kernel > v2.6.35. Recent
> + kernels have a fancy-pants method of restarting syscalls.
:)
> +#ifdef __LP64__
> +size_t
> +arch_type_sizeof(struct process *proc, struct arg_type_info *info)
Define this always, not only on __LP64__. Make the size contingent on
proc->e_machine, proc->e_class or proc->e_abi.
Thanks for the submission, and sorry it took me this long to get around
to it. Unfortunately I can't promise this will get better any time
soon.
Petr
_______________________________________________
Ltrace-devel mailing list
[email protected]
http://lists.alioth.debian.org/cgi-bin/mailman/listinfo/ltrace-devel
