xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH] x86: please Clang in arch_set_info_guest()
@ 2021-06-09 13:14 Jan Beulich
  2021-06-09 15:45 ` Andrew Cooper
  0 siblings, 1 reply; 3+ messages in thread
From: Jan Beulich @ 2021-06-09 13:14 UTC (permalink / raw)
  To: xen-devel; +Cc: Andrew Cooper, Wei Liu, Roger Pau Monné

Clang 10 reports

domain.c:1328:10: error: variable 'cr3_mfn' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
    if ( !compat )
         ^~~~~~~
domain.c:1334:34: note: uninitialized use occurs here
    cr3_page = get_page_from_mfn(cr3_mfn, d);
                                 ^~~~~~~
domain.c:1328:5: note: remove the 'if' if its condition is always true
    if ( !compat )
    ^~~~~~~~~~~~~~
domain.c:1042:18: note: initialize the variable 'cr3_mfn' to silence this warning
    mfn_t cr3_mfn;
                 ^
                  = 0
domain.c:1189:14: error: variable 'fail' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
        if ( !compat )
             ^~~~~~~
domain.c:1211:9: note: uninitialized use occurs here
        fail |= v->arch.pv.gdt_ents != c(gdt_ents);
        ^~~~
domain.c:1189:9: note: remove the 'if' if its condition is always true
        if ( !compat )
        ^~~~~~~~~~~~~~
domain.c:1187:18: note: initialize the variable 'fail' to silence this warning
        bool fail;
                 ^
                  = false

despite this being a build with -O2 in effect, and despite "compat"
being constant "false" when CONFIG_COMPAT (and hence CONFIG_PV32) is not
defined, as it gets set at the top of the function from the result of
is_pv_32bit_domain().

Re-arrange the two "offending" if()s such that when COMPAT=n the
respective variables will be seen as unconditionally initialized. The
original aim was to have the !compat cases first, though.

Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
---
I wonder how many more there are to come.

--- a/xen/arch/x86/domain.c
+++ b/xen/arch/x86/domain.c
@@ -1186,7 +1186,17 @@ int arch_set_info_guest(
         unsigned long pfn = pagetable_get_pfn(v->arch.guest_table);
         bool fail;
 
-        if ( !compat )
+#ifdef CONFIG_COMPAT
+        if ( compat )
+        {
+            l4_pgentry_t *l4tab = map_domain_page(_mfn(pfn));
+
+            pfn = l4e_get_pfn(*l4tab);
+            unmap_domain_page(l4tab);
+            fail = compat_pfn_to_cr3(pfn) != c.cmp->ctrlreg[3];
+        }
+        else
+#endif
         {
             fail = xen_pfn_to_cr3(pfn) != c.nat->ctrlreg[3];
             if ( pagetable_is_null(v->arch.guest_table_user) )
@@ -1197,16 +1207,6 @@ int arch_set_info_guest(
                 fail |= xen_pfn_to_cr3(pfn) != c.nat->ctrlreg[1];
             }
         }
-#ifdef CONFIG_COMPAT
-        else
-        {
-            l4_pgentry_t *l4tab = map_domain_page(_mfn(pfn));
-
-            pfn = l4e_get_pfn(*l4tab);
-            unmap_domain_page(l4tab);
-            fail = compat_pfn_to_cr3(pfn) != c.cmp->ctrlreg[3];
-        }
-#endif
 
         fail |= v->arch.pv.gdt_ents != c(gdt_ents);
         for ( i = 0; !fail && i < nr_gdt_frames; ++i )
@@ -1325,12 +1325,12 @@ int arch_set_info_guest(
 
     set_bit(_VPF_in_reset, &v->pause_flags);
 
-    if ( !compat )
-        cr3_mfn = _mfn(xen_cr3_to_pfn(c.nat->ctrlreg[3]));
 #ifdef CONFIG_COMPAT
-    else
+    if ( compat )
         cr3_mfn = _mfn(compat_cr3_to_pfn(c.cmp->ctrlreg[3]));
+    else
 #endif
+        cr3_mfn = _mfn(xen_cr3_to_pfn(c.nat->ctrlreg[3]));
     cr3_page = get_page_from_mfn(cr3_mfn, d);
 
     if ( !cr3_page )



^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] x86: please Clang in arch_set_info_guest()
  2021-06-09 13:14 [PATCH] x86: please Clang in arch_set_info_guest() Jan Beulich
@ 2021-06-09 15:45 ` Andrew Cooper
  2021-06-10  7:37   ` Jan Beulich
  0 siblings, 1 reply; 3+ messages in thread
From: Andrew Cooper @ 2021-06-09 15:45 UTC (permalink / raw)
  To: Jan Beulich, xen-devel; +Cc: Wei Liu, Roger Pau Monné

On 09/06/2021 14:14, Jan Beulich wrote:
> Clang 10 reports
>
> domain.c:1328:10: error: variable 'cr3_mfn' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>     if ( !compat )
>          ^~~~~~~
> domain.c:1334:34: note: uninitialized use occurs here
>     cr3_page = get_page_from_mfn(cr3_mfn, d);
>                                  ^~~~~~~
> domain.c:1328:5: note: remove the 'if' if its condition is always true
>     if ( !compat )
>     ^~~~~~~~~~~~~~
> domain.c:1042:18: note: initialize the variable 'cr3_mfn' to silence this warning
>     mfn_t cr3_mfn;
>                  ^
>                   = 0
> domain.c:1189:14: error: variable 'fail' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>         if ( !compat )
>              ^~~~~~~
> domain.c:1211:9: note: uninitialized use occurs here
>         fail |= v->arch.pv.gdt_ents != c(gdt_ents);
>         ^~~~
> domain.c:1189:9: note: remove the 'if' if its condition is always true
>         if ( !compat )
>         ^~~~~~~~~~~~~~
> domain.c:1187:18: note: initialize the variable 'fail' to silence this warning
>         bool fail;
>                  ^
>                   = false
>
> despite this being a build with -O2 in effect, and despite "compat"
> being constant "false" when CONFIG_COMPAT (and hence CONFIG_PV32) is not
> defined, as it gets set at the top of the function from the result of
> is_pv_32bit_domain().
>
> Re-arrange the two "offending" if()s such that when COMPAT=n the
> respective variables will be seen as unconditionally initialized. The
> original aim was to have the !compat cases first, though.
>
> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
> Signed-off-by: Jan Beulich <jbeulich@suse.com>
> ---
> I wonder how many more there are to come.

https://gitlab.com/xen-project/patchew/xen/-/pipelines/317744453

Everything seems ok now.  The failure is a known arm32 randconfig issue
which still hasn't been fixed, and is unrelated to this.

Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] x86: please Clang in arch_set_info_guest()
  2021-06-09 15:45 ` Andrew Cooper
