[RFC PATCH 0/4] buildsys: More fixes to use GCC on macOS

[RFC PATCH 0/4] buildsys: More fixes to use GCC on macOS

[Philippe Mathieu-Daudé via]

Few fixes to be able to use GCC extensions which are not
available on Clang.

Philippe Mathieu-Daudé (4):
  osdep: Avoid using Clang-specific __builtin_available()
  osdep: Un-inline qemu_thread_jit_execute/write
  audio: Rename coreaudio extension to use Objective-C compiler
  ui/cocoa: Ignore 'initializer overrides' warnings

 audio/{coreaudio.c => coreaudio.m} |  0
 audio/meson.build                  |  2 +-
 include/qemu/osdep.h               | 21 ++-------------------
 ui/cocoa.m                         |  5 +++++
 util/osdep.c                       | 20 ++++++++++++++++++++
 5 files changed, 28 insertions(+), 20 deletions(-)
 rename audio/{coreaudio.c => coreaudio.m} (100%)

-- 
2.34.1

[RFC PATCH 1/4] osdep: Avoid using Clang-specific __builtin_available()

[Philippe Mathieu-Daudé via]

Replace MAC_OS_X_VERSION_MAX_ALLOWED upper check by the lower
one (MAC_OS_X_VERSION_MIN_REQUIRED) and remove the Clang specific
__builtin_available() to allow building with GCC.

