Re: [Qemu-devel] [PATCH] target_posix_types.h

[Thayne Harbaugh <thayne@c2.net>]

On Wed, 2007-11-14 at 20:14 +0100, Fabrice Bellard wrote:
> Thayne Harbaugh wrote:
> > On Wed, 2007-11-14 at 19:32 +0100, Fabrice Bellard wrote:
> >> Thayne Harbaugh wrote:
> >>> This patch, 44_target_posix_types.patch provides target specific posix
> >>> types.  These types improve target structure creation, code similarity
> >>> to kernel code and improve type casting for assignment between target
> >>> and host.
> >> Why is it needed ?
> > 
> > 
> > It's not *necessary*, but it makes code more readable and it simplifies
> > having to always check for what a typedef on a target might map to.  It
> > makes target structures more comparable to their structures in the
> > kernel.
> > 
> > A simple example:
> > 
> > struct target_timeval {
> >     abi_long tv_sec;
> >     abi_long tv_usec;
> > };
> > 
> > vs.
> > 
> > struct target_timeval {
> >     target_time_t tv_sec;
> >     target_suseconds_t tv_usec;
> > };
> > 
> > 
> > It also makes type conversion between target and host more obvious.
> > 
> > It also means that more code can be shared rather than #ifdef'ed when
> > targets differ on their base definitions.
> > 
> > It's just another level of abstraction, we can always stick with
> > abi_long, abi_ulong, etc..  I've just noticed that size and sign
> > handling when converting between target and host are a common source of
> > errors and this simplifies things.  We use it in all our patches and it
> > has helped simplify and fix errors.
> 
> I don't like adding levels of abstraction unless I am really forced. I 
> think these types makes the code more difficult to understand without 
> real added value. In particular, it does not help for sign conversions.

I should have provided an example:

Let's look at time_t and target_time_t which might have a size of 4 or
of 8.

IMO it's easier to write this:

time_t time;
get_user(time, time_addr, target_time_t);
and
put_user(time, time_addr, target_time_t);

get_user then does the lock and calls __get_user() which uses the type
information when assigning to time - that is what correctly handles any
sign extending.

What is common now is this:

time_t time;
time = tget32(time_addr);
and
tput32(time_addr, time);

This means that time_t had to be tracked down on varying architectures
to find the size and there was an assumption made that time_t is 32 bits
- which isn't true for all targets.  The next problem is that if the
target is 32 bits but the host is 64 bits then there's a sign extension
problem because (time_t)-1 is used for an error condition.  If you don't
correctly assign assign the 32-bit -1 to a 64-bit type then, rather than
-1, you get 4294967295.

Now maybe a solution is this:

time = tget1(time_addr);
and
tputl(time_addr, time);

That may fix the problem of getting the size correct depending on
whether the target is 32 or 64 bit, but it still doesn't solve the
problem of correctly sign-extending when assigning between the target
and the host.

I've sent patches to fix this type of bug to the list.  I have possibly
half-a-dozen more patches in my tree that fix this same type of bug: -1
becoming a very large positive number.

Maybe I'm missing the current, correct way of doing this.  Please
enlighten me.  If I've missed something then I apologize for wasting
everyone's time.

Thank you.