diff mbox series

x86: please Clang in arch_set_info_guest()

Message ID c758de2b-c453-4dba-ddc0-0c9548172c6e@suse.com (mailing list archive)
State New
Headers show
Series x86: please Clang in arch_set_info_guest() | expand

Commit Message

Jan Beulich June 9, 2021, 1:14 p.m. UTC
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.

Comments

Andrew Cooper June 9, 2021, 3:45 p.m. UTC | #1
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>
Jan Beulich June 10, 2021, 7:37 a.m. UTC | #2
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
diff mbox series

Patch

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