[PATCH v2 0/4] Adds support for running QEMU natively on windows-arm64

[PATCH v2 0/4] Adds support for running QEMU natively on windows-arm64

[Pierrick Bouvier]

Since v1:

- Comment why we use generic version of flush_idcache_range
- Ensure __mingw_setjmp/longjmp are available using meson
- Fix a warning by calling g_assert_not_reached() instead of initializing a
  variable

As before this was tested with:
- make check
- boot an x64 debian bullseye vm
- boot an arm64 ubuntu 22.10 vm

Thanks

Pierrick Bouvier (4):
  util/cacheflush: fix cache on windows-arm64
  sysemu/os-win32: fix setjmp/longjmp on windows-arm64
  qga/vss-win32: fix warning for clang++-15
  target/ppc: fix warning with clang-15

 include/sysemu/os-win32.h | 21 +++++++++++++++++++--
 meson.build               | 22 ++++++++++++++++++++++
 qga/vss-win32/install.cpp |  2 +-
 target/ppc/dfp_helper.c   |  4 ++--
 util/cacheflush.c         | 10 +++++++---
 5 files changed, 51 insertions(+), 8 deletions(-)

-- 
2.30.2

[PATCH v2 1/4] util/cacheflush: fix cache on windows-arm64

[Pierrick Bouvier]

ctr_el0 access is privileged on this platform and fails as an illegal
instruction.

Windows does not offer a way to flush data cache from userspace, and
only FlushInstructionCache is available in Windows API.

The generic implementation of flush_idcache_range uses,
__builtin___clear_cache, which already use the FlushInstructionCache
function. So we rely on that.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 util/cacheflush.c | 10 +++++++---
 1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/util/cacheflush.c b/util/cacheflush.c
index 2c2c73e085..0a0acd70fa 100644
--- a/util/cacheflush.c
+++ b/util/cacheflush.c
@@ -121,8 +121,10 @@ static void sys_cache_info(int *isize, int *dsize)
 static bool have_coherent_icache;
 #endif
 