Remove the Clang specific __builtin_available() to allow building
with GCC, otherwise we get:

  include/qemu/osdep.h: In function 'qemu_thread_jit_write':
  include/qemu/osdep.h:787:9: warning: implicit declaration of function '__builtin_available'; did you mean '__builtin_scalbl'? [-Wimplicit-function-declaration]
    787 |     if (__builtin_available(macOS 11.0, *)) {
        |         ^~~~~~~~~~~~~~~~~~~
        |         __builtin_scalbl
  include/qemu/osdep.h:787:9: warning: nested extern declaration of '__builtin_available' [-Wnested-externs]
  include/qemu/osdep.h:787:29: error: 'macOS' undeclared (first use in this function)
    787 |     if (__builtin_available(macOS 11.0, *)) {
        |                             ^~~~~
  include/qemu/osdep.h:787:29: note: each undeclared identifier is reported only once for each function it appears in
  include/qemu/osdep.h:787:34: error: expected ')' before numeric constant
    787 |     if (__builtin_available(macOS 11.0, *)) {
        |                            ~     ^~~~~
        |                                  )

Replace the MAC_OS_X_VERSION_MAX_ALLOWED upper check by a lower
one (with MAC_OS_X_VERSION_MIN_REQUIRED) to avoid on Catalina:

  include/qemu/osdep.h:780:5: warning: 'pthread_jit_write_protect_np' is only available on macOS 11.0 or newer [-Wunguarded-availability-new]
      pthread_jit_write_protect_np(true);
      ^~~~~~~~~~~~~~~~~~~~~~~~~~~~
  /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX.sdk/usr/include/pthread.h:561:6: note: 'pthread_jit_write_protect_np' has been marked as being introduced in macOS 11.0 here, but the deployment target is macOS 10.15.0
  void pthread_jit_write_protect_np(int enabled);
       ^

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/qemu/osdep.h | 10 +++-------
 1 file changed, 3 insertions(+), 7 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index d1660d67fa..1e7a002339 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -774,19 +774,15 @@ size_t qemu_get_host_physmem(void);
  * for the current thread.
  */
 #if defined(MAC_OS_VERSION_11_0) && \
-    MAC_OS_X_VERSION_MAX_ALLOWED >= MAC_OS_VERSION_11_0
+    MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
 static inline void qemu_thread_jit_execute(void)
 {
-    if (__builtin_available(macOS 11.0, *)) {
-        pthread_jit_write_protect_np(true);
-    }
+    pthread_jit_write_protect_np(true);
 }
 
 static inline void qemu_thread_jit_write(void)
 {
-    if (__builtin_available(macOS 11.0, *)) {
-        pthread_jit_write_protect_np(false);
-    }
+    pthread_jit_write_protect_np(false);
 }
 #else
 static inline void qemu_thread_jit_write(void) {}
-- 
2.34.1

[PATCH 2/4] osdep: Un-inline qemu_thread_jit_execute/write

[Philippe Mathieu-Daudé via]

qemu_thread_jit_execute() and qemu_thread_jit_write() call
pthread_jit_write_protect_np() which is declared in "pthread.h".
Since we don't want all C files to preprocess unused headers,
avoid adding yet another header here and move the function
definitions to osdep.c, un-inlining them.

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 include/qemu/osdep.h | 17 ++---------------
 util/osdep.c         | 20 ++++++++++++++++++++
 2 files changed, 22 insertions(+), 15 deletions(-)

diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
index 1e7a002339..785884728b 100644
--- a/include/qemu/osdep.h
+++ b/include/qemu/osdep.h
@@ -773,21 +773,8 @@ size_t qemu_get_host_physmem(void);
  * Toggle write/execute on the pages marked MAP_JIT
  * for the current thread.
  */
-#if defined(MAC_OS_VERSION_11_0) && \
-    MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
-static inline void qemu_thread_jit_execute(void)
-{
-    pthread_jit_write_protect_np(true);
-}
-
-static inline void qemu_thread_jit_write(void)
-{
-    pthread_jit_write_protect_np(false);
-}
-#else
-static inline void qemu_thread_jit_write(void) {}
-static inline void qemu_thread_jit_execute(void) {}
-#endif
+void qemu_thread_jit_execute(void);
+void qemu_thread_jit_write(void);
 
 /**
  * Platforms which do not support system() return ENOSYS
diff --git a/util/osdep.c b/util/osdep.c
index 42a0a4986a..b228a53612 100644
--- a/util/osdep.c
+++ b/util/osdep.c
@@ -124,6 +124,26 @@ int qemu_mprotect_none(void *addr, size_t size)
 #endif
 }
 
+static void qemu_pthread_jit_write_protect(bool enabled)
+{
+#if defined(MAC_OS_VERSION_11_0) \
+        && MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
+    if (__builtin_available(macOS 11.0, *)) {
+        pthread_jit_write_protect_np(enabled);
+    }
+#endif
+}
+
+void qemu_thread_jit_execute(void)
+{
+    qemu_pthread_jit_write_protect(true);
+}
+
+void qemu_thread_jit_write(void)
+{
+    qemu_pthread_jit_write_protect(false);
+}
+
 #ifndef _WIN32
 
 static int fcntl_op_setlk = -1;
-- 
2.34.1

Re: [PATCH 2/4] osdep: Un-inline qemu_thread_jit_execute/write

[Akihiko Odaki]

On Tue, Feb 15, 2022 at 9:06 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> qemu_thread_jit_execute() and qemu_thread_jit_write() call
> pthread_jit_write_protect_np() which is declared in "pthread.h".
> Since we don't want all C files to preprocess unused headers,
> avoid adding yet another header here and move the function
> definitions to osdep.c, un-inlining them.
>
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---
>  include/qemu/osdep.h | 17 ++---------------
>  util/osdep.c         | 20 ++++++++++++++++++++
>  2 files changed, 22 insertions(+), 15 deletions(-)
>
> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
> index 1e7a002339..785884728b 100644
> --- a/include/qemu/osdep.h
> +++ b/include/qemu/osdep.h
> @@ -773,21 +773,8 @@ size_t qemu_get_host_physmem(void);
>   * Toggle write/execute on the pages marked MAP_JIT
>   * for the current thread.
>   */
> -#if defined(MAC_OS_VERSION_11_0) && \
> -    MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
> -static inline void qemu_thread_jit_execute(void)
> -{
> -    pthread_jit_write_protect_np(true);
> -}
> -
> -static inline void qemu_thread_jit_write(void)
> -{
> -    pthread_jit_write_protect_np(false);
> -}
> -#else
> -static inline void qemu_thread_jit_write(void) {}
> -static inline void qemu_thread_jit_execute(void) {}
> -#endif
> +void qemu_thread_jit_execute(void);
> +void qemu_thread_jit_write(void);
>
>  /**
>   * Platforms which do not support system() return ENOSYS
> diff --git a/util/osdep.c b/util/osdep.c
> index 42a0a4986a..b228a53612 100644
> --- a/util/osdep.c
> +++ b/util/osdep.c
> @@ -124,6 +124,26 @@ int qemu_mprotect_none(void *addr, size_t size)
>  #endif
>  }
>
> +static void qemu_pthread_jit_write_protect(bool enabled)
> +{
> +#if defined(MAC_OS_VERSION_11_0) \
> +        && MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
> +    if (__builtin_available(macOS 11.0, *)) {
> +        pthread_jit_write_protect_np(enabled);
> +    }
> +#endif
> +}
> +
> +void qemu_thread_jit_execute(void)
> +{
> +    qemu_pthread_jit_write_protect(true);
> +}
> +
> +void qemu_thread_jit_write(void)
> +{
> +    qemu_pthread_jit_write_protect(false);
> +}
> +
>  #ifndef _WIN32
>
>  static int fcntl_op_setlk = -1;
> --
> 2.34.1
>

Is this for compile-time reduction? If so, it would be nice if you
provide some numbers. It should have some explanation of the advantage
otherwise.

Re: [PATCH 2/4] osdep: Un-inline qemu_thread_jit_execute/write

[Philippe Mathieu-Daudé via]

On 15/2/22 14:09, Akihiko Odaki wrote:
> On Tue, Feb 15, 2022 at 9:06 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> qemu_thread_jit_execute() and qemu_thread_jit_write() call
>> pthread_jit_write_protect_np() which is declared in "pthread.h".
>> Since we don't want all C files to preprocess unused headers,
>> avoid adding yet another header here and move the function
>> definitions to osdep.c, un-inlining them.
>>
>> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
>> ---
>>   include/qemu/osdep.h | 17 ++---------------
>>   util/osdep.c         | 20 ++++++++++++++++++++
>>   2 files changed, 22 insertions(+), 15 deletions(-)
>>
>> diff --git a/include/qemu/osdep.h b/include/qemu/osdep.h
>> index 1e7a002339..785884728b 100644
>> --- a/include/qemu/osdep.h
>> +++ b/include/qemu/osdep.h
>> @@ -773,21 +773,8 @@ size_t qemu_get_host_physmem(void);
>>    * Toggle write/execute on the pages marked MAP_JIT
>>    * for the current thread.
>>    */
>> -#if defined(MAC_OS_VERSION_11_0) && \
>> -    MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
>> -static inline void qemu_thread_jit_execute(void)
>> -{
>> -    pthread_jit_write_protect_np(true);
>> -}
>> -
>> -static inline void qemu_thread_jit_write(void)
>> -{
>> -    pthread_jit_write_protect_np(false);
>> -}
>> -#else
>> -static inline void qemu_thread_jit_write(void) {}
>> -static inline void qemu_thread_jit_execute(void) {}
>> -#endif
>> +void qemu_thread_jit_execute(void);
>> +void qemu_thread_jit_write(void);
>>
>>   /**
>>    * Platforms which do not support system() return ENOSYS
>> diff --git a/util/osdep.c b/util/osdep.c
>> index 42a0a4986a..b228a53612 100644
>> --- a/util/osdep.c
>> +++ b/util/osdep.c
>> @@ -124,6 +124,26 @@ int qemu_mprotect_none(void *addr, size_t size)
>>   #endif
>>   }
>>
>> +static void qemu_pthread_jit_write_protect(bool enabled)
>> +{
>> +#if defined(MAC_OS_VERSION_11_0) \
>> +        && MAC_OS_X_VERSION_MIN_REQUIRED >= MAC_OS_VERSION_11_0
>> +    if (__builtin_available(macOS 11.0, *)) {
>> +        pthread_jit_write_protect_np(enabled);
>> +    }
>> +#endif
>> +}
>> +
>> +void qemu_thread_jit_execute(void)
>> +{
>> +    qemu_pthread_jit_write_protect(true);
>> +}
>> +
>> +void qemu_thread_jit_write(void)
>> +{
>> +    qemu_pthread_jit_write_protect(false);
>> +}
>> +
>>   #ifndef _WIN32
>>
>>   static int fcntl_op_setlk = -1;
>> --
>> 2.34.1
>>
> 
> Is this for compile-time reduction? If so, it would be nice if you
> provide some numbers. It should have some explanation of the advantage
> otherwise.

No, no number, but when using GCC on Monterey, the C files not including
"pthread.h" complain qemu_pthread_jit_write_protect() is not declared,
although not using qemu_thread_jit_execute(). I'll check why GCC is not
eliding unused inlined code.

[RFC PATCH 3/4] audio: Rename coreaudio extension to use Objective-C compiler

[Philippe Mathieu-Daudé via]

The coreaudio library includes Objective-C declarations (using the
caret '^' symbol to declare block references [*]). When building
with a C compiler we get:

  [175/839] Compiling C object libcommon.fa.p/audio_coreaudio.c.o
    In file included from /Library/Developer/CommandLineTools/SDKs/MacOSX12.sdk/System/Library/Frameworks/CoreAudio.framework/Headers/CoreAudio.h:18,
                     from ../../audio/coreaudio.c:26:
    /Library/Developer/CommandLineTools/SDKs/MacOSX12.sdk/System/Library/Frameworks/CoreAudio.framework/Headers/AudioHardware.h:162:2: error: expected identifier or '(' before '^' token
      162 | (^AudioObjectPropertyListenerBlock)(    UInt32                              inNumberAddresses,
          |  ^
    FAILED: libcommon.fa.p/audio_coreaudio.c.o

Rename the file to use the Objective-C default extension (.m) so
meson calls the correct compiler.

[*] https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/ProgrammingWithObjectiveC/WorkingwithBlocks/WorkingwithBlocks.html

Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
---
 audio/{coreaudio.c => coreaudio.m} | 0
 audio/meson.build                  | 2 +-
 2 files changed, 1 insertion(+), 1 deletion(-)
 rename audio/{coreaudio.c => coreaudio.m} (100%)

diff --git a/audio/coreaudio.c b/audio/coreaudio.m
similarity index 100%
rename from audio/coreaudio.c
rename to audio/coreaudio.m
diff --git a/audio/meson.build b/audio/meson.build
index d9b295514f..94dab16891 100644
--- a/audio/meson.build
+++ b/audio/meson.build
@@ -7,7 +7,7 @@ softmmu_ss.add(files(
   'wavcapture.c',
 ))
 
-softmmu_ss.add(when: coreaudio, if_true: files('coreaudio.c'))
+softmmu_ss.add(when: coreaudio, if_true: files('coreaudio.m'))
 softmmu_ss.add(when: dsound, if_true: files('dsoundaudio.c', 'audio_win_int.c'))
 
 audio_modules = {}
-- 
2.34.1

Re: [RFC PATCH 3/4] audio: Rename coreaudio extension to use Objective-C compiler

[Christian Schoenebeck]

On Dienstag, 15. Februar 2022 13:06:24 CET Philippe Mathieu-Daudé via wrote:
> The coreaudio library includes Objective-C declarations (using the
> caret '^' symbol to declare block references [*]). When building
> with a C compiler we get:
> 
>   [175/839] Compiling C object libcommon.fa.p/audio_coreaudio.c.o
>     In file included from
> /Library/Developer/CommandLineTools/SDKs/MacOSX12.sdk/System/Library/Framew
> orks/CoreAudio.framework/Headers/CoreAudio.h:18, from
> ../../audio/coreaudio.c:26:
>    
> /Library/Developer/CommandLineTools/SDKs/MacOSX12.sdk/System/Library/Framew
> orks/CoreAudio.framework/Headers/AudioHardware.h:162:2: error: expected
> identifier or '(' before '^' token 162 |
> (^AudioObjectPropertyListenerBlock)(    UInt32                             
> inNumberAddresses,
>           |  ^
> 
>     FAILED: libcommon.fa.p/audio_coreaudio.c.o
> 
> Rename the file to use the Objective-C default extension (.m) so
> meson calls the correct compiler.
> 
> [*]
> https://developer.apple.com/library/archive/documentation/Cocoa/Conceptual/
> ProgrammingWithObjectiveC/WorkingwithBlocks/WorkingwithBlocks.html
> 
> Signed-off-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> ---

We already had a similar discussion before [1] whether to use .c and add
required compiler flags or using .m. I find this solution more appropriate.

Reviewed-by: Christian Schoenebeck <qemu_oss@crudebyte.com>

[1] https://lore.kernel.org/all/7053351.4JHWUSIRgT@silver/

Best regards,
Christian Schoenebeck

>  audio/{coreaudio.c => coreaudio.m} | 0
>  audio/meson.build                  | 2 +-
>  2 files changed, 1 insertion(+), 1 deletion(-)
>  rename audio/{coreaudio.c => coreaudio.m} (100%)
> 
> diff --git a/audio/coreaudio.c b/audio/coreaudio.m
> similarity index 100%
> rename from audio/coreaudio.c
> rename to audio/coreaudio.m
> diff --git a/audio/meson.build b/audio/meson.build
> index d9b295514f..94dab16891 100644
> --- a/audio/meson.build
> +++ b/audio/meson.build
> @@ -7,7 +7,7 @@ softmmu_ss.add(files(
>    'wavcapture.c',
>  ))
> 
> -softmmu_ss.add(when: coreaudio, if_true: files('coreaudio.c'))
> +softmmu_ss.add(when: coreaudio, if_true: files('coreaudio.m'))
>  softmmu_ss.add(when: dsound, if_true: files('dsoundaudio.c',
> 'audio_win_int.c'))
> 
>  audio_modules = {}

[RFC PATCH 4/4] ui/cocoa: Ignore 'initializer overrides' warnings

[Philippe Mathieu-Daudé via]

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(+)

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) {
-- 
2.34.1

Re: [RFC PATCH 4/4] ui/cocoa: Ignore 'initializer overrides' warnings

[Peter Maydell]

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

Re: [RFC PATCH 4/4] ui/cocoa: Ignore 'initializer overrides' warnings

[Akihiko Odaki]

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,

Re: [RFC PATCH 4/4] ui/cocoa: Ignore 'initializer overrides' warnings

[Philippe Mathieu-Daudé via]

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 =)

Re: [RFC PATCH 4/4] ui/cocoa: Ignore 'initializer overrides' warnings

[Christian Schoenebeck]

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

Re: [RFC PATCH 4/4] ui/cocoa: Ignore 'initializer overrides' warnings

[Peter Maydell]

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

Re: [RFC PATCH 4/4] ui/cocoa: Ignore 'initializer overrides' warnings

[Philippe Mathieu-Daudé via]

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.

Re: [RFC PATCH 4/4] ui/cocoa: Ignore 'initializer overrides' warnings

[Christian Schoenebeck]

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

Re: [RFC PATCH 4/4] ui/cocoa: Ignore 'initializer overrides' warnings

[Peter Maydell]

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

Re: [RFC PATCH 0/4] buildsys: More fixes to use GCC on macOS

[Akihiko Odaki]

On Tue, Feb 15, 2022 at 9:06 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> Few fixes to be able to use GCC extensions which are not
> available on Clang.
>
> Philippe Mathieu-Daudé (4):
>   osdep: Avoid using Clang-specific __builtin_available()
>   osdep: Un-inline qemu_thread_jit_execute/write
>   audio: Rename coreaudio extension to use Objective-C compiler
>   ui/cocoa: Ignore 'initializer overrides' warnings
>
>  audio/{coreaudio.c => coreaudio.m} |  0
>  audio/meson.build                  |  2 +-
>  include/qemu/osdep.h               | 21 ++-------------------
>  ui/cocoa.m                         |  5 +++++
>  util/osdep.c                       | 20 ++++++++++++++++++++
>  5 files changed, 28 insertions(+), 20 deletions(-)
>  rename audio/{coreaudio.c => coreaudio.m} (100%)
>
> --
> 2.34.1
>

Compiler portability is always nice to have. Making QEMU on macOS
compatible with GCC is good, but I don't think that would justify
abandoning compatibility with Clang.

Regards,
Akihiko Odaki

Re: [RFC PATCH 0/4] buildsys: More fixes to use GCC on macOS

[Philippe Mathieu-Daudé via]

On 15/2/22 14:06, Akihiko Odaki wrote:
> On Tue, Feb 15, 2022 at 9:06 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>>
>> Few fixes to be able to use GCC extensions which are not
>> available on Clang.
>>
>> Philippe Mathieu-Daudé (4):
>>    osdep: Avoid using Clang-specific __builtin_available()
>>    osdep: Un-inline qemu_thread_jit_execute/write
>>    audio: Rename coreaudio extension to use Objective-C compiler
>>    ui/cocoa: Ignore 'initializer overrides' warnings
>>
>>   audio/{coreaudio.c => coreaudio.m} |  0
>>   audio/meson.build                  |  2 +-
>>   include/qemu/osdep.h               | 21 ++-------------------
>>   ui/cocoa.m                         |  5 +++++
>>   util/osdep.c                       | 20 ++++++++++++++++++++
>>   5 files changed, 28 insertions(+), 20 deletions(-)
>>   rename audio/{coreaudio.c => coreaudio.m} (100%)
>>
>> --
>> 2.34.1
>>
> 
> Compiler portability is always nice to have. Making QEMU on macOS
> compatible with GCC is good, but I don't think that would justify
> abandoning compatibility with Clang.

I am certainly not abandoning compatibility with Clang. What gives
you this impression?

Re: [RFC PATCH 0/4] buildsys: More fixes to use GCC on macOS

[Akihiko Odaki]

On Tue, Feb 15, 2022 at 10:25 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
>
> On 15/2/22 14:06, Akihiko Odaki wrote:
> > On Tue, Feb 15, 2022 at 9:06 PM Philippe Mathieu-Daudé <f4bug@amsat.org> wrote:
> >>
> >> Few fixes to be able to use GCC extensions which are not
> >> available on Clang.
> >>
> >> Philippe Mathieu-Daudé (4):
> >>    osdep: Avoid using Clang-specific __builtin_available()
> >>    osdep: Un-inline qemu_thread_jit_execute/write
> >>    audio: Rename coreaudio extension to use Objective-C compiler
> >>    ui/cocoa: Ignore 'initializer overrides' warnings
> >>
> >>   audio/{coreaudio.c => coreaudio.m} |  0
> >>   audio/meson.build                  |  2 +-
> >>   include/qemu/osdep.h               | 21 ++-------------------
> >>   ui/cocoa.m                         |  5 +++++
> >>   util/osdep.c                       | 20 ++++++++++++++++++++
> >>   5 files changed, 28 insertions(+), 20 deletions(-)
> >>   rename audio/{coreaudio.c => coreaudio.m} (100%)
> >>
> >> --
> >> 2.34.1
> >>
> >
> > Compiler portability is always nice to have. Making QEMU on macOS
> > compatible with GCC is good, but I don't think that would justify
> > abandoning compatibility with Clang.
>
> I am certainly not abandoning compatibility with Clang. What gives
> you this impression?

I read the description as it says it allows to introduce GCC
extensions which breaks builds with Clang.