[PATCH 0/7] xen: Switch to using -Og for debug builds

[PATCH 0/7] xen: Switch to using -Og for debug builds

[Andrew Cooper]

As with the toolstack side, we ought to use -Og for debug builds.

All fixes are trivial.  The first 3 are understandable, given reduced
optimisations.  The next 3 are, AFAICT, bogus diagnostics.

Andrew Cooper (7):
  xen/arm: Make make_cpus_node() compile at -Og
  x86/shim: Fix compilation at -Og
  x86/sysctl: Make arch_do_sysctl() compile at -Og
  x86/irq: Make create_irq() compile at -Og
  xen/efi: Make efi_start() compile at -Og
  x86/shadow: Make _shadow_prealloc() compile at -Og
  xen: Use -Og for debug builds when available

 xen/Makefile                    | 4 +++-
 xen/arch/arm/domain_build.c     | 2 +-
 xen/arch/x86/irq.c              | 2 +-
 xen/arch/x86/mm/shadow/common.c | 2 +-
 xen/arch/x86/pv/shim.c          | 6 +++---
 xen/arch/x86/sysctl.c           | 4 ++--
 xen/common/efi/boot.c           | 2 +-
 7 files changed, 12 insertions(+), 10 deletions(-)

-- 
2.11.0

[PATCH 1/7] xen/arm: Make make_cpus_node() compile at -Og

[Andrew Cooper]

When compiling at -Og:

  domain_build.c: In function 'make_cpus_node':
  domain_build.c:926:12: error: 'clock_valid' may be used uninitialized in this function [-Werror=maybe-uninitialized]
    926 |         if ( clock_valid )
        |            ^

The compiler hasn't spotted that clock_valid is always initialised after the
"if ( !compatible )" check.  Initialise clock_valid to false.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
---
 xen/arch/arm/domain_build.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
index b1d7b9849f..b10f5c8f85 100644
--- a/xen/arch/arm/domain_build.c
+++ b/xen/arch/arm/domain_build.c
@@ -831,7 +831,7 @@ static int __init make_cpus_node(const struct domain *d, void *fdt)
     /* Placeholder for cpu@ + a 32-bit hexadecimal number + \0 */
     char buf[13];
     u32 clock_frequency;
-    bool clock_valid;
+    bool clock_valid = false;
     uint64_t mpidr_aff;
 
     dt_dprintk("Create cpus node\n");
-- 
2.11.0

Re: [PATCH 1/7] xen/arm: Make make_cpus_node() compile at -Og

[Julien Grall]

Hi Andrew,

On 19/04/2021 15:01, Andrew Cooper wrote:
> When compiling at -Og:
> 
>    domain_build.c: In function 'make_cpus_node':
>    domain_build.c:926:12: error: 'clock_valid' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>      926 |         if ( clock_valid )
>          |            ^
> 
> The compiler hasn't spotted that clock_valid is always initialised after the
> "if ( !compatible )" check.  Initialise clock_valid to false.

Can you confirm which version this is affecting? We bumped the minimum 
version of GCC recently, so I want to make sure we don't add code to 
silence older compilers.

> Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
> ---
> CC: Stefano Stabellini <sstabellini@kernel.org>
> CC: Julien Grall <julien@xen.org>
> CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
> ---
>   xen/arch/arm/domain_build.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/arm/domain_build.c b/xen/arch/arm/domain_build.c
> index b1d7b9849f..b10f5c8f85 100644
> --- a/xen/arch/arm/domain_build.c
> +++ b/xen/arch/arm/domain_build.c
> @@ -831,7 +831,7 @@ static int __init make_cpus_node(const struct domain *d, void *fdt)
>       /* Placeholder for cpu@ + a 32-bit hexadecimal number + \0 */
>       char buf[13];
>       u32 clock_frequency;
> -    bool clock_valid;
> +    bool clock_valid = false;

Would you mind to add something like:

