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

[Thayne Harbaugh]

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 means that someone still needed to track down what a target time_t
is.  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.