Re: [Qemu-devel] [RFC] Can we remove special handling of standard headers (introduced for dyngen / OSX?)

[Blue Swirl]

On Mon, Oct 11, 2010 at 4:22 PM, Stefan Weil <address@hidden> wrote:
> Am 10.10.2010 00:46, schrieb Andreas Färber:
>>
>> Am 04.10.2010 um 21:29 schrieb Stefan Weil:
>>
>>> Am 25.09.2010 09:46, schrieb Blue Swirl:
>>>>
>>>> On Thu, Sep 23, 2010 at 8:44 PM, Stefan Weil <address@hidden>
>>>> wrote:
>>>>>
>>>>> Am 23.09.2010 22:33, schrieb Blue Swirl:
>>>>>>
>>>>>> On Thu, Sep 23, 2010 at 7:28 PM, Stefan Weil <address@hidden>
>>>>>> wrote:
>>>>>>>
>>>>>>> Replace the remaining format attribute printf by macro
>>>>>>> GCC_FMT_ATTR which uses gnu_printf (if supported).
>>>>>>>
>>>>>>> This needs additional code changes:
>>>>>>>
>>>>>>> * Add qemu-common.h (which defined GCC_FMT_ATTR) were needed.
>>>>>>>
>>>>>>> * Remove standard includes when qemu-common.h was added.
>>>>>>> qemu-common.h already provides these includes.
>>>>>>>
>>>>>>> * Remove local definitions which now come from stdio.h.
>>>>>>> These definitions were needed before tcg was introduced.
>>>>>>> They raise conflicts when qemu-common.h is included.
>>>>>>
>>>>>> IIRC the problem was that some system headers were incompatible with
>>>>>> global asm variables. There is still one, AREG0.
>>>>>>
>>>>>> But I'd rather not keep the hideous local definitions forever. Maybe
>>>>>> those systems which are broken by the patch are not interesting
>>>>>> anymore?
>>>>>
>>>>> Are there such systems? Or did the problems with earlier
>>>>> versions arise from the fact that a lot of global asm variables
>>>>> were reserved by qemu? How could a correctly defined AREG0
>>>>> interfere with system headers?
>>>>
>>>> One explanation is that the code in target-*/op_helper.c may assume
>>>> that env/AREG0 is always available but since library functions may
>>>> clobber that, we want to hide them.
>>>>
>>>>> For linux and win32, I did not notice problems caused by these changes.
>>>>
>>>> It looks like that code has origins in one of the very first patches,
>>>> r16 or ba1c6e37fc5efc0f3d1e50d0760f9f4a1061187b. It was moved from
>>>> exec-i386.h to dyngen-exec.h by r236 or
>>>> 79638566e5b87058e92f537b989df0dbc23f8b41. R997 or
>>>> 1e6cae953d6a37216359b79e05d23e1bf4dc6bbe added a warning about system
>>>> headers. The commits around that add OS X support, maybe the problem
>>>> was there?
>>>
>>>
>>> Current qemu code includes several locations where standard headers
>>> like stdio.h are forbidden. See qemu-common.h (__DYNGEN_EXEC_H__),
>>> dyngen-exec.h, cris-dis.h and perhaps more examples of this
>>> restriction.
>>>
>>> Who knows the reason for this restriction? Was it use of global registers
>>> with dyngen? Was it needed only for certain hosts (maybe OSX) or old
>>> versions of gcc?
>>>
>>> Most important: Is this restriction still needed?
>>>
>>> It would simplify things like common declarations and also allow
>>> cleaner code (without private declarations for FILE etc.) if we could
>>> remove this restriction.
>>>
>>> In my personal test environment (gcc-4.x, linux on different hosts,
>>> win32)
>>> the restriction was not needed.
>>
>> Tested-by: Andreas Färber <address@hidden>
>>
>> Patch 2/3 [1] compiles without warnings on Mac OS X v10.5 ppc64, using
>> Apple's GCC 4.0.1.
>> As guests, AIX/ppc64, Haiku/ppc, Fedora/x64 and Haiku/i386 booted as
>> before.
>>
>> Andreas
>>
>> [1] http://patchwork.ozlabs.org/patch/65574/
>
> Andreas, thank you for testing.
>
> Blue Swirl, for the next steps towards getting full format checks
> patch 2/3 or something equivalent is needed because GCC_FMT_ATTR
> (which is defined in qemu-common.h) should be available
> everywhere.
>
> With Andreas test results, we now know that Linux, OSX and Win32
> don't need the restriction regarding system include files like stdio.h.
>
> Do you think more tests are needed, should we wait for more feedback
> or can the patch be applied?

What about the library compatibility situation, do we know anything about that?

On the other hand, we could apply the patch and wait for bug reports.