/* Initialized silence at least GCC X.Y. */

With X.Y replaced with the version.

Cheers,

-- 
Julien Grall

[PATCH 2/7] x86/shim: Fix compilation at -Og

[Andrew Cooper]

When compiling at -Og:

  shim.c: In function ‘write_start_info’:
  shim.c:288:22: error: ‘param’ may be used uninitialized in this function[-Werror=maybe-uninitialized]
       si->store_evtchn = param;
       ~~~~~~~~~~~~~~~~~^~~~~~~

and a slew of knock-on failures.  All are caused by
xen_hypercall_hvm_get_param(), and presumably insufficient analysis to observe
that *value is always written on the ret=0 path.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/pv/shim.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/xen/arch/x86/pv/shim.c b/xen/arch/x86/pv/shim.c
index d16c0048c0..a05aaa7bcc 100644
--- a/xen/arch/x86/pv/shim.c
+++ b/xen/arch/x86/pv/shim.c
@@ -282,7 +282,7 @@ static void write_start_info(struct domain *d)
     struct cpu_user_regs *regs = guest_cpu_user_regs();
     start_info_t *si = map_domain_page(_mfn(is_pv_32bit_domain(d) ? regs->edx
                                                                   : regs->rdx));
-    uint64_t param;
+    uint64_t param = 0;
 
     snprintf(si->magic, sizeof(si->magic), "xen-3.0-x86_%s",
              is_pv_32bit_domain(d) ? "32p" : "64");
@@ -311,8 +311,8 @@ int pv_shim_shutdown(uint8_t reason)
     struct domain *d = current->domain;
     struct vcpu *v;
     unsigned int i;
-    uint64_t old_store_pfn, old_console_pfn = 0, store_pfn, console_pfn;
-    uint64_t store_evtchn, console_evtchn;
+    uint64_t old_store_pfn = 0, old_console_pfn = 0, store_pfn = 0, console_pfn = 0;
+    uint64_t store_evtchn = 0, console_evtchn = 0;
     long rc;
 
     if ( reason != SHUTDOWN_suspend )
-- 
2.11.0

[PATCH 3/7] x86/sysctl: Make arch_do_sysctl() compile at -Og

[Andrew Cooper]

When compiling at -Og:

  sysctl.c: In function ‘arch_do_sysctl’:
  sysctl.c:197:19: error: ‘hcpu’ may be used uninitialized in this function[-Werror=maybe-uninitialized]
               ret = continue_hypercall_on_cpu(0, fn, hcpu);
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

  sysctl.c: In function ‘arch_do_sysctl’:
  sysctl.c:197:19: error: ‘fn’ may be used uninitialized in this function[-Werror=maybe-uninitialized]
               ret = continue_hypercall_on_cpu(0, fn, hcpu);
                     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

