[Top][All Lists]
[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]
Re: [Qemu-devel] [PATCH] target_posix_types.h
From: |
Thayne Harbaugh |
Subject: |
Re: [Qemu-devel] [PATCH] target_posix_types.h |
Date: |
Wed, 14 Nov 2007 12:53:44 -0700 |
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.