| Message ID | 20181208111606.8505-2-marcandre.lureau@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Hi, | expand |
Marc-André Lureau <marcandre.lureau@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.lureau@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?
Hi On Mon, Dec 10, 2018 at 1:52 PM Markus Armbruster <armbru@redhat.com> wrote: > > Marc-André Lureau <marcandre.lureau@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.lureau@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?
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Mon, Dec 10, 2018 at 1:52 PM Markus Armbruster <armbru@redhat.com> wrote: >> >> Marc-André Lureau <marcandre.lureau@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.lureau@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. All we have right now to help with enforcing our osdep.h rule is scripts/clean-includes. We run it periodically to fix rule breakers. It's manual, because we have a few .c files in the tree where the rule doesn't apply, and the script doesn't know about them. Fixable, I guess. Most recent run: Subject: [PATCH] Clean up includes Message-Id: <20181204172535.2799-1-armbru@redhat.com> https://lists.nongnu.org/archive/html/qemu-devel/2018-12/msg00605.html The obvious improvement would be flagging rule breakers before they get committed. checkpatch.pl could flag patches adding .c files that don't include "qemu/osdep.h" first. The "first" part might be a bit annoying to code. The "adding files" part already exists: checkpatch.pl warns when you add a file without updating MAINTAINERS. checkpatch.pl could also flag patches removing #include "qemu/osdep.h". Other projects use a syntax-check make target to check sources. If we decide we like that better than checkpatch.pl for some checks, we can do that.
Hi On Mon, Dec 10, 2018 at 5:28 PM Markus Armbruster <armbru@redhat.com> wrote: > > Marc-André Lureau <marcandre.lureau@gmail.com> writes: > > > Hi > > > > On Mon, Dec 10, 2018 at 1:52 PM Markus Armbruster <armbru@redhat.com> wrote: > >> > >> Marc-André Lureau <marcandre.lureau@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.lureau@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 ? > > All we have right now to help with enforcing our osdep.h rule is > scripts/clean-includes. We run it periodically to fix rule breakers. > It's manual, because we have a few .c files in the tree where the rule > doesn't apply, and the script doesn't know about them. Fixable, I > guess. Most recent run: > > Subject: [PATCH] Clean up includes > Message-Id: <20181204172535.2799-1-armbru@redhat.com> > https://lists.nongnu.org/archive/html/qemu-devel/2018-12/msg00605.html > > The obvious improvement would be flagging rule breakers before they get > committed. > > checkpatch.pl could flag patches adding .c files that don't include > "qemu/osdep.h" first. The "first" part might be a bit annoying to code. > The "adding files" part already exists: checkpatch.pl warns when you add > a file without updating MAINTAINERS. > > checkpatch.pl could also flag patches removing #include "qemu/osdep.h". > > Other projects use a syntax-check make target to check sources. If we > decide we like that better than checkpatch.pl for some checks, we can do > that.
On Mon, Dec 10, 2018 at 02:28:26PM +0100, Markus Armbruster wrote: > Marc-André Lureau <marcandre.lureau@gmail.com> writes: > > > Hi > > > > On Mon, Dec 10, 2018 at 1:52 PM Markus Armbruster <armbru@redhat.com> wrote: > >> > >> Marc-André Lureau <marcandre.lureau@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.lureau@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. > > All we have right now to help with enforcing our osdep.h rule is > scripts/clean-includes. We run it periodically to fix rule breakers. > It's manual, because we have a few .c files in the tree where the rule > doesn't apply, and the script doesn't know about them. Fixable, I > guess. Most recent run: > > Subject: [PATCH] Clean up includes > Message-Id: <20181204172535.2799-1-armbru@redhat.com> > https://lists.nongnu.org/archive/html/qemu-devel/2018-12/msg00605.html > > The obvious improvement would be flagging rule breakers before they get > committed. > > checkpatch.pl could flag patches adding .c files that don't include > "qemu/osdep.h" first. The "first" part might be a bit annoying to code. You can get this logic from GNULIBs syntax-check make rules. It uses this perl code to check that config.h is included first: while (<>) { if (/^# *include\b/) { if (not m{^# *include <config\.h>}) { print "$ARGV\n"; $$ret = 1; } close ARGV; } } Regards, Daniel
Marc-André Lureau <marcandre.lureau@gmail.com> writes: > Hi > > On Mon, Dec 10, 2018 at 5:28 PM Markus Armbruster <armbru@redhat.com> wrote: >> >> Marc-André Lureau <marcandre.lureau@gmail.com> writes: >> >> > Hi >> > >> > On Mon, Dec 10, 2018 at 1:52 PM Markus Armbruster <armbru@redhat.com> wrote: >> >> >> >> Marc-André Lureau <marcandre.lureau@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.lureau@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.
Daniel P. Berrangé <berrange@redhat.com> writes: > On Mon, Dec 10, 2018 at 02:28:26PM +0100, Markus Armbruster wrote: [...] >> checkpatch.pl could flag patches adding .c files that don't include >> "qemu/osdep.h" first. The "first" part might be a bit annoying to code. > > You can get this logic from GNULIBs syntax-check make rules. It uses > this perl code to check that config.h is included first: > > while (<>) { > if (/^# *include\b/) { > if (not m{^# *include <config\.h>}) { > print "$ARGV\n"; > $$ret = 1; > } > close ARGV; > } > } Thanks!
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" '''))
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.lureau@redhat.com> --- scripts/qapi/types.py | 3 +++ 1 file changed, 3 insertions(+)