-#if defined(__aarch64__) && !defined(CONFIG_DARWIN)
-/* Apple does not expose CTR_EL0, so we must use system interfaces. */
+#if defined(__aarch64__) && !defined(CONFIG_DARWIN) && !defined(CONFIG_WIN32)
+/* Apple does not expose CTR_EL0, so we must use system interfaces.
+ * Windows neither, but we use a generic implementation of flush_idcache_range
+ * in this case. */
 static uint64_t save_ctr_el0;
 static void arch_cache_info(int *isize, int *dsize)
 {
@@ -225,7 +227,9 @@ static void __attribute__((constructor)) init_cache_info(void)
 
 /* Caches are coherent and do not require flushing; symbol inline. */
 
-#elif defined(__aarch64__)
+#elif defined(__aarch64__) && !defined(CONFIG_WIN32)
+/* For Windows, we use generic implementation of flush_idcache_range, that
+ * performs a call to FlushInstructionCache, through __builtin___clear_cache */
 
 #ifdef CONFIG_DARWIN
 /* Apple does not expose CTR_EL0, so we must use system interfaces. */
-- 
2.30.2

Re: [PATCH v2 1/4] util/cacheflush: fix cache on windows-arm64

[Richard Henderson]

On 2/16/23 03:49, Pierrick Bouvier wrote:
> ctr_el0 access is privileged on this platform and fails as an illegal
> instruction.
> 
> Windows does not offer a way to flush data cache from userspace, and
> only FlushInstructionCache is available in Windows API.
> 
> The generic implementation of flush_idcache_range uses,
> __builtin___clear_cache, which already use the FlushInstructionCache
> function. So we rely on that.
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

This is reasonable for now.  I'll note that gcc does not yet support windows for aarch64, 
and I would guess this would be fixed for libgcc at such time as.

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>


r~


> ---
>   util/cacheflush.c | 10 +++++++---
>   1 file changed, 7 insertions(+), 3 deletions(-)
> 
> diff --git a/util/cacheflush.c b/util/cacheflush.c
> index 2c2c73e085..0a0acd70fa 100644
> --- a/util/cacheflush.c
> +++ b/util/cacheflush.c
> @@ -121,8 +121,10 @@ static void sys_cache_info(int *isize, int *dsize)
>   static bool have_coherent_icache;
>   #endif
>   
> -#if defined(__aarch64__) && !defined(CONFIG_DARWIN)
> -/* Apple does not expose CTR_EL0, so we must use system interfaces. */
> +#if defined(__aarch64__) && !defined(CONFIG_DARWIN) && !defined(CONFIG_WIN32)
> +/* Apple does not expose CTR_EL0, so we must use system interfaces.
> + * Windows neither, but we use a generic implementation of flush_idcache_range
> + * in this case. */
>   static uint64_t save_ctr_el0;
>   static void arch_cache_info(int *isize, int *dsize)
>   {
> @@ -225,7 +227,9 @@ static void __attribute__((constructor)) init_cache_info(void)
>   
>   /* Caches are coherent and do not require flushing; symbol inline. */
>   
> -#elif defined(__aarch64__)
> +#elif defined(__aarch64__) && !defined(CONFIG_WIN32)
> +/* For Windows, we use generic implementation of flush_idcache_range, that
> + * performs a call to FlushInstructionCache, through __builtin___clear_cache */
>   
>   #ifdef CONFIG_DARWIN
>   /* Apple does not expose CTR_EL0, so we must use system interfaces. */

Re: [PATCH v2 1/4] util/cacheflush: fix cache on windows-arm64

[Pierrick Bouvier]

On 2/16/23 20:42, Richard Henderson wrote:
> On 2/16/23 03:49, Pierrick Bouvier wrote:
>> ctr_el0 access is privileged on this platform and fails as an illegal
>> instruction.
>>
>> Windows does not offer a way to flush data cache from userspace, and
>> only FlushInstructionCache is available in Windows API.
>>
>> The generic implementation of flush_idcache_range uses,
>> __builtin___clear_cache, which already use the FlushInstructionCache
>> function. So we rely on that.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> 
> This is reasonable for now.  I'll note that gcc does not yet support windows for aarch64,
> and I would guess this would be fixed for libgcc at such time as.
> 
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> 

For completeness, and after asking to Microsoft,
FlushInstructionCache does DCache-Clean-to-PoU + 
ICache-Invalidate-to-PoU, which is equivalent to calling dccvau + icivau.

Thus, it's doing the right thing.

Pierrick

> 
> r~
> 
> 
>> ---
>>    util/cacheflush.c | 10 +++++++---
>>    1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/util/cacheflush.c b/util/cacheflush.c
>> index 2c2c73e085..0a0acd70fa 100644
>> --- a/util/cacheflush.c
>> +++ b/util/cacheflush.c
>> @@ -121,8 +121,10 @@ static void sys_cache_info(int *isize, int *dsize)
>>    static bool have_coherent_icache;
>>    #endif
>>    
>> -#if defined(__aarch64__) && !defined(CONFIG_DARWIN)
>> -/* Apple does not expose CTR_EL0, so we must use system interfaces. */
>> +#if defined(__aarch64__) && !defined(CONFIG_DARWIN) && !defined(CONFIG_WIN32)
>> +/* Apple does not expose CTR_EL0, so we must use system interfaces.
>> + * Windows neither, but we use a generic implementation of flush_idcache_range
>> + * in this case. */
>>    static uint64_t save_ctr_el0;
>>    static void arch_cache_info(int *isize, int *dsize)
>>    {
>> @@ -225,7 +227,9 @@ static void __attribute__((constructor)) init_cache_info(void)
>>    
>>    /* Caches are coherent and do not require flushing; symbol inline. */
>>    
>> -#elif defined(__aarch64__)
>> +#elif defined(__aarch64__) && !defined(CONFIG_WIN32)
>> +/* For Windows, we use generic implementation of flush_idcache_range, that
>> + * performs a call to FlushInstructionCache, through __builtin___clear_cache */
>>    
>>    #ifdef CONFIG_DARWIN
>>    /* Apple does not expose CTR_EL0, so we must use system interfaces. */
> 

Re: [PATCH v2 1/4] util/cacheflush: fix cache on windows-arm64

[Peter Maydell]

On Thu, 16 Feb 2023 at 13:49, Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
>
> ctr_el0 access is privileged on this platform and fails as an illegal
> instruction.
>
> Windows does not offer a way to flush data cache from userspace, and
> only FlushInstructionCache is available in Windows API.
>
> The generic implementation of flush_idcache_range uses,
> __builtin___clear_cache, which already use the FlushInstructionCache
> function. So we rely on that.
>
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>  util/cacheflush.c | 10 +++++++---
>  1 file changed, 7 insertions(+), 3 deletions(-)
>
> diff --git a/util/cacheflush.c b/util/cacheflush.c
> index 2c2c73e085..0a0acd70fa 100644
> --- a/util/cacheflush.c
> +++ b/util/cacheflush.c
> @@ -121,8 +121,10 @@ static void sys_cache_info(int *isize, int *dsize)
>  static bool have_coherent_icache;
>  #endif
>
> -#if defined(__aarch64__) && !defined(CONFIG_DARWIN)
> -/* Apple does not expose CTR_EL0, so we must use system interfaces. */
> +#if defined(__aarch64__) && !defined(CONFIG_DARWIN) && !defined(CONFIG_WIN32)
> +/* Apple does not expose CTR_EL0, so we must use system interfaces.
> + * Windows neither, but we use a generic implementation of flush_idcache_range
> + * in this case. */

QEMU multiline comment syntax requires the /* and */ to be
on lines of their own (here and in your other comment).

thanks
-- PMM

Re: [PATCH v2 1/4] util/cacheflush: fix cache on windows-arm64

[Pierrick Bouvier]

On 2/17/23 16:32, Peter Maydell wrote:
> On Thu, 16 Feb 2023 at 13:49, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
>>
>> ctr_el0 access is privileged on this platform and fails as an illegal
>> instruction.
>>
>> Windows does not offer a way to flush data cache from userspace, and
>> only FlushInstructionCache is available in Windows API.
>>
>> The generic implementation of flush_idcache_range uses,
>> __builtin___clear_cache, which already use the FlushInstructionCache
>> function. So we rely on that.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>   util/cacheflush.c | 10 +++++++---
>>   1 file changed, 7 insertions(+), 3 deletions(-)
>>
>> diff --git a/util/cacheflush.c b/util/cacheflush.c
>> index 2c2c73e085..0a0acd70fa 100644
>> --- a/util/cacheflush.c
>> +++ b/util/cacheflush.c
>> @@ -121,8 +121,10 @@ static void sys_cache_info(int *isize, int *dsize)
>>   static bool have_coherent_icache;
>>   #endif
>>
>> -#if defined(__aarch64__) && !defined(CONFIG_DARWIN)
>> -/* Apple does not expose CTR_EL0, so we must use system interfaces. */
>> +#if defined(__aarch64__) && !defined(CONFIG_DARWIN) && !defined(CONFIG_WIN32)
>> +/* Apple does not expose CTR_EL0, so we must use system interfaces.
>> + * Windows neither, but we use a generic implementation of flush_idcache_range
>> + * in this case. */
> 
> QEMU multiline comment syntax requires the /* and */ to be
> on lines of their own (here and in your other comment).
> 

Sorry, I missed that point in QEMU coding style.
Will update this 👍

> thanks
> -- PMM

[PATCH v2 2/4] sysemu/os-win32: fix setjmp/longjmp on windows-arm64

[Pierrick Bouvier]

Windows implementation of setjmp/longjmp is done in
C:/WINDOWS/system32/ucrtbase.dll. Alas, on arm64, it seems to *always*
perform stack unwinding, which crashes from generated code.

By using alternative implementation built in mingw, we avoid doing stack
unwinding and this fixes crash when calling longjmp.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 include/sysemu/os-win32.h | 21 +++++++++++++++++++--
 meson.build               | 22 ++++++++++++++++++++++
 2 files changed, 41 insertions(+), 2 deletions(-)

diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index 5b38c7bd04..634931a387 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -51,14 +51,31 @@ typedef struct sockaddr_un {
 extern "C" {
 #endif
 
-#if defined(_WIN64)
+#if defined(__aarch64__)
+#ifndef CONFIG_MINGW64_HAS_SETJMP_LONGJMP
+#error mingw must provide setjmp/longjmp for windows-arm64
+#endif
+/* On windows-arm64, setjmp is available in only one variant, and longjmp always
+ * does stack unwinding. This crash with generated code.
+ * Thus, we use another implementation of setjmp (not windows one), coming from
+ * mingw, which never performs stack unwinding. */
+#undef setjmp
+#undef longjmp
+/* These functions are not declared in setjmp.h because __aarch64__ defines
+ * setjmp to _setjmpex instead. However, they are still defined in libmingwex.a,
+ * which gets linked automatically. */
+extern int __mingw_setjmp(jmp_buf);
+extern void __attribute__((noreturn)) __mingw_longjmp(jmp_buf, int);
+#define setjmp(env) __mingw_setjmp(env)
+#define longjmp(env, val) __mingw_longjmp(env, val)
+#elif defined(_WIN64)
 /* On w64, setjmp is implemented by _setjmp which needs a second parameter.
  * If this parameter is NULL, longjump does no stack unwinding.
  * That is what we need for QEMU. Passing the value of register rsp (default)
  * lets longjmp try a stack unwinding which will crash with generated code. */
 # undef setjmp
 # define setjmp(env) _setjmp(env, NULL)
-#endif
+#endif /* __aarch64__ */
 /* QEMU uses sigsetjmp()/siglongjmp() as the portable way to specify
  * "longjmp and don't touch the signal masks". Since we know that the
  * savemask parameter will always be zero we can safely define these
diff --git a/meson.build b/meson.build
index 4ba3bf3431..7f55205437 100644
--- a/meson.build
+++ b/meson.build
@@ -2450,6 +2450,28 @@ if targetos == 'windows'
     }''', name: '_lock_file and _unlock_file'))
 endif
 
+if targetos == 'windows'
+  mingw_has_setjmp_longjmp = cc.links('''
+    #include <setjmp.h>
+    int main(void) {
+      // these functions are not available in setjmp header, but may be
+      // available at link time, from libmingwex.a
+      extern int __mingw_setjmp(jmp_buf);
+      extern void __attribute__((noreturn)) __mingw_longjmp(jmp_buf, int);
+      jmp_buf env;
+      __mingw_setjmp(env);
+      __mingw_longjmp(env, 0);
+    }
+  ''', name: 'mingw setjmp and longjmp')
+
+  config_host_data.set('CONFIG_MINGW64_HAS_SETJMP_LONGJMP',
+                       mingw_has_setjmp_longjmp)
+
+  if cpu == 'aarch64' and not mingw_has_setjmp_longjmp
+    error('mingw must provide setjmp/longjmp for windows-arm64')
+  endif
+endif
+
 ########################
 # Target configuration #
 ########################
-- 
2.30.2

Re: [PATCH v2 2/4] sysemu/os-win32: fix setjmp/longjmp on windows-arm64

[Richard Henderson]

On 2/16/23 03:49, Pierrick Bouvier wrote:
> Windows implementation of setjmp/longjmp is done in
> C:/WINDOWS/system32/ucrtbase.dll. Alas, on arm64, it seems to *always*
> perform stack unwinding, which crashes from generated code.
> 
> By using alternative implementation built in mingw, we avoid doing stack
> unwinding and this fixes crash when calling longjmp.
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   include/sysemu/os-win32.h | 21 +++++++++++++++++++--
>   meson.build               | 22 ++++++++++++++++++++++
>   2 files changed, 41 insertions(+), 2 deletions(-)

Ugly, but workable.

Acked-by: Richard Henderson <richard.henderson@linaro.org>

Ideally we'd interact properly with system unwinding.  It looks like we'd use 
RtlAddFunctionTable, but the documentation is spread out and I've not found all of the bits.

We already do something similar for gdb -- see tcg/tcg.c, tcg_register_jit_int, and 
tcg/aarch64/tcg-target.c.inc, debug_frame.


r~

Re: [PATCH v2 2/4] sysemu/os-win32: fix setjmp/longjmp on windows-arm64

[Pierrick Bouvier]

On 2/16/23 21:02, Richard Henderson wrote:
> On 2/16/23 03:49, Pierrick Bouvier wrote:
>> Windows implementation of setjmp/longjmp is done in
>> C:/WINDOWS/system32/ucrtbase.dll. Alas, on arm64, it seems to *always*
>> perform stack unwinding, which crashes from generated code.
>>
>> By using alternative implementation built in mingw, we avoid doing stack
>> unwinding and this fixes crash when calling longjmp.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    include/sysemu/os-win32.h | 21 +++++++++++++++++++--
>>    meson.build               | 22 ++++++++++++++++++++++
>>    2 files changed, 41 insertions(+), 2 deletions(-)
> 
> Ugly, but workable.
> 
> Acked-by: Richard Henderson <richard.henderson@linaro.org>
> 
> Ideally we'd interact properly with system unwinding.  It looks like we'd use
> RtlAddFunctionTable, but the documentation is spread out and I've not found all of the bits.
> 
> We already do something similar for gdb -- see tcg/tcg.c, tcg_register_jit_int, and
> tcg/aarch64/tcg-target.c.inc, debug_frame.
> 

Thanks for the idea.
For the sake of completeness, using RtlInstallFunctionTableCallback 
could be a better strategy, as it allows to have a callback called only 
during stack unwinding [1].

Meanwhile, I'll ask to our contact in MSFT if it's possible to perform a 
setjmp/longjmp that does not trigger stack unwinding on aarch64.

[1] 
https://learn.microsoft.com/en-us/windows/win32/api/winnt/nf-winnt-rtlinstallfunctiontablecallback

> 
> r~

Re: [PATCH v2 2/4] sysemu/os-win32: fix setjmp/longjmp on windows-arm64

[Richard Henderson]

On 2/19/23 23:53, Pierrick Bouvier wrote:
> Thanks for the idea.
> For the sake of completeness, using RtlInstallFunctionTableCallback could be a better 
> strategy, as it allows to have a callback called only during stack unwinding [1].

Interesting idea.

I suspect that is of more use when the JIT is more complicated than ours.  In particular, 
notice the OutOfProcessCallbackDll member (for use by the debugger). Whereas our unwind 
info is static, and can exist in read-only memory.


r~

[PATCH v2 3/4] qga/vss-win32: fix warning for clang++-15

[Pierrick Bouvier]

Reported when compiling with clang-windows-arm64.

../qga/vss-win32/install.cpp:537:9: error: variable 'hr' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
    if (!(ControlService(service, SERVICE_CONTROL_STOP, NULL))) {
        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../qga/vss-win32/install.cpp:545:12: note: uninitialized use occurs here
    return hr;
           ^~
Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 qga/vss-win32/install.cpp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
index b57508fbe0..b8087e5baa 100644
--- a/qga/vss-win32/install.cpp
+++ b/qga/vss-win32/install.cpp
@@ -518,7 +518,7 @@ namespace _com_util
 /* Stop QGA VSS provider service using Winsvc API  */
 STDAPI StopService(void)
 {
-    HRESULT hr;
+    HRESULT hr = S_OK;
     SC_HANDLE manager = OpenSCManager(NULL, NULL, SC_MANAGER_ALL_ACCESS);
     SC_HANDLE service = NULL;
 
-- 
2.30.2

Re: [PATCH v2 3/4] qga/vss-win32: fix warning for clang++-15

[Philippe Mathieu-Daudé]

On 16/2/23 14:49, Pierrick Bouvier wrote:
> Reported when compiling with clang-windows-arm64.
> 
> ../qga/vss-win32/install.cpp:537:9: error: variable 'hr' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>      if (!(ControlService(service, SERVICE_CONTROL_STOP, NULL))) {
>          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../qga/vss-win32/install.cpp:545:12: note: uninitialized use occurs here
>      return hr;
>             ^~

Fixes: 917ebcb170 ("qga-win: Fix QGA VSS Provider service stop failure")
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

(please also carry the 'Fixes:' tags when respining patches)

> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   qga/vss-win32/install.cpp | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/qga/vss-win32/install.cpp b/qga/vss-win32/install.cpp
> index b57508fbe0..b8087e5baa 100644
> --- a/qga/vss-win32/install.cpp
> +++ b/qga/vss-win32/install.cpp
> @@ -518,7 +518,7 @@ namespace _com_util
>   /* Stop QGA VSS provider service using Winsvc API  */
>   STDAPI StopService(void)
>   {
> -    HRESULT hr;
> +    HRESULT hr = S_OK;
>       SC_HANDLE manager = OpenSCManager(NULL, NULL, SC_MANAGER_ALL_ACCESS);
>       SC_HANDLE service = NULL;
>   

Re: [PATCH v2 3/4] qga/vss-win32: fix warning for clang++-15

[Pierrick Bouvier]

Done!

On 2/20/23 20:30, Philippe Mathieu-Daudé wrote:
> Fixes: 917ebcb170 ("qga-win: Fix QGA VSS Provider service stop failure")
> Reviewed-by: Philippe Mathieu-Daudé<philmd@linaro.org>

[PATCH v2 4/4] target/ppc: fix warning with clang-15

[Pierrick Bouvier]

When compiling for windows-arm64 using clang-15, it reports a sometimes
uninitialized variable. This seems to be a false positive, as a default
case guards switch expressions, preventing to return an uninitialized
value, but clang seems unhappy with assert(0) definition.

Change code to g_assert_not_reached() fix the warning.

Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
---
 target/ppc/dfp_helper.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/target/ppc/dfp_helper.c b/target/ppc/dfp_helper.c
index cc024316d5..0a40bcfee3 100644
--- a/target/ppc/dfp_helper.c
+++ b/target/ppc/dfp_helper.c
@@ -121,7 +121,7 @@ static void dfp_set_round_mode_from_immediate(uint8_t r, uint8_t rmc,
         case 3: /* use FPSCR rounding mode */
             return;
         default:
-            assert(0); /* cannot get here */
+            g_assert_not_reached(); /* cannot get here */
         }
     } else { /* r == 1 */
         switch (rmc & 3) {
@@ -138,7 +138,7 @@ static void dfp_set_round_mode_from_immediate(uint8_t r, uint8_t rmc,
             rnd = DEC_ROUND_HALF_DOWN;
             break;
         default:
-            assert(0); /* cannot get here */
+            g_assert_not_reached(); /* cannot get here */
         }
     }
     decContextSetRounding(&dfp->context, rnd);
-- 
2.30.2

Re: [PATCH v2 4/4] target/ppc: fix warning with clang-15

[Richard Henderson]

On 2/16/23 03:49, Pierrick Bouvier wrote:
> When compiling for windows-arm64 using clang-15, it reports a sometimes
> uninitialized variable. This seems to be a false positive, as a default
> case guards switch expressions, preventing to return an uninitialized
> value, but clang seems unhappy with assert(0) definition.
> 
> Change code to g_assert_not_reached() fix the warning.
> 
> Signed-off-by: Pierrick Bouvier<pierrick.bouvier@linaro.org>
> ---
>   target/ppc/dfp_helper.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)

Reviewed-by: Richard Henderson <richard.henderson@linaro.org>

r~

Re: [PATCH v2 4/4] target/ppc: fix warning with clang-15

[Philippe Mathieu-Daudé]

On 16/2/23 14:49, Pierrick Bouvier wrote:
> When compiling for windows-arm64 using clang-15, it reports a sometimes
> uninitialized variable. This seems to be a false positive, as a default
> case guards switch expressions, preventing to return an uninitialized
> value, but clang seems unhappy with assert(0) definition.
> 
> Change code to g_assert_not_reached() fix the warning.
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>   target/ppc/dfp_helper.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/target/ppc/dfp_helper.c b/target/ppc/dfp_helper.c
> index cc024316d5..0a40bcfee3 100644
> --- a/target/ppc/dfp_helper.c
> +++ b/target/ppc/dfp_helper.c
> @@ -121,7 +121,7 @@ static void dfp_set_round_mode_from_immediate(uint8_t r, uint8_t rmc,
>           case 3: /* use FPSCR rounding mode */
>               return;
>           default:
> -            assert(0); /* cannot get here */
> +            g_assert_not_reached(); /* cannot get here */

If you respin to update the comments to match QEMU style (also
the // in configure), please remove this pointless comment here.

Re: [PATCH v2 4/4] target/ppc: fix warning with clang-15

[Pierrick Bouvier]

On 2/17/23 18:23, Philippe Mathieu-Daudé wrote:
> On 16/2/23 14:49, Pierrick Bouvier wrote:
>> When compiling for windows-arm64 using clang-15, it reports a sometimes
>> uninitialized variable. This seems to be a false positive, as a default
>> case guards switch expressions, preventing to return an uninitialized
>> value, but clang seems unhappy with assert(0) definition.
>>
>> Change code to g_assert_not_reached() fix the warning.
>>
>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>    target/ppc/dfp_helper.c | 4 ++--
>>    1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/target/ppc/dfp_helper.c b/target/ppc/dfp_helper.c
>> index cc024316d5..0a40bcfee3 100644
>> --- a/target/ppc/dfp_helper.c
>> +++ b/target/ppc/dfp_helper.c
>> @@ -121,7 +121,7 @@ static void dfp_set_round_mode_from_immediate(uint8_t r, uint8_t rmc,
>>            case 3: /* use FPSCR rounding mode */
>>                return;
>>            default:
>> -            assert(0); /* cannot get here */
>> +            g_assert_not_reached(); /* cannot get here */
> 
> If you respin to update the comments to match QEMU style (also
> the // in configure), please remove this pointless comment here.

Sure, will do.