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

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

[Pierrick Bouvier]

Since v3:
- Remove CONFIG_MINGW64_HAS_SETJMP_LONGJMP (only check in meson)
- Fix comment to refer to windows-x64 vs windows-arm64

Since v2:

- Delete superfluous comment on unreachable code
- Fix style for multiline comments

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 | 28 ++++++++++++++++++++++++----
 meson.build               | 21 +++++++++++++++++++++
 qga/vss-win32/install.cpp |  2 +-
 target/ppc/dfp_helper.c   |  4 ++--
 util/cacheflush.c         | 14 +++++++++++---
 5 files changed, 59 insertions(+), 10 deletions(-)

--
2.30.2

[PATCH v4 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>
Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
---
 util/cacheflush.c | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/util/cacheflush.c b/util/cacheflush.c
index 2c2c73e085..06c2333a60 100644
--- a/util/cacheflush.c
+++ b/util/cacheflush.c
@@ -121,8 +121,12 @@ 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 +229,11 @@ 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 v4 1/4] util/cacheflush: fix cache on windows-arm64

[Richard Henderson]

On 2/21/23 05:30, 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>
> Reviewed-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   util/cacheflush.c | 14 +++++++++++---
>   1 file changed, 11 insertions(+), 3 deletions(-)

Queueing this to tcg-next.


r~

[PATCH v4 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>
Acked-by: Richard Henderson <richard.henderson@linaro.org>
---
 include/sysemu/os-win32.h | 28 ++++++++++++++++++++++++----
 meson.build               | 21 +++++++++++++++++++++
 2 files changed, 45 insertions(+), 4 deletions(-)

diff --git a/include/sysemu/os-win32.h b/include/sysemu/os-win32.h
index 5b38c7bd04..97d0243aee 100644
--- a/include/sysemu/os-win32.h
+++ b/include/sysemu/os-win32.h
@@ -51,14 +51,34 @@ typedef struct sockaddr_un {
 extern "C" {
 #endif
 
-#if defined(_WIN64)
-/* On w64, setjmp is implemented by _setjmp which needs a second parameter.
+#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 windows-x64, 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. */
+ * 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 a76c855312..b676c831b3 100644
--- a/meson.build
+++ b/meson.build
@@ -2470,6 +2470,27 @@ 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')
+
+  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 v4 2/4] sysemu/os-win32: fix setjmp/longjmp on windows-arm64

[Philippe Mathieu-Daudé]

On 21/2/23 16:30, 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>
> Acked-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/sysemu/os-win32.h | 28 ++++++++++++++++++++++++----
>   meson.build               | 21 +++++++++++++++++++++
>   2 files changed, 45 insertions(+), 4 deletions(-)


> -#if defined(_WIN64)
> -/* On w64, setjmp is implemented by _setjmp which needs a second parameter.
> +#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 windows-x64, 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. */
> + * lets longjmp try a stack unwinding which will crash with generated code.
> + */
>   # undef setjmp
>   # define setjmp(env) _setjmp(env, NULL)
> -#endif
> +#endif /* __aarch64__ */

This comment is confusing, the previous if ladder is about i86. Maybe
better not add any comment?

Otherwise,
Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Pierrick Bouvier]

On 2/21/23 23:27, Philippe Mathieu-Daudé wrote:
> On 21/2/23 16:30, 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>
>> Acked-by: Richard Henderson <richard.henderson@linaro.org>
>> ---
>>    include/sysemu/os-win32.h | 28 ++++++++++++++++++++++++----
>>    meson.build               | 21 +++++++++++++++++++++
>>    2 files changed, 45 insertions(+), 4 deletions(-)
> 
> 
>> -#if defined(_WIN64)
>> -/* On w64, setjmp is implemented by _setjmp which needs a second parameter.
>> +#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 windows-x64, 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. */
>> + * lets longjmp try a stack unwinding which will crash with generated code.
>> + */
>>    # undef setjmp
>>    # define setjmp(env) _setjmp(env, NULL)
>> -#endif
>> +#endif /* __aarch64__ */
> 
> This comment is confusing, the previous if ladder is about i86. Maybe
> better not add any comment?

If I am not mistaken, before we had:

#if x64
define setjmp as _setjmp(env, 0)
#endif
// nothing done for x86

and now we have:

#if aarch64
define setjmp as __mingw_setjmp
define longjmp as __mingw_longjmp
#elif x64
define setjmp as _setjmp(env, 0)
#endif
// nothing done for x86

Maybe the patch format is confusing, or I missed what you pointed.

Pierrick

> 
> Otherwise,
> Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>
> 

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

[Philippe Mathieu-Daudé]

On 22/2/23 17:08, Pierrick Bouvier wrote:
> On 2/21/23 23:27, Philippe Mathieu-Daudé wrote:
>> On 21/2/23 16:30, 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>
>>> Acked-by: Richard Henderson <richard.henderson@linaro.org>
>>> ---
>>>    include/sysemu/os-win32.h | 28 ++++++++++++++++++++++++----
>>>    meson.build               | 21 +++++++++++++++++++++
>>>    2 files changed, 45 insertions(+), 4 deletions(-)
>>
>>
>>> -#if defined(_WIN64)
>>> -/* On w64, setjmp is implemented by _setjmp which needs a second 
>>> parameter.
>>> +#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 windows-x64, 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. */
>>> + * lets longjmp try a stack unwinding which will crash with 
>>> generated code.
>>> + */
>>>    # undef setjmp
>>>    # define setjmp(env) _setjmp(env, NULL)
>>> -#endif
>>> +#endif /* __aarch64__ */
>>
>> This comment is confusing, the previous if ladder is about i86. Maybe
>> better not add any comment?
> 
> If I am not mistaken, before we had:
> 
> #if x64
> define setjmp as _setjmp(env, 0)
> #endif
> // nothing done for x86
> 
> and now we have:
> 
> #if aarch64
> define setjmp as __mingw_setjmp
> define longjmp as __mingw_longjmp
> #elif x64
> define setjmp as _setjmp(env, 0)
> #endif
> // nothing done for x86
> 
> Maybe the patch format is confusing, or I missed what you pointed.

Oh OK, we are good then, sorry :)

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

