Marc-André Lureau <marcandre.lur...@gmail.com> writes: > Hi > > On Mon, Dec 10, 2018 at 5:28 PM Markus Armbruster <arm...@redhat.com> wrote: >> >> Marc-André Lureau <marcandre.lur...@gmail.com> writes: >> >> > Hi >> > >> > On Mon, Dec 10, 2018 at 1:52 PM Markus Armbruster <arm...@redhat.com> >> > wrote: >> >> >> >> Marc-André Lureau <marcandre.lur...@redhat.com> writes: >> >> >> >> > Now that the schema can be configured, it is crucial that all types >> >> > are configured the same. Make sure config-host.h is included, by >> >> > checking osdep.h inclusion. The build-sys tracks the dependency and >> >> > rebuilds the types if the configuration changed. >> >> > >> >> > Signed-off-by: Marc-André Lureau <marcandre.lur...@redhat.com> >> >> > --- >> >> > scripts/qapi/types.py | 3 +++ >> >> > 1 file changed, 3 insertions(+) >> >> > >> >> > diff --git a/scripts/qapi/types.py b/scripts/qapi/types.py >> >> > index fd7808103c..3bb9cb6d47 100644 >> >> > --- a/scripts/qapi/types.py >> >> > +++ b/scripts/qapi/types.py >> >> > @@ -201,6 +201,9 @@ class >> >> > QAPISchemaGenTypeVisitor(QAPISchemaModularCVisitor): >> >> > ''', >> >> > types=types, visit=visit)) >> >> > self._genh.preamble_add(mcgen(''' >> >> > +#ifndef QEMU_OSDEP_H >> >> > +#error Please include osdep.h first! >> >> > +#endif >> >> > #include "qapi/qapi-builtin-types.h" >> >> > ''')) >> >> >> >> I understand why you propose this patch. The QAPI-generated headers use >> >> #ifdef CONFIG_FOO. The configuration header "qemu/osdep.h" must be >> >> consistently included before the generated headers, or else you end up >> >> with nasty bugs, such as the same enum having different values in >> >> different .o, or the same struct having a different layout. >> >> >> >> But this applies to *all* headers that use #ifdef. Why check it here, >> >> but not there? What makes the QAPI-generated headers special? >> >> >> > >> > It's the discussion about #if in headers (and enums in particular) >> > that started this. We want to make sure all compilation units share >> > the same data structure/ABI. I proposed to include osdep.h in qapi >> > headers, but that was rejected. >> > The warning is a different approach. I agree it could apply to all >> > headers. Do you think I should update all headers as well? >> >> That would replace the rule 'all .c files must include "qemu/osdep.h" >> first' by 'all .h files must check "qemu/osdep.h" has been included >> already', which is not an improvement. > > That would be an improvement since headers may also be impacted by osdep.h > > Alternatively, why not use -include ?
Requiring .c to include the configuration header first follows established Autoconf practice. The "first" part covers all headers. I've seen plenty of autoconfiscated code, yet none where the headers double-check the configuration header has been included already. Such a check is neither sufficient nor really necessary. Checking the .c is both. I have no strong feelings about using -include instead. I'd prefer to avoid the difference to what other projects do, though. Circling back to the issue that made you propose this patch. I think I'm (belatedly) coming around to your initial view that the use of conditionals in generated QAPI code is safe enough. I shouldn't be worried about #if defined(CONFIG_HOST_THING) where CONFIG_HOST_THING belongs to config-host.h. That's always pulled in via qemu/osdep.h. I also shouldn't be worried about #if defined(CONFIG_TARGET_THING) where CONFIG_TARGET_THING belongs to config-target.h. qemu/osdep.h pulls in either that or exec/poison.h. I even shouldn't be worried about #if defined(RANDOM_MACRO) as long as qemu/osdep.h pulls in its owner or poisons it. I might legitimately be worried about the "as long as" part. We could perhaps extract the ifconds and look for macros that belong neither to config-host.h nor config-target.h. But I'm not sure it's worth the bother.