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

[Petr Machata]

2015-08-04 22:12 GMT+02:00 Faraz Shahbazker <faraz.shahbazker at imgtec.com>:
> 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.

OK.

>> 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.

Please do.

>>> +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?

Using C99 is OK.  I think we are around the point where using C11
might actually be worth considering if it happens to make sense.
(Static asserts and _Generic are the two features that seemed
interesting in that standard.)

Referencing the array directly is probably OK as well.

>>> +#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.

I think what I had in mind was, don't ifdef on LP64, instead implement
always, and where sizes would differ between 32 and 64 bit, make a
runtime condition.  I.e. have it runtime-selected, drop all the ifdefs
that you possibly can.  Have the compiler see as much code as
possible, so that it can catch bugs from other configurations as well.
(Not that C is that great at static typing, but still.)

> 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).

I'm not sure what you are trying to say here.  But if you look at x86
or ppc backend, those have one arch_type_sizeof for both 32-bit and
64-bit ABI(s), having a bunch of constants for everything but long,
and for long they have proc->e_machine == 64-bit one ? 8 : 4.  Does
the condition need to be much more involved than this?

Abolishing conditional compilation is important to me, as this stuff
can easily get out of hand and make a terrible mess of exceptions
within exceptions.

(In my ideal world, the interface to machine-specific parts would be
tiny and be essentially just fetching registers.  90+% of ltrace can
be expressed in platform-independent C.  Having large swaths of logic
hidden in sysdeps/ or conditionally compiled makes large-scale changes
and refactoring in the core difficult and guaranteed to break some
backend.)

Thank you,
Petr