[Richard Henderson]

On 2/21/23 05:30, 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>
> Acked-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   include/sysemu/os-win32.h | 28 ++++++++++++++++++++++++----
>   meson.build               | 21 +++++++++++++++++++++
>   2 files changed, 45 insertions(+), 4 deletions(-)

Queueing this to tcg-next.


r~

[PATCH v4 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>
Fixes: 917ebcb170 ("qga-win: Fix QGA VSS Provider service stop failure")
Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>
Reviewed-by: Philippe Mathieu-Daudé <philmd@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 v4 3/4] qga/vss-win32: fix warning for clang++-15

[Pierrick Bouvier]

Sorry to come back on this, but it seems this specific commit was not 
integrated in trunk.

@Konstantin Kostiuk: If you plan to integrate this later (before 8.0 
tag), sorry for the noise. Since rc1 was published today, I think it may 
have been "lost".

If someone wants to merge it, that would be nice.

Thanks,
Pierrick

On 2/21/23 16:30, 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>
> Fixes: 917ebcb170 ("qga-win: Fix QGA VSS Provider service stop failure")
> Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>
> Reviewed-by: Philippe Mathieu-Daudé <philmd@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 v4 3/4] qga/vss-win32: fix warning for clang++-15

[Konstantin Kostiuk]

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

Hi Pierrick,

Thanks for reminding me. You are fully right to ping me. I really lost this
commit.
As QEMU is already at the code freeze stage, I don't want to push this into
8.0.
I hope it will be ok to merge after 8.0 was released.

Best Regards,
Konstantin Kostiuk.


On Tue, Mar 21, 2023 at 11:48 PM Pierrick Bouvier <
pierrick.bouvier@linaro.org> wrote:

> Sorry to come back on this, but it seems this specific commit was not
> integrated in trunk.
>
> @Konstantin Kostiuk: If you plan to integrate this later (before 8.0
> tag), sorry for the noise. Since rc1 was published today, I think it may
> have been "lost".
>
> If someone wants to merge it, that would be nice.
>
> Thanks,
> Pierrick
>
> On 2/21/23 16:30, 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>
> > Fixes: 917ebcb170 ("qga-win: Fix QGA VSS Provider service stop failure")
> > Reviewed-by: Konstantin Kostiuk <kkostiuk@redhat.com>
> > Reviewed-by: Philippe Mathieu-Daudé <philmd@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;
> >
>

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

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

[Daniel P. Berrangé]

