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

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

[Pierrick Bouvier]

Note: As it's my first submission using git send-email and on QEMU, don't
hesitate to report if anything is wrong/missing.

To compile this, the fastest route is to use msys2, setup with clangarm64
repository. This way, all dependencies can be installed easily, and clang is
available directly. Please note that GCC currently is not able to generate code
for this platform, so only clang can be used for now.

---

After having being able to compile, I had to fix two blocking issues:
- illegal instruction
- setjmp/longjmp segfaulting

Then, two warnings were reported, and their fix is included in this series as
well, so we can have clean support out of the box on this platform.

---

This series was tested with:
- make check (find a full log here [1], compiled using this script [2])
- installing and booting a debian bullseye x64
- installing and booting an ubuntu 22.10 arm64

Despite the slowness of an emulated system, it works well!

[1] https://gitlab.com/Linaro/windowsonarm/packages/qemu/-/jobs/3761641695
[2] https://gitlab.com/Linaro/windowsonarm/packages/qemu/-/blob/master/recipe.sh

---

As it has been discussed privately inside Linaro, we can allocate one Windows on
Arm machine (running in our lab), to add a runner for gitlab CI. But this is
probably a discussion for another thread.

---

Pierrick Bouvier (4):
  util/cacheflush: fix illegal instruction 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 | 18 ++++++++++++++++--
 qga/vss-win32/install.cpp |  2 +-
 target/ppc/dfp_helper.c   |  4 ++--
 util/cacheflush.c         |  5 +++--
 4 files changed, 22 insertions(+), 7 deletions(-)

-- 
2.30.2

[PATCH 1/4] util/cacheflush: fix illegal instruction on windows-arm64

[Pierrick Bouvier]

mrs instruction fails as an illegal instruction.
For now, no cache information is retrieved for this platform.
It could be specialized later, using Windows API.

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

diff --git a/util/cacheflush.c b/util/cacheflush.c
index 2c2c73e085..149f103d32 100644
--- a/util/cacheflush.c
+++ b/util/cacheflush.c
@@ -121,8 +121,9 @@ static void sys_cache_info(int *isize, int *dsize)
 static bool have_coherent_icache;
 #endif
 
-#if defined(__aarch64__) && !defined(CONFIG_DARWIN)
+#if defined(__aarch64__) && !defined(CONFIG_DARWIN) && !defined(CONFIG_WIN32)
 /* Apple does not expose CTR_EL0, so we must use system interfaces. */
+/* Does not work on windows-arm64, illegal instruction using mrs */
 static uint64_t save_ctr_el0;
 static void arch_cache_info(int *isize, int *dsize)
 {
@@ -225,7 +226,7 @@ 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)
 
 #ifdef CONFIG_DARWIN
 /* Apple does not expose CTR_EL0, so we must use system interfaces. */
-- 
2.30.2

Re: [PATCH 1/4] util/cacheflush: fix illegal instruction on windows-arm64

[Peter Maydell]

On Mon, 13 Feb 2023 at 20:50, Pierrick Bouvier
<pierrick.bouvier@linaro.org> wrote:
>
> mrs instruction fails as an illegal instruction.
> For now, no cache information is retrieved for this platform.
> It could be specialized later, using Windows API.

Unless I'm misreading the code, there's a sys_cache_info()
implementation that's only guarded by if defined(_WIN32), so
presumably we're using that on AArch64 also. Does it return
sensible values ?

> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> ---
>  util/cacheflush.c | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/util/cacheflush.c b/util/cacheflush.c
> index 2c2c73e085..149f103d32 100644
> --- a/util/cacheflush.c
> +++ b/util/cacheflush.c
> @@ -121,8 +121,9 @@ static void sys_cache_info(int *isize, int *dsize)
>  static bool have_coherent_icache;
>  #endif
>
> -#if defined(__aarch64__) && !defined(CONFIG_DARWIN)
> +#if defined(__aarch64__) && !defined(CONFIG_DARWIN) && !defined(CONFIG_WIN32)
>  /* Apple does not expose CTR_EL0, so we must use system interfaces. */
> +/* Does not work on windows-arm64, illegal instruction using mrs */

This comment should be better integrated with the previous, because
the reason for the illegal instruction exception on Windows is the
same as for macos -- the OS doesn't expose CTR_EL0 to userspace.

>  static uint64_t save_ctr_el0;
>  static void arch_cache_info(int *isize, int *dsize)
>  {
> @@ -225,7 +226,7 @@ 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)

