Re: [Qemu-devel] [PATCH] Clean up includes

[Markus Armbruster]

BALATON Zoltan <address@hidden> writes:

> On Wed, 13 Mar 2019, Markus Armbruster wrote:
>> BALATON Zoltan <address@hidden> writes:
>>> On Wed, 13 Mar 2019, Markus Armbruster wrote:
>>>> Clean up includes so that osdep.h is included first and headers
>>>> which it implies are not included manually.
>>>>
>>>> This commit was created with scripts/clean-includes, with the changes
>>>> to the following files manually reverted:
>>>>
>>>>    contrib/libvhost-user/libvhost-user-glib.h
>>>>    contrib/libvhost-user/libvhost-user.c
>>>>    contrib/libvhost-user/libvhost-user.h
>>>>    linux-user/mips64/cpu_loop.c
>>>>    linux-user/mips64/signal.c
>>>>    linux-user/sparc64/cpu_loop.c
>>>>    linux-user/sparc64/signal.c
>>>>    linux-user/x86_64/cpu_loop.c
>>>>    linux-user/x86_64/signal.c
>>>>    slirp/src/*
>>>>    target/s390x/gen-features.c
>>>>    tests/migration/s390x/a-b-bios.c
>>>>    tests/test-rcu-simpleq.c
>>>>    tests/test-rcu-tailq.c
>>>>    tests/uefi-test-tools/UefiTestToolsPkg/BiosTablesTest/BiosTablesTest.c
>>>>
>>>> We're in the process of spinning out slirp/.  tests/uefi-test-tools/
>>>> is guest software.  The remaining reverts are the same as in commit
>>>> b7d89466dde.
>>>>
>>>> Signed-off-by: Markus Armbruster <address@hidden>
>> [...]
>>>> diff --git a/hw/display/ati_2d.c b/hw/display/ati_2d.c
>>>> index bc98ba6eeb..f31b3c27c7 100644
>>>> --- a/hw/display/ati_2d.c
>>>> +++ b/hw/display/ati_2d.c
>>>> @@ -7,6 +7,7 @@
>>>>  * This work is licensed under the GNU GPL license version 2 or later.
>>>>  */
>>>>
>>>> +#include "qemu/osdep.h"
>>>> #include "ati_int.h"
>>>> #include "ati_regs.h"
>>>> #include "qemu/log.h"
>>>> diff --git a/hw/display/ati_dbg.c b/hw/display/ati_dbg.c
>>>> index 1e6c32624e..b045f81d06 100644
>>>> --- a/hw/display/ati_dbg.c
>>>> +++ b/hw/display/ati_dbg.c
>>>> @@ -1,3 +1,4 @@
>>>> +#include "qemu/osdep.h"
>>>> #include "ati_int.h"
>>>>
>>>> #ifdef DEBUG_ATI
>>>> diff --git a/hw/display/ati_int.h b/hw/display/ati_int.h
>>>> index a6f3e20e63..2f426064cf 100644
>>>> --- a/hw/display/ati_int.h
>>>> +++ b/hw/display/ati_int.h
>>>> @@ -9,7 +9,6 @@
>>>> #ifndef ATI_INT_H
>>>> #define ATI_INT_H
>>>>
>>>> -#include "qemu/osdep.h"
>>>
>>> What's wrong with ati_int.h including osdep.h first and everything
>>> else including ati_int.h first? I think it was OK that way so unless
>>> there's a good reason to explicitely include osdep in all files that
>>> also include ati_int.h I think these should not be changed. For the
>>> ati model we need ati_int.h included first so it's OK to include
>>> osdep.h from there.
>>
>> ./HACKING explains:
>>
>>    1.2. Include directives
>>
>>    Order include directives as follows:
>>
>>    #include "qemu/osdep.h"  /* Always first... */
>>    #include <...>           /* then system headers... */
>>    #include "..."           /* and finally QEMU headers. */
>>
>>    The "qemu/osdep.h" header contains preprocessor macros that affect the 
>> behavior
>>    of core system headers like <stdint.h>.  It must be the first include so 
>> that
>>    core system headers included by external libraries get the preprocessor 
>> macros
>>    that QEMU depends on.
>>
>>    Do not include "qemu/osdep.h" from header files since the .c file will 
>> have
>>    already included it.
>
> I'm not convinced. The rule is to include osdep.h first and it's
> currently satisified without this change as well but if it makes
> clean-includes script happy then I don't mind.

The rule is actually to include osdep.h first in .c, and never in .h.

We chose this rule because it makes conformance locally obvious.  You
don't have to chase down a chain of first includes to verify it ends in
"osdep.h" and none of the intermediate headers have anything before
their first #include.

The obviousness translates into robustness here.  If you hide #include
"osdep.h" in a header, you can easily lose "firstness" in a
harmless-looking change to any of the sources involved.  Pray this
breaks the build, because if it breaks only at run-time, it'll be nasty
to debug.