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

[Faraz Shahbazker]

On 07/15/2015 03:24 PM, Petr Machata wrote:
> 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.
While I agree with the above, it seems like something that should be 
part of a general refactoring, so I don't want to muddy the MIPS patch with it.

>> --- 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.
Agreed.
 
> This should also break if MIPS runs in big endian mode, unless I'm
> missing something.
Haven't tested on big-endian, but I think it shouldn't break. For 32-bit process, 
the address stored at DT_MIPS_RLD_MAP is also 32-bit. We should always be able to 
find it within the first 4 relative bytes. OTOH, endianness is not my strong suite,
so I will double-check.

>> @@ -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.
No good reason. It seemed like a good way to highlight 64-bit code for the 64-bit 
patch. I will remove __LP64__ from all instances where it only serves to cut down
code size.

>> +/**
>> +   \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?
No good reason. With C99 designated initializers I'd want to drop abi_select() 
altogether and index the arrays directly in syscall_p(). Does that sound right?

>> +#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.
Defining always is fine, I am not quite clear on the second point. 
The aberration from normal case (where size is simply sizeof<type>) is 
only necessary for tracing 32-bit process with 64-bit ltrace, so __LP64__ 
will have to figure in the check in addition to say proc->e_class. 
I've used mask_32bit flag because it exactly captures both __LP64__ and 
(proc->e_class == ELFCLASS32).

Thank you for looking at this.

Regards,
Faraz Shahbazker