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.