| Message ID | 20220215120625.64711-5-f4bug@amsat.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | buildsys: More fixes to use GCC on macOS | expand |
On Tue, 15 Feb 2022 at 12:13, Philippe Mathieu-Daudé via <qemu-devel@nongnu.org> wrote: > > We globally ignore the 'initializer overrides' warnings in C code > since commit c1556a812a ("configure: Disable (clang) > initializer-overrides warnings"). Unfortunately the ${gcc_flags} > variable is not propagated to Objective-C flags ($OBJCFLAGS). > Instead of reworking the configure script to test all supported > C flags in Objective-C and sanitize them, ignore the warning > locally with the GCC diagnostic #pragma (Clang ignores it). > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> I'm not really a fan of #pragma GCC diagnostic directives in specific source files, unless there's no alternative or the issue really is specific to one file. thanks -- PMM
On Dienstag, 15. Februar 2022 13:06:25 CET Philippe Mathieu-Daudé via wrote: > We globally ignore the 'initializer overrides' warnings in C code > since commit c1556a812a ("configure: Disable (clang) > initializer-overrides warnings"). Unfortunately the ${gcc_flags} > variable is not propagated to Objective-C flags ($OBJCFLAGS). > Instead of reworking the configure script to test all supported > C flags in Objective-C and sanitize them, ignore the warning > locally with the GCC diagnostic #pragma (Clang ignores it). > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > --- What about this instead: diff --git a/ui/cocoa.m b/ui/cocoa.m index ac18e14ce0..df9b9be999 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -652,9 +652,7 @@ QemuCocoaView *cocoaView; /* translates Macintosh keycodes to QEMU's keysym */ - int without_control_translation[] = { - [0 ... 0xff] = 0, // invalid key - + int without_control_translation[256] = { [kVK_UpArrow] = QEMU_KEY_UP, [kVK_DownArrow] = QEMU_KEY_DOWN, [kVK_RightArrow] = QEMU_KEY_RIGHT, @@ -667,9 +665,7 @@ QemuCocoaView *cocoaView; [kVK_Delete] = QEMU_KEY_BACKSPACE, }; - int with_control_translation[] = { - [0 ... 0xff] = 0, // invalid key - + int with_control_translation[256] = { [kVK_UpArrow] = QEMU_KEY_CTRL_UP, [kVK_DownArrow] = QEMU_KEY_CTRL_DOWN, [kVK_RightArrow] = QEMU_KEY_CTRL_RIGHT, That warning should only be raised on overlapping designated initializations which strictly is undefined behaviour. Because which order defines the precedence of overlapping initializers, is it the order of the designated intializer list, or rather the memory order? See also: https://stackoverflow.com/questions/40920714/is-full-followed-by-partial-initialization-of-a-subobject-undefined-behavior So I have my doubts whether this warning suppression should be used in QEMU at all. Best regards, Christian Schoenebeck
On Tue, Feb 15, 2022 at 9:35 PM Peter Maydell <peter.maydell@linaro.org> wrote: > > On Tue, 15 Feb 2022 at 12:13, Philippe Mathieu-Daudé via > <qemu-devel@nongnu.org> wrote: > > > > We globally ignore the 'initializer overrides' warnings in C code > > since commit c1556a812a ("configure: Disable (clang) > > initializer-overrides warnings"). Unfortunately the ${gcc_flags} > > variable is not propagated to Objective-C flags ($OBJCFLAGS). > > Instead of reworking the configure script to test all supported > > C flags in Objective-C and sanitize them, ignore the warning > > locally with the GCC diagnostic #pragma (Clang ignores it). > > > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > I'm not really a fan of #pragma GCC diagnostic directives in > specific source files, unless there's no alternative or > the issue really is specific to one file. > > thanks > -- PMM What about fixing then? Something like this should do: --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -652,9 +652,7 @@ - (void) handleMonitorInput:(NSEvent *)event /* translates Macintosh keycodes to QEMU's keysym */ - int without_control_translation[] = { - [0 ... 0xff] = 0, // invalid key - + int without_control_translation[256] = { [kVK_UpArrow] = QEMU_KEY_UP, [kVK_DownArrow] = QEMU_KEY_DOWN, [kVK_RightArrow] = QEMU_KEY_RIGHT, @@ -667,9 +665,7 @@ - (void) handleMonitorInput:(NSEvent *)event [kVK_Delete] = QEMU_KEY_BACKSPACE, }; - int with_control_translation[] = { - [0 ... 0xff] = 0, // invalid key - + int with_control_translation[256] = { [kVK_UpArrow] = QEMU_KEY_CTRL_UP, [kVK_DownArrow] = QEMU_KEY_CTRL_DOWN, [kVK_RightArrow] = QEMU_KEY_CTRL_RIGHT,
On 15/2/22 14:19, Akihiko Odaki wrote: > On Tue, Feb 15, 2022 at 9:35 PM Peter Maydell <peter.maydell@linaro.org> wrote: >> >> On Tue, 15 Feb 2022 at 12:13, Philippe Mathieu-Daudé via >> <qemu-devel@nongnu.org> wrote: >>> >>> We globally ignore the 'initializer overrides' warnings in C code >>> since commit c1556a812a ("configure: Disable (clang) >>> initializer-overrides warnings"). Unfortunately the ${gcc_flags} >>> variable is not propagated to Objective-C flags ($OBJCFLAGS). >>> Instead of reworking the configure script to test all supported >>> C flags in Objective-C and sanitize them, ignore the warning >>> locally with the GCC diagnostic #pragma (Clang ignores it). >>> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >> >> I'm not really a fan of #pragma GCC diagnostic directives in >> specific source files, unless there's no alternative or >> the issue really is specific to one file. >> >> thanks >> -- PMM > > What about fixing then? Something like this should do: > > --- a/ui/cocoa.m > +++ b/ui/cocoa.m > @@ -652,9 +652,7 @@ - (void) handleMonitorInput:(NSEvent *)event > > /* translates Macintosh keycodes to QEMU's keysym */ > > - int without_control_translation[] = { > - [0 ... 0xff] = 0, // invalid key > - > + int without_control_translation[256] = { > [kVK_UpArrow] = QEMU_KEY_UP, > [kVK_DownArrow] = QEMU_KEY_DOWN, > [kVK_RightArrow] = QEMU_KEY_RIGHT, Clever =)
On Tue, 15 Feb 2022 at 13:18, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > On Dienstag, 15. Februar 2022 13:06:25 CET Philippe Mathieu-Daudé via wrote: > > We globally ignore the 'initializer overrides' warnings in C code > > since commit c1556a812a ("configure: Disable (clang) > > initializer-overrides warnings"). Unfortunately the ${gcc_flags} > > variable is not propagated to Objective-C flags ($OBJCFLAGS). > > Instead of reworking the configure script to test all supported > > C flags in Objective-C and sanitize them, ignore the warning > > locally with the GCC diagnostic #pragma (Clang ignores it). > > > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > --- > > What about this instead: > > diff --git a/ui/cocoa.m b/ui/cocoa.m > index ac18e14ce0..df9b9be999 100644 > --- a/ui/cocoa.m > +++ b/ui/cocoa.m > @@ -652,9 +652,7 @@ QemuCocoaView *cocoaView; > > /* translates Macintosh keycodes to QEMU's keysym */ > > - int without_control_translation[] = { > - [0 ... 0xff] = 0, // invalid key > - > + int without_control_translation[256] = { I think this won't zero-initialize, because this is a function level variable, not a global or static, but maybe ObjectiveC rules differ from C here? (Besides, it really should be a static const array.) That said... > That warning should only be raised on overlapping designated initializations > which strictly is undefined behaviour. Because which order defines the > precedence of overlapping initializers, is it the order of the designated > intializer list, or rather the memory order? This is defined: textually last initializer wins. https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html > See also: > https://stackoverflow.com/questions/40920714/is-full-followed-by-partial-initialization-of-a-subobject-undefined-behavior That's about struct initializers; we're dealing with array initializers here. > So I have my doubts whether this warning suppression should be used in QEMU at > all. The warning is useful in the pure-standard-C world where there is no such thing as the "range initialization" syntax. In that case trying to initialize the same array member twice is very likely a bug. However, if you use range initialization then the pattern "initialize a range first, then override some specific members within it" is very common and the warning is incorrect here. We use and like the range-initialization syntax, and so we disable the -Winitializer-overrides warnings. The bug here is just that we are incorrectly failing to apply the warning flags we use for C code when we compile ObjC files. thanks -- PMM
On 15/2/22 14:45, Peter Maydell wrote: > On Tue, 15 Feb 2022 at 13:18, Christian Schoenebeck > <qemu_oss@crudebyte.com> wrote: >> >> On Dienstag, 15. Februar 2022 13:06:25 CET Philippe Mathieu-Daudé via wrote: >>> We globally ignore the 'initializer overrides' warnings in C code >>> since commit c1556a812a ("configure: Disable (clang) >>> initializer-overrides warnings"). Unfortunately the ${gcc_flags} >>> variable is not propagated to Objective-C flags ($OBJCFLAGS). >>> Instead of reworking the configure script to test all supported >>> C flags in Objective-C and sanitize them, ignore the warning >>> locally with the GCC diagnostic #pragma (Clang ignores it). >>> >>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> >>> --- > The warning is useful in the pure-standard-C world where there > is no such thing as the "range initialization" syntax. In that > case trying to initialize the same array member twice is very > likely a bug. However, if you use range initialization then > the pattern "initialize a range first, then override some specific > members within it" is very common and the warning is incorrect here. > We use and like the range-initialization syntax, and so we disable > the -Winitializer-overrides warnings. The bug here is just that > we are incorrectly failing to apply the warning flags we use > for C code when we compile ObjC files. Yes, but simply adding nowarn_flags to objc_args isn't enough, we need to filter the flags similarly to what is done in C to avoid: [7/373] Compiling Objective-C object libcommon.fa.p/audio_coreaudio.m.o warning: unknown warning option '-Wold-style-declaration'; did you mean '-Wout-of-line-declaration'? [-Wunknown-warning-option] warning: unknown warning option '-Wimplicit-fallthrough=2'; did you mean '-Wimplicit-fallthrough'? [-Wunknown-warning-option] 2 warnings generated. [34/373] Compiling Objective-C object libcommon.fa.p/ui_cocoa.m.o warning: unknown warning option '-Wold-style-declaration'; did you mean '-Wout-of-line-declaration'? [-Wunknown-warning-option] warning: unknown warning option '-Wimplicit-fallthrough=2'; did you mean '-Wimplicit-fallthrough'? [-Wunknown-warning-option] 2 warnings generated.
On Dienstag, 15. Februar 2022 14:45:00 CET Peter Maydell wrote: > On Tue, 15 Feb 2022 at 13:18, Christian Schoenebeck > > <qemu_oss@crudebyte.com> wrote: > > On Dienstag, 15. Februar 2022 13:06:25 CET Philippe Mathieu-Daudé via wrote: > > > We globally ignore the 'initializer overrides' warnings in C code > > > since commit c1556a812a ("configure: Disable (clang) > > > initializer-overrides warnings"). Unfortunately the ${gcc_flags} > > > variable is not propagated to Objective-C flags ($OBJCFLAGS). > > > Instead of reworking the configure script to test all supported > > > C flags in Objective-C and sanitize them, ignore the warning > > > locally with the GCC diagnostic #pragma (Clang ignores it). > > > > > > Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> > > > --- > > > > What about this instead: > > > > diff --git a/ui/cocoa.m b/ui/cocoa.m > > index ac18e14ce0..df9b9be999 100644 > > --- a/ui/cocoa.m > > +++ b/ui/cocoa.m > > @@ -652,9 +652,7 @@ QemuCocoaView *cocoaView; > > > > /* translates Macintosh keycodes to QEMU's keysym */ > > > > - int without_control_translation[] = { > > - [0 ... 0xff] = 0, // invalid key > > - > > + int without_control_translation[256] = { > > I think this won't zero-initialize, because this is a function > level variable, not a global or static, but maybe ObjectiveC > rules differ from C here? (Besides, it really should be > a static const array.) That said... That's a base requirement for designated initializers to fallback to automatic default initialization (zero) for omitted members. It's like: int a[10] = { }; // all zero It works for me (including on function level) with both GCC and clang, on Linux and macOS: #include <stdio.h> int main() { int a[] = { [4] = 409, [6] = 609, [9] = 909 }; for (int i = 0; i < 10; ++i) printf("a[%d] = %d\n", i, a[i]); return 0; } Was this ever not the case? > > That warning should only be raised on overlapping designated > > initializations which strictly is undefined behaviour. Because which > > order defines the precedence of overlapping initializers, is it the order > > of the designated intializer list, or rather the memory order? > > This is defined: textually last initializer wins. > https://gcc.gnu.org/onlinedocs/gcc/Designated-Inits.html > > > See also: > > https://stackoverflow.com/questions/40920714/is-full-followed-by-partial-i > > nitialization-of-a-subobject-undefined-behavior > That's about struct initializers; we're dealing with array initializers > here. Yes, but if you suppress this warning globally, you shut up the potential warning for the linked case as well. And as demonstrated there, you would end up with different/unexpected behaviour depending on the precise compiler being used. So I still think it is not a good idea to suppress this warning globally. Best regards, Christian Schoenebeck > > So I have my doubts whether this warning suppression should be used in > > QEMU at all. > > The warning is useful in the pure-standard-C world where there > is no such thing as the "range initialization" syntax. In that > case trying to initialize the same array member twice is very > likely a bug. However, if you use range initialization then > the pattern "initialize a range first, then override some specific > members within it" is very common and the warning is incorrect here. > We use and like the range-initialization syntax, and so we disable > the -Winitializer-overrides warnings. The bug here is just that > we are incorrectly failing to apply the warning flags we use > for C code when we compile ObjC files. > > thanks > -- PMM
On Tue, 15 Feb 2022 at 14:11, Christian Schoenebeck <qemu_oss@crudebyte.com> wrote: > > On Dienstag, 15. Februar 2022 14:45:00 CET Peter Maydell wrote: > > I think this won't zero-initialize, because this is a function > > level variable, not a global or static, but maybe ObjectiveC > > rules differ from C here? (Besides, it really should be > > a static const array.) That said... > > That's a base requirement for designated initializers to fallback to automatic > default initialization (zero) for omitted members. It's like: > > int a[10] = { }; // all zero Ah, yes, you're right. > Yes, but if you suppress this warning globally, you shut up the potential > warning for the linked case as well. And as demonstrated there, you would end > up with different/unexpected behaviour depending on the precise compiler being > used. > > So I still think it is not a good idea to suppress this warning globally. We *must* suppress it globally, because we make wide use of the array-range-initializer idiom that it incorrectly complains about, and we don't want to discourage use of that. This is fairly standard for compiler warnings: sometimes the compiler complains too much, or the intent of the warning is something we disagree with. Those warnings we disable or don't enable. In this particular case, if the compilers ever develop versions of this warning that complain about actual bugs without also warning about false positives like this one, we could turn the warning on again. Put another way, there's a tradeoff here. Sometimes it's worth distorting the way we would write code to avoid compiler warnings, because we'd like to keep them enabled to catch some real bugs. Sometimes the distortion would be too much, and then we take the choice to disable the warning. There's a judgement call based on how much perfectly-fine code the compiler is warning about and how much worse the rewritten code would be. In the case of this warning we've decided that the tradeoff favours disabling the warning. Having made that decision on a codebase-wide basis, we should then just adhere to it whether we're building ObjC or C. thanks -- PMM
diff --git a/ui/cocoa.m b/ui/cocoa.m index 30702d31a5..8956694447 100644 --- a/ui/cocoa.m +++ b/ui/cocoa.m @@ -638,6 +638,9 @@ QemuCocoaView *cocoaView; qkbd_state_key_event(kbd, keycode, !qkbd_state_key_get(kbd, keycode)); } +#pragma GCC diagnostic push +#pragma GCC diagnostic ignored "-Winitializer-overrides" + // Does the work of sending input to the monitor - (void) handleMonitorInput:(NSEvent *)event { @@ -702,6 +705,8 @@ QemuCocoaView *cocoaView; } } +#pragma GCC diagnostic pop + - (bool) handleEvent:(NSEvent *)event { if(!allow_events) {
We globally ignore the 'initializer overrides' warnings in C code since commit c1556a812a ("configure: Disable (clang) initializer-overrides warnings"). Unfortunately the ${gcc_flags} variable is not propagated to Objective-C flags ($OBJCFLAGS). Instead of reworking the configure script to test all supported C flags in Objective-C and sanitize them, ignore the warning locally with the GCC diagnostic #pragma (Clang ignores it). Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org> --- ui/cocoa.m | 5 +++++ 1 file changed, 5 insertions(+)