[Ltrace-devel] [RFC][PATCH] Add support for mips64 n32/n64

[Petr Machata]

Faraz Shahbazker <faraz.shahbazker at imgtec.com> 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