BALATON Zoltan <bala...@eik.bme.hu> writes: > On Wed, 13 Mar 2019, Markus Armbruster wrote: >> BALATON Zoltan <bala...@eik.bme.hu> 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 <arm...@redhat.com> >> [...] >>>> 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.