These look to be caused by insufficient analysis around the !ret conditionals.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/sysctl.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/xen/arch/x86/sysctl.c b/xen/arch/x86/sysctl.c
index aff52a13f3..6ac09bac79 100644
--- a/xen/arch/x86/sysctl.c
+++ b/xen/arch/x86/sysctl.c
@@ -150,8 +150,8 @@ long arch_do_sysctl(
         unsigned int cpu = sysctl->u.cpu_hotplug.cpu;
         unsigned int op  = sysctl->u.cpu_hotplug.op;
         bool plug;
-        long (*fn)(void *);
-        void *hcpu;
+        long (*fn)(void *) = NULL;
+        void *hcpu = NULL;
 
         switch ( op )
         {
-- 
2.11.0

[PATCH 4/7] x86/irq: Make create_irq() compile at -Og

[Andrew Cooper]

When compiling at -Og:

  irq.c: In function ‘create_irq’:
  irq.c:310:38: error: ‘desc’ may be used uninitialized in this function[-Werror=maybe-uninitialized]
               desc->arch.creator_domid = currd->domain_id;
               ~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~

This diagnostic is bogus, because desc is already read on earlier paths.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/irq.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/irq.c b/xen/arch/x86/irq.c
index a1693f92dd..72b86c6155 100644
--- a/xen/arch/x86/irq.c
+++ b/xen/arch/x86/irq.c
@@ -264,7 +264,7 @@ void __init clear_irq_vector(int irq)
 int create_irq(nodeid_t node, bool grant_access)
 {
     int irq, ret;
-    struct irq_desc *desc;
+    struct irq_desc *desc = NULL;
 
     for (irq = nr_irqs_gsi; irq < nr_irqs; irq++)
     {
-- 
2.11.0

[PATCH 5/7] xen/efi: Make efi_start() compile at -Og

[Andrew Cooper]

When compiling at -Og:

  boot.c: In function 'efi_start':
  boot.c:1339:9: error: 'argc' may be used uninitialized in this function [-Werror=maybe-uninitialized]
   1339 |         efi_arch_handle_cmdline(argc ? *argv : NULL, options, name.s);
        |         ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

although this appears to be limited to the ARM build only.  It also seems to
be bogus, because it is immediately preceding by a read of argc which doesn't
yield a diagnostic.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
---
 xen/common/efi/boot.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/common/efi/boot.c b/xen/common/efi/boot.c
index 63e289ab85..f1e7a5267c 100644
--- a/xen/common/efi/boot.c
+++ b/xen/common/efi/boot.c
@@ -1126,7 +1126,7 @@ efi_start(EFI_HANDLE ImageHandle, EFI_SYSTEM_TABLE *SystemTable)
     static EFI_GUID __initdata shim_lock_guid = SHIM_LOCK_PROTOCOL_GUID;
     EFI_LOADED_IMAGE *loaded_image;
     EFI_STATUS status;
-    unsigned int i, argc;
+    unsigned int i, argc = 0;
     CHAR16 **argv, *file_name, *cfg_file_name = NULL, *options = NULL;
     UINTN gop_mode = ~0;
     EFI_SHIM_LOCK_PROTOCOL *shim_lock;
-- 
2.11.0

[PATCH 6/7] x86/shadow: Make _shadow_prealloc() compile at -Og

[Andrew Cooper]

When compiling at -Og:

  In file included from
  /builds/xen-project/people/andyhhp/xen/xen/include/asm/domain.h:4:0,
                   from /builds/xen-project/people/andyhhp/xen/xen/include/xen/domain.h:8,
                   from /builds/xen-project/people/andyhhp/xen/xen/include/xen/sched.h:11,
                   from /builds/xen-project/people/andyhhp/xen/xen/include/xen/ioreq.h:22,
                   from common.c:23:
  common.c: In function '_shadow_prealloc':
  /builds/xen-project/people/andyhhp/xen/xen/include/xen/mm.h:252:55: error: 't' may be used uninitialized in this function [-Werror=maybe-uninitialized]
       return page != head->next ? pdx_to_page(page->list.prev) : NULL;
                                                         ^
  common.c:933:28: note: 't' was declared here
       struct page_info *sp, *t;
                              ^

I'm not certain the analysis is correct.  't' is a temporary variable, and is
clearly initialised before use in foreach_pinned_shadow().  Either way,
initialising it to NULL placates the compiler.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Tim Deegan <tim@xen.org>
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
---
 xen/arch/x86/mm/shadow/common.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/mm/shadow/common.c b/xen/arch/x86/mm/shadow/common.c
index b99ca14e71..737e6b365a 100644
--- a/xen/arch/x86/mm/shadow/common.c
+++ b/xen/arch/x86/mm/shadow/common.c
@@ -931,7 +931,7 @@ static inline void trace_shadow_prealloc_unpin(struct domain *d, mfn_t smfn)
 static void _shadow_prealloc(struct domain *d, unsigned int pages)
 {
     struct vcpu *v;
-    struct page_info *sp, *t;
+    struct page_info *sp, *t = NULL;
     mfn_t smfn;
     int i;
 
-- 
2.11.0

Re: [PATCH 6/7] x86/shadow: Make _shadow_prealloc() compile at -Og

[Tim Deegan]

At 15:01 +0100 on 19 Apr (1618844491), Andrew Cooper wrote:
> When compiling at -Og:
> 
>   In file included from
>   /builds/xen-project/people/andyhhp/xen/xen/include/asm/domain.h:4:0,
>                    from /builds/xen-project/people/andyhhp/xen/xen/include/xen/domain.h:8,
>                    from /builds/xen-project/people/andyhhp/xen/xen/include/xen/sched.h:11,
>                    from /builds/xen-project/people/andyhhp/xen/xen/include/xen/ioreq.h:22,
>                    from common.c:23:
>   common.c: In function '_shadow_prealloc':
>   /builds/xen-project/people/andyhhp/xen/xen/include/xen/mm.h:252:55: error: 't' may be used uninitialized in this function [-Werror=maybe-uninitialized]
>        return page != head->next ? pdx_to_page(page->list.prev) : NULL;
>                                                          ^
>   common.c:933:28: note: 't' was declared here
>        struct page_info *sp, *t;
>                               ^
> 
> I'm not certain the analysis is correct.  't' is a temporary variable, and is
> clearly initialised before use in foreach_pinned_shadow().  Either way,
> initialising it to NULL placates the compiler.

Yeah, this analysis seems wrong to me too - if nothing else, why does
it not complain about the identical code in shadow_blow_tables() below?

That said, since the non-debug build doesn't complain here, presumably it
will be able to elide this dead store.

Acked-by: Tim Deegan <tim@xen.org>

[PATCH 7/7] xen: Use -Og for debug builds when available

[Andrew Cooper]

The recommended optimisation level for debugging is -Og, and is what tools
such as gdb prefer.  In practice, it equates to -01 with a few specific
optimisations turned off.

While the use of gdb isn't necessarily very helpful for Xen, the disassembly
will have fewer structural transformations vs C, and therefore will be easier
to follow.

Signed-off-by: Andrew Cooper <andrew.cooper3@citrix.com>
---
CC: Jan Beulich <JBeulich@suse.com>
CC: Roger Pau Monné <roger.pau@citrix.com>
CC: Wei Liu <wl@xen.org>
CC: Stefano Stabellini <sstabellini@kernel.org>
CC: Julien Grall <julien@xen.org>
CC: Volodymyr Babchuk <Volodymyr_Babchuk@epam.com>
CC: Juergen Gross <jgross@suse.com>

Successful CI runs:
  https://gitlab.com/xen-project/people/andyhhp/xen/-/pipelines/287769787
  https://cirrus-ci.com/build/5086280275984384
---
 xen/Makefile | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/xen/Makefile b/xen/Makefile
index 9f3be7766d..128de93f5e 100644
--- a/xen/Makefile
+++ b/xen/Makefile
@@ -195,7 +195,9 @@ include/config/%.conf include/config/%.conf.cmd: $(KCONFIG_CONFIG)
 	$(MAKE) $(kconfig) syncconfig
 
 ifeq ($(CONFIG_DEBUG),y)
-CFLAGS += -O1
+# Use -Og if available, -O1 otherwise
+dbg_opt_level := $(call cc-option,$(CC),-Og,-O1)
+CFLAGS += $(dbg_opt_level)
 else
 CFLAGS += -O2
 endif
-- 
2.11.0

Re: [PATCH 0/7] xen: Switch to using -Og for debug builds

[Jan Beulich]

On 19.04.2021 16:01, Andrew Cooper wrote:
> As with the toolstack side, we ought to use -Og for debug builds.
> 
> All fixes are trivial.  The first 3 are understandable, given reduced
> optimisations.  The next 3 are, AFAICT, bogus diagnostics.
> 
> Andrew Cooper (7):
>   xen/arm: Make make_cpus_node() compile at -Og
>   x86/shim: Fix compilation at -Og
>   x86/sysctl: Make arch_do_sysctl() compile at -Og
>   x86/irq: Make create_irq() compile at -Og
>   xen/efi: Make efi_start() compile at -Og
>   x86/shadow: Make _shadow_prealloc() compile at -Og
>   xen: Use -Og for debug builds when available

Acked-by: Jan Beulich <jbeulich@suse.com>

I'd like to ask though that at least for the bogus warnings you
amend the commit messages with the gcc version these were observed
with. Perhaps even those seemingly bogus initializers would
benefit from a very brief comment (or else there's a fair chance
of them getting removed again at some point).

Jan

Re: [PATCH 0/7] xen: Switch to using -Og for debug builds

[Andrew Cooper]

On 19/04/2021 16:45, Jan Beulich wrote:
> On 19.04.2021 16:01, Andrew Cooper wrote:
>> As with the toolstack side, we ought to use -Og for debug builds.
>>
>> All fixes are trivial.  The first 3 are understandable, given reduced
>> optimisations.  The next 3 are, AFAICT, bogus diagnostics.
>>
>> Andrew Cooper (7):
>>   xen/arm: Make make_cpus_node() compile at -Og
>>   x86/shim: Fix compilation at -Og
>>   x86/sysctl: Make arch_do_sysctl() compile at -Og
>>   x86/irq: Make create_irq() compile at -Og
>>   xen/efi: Make efi_start() compile at -Og
>>   x86/shadow: Make _shadow_prealloc() compile at -Og
>>   xen: Use -Og for debug builds when available
> Acked-by: Jan Beulich <jbeulich@suse.com>
>
> I'd like to ask though that at least for the bogus warnings you
> amend the commit messages with the gcc version these were observed
> with. Perhaps even those seemingly bogus initializers would
> benefit from a very brief comment (or else there's a fair chance
> of them getting removed again at some point).

I'm afraid I don't have that information easily to hand, but all issues
were found by distro-provided compilers included in our Gitlab
infrastructure.

~Andrew

Re: [PATCH 0/7] xen: Switch to using -Og for debug builds

[Jan Beulich]

On 21.04.2021 11:31, Andrew Cooper wrote:
> On 19/04/2021 16:45, Jan Beulich wrote:
>> On 19.04.2021 16:01, Andrew Cooper wrote:
>>> As with the toolstack side, we ought to use -Og for debug builds.
>>>
>>> All fixes are trivial.  The first 3 are understandable, given reduced
>>> optimisations.  The next 3 are, AFAICT, bogus diagnostics.
>>>
>>> Andrew Cooper (7):
>>>   xen/arm: Make make_cpus_node() compile at -Og
>>>   x86/shim: Fix compilation at -Og
>>>   x86/sysctl: Make arch_do_sysctl() compile at -Og
>>>   x86/irq: Make create_irq() compile at -Og
>>>   xen/efi: Make efi_start() compile at -Og
>>>   x86/shadow: Make _shadow_prealloc() compile at -Og
>>>   xen: Use -Og for debug builds when available
>> Acked-by: Jan Beulich <jbeulich@suse.com>
>>
>> I'd like to ask though that at least for the bogus warnings you
>> amend the commit messages with the gcc version these were observed
>> with. Perhaps even those seemingly bogus initializers would
>> benefit from a very brief comment (or else there's a fair chance
>> of them getting removed again at some point).
> 
> I'm afraid I don't have that information easily to hand, but all issues
> were found by distro-provided compilers included in our Gitlab
> infrastructure.

Well, okay, then we'll need to live with said risk. As an aside I'd
like to remind you though that on past occasions of working around
compiler oddities, I was asked as well to provide compiler version
information ...

Jan