On Wed, Mar 22, 2023 at 07:18:11PM +0200, Konstantin Kostiuk wrote:
> Hi Pierrick,
> 
> Thanks for reminding me. You are fully right to ping me. I really lost this
> commit.
> As QEMU is already at the code freeze stage, I don't want to push this into
> 8.0.

FWIW, this kind of fix is perfectly ok to merge during code freeze,
especially in rc0/rc1 timeframe. The initial freeze date merely cuts
off new feature additions, but bug fixes are still allowed. Once we
get to rc2 then we're more focused on bug fixes that target regressions
from the previous release.

With regards,
Daniel
-- 
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

[PATCH v4 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>
Reviewed-by: Richard Henderson <richard.henderson@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..5967ea07a9 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();
         }
     } 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();
         }
     }
     decContextSetRounding(&dfp->context, rnd);
-- 
2.30.2

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

[Philippe Mathieu-Daudé]

On 21/2/23 16:30, 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.

$ git grep 'assert(0)' | wc -l
       96

Should we mass-update and forbid 'assert(0)' adding a check in
scripts/checkpatch.pl? Otherwise we'll keep getting similar clang
warnings...

> Change code to g_assert_not_reached() fix the warning.
> 
> Signed-off-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
> Reviewed-by: Richard Henderson <richard.henderson@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..5967ea07a9 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();

Reviewed-by: Philippe Mathieu-Daudé <philmd@linaro.org>

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

[Richard Henderson]

On 2/21/23 12:30, Philippe Mathieu-Daudé wrote:
> On 21/2/23 16:30, 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.
> 
> $ git grep 'assert(0)' | wc -l
>        96
> 
> Should we mass-update and forbid 'assert(0)' adding a check in
> scripts/checkpatch.pl? Otherwise we'll keep getting similar clang
> warnings...

I just think assert(0) produces a less clean error message, so on that basis yes, we 
should replace them all.  Perhaps abort() as well, unless there's an error_report 
immediately preceding.

The fact that assert(0) was seen to fall through is a system header bug.  I see we have a 
workaround in include/qemu/osdep.h for __MINGW32__, but I guess this doesn't trigger for 
arm64?  Pierrick, would you mind testing a change there?


r~

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

[Pierrick Bouvier]

On 2/22/23 00:43, Richard Henderson wrote:
> On 2/21/23 12:30, Philippe Mathieu-Daudé wrote:
>> On 21/2/23 16:30, 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.
>>
>> $ git grep 'assert(0)' | wc -l
>>         96
>>
>> Should we mass-update and forbid 'assert(0)' adding a check in
>> scripts/checkpatch.pl? Otherwise we'll keep getting similar clang
>> warnings...
> 
> I just think assert(0) produces a less clean error message, so on that basis yes, we
> should replace them all.  Perhaps abort() as well, unless there's an error_report
> immediately preceding.
> 

If we start exploring this way, why not define explicit noreturn 
functions for this: unreachable() and todo()/unimplemented(). Names are 
inspired from the same Rust macros [1]. IMHO, all assert(0) fall into 
one of those two categories. It's easy to grep, and explicit.

The advantage, besides clarity, would be to guarantee it is never 
removed. I noticed qemu can't be compiled with NDEBUG for now, but with 
this, it would remove all assert invocations, including assert(0).

[1] https://doc.rust-lang.org/std/index.html#macros

> The fact that assert(0) was seen to fall through is a system header bug.  I see we have a
> workaround in include/qemu/osdep.h for __MINGW32__, but I guess this doesn't trigger for
> arm64?  Pierrick, would you mind testing a change there?
> 

I can confirm this workaround is enabled for windows-arm64.

Indeed, from msys2 clangarm64 env:
$ echo | cc -dM -E - | grep MINGW
#define __MINGW32__ 1
#define __MINGW64__ 1
$ cc --version
clang version 15.0.7
Target: aarch64-w64-windows-gnu

In more, it is needed. Without the workaround in osdep.h, compilation 
fails with this message:

qemu/include/exec/exec-all.h:619:5: error: call to 
qemu_build_not_reached_always declared with 'error' attribute: code path 
is reachable
     qemu_build_not_reached();
     ^
qemu/include/qemu/osdep.h:242:35: note: expanded from macro 
'qemu_build_not_reached'
#define qemu_build_not_reached()  qemu_build_not_reached_always()
                                   ^
1 error generated.

> 
> r~