@ 2021-06-10  7:37   ` Jan Beulich
  0 siblings, 0 replies; 3+ messages in thread
From: Jan Beulich @ 2021-06-10  7:37 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Wei Liu, Roger Pau Monné, xen-devel

On 09.06.2021 17:45, Andrew Cooper wrote:
> On 09/06/2021 14:14, Jan Beulich wrote:
>> Clang 10 reports
>>
>> domain.c:1328:10: error: variable 'cr3_mfn' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>>     if ( !compat )
>>          ^~~~~~~
>> domain.c:1334:34: note: uninitialized use occurs here
>>     cr3_page = get_page_from_mfn(cr3_mfn, d);
>>                                  ^~~~~~~
>> domain.c:1328:5: note: remove the 'if' if its condition is always true
>>     if ( !compat )
>>     ^~~~~~~~~~~~~~
>> domain.c:1042:18: note: initialize the variable 'cr3_mfn' to silence this warning
>>     mfn_t cr3_mfn;
>>                  ^
>>                   = 0
>> domain.c:1189:14: error: variable 'fail' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
>>         if ( !compat )
>>              ^~~~~~~
>> domain.c:1211:9: note: uninitialized use occurs here
>>         fail |= v->arch.pv.gdt_ents != c(gdt_ents);
>>         ^~~~
>> domain.c:1189:9: note: remove the 'if' if its condition is always true
>>         if ( !compat )
>>         ^~~~~~~~~~~~~~
>> domain.c:1187:18: note: initialize the variable 'fail' to silence this warning
>>         bool fail;
>>                  ^
>>                   = false
>>
>> despite this being a build with -O2 in effect, and despite "compat"
>> being constant "false" when CONFIG_COMPAT (and hence CONFIG_PV32) is not
>> defined, as it gets set at the top of the function from the result of
>> is_pv_32bit_domain().
>>
>> Re-arrange the two "offending" if()s such that when COMPAT=n the
>> respective variables will be seen as unconditionally initialized. The
>> original aim was to have the !compat cases first, though.
>>
>> Reported-by: Andrew Cooper <andrew.cooper3@citrix.com>
>> Signed-off-by: Jan Beulich <jbeulich@suse.com>
>> ---
>> I wonder how many more there are to come.
> 
> https://gitlab.com/xen-project/patchew/xen/-/pipelines/317744453
> 
> Everything seems ok now.  The failure is a known arm32 randconfig issue
> which still hasn't been fixed, and is unrelated to this.

Well, the question was primarily for current code and the presently used
Clang version (which you say looks okay now), but also for arbitrary
code changes which may trigger the same issue for any other similar
constructs, plus also for future Clang versions, which may become even
pickier. And not to forget .config variations.

> Acked-by: Andrew Cooper <andrew.cooper3@citrix.com>

Thanks.

Jan



^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-06-10  7:37 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-09 13:14 [PATCH] x86: please Clang in arch_set_info_guest() Jan Beulich
2021-06-09 15:45 ` Andrew Cooper
2021-06-10  7:37   ` Jan Beulich

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).