This will cause us to not use the generic aarch64 flush_idcache_range(),
which uses DC CVAU and IC IVAU. Does that not work on Windows?

If it doesn't then I think the ifdeffery would be more clearly
structured as

#elif defined(__aarch64__)

ifdef CONFIG_DARWIN
[macos implementation of flush_idcache_range]
#elif defined(CONFIG_WIN32)
/* Explanation here of why the generic version doesn't work */
#else
/* generic version */
#endif

#elif defined(__mips__)

etc

thanks
-- PMM

Re: [PATCH 1/4] util/cacheflush: fix illegal instruction on windows-arm64

[Richard Henderson]

On 2/14/23 06:44, Peter Maydell wrote:
> This will cause us to not use the generic aarch64 flush_idcache_range(),
> which uses DC CVAU and IC IVAU. Does that not work on Windows?
> 
> If it doesn't then I think the ifdeffery would be more clearly
> structured as
> 
> #elif defined(__aarch64__)
> 
> ifdef CONFIG_DARWIN
> [macos implementation of flush_idcache_range]
> #elif defined(CONFIG_WIN32)
> /* Explanation here of why the generic version doesn't work */
> #else
> /* generic version */
> #endif
> 
> #elif defined(__mips__)

More specifically, there *must* be a replacement, or TCG will not work.
It appears as if FlushInstructionCache is the right syscall on windows.

I cannot find documentation for a data cache flush.  But until there is also a way to 
allocate split w^x memory regions (tcg/region.c, alloc_code_gen_buffer_splitwx), you need 
not worry about that.  You could reasonably assert(rx == rw) there.


r~

Re: [PATCH 1/4] util/cacheflush: fix illegal instruction on windows-arm64

[Pierrick Bouvier]

On 2/14/23 17:44, Peter Maydell wrote:
> On Mon, 13 Feb 2023 at 20:50, Pierrick Bouvier
> <pierrick.bouvier@linaro.org> wrote:
>>
>> mrs instruction fails as an illegal instruction.
>> For now, no cache information is retrieved for this platform.
>> It could be specialized later, using Windows API.
> 
> Unless I'm misreading the code, there's a sys_cache_info()
> implementation that's only guarded by if defined(_WIN32), so
> presumably we're using that on AArch64 also. Does it return
> sensible values ?
> 

Yes, it works on arm64, and I checked it was returned by the expected 
"windows version" function sys_cache_info.
On my machine, having a snapdragon 8cx gen3 processor, it returns (64, 64).

It's on par with what I can see from a WSL1 environment (Linux, without 
VM) on the same machine:
$ getconf LEVEL1_DCACHE_LINESIZE
64
$ getconf LEVEL1_ICACHE_LINESIZE
64

>> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
>> ---
>>   util/cacheflush.c | 5 +++--
>>   1 file changed, 3 insertions(+), 2 deletions(-)
>>
>> diff --git a/util/cacheflush.c b/util/cacheflush.c
>> index 2c2c73e085..149f103d32 100644
>> --- a/util/cacheflush.c
>> +++ b/util/cacheflush.c
>> @@ -121,8 +121,9 @@ static void sys_cache_info(int *isize, int *dsize)
>>   static bool have_coherent_icache;
>>   #endif
>>
>> -#if defined(__aarch64__) && !defined(CONFIG_DARWIN)
>> +#if defined(__aarch64__) && !defined(CONFIG_DARWIN) && !defined(CONFIG_WIN32)
>>   /* Apple does not expose CTR_EL0, so we must use system interfaces. */
>> +/* Does not work on windows-arm64, illegal instruction using mrs */
> 
> This comment should be better integrated with the previous, because
> the reason for the illegal instruction exception on Windows is the
> same as for macos -- the OS doesn't expose CTR_EL0 to userspace.
> 

I'll update the comment 👍

>>   static uint64_t save_ctr_el0;
>>   static void arch_cache_info(int *isize, int *dsize)
>>   {
>> @@ -225,7 +226,7 @@ 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)
> 
> This will cause us to not use the generic aarch64 flush_idcache_range(),
> which uses DC CVAU and IC IVAU. Does that not work on Windows?
> 
> If it doesn't then I think the ifdeffery would be more clearly
> structured as
> 
> #elif defined(__aarch64__)
> 
> ifdef CONFIG_DARWIN
> [macos implementation of flush_idcache_range]
> #elif defined(CONFIG_WIN32)
> /* Explanation here of why the generic version doesn't work */
> #else
> /* generic version */
> #endif
> 
> #elif defined(__mips__)
> 
> etc
>

For now, the generic flush_idcache_range, using __builtin___clear_cache 
is used. Richard mentioned 'there *must* be a replacement, or TCG will 
not work'.

As said in the cover letter, I could successfully install and boot an 
arm64 and x64 vm.

I'm not an expert on this area, but I imagine that booting a full VM 
will force TCG to emit code at the same address twice (after having 
generated enough translated blocks), which shows that generic 
flush_idcache_range works. Is that reasoning correct?

Do you think we still need a specialized version for windows-arm64?

> thanks
> -- PMM

Re: [PATCH 1/4] util/cacheflush: fix illegal instruction on windows-arm64

[Richard Henderson]

On 2/15/23 02:49, Pierrick Bouvier wrote:
> I'm not an expert on this area, but I imagine that booting a full VM will force TCG to 
> emit code at the same address twice (after having generated enough translated blocks), 
> which shows that generic flush_idcache_range works. Is that reasoning correct?
> 
> Do you think we still need a specialized version for windows-arm64?

No, I don't think so.  It would be ideal if we were able to read CTR_EL0, because we make 
use of the IDC field, but 0 is a safe default.


r~

Re: [PATCH 1/4] util/cacheflush: fix illegal instruction on windows-arm64

[Pierrick Bouvier]

After some investigation on this, I found that even faking ctr_el0 
content does not work. Indeed, "dc cvau" and "ic ivau" both are 
privileged too (and fail with illegal instruction).

I started looking how other projects are handling this. In the case of 
firefox js engine, they simply perform a FlushInstructionCache [1], and 
nothing for data cache. From what I found on various websites, there is 
no way to perform any data cache flush from userspace under Windows.

Out of curiosity, I looked in llvm how __builtin___clear_cache was 
implemented, and for windows-arm64, it's simply a call to the same 
function, FlushInstructionCache [2].

This explains why the generic implementation of flush_idcache_range does 
the correct thing for this platform, and why tests I ran worked. Thus, 
it's probably the best approach we can have for now.

[1] 
https://hg.mozilla.org/mozilla-central/file/tip/js/src/jit/arm64/vixl/MozCpu-vixl.cpp#l110
[2] 
https://github.com/llvm/llvm-project/blob/574e417460cdfebb8157fbe61b6a015e44856f64/compiler-rt/lib/builtins/clear_cache.c#L66

On 2/15/23 19:22, Richard Henderson wrote:
> On 2/15/23 02:49, Pierrick Bouvier wrote:
>> I'm not an expert on this area, but I imagine that booting a full VM will force TCG to
>> emit code at the same address twice (after having generated enough translated blocks),
>> which shows that generic flush_idcache_range works. Is that reasoning correct?
>>
>> Do you think we still need a specialized version for windows-arm64?
> 
> No, I don't think so.  It would be ideal if we were able to read CTR_EL0, because we make
> use of the IDC field, but 0 is a safe default.
> 
> 
> r~

[PATCH 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 | 18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index 5b38c7bd04..84f62d0a17 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -51,14 +51,28 @@ typedef struct sockaddr_un {
 extern "C" {
 #endif
 
-#if defined(_WIN64)
+#if defined(__aarch64__)
+/* 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
-- 
2.30.2

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

[Philippe Mathieu-Daudé]

Hi Pierrick,

On 13/2/23 17:13, 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 | 18 ++++++++++++++++--
>   1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
> index 5b38c7bd04..84f62d0a17 100644
> --- a/include/sysemu/os-win32.h
> +++ b/include/sysemu/os-win32.h
> @@ -51,14 +51,28 @@ typedef struct sockaddr_un {
>   extern "C" {
>   #endif
>   
> -#if defined(_WIN64)
> +#if defined(__aarch64__)

Shouldn't we check for __MINGW64__?

    #if defined(__aarch64__) && defined(__MINGW64__)

> +/* 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. */

So this is not stable. Better would be to check the symbols availability
at link-time via meson; see for example glusterfs_ftruncate_has_stat
which defines CONFIG_GLUSTERFS_FTRUNCATE_HAS_STAT.

A similar check could define CONFIG_MINGW64_HAS_SETJMP_LONGJMP.

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

Regards,

Phil.

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

[Pierrick Bouvier]

On 2/14/23 08:11, Philippe Mathieu-Daudé wrote:
> Hi Pierrick,
> 
> On 13/2/23 17:13, 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 | 18 ++++++++++++++++--
>>    1 file changed, 16 insertions(+), 2 deletions(-)
>>
>> diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
>> index 5b38c7bd04..84f62d0a17 100644
>> --- a/include/sysemu/os-win32.h
>> +++ b/include/sysemu/os-win32.h
>> @@ -51,14 +51,28 @@ typedef struct sockaddr_un {
>>    extern "C" {
>>    #endif
>>    
>> -#if defined(_WIN64)
>> +#if defined(__aarch64__)
> 
> Shouldn't we check for __MINGW64__?
> 
>      #if defined(__aarch64__) && defined(__MINGW64__)
>

I thought QEMU was only compiled using MinGW under Windows, (from CI, 
that's the case, [1], [2]), but maybe that's a wrong assumption.

[1] 
https://gitlab.com/qemu-project/qemu/-/blob/master/.gitlab-ci.d/windows.yml
[2] 
https://gitlab.com/qemu-project/qemu/-/blob/master/tests/docker/dockerfiles/fedora-win64-cross.docker

For windows-arm64, we need an alternative setjmp/longjmp implementation 
(__builtin_setjmp and __builtin_longjmp in clang are not available), 
thus, that makes MinGW mandatory, at least for this platform.

Would adding this be satisfying? Or better to add this check in Meson?
#ifndef __MINGW64__
#error MinGW must be available for this platform
#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. */
> 
> So this is not stable. Better would be to check the symbols availability
> at link-time via meson; see for example glusterfs_ftruncate_has_stat
> which defines CONFIG_GLUSTERFS_FTRUNCATE_HAS_STAT.
> 
> A similar check could define CONFIG_MINGW64_HAS_SETJMP_LONGJMP.
> 

You're right, it's not stable. Checking this using meson sounds a good 
approach.

>> +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
> 
> Regards,
> 
> Phil.

[PATCH 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 3/4] qga/vss-win32: fix warning for clang++-15

[Konstantin Kostiuk]

[-- Attachment #1: Type: text/plain, Size: 1233 bytes --]

Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>



On Mon, Feb 13, 2023 at 6:14 PM Pierrick Bouvier <
pierrick.bouvier@linaro.org> 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;
>            ^~
> 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
>
>

[-- Attachment #2: Type: text/html, Size: 1901 bytes --]

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

[Cédric Le Goater]

On 2/13/23 17:13, 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;
>             ^~
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.

> ---
>   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;
>   

[PATCH 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 definition.

Setting the rnd variable to zero does not hurt anyway.

../target/ppc/dfp_helper.c:141:13: error: variable 'rnd' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]                                                                              assert(0); /* cannot get here */                                                                                                                                                                                    ^~~~~~~~~
../include/qemu/osdep.h:229:20: note: expanded from macro 'assert'                                                                                                                                        #define assert(x)  g_assert(x)                                                                                                                                                                                                         ^~~~~~~~~~~
/clangarm64/bin/../include/glib-2.0/glib/gtestutils.h:235:49: note: expanded from macro 'g_assert'                                                                                                                               if G_LIKELY (expr) ; else \
                                                ^~~~~~~~~~~~~~~
/clangarm64/bin/../include/glib-2.0/glib/gmacros.h:1186:25: note: expanded from macro 'G_LIKELY'
                        ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
../target/ppc/dfp_helper.c:144:42: note: uninitialized use occurs here
    decContextSetRounding(&dfp->context, rnd);

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..0b4b280683 100644
--- a/target/ppc/dfp_helper.c
+++ b/target/ppc/dfp_helper.c
@@ -69,7 +69,7 @@ struct PPC_DFP {
 
 static void dfp_prepare_rounding_mode(decContext *context, uint64_t fpscr)
 {
-    enum rounding rnd;
+    enum rounding rnd = 0;
 
     switch ((fpscr & FP_DRN) >> FPSCR_DRN0) {
     case 0:
@@ -106,7 +106,7 @@ static void dfp_prepare_rounding_mode(decContext *context, uint64_t fpscr)
 static void dfp_set_round_mode_from_immediate(uint8_t r, uint8_t rmc,
                                                   struct PPC_DFP *dfp)
 {
-    enum rounding rnd;
+    enum rounding rnd = 0;
     if (r == 0) {
         switch (rmc & 3) {
         case 0:
-- 
2.30.2

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

[Cédric Le Goater]

On 2/13/23 17:13, 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 definition.
> 
> Setting the rnd variable to zero does not hurt anyway.
> 
> ../target/ppc/dfp_helper.c:141:13: error: variable 'rnd' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]                                                                              assert(0); /* cannot get here */                                                                                                                                                                                    ^~~~~~~~~
> ../include/qemu/osdep.h:229:20: note: expanded from macro 'assert'                                                                                                                                        #define assert(x)  g_assert(x)                                                                                                                                                                                                         ^~~~~~~~~~~
> /clangarm64/bin/../include/glib-2.0/glib/gtestutils.h:235:49: note: expanded from macro 'g_assert'                                                                                                                               if G_LIKELY (expr) ; else \
>                                                  ^~~~~~~~~~~~~~~
> /clangarm64/bin/../include/glib-2.0/glib/gmacros.h:1186:25: note: expanded from macro 'G_LIKELY'
>                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../target/ppc/dfp_helper.c:144:42: note: uninitialized use occurs here
>      decContextSetRounding(&dfp->context, rnd);
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>

Reviewed-by: Cédric Le Goater <clg@kaod.org>

Thanks,

C.


> ---
>   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..0b4b280683 100644
> --- a/target/ppc/dfp_helper.c
> +++ b/target/ppc/dfp_helper.c
> @@ -69,7 +69,7 @@ struct PPC_DFP {
>   
>   static void dfp_prepare_rounding_mode(decContext *context, uint64_t fpscr)
>   {
> -    enum rounding rnd;
> +    enum rounding rnd = 0;
>   
>       switch ((fpscr & FP_DRN) >> FPSCR_DRN0) {
>       case 0:
> @@ -106,7 +106,7 @@ static void dfp_prepare_rounding_mode(decContext *context, uint64_t fpscr)
>   static void dfp_set_round_mode_from_immediate(uint8_t r, uint8_t rmc,
>                                                     struct PPC_DFP *dfp)
>   {
> -    enum rounding rnd;
> +    enum rounding rnd = 0;
>       if (r == 0) {
>           switch (rmc & 3) {
>           case 0:

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

[Philippe Mathieu-Daudé]

On 13/2/23 17:13, 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 definition.
> 
> Setting the rnd variable to zero does not hurt anyway.
> 
> ../target/ppc/dfp_helper.c:141:13: error: variable 'rnd' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]                                                                              assert(0); /* cannot get here */                                                                                                                                                                                    ^~~~~~~~~
> ../include/qemu/osdep.h:229:20: note: expanded from macro 'assert'                                                                                                                                        #define assert(x)  g_assert(x)                                                                                                                                                                                                         ^~~~~~~~~~~
> /clangarm64/bin/../include/glib-2.0/glib/gtestutils.h:235:49: note: expanded from macro 'g_assert'                                                                                                                               if G_LIKELY (expr) ; else \
>                                                  ^~~~~~~~~~~~~~~
> /clangarm64/bin/../include/glib-2.0/glib/gmacros.h:1186:25: note: expanded from macro 'G_LIKELY'
>                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../target/ppc/dfp_helper.c:144:42: note: uninitialized use occurs here
>      decContextSetRounding(&dfp->context, rnd);
> 
> 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..0b4b280683 100644
> --- a/target/ppc/dfp_helper.c
> +++ b/target/ppc/dfp_helper.c
> @@ -69,7 +69,7 @@ struct PPC_DFP {
>   
>   static void dfp_prepare_rounding_mode(decContext *context, uint64_t fpscr)
>   {
> -    enum rounding rnd;
> +    enum rounding rnd = 0;
>   
>       switch ((fpscr & FP_DRN) >> FPSCR_DRN0) {
>       case 0:
> @@ -106,7 +106,7 @@ static void dfp_prepare_rounding_mode(decContext *context, uint64_t fpscr)
>   static void dfp_set_round_mode_from_immediate(uint8_t r, uint8_t rmc,
>                                                     struct PPC_DFP *dfp)
>   {
> -    enum rounding rnd;
> +    enum rounding rnd = 0;

Could DEC_ROUND_DEFAULT be clearer?

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

[Pierrick Bouvier]

On 2/14/23 08:14, Philippe Mathieu-Daudé wrote:
> On 13/2/23 17:13, 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 definition.
>>
>> Setting the rnd variable to zero does not hurt anyway.
>>
>> ../target/ppc/dfp_helper.c:141:13: error: variable 'rnd' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]                                                                              assert(0); /* cannot get here */                                                                                                                                                                                    ^~~~~~~~~
>> ../include/qemu/osdep.h:229:20: note: expanded from macro 'assert'                                                                                                                                        #define assert(x)  g_assert(x)                                                                                                                                                                                                         ^~~~~~~~~~~
>> /clangarm64/bin/../include/glib-2.0/glib/gtestutils.h:235:49: note: expanded from macro 'g_assert'                                                                                                                               if G_LIKELY (expr) ; else \
>>                                                   ^~~~~~~~~~~~~~~
>> /clangarm64/bin/../include/glib-2.0/glib/gmacros.h:1186:25: note: expanded from macro 'G_LIKELY'
>>                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> ../target/ppc/dfp_helper.c:144:42: note: uninitialized use occurs here
>>       decContextSetRounding(&dfp->context, rnd);
>>
>> 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..0b4b280683 100644
>> --- a/target/ppc/dfp_helper.c
>> +++ b/target/ppc/dfp_helper.c
>> @@ -69,7 +69,7 @@ struct PPC_DFP {
>>    
>>    static void dfp_prepare_rounding_mode(decContext *context, uint64_t fpscr)
>>    {
>> -    enum rounding rnd;
>> +    enum rounding rnd = 0;
>>    
>>        switch ((fpscr & FP_DRN) >> FPSCR_DRN0) {
>>        case 0:
>> @@ -106,7 +106,7 @@ static void dfp_prepare_rounding_mode(decContext *context, uint64_t fpscr)
>>    static void dfp_set_round_mode_from_immediate(uint8_t r, uint8_t rmc,
>>                                                      struct PPC_DFP *dfp)
>>    {
>> -    enum rounding rnd;
>> +    enum rounding rnd = 0;
> 
> Could DEC_ROUND_DEFAULT be clearer?
> 

I missed that macro definition, and that seems like a good default 
value. I'll change to this.

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

[Richard Henderson]

On 2/13/23 06:13, 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 definition.
> 
> Setting the rnd variable to zero does not hurt anyway.
> 
> ../target/ppc/dfp_helper.c:141:13: error: variable 'rnd' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]                                                                              assert(0); /* cannot get here */                                                                                                                                                                                    ^~~~~~~~~
> ../include/qemu/osdep.h:229:20: note: expanded from macro 'assert'                                                                                                                                        #define assert(x)  g_assert(x)                                                                                                                                                                                                         ^~~~~~~~~~~
> /clangarm64/bin/../include/glib-2.0/glib/gtestutils.h:235:49: note: expanded from macro 'g_assert'                                                                                                                               if G_LIKELY (expr) ; else \
>                                                  ^~~~~~~~~~~~~~~
> /clangarm64/bin/../include/glib-2.0/glib/gmacros.h:1186:25: note: expanded from macro 'G_LIKELY'
>                          ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../target/ppc/dfp_helper.c:144:42: note: uninitialized use occurs here
>      decContextSetRounding(&dfp->context, rnd);

I think the problem is with assert(0) not being seen to terminate.
Replace these with g_assert_not_reached().


r~

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

[Pierrick Bouvier]

On 2/14/23 19:10, Richard Henderson wrote:
> On 2/13/23 06:13, 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 definition.
>>
>> Setting the rnd variable to zero does not hurt anyway.
>>
>> ../target/ppc/dfp_helper.c:141:13: error: variable 'rnd' is used uninitialized whenever 'if' condition is true [-Werror,-Wsometimes-uninitialized]                                                                              assert(0); /* cannot get here */                                                                                                                                                                                    ^~~~~~~~~
>> ../include/qemu/osdep.h:229:20: note: expanded from macro 'assert'                                                                                                                                        #define assert(x)  g_assert(x)                                                                                                                                                                                                         ^~~~~~~~~~~
>> /clangarm64/bin/../include/glib-2.0/glib/gtestutils.h:235:49: note: expanded from macro 'g_assert'                                                                                                                               if G_LIKELY (expr) ; else \
>>                                                   ^~~~~~~~~~~~~~~
>> /clangarm64/bin/../include/glib-2.0/glib/gmacros.h:1186:25: note: expanded from macro 'G_LIKELY'
>>                           ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>> ../target/ppc/dfp_helper.c:144:42: note: uninitialized use occurs here
>>       decContextSetRounding(&dfp->context, rnd);
> 
> I think the problem is with assert(0) not being seen to terminate.
> Replace these with g_assert_not_reached().
> 

Indeed, that solves the issue. Thanks for the suggestion, I'll use this 
instead.

> 
> r~
>