xen-devel.lists.xenproject.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 0/7] xen: Switch to using -Og for debug builds
@ 2021-04-19 14:01 Andrew Cooper
  2021-04-19 14:01 ` [PATCH 1/7] xen/arm: Make make_cpus_node() compile at -Og Andrew Cooper
                   ` (7 more replies)
  0 siblings, 8 replies; 13+ messages in thread
From: Andrew Cooper @ 2021-04-19 14:01 UTC (permalink / raw)
  To: Xen-devel
  Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné,
	Wei Liu, Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	Juergen Gross, Tim Deegan

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



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

* [PATCH 1/7] xen/arm: Make make_cpus_node() compile at -Og
  2021-04-19 14:01 [PATCH 0/7] xen: Switch to using -Og for debug builds Andrew Cooper
@ 2021-04-19 14:01 ` Andrew Cooper
  2021-04-22 18:20   ` Julien Grall
  2021-04-19 14:01 ` [PATCH 2/7] x86/shim: Fix compilation " Andrew Cooper
                   ` (6 subsequent siblings)
  7 siblings, 1 reply; 13+ messages in thread
From: Andrew Cooper @ 2021-04-19 14:01 UTC (permalink / raw)
  To: Xen-devel
  Cc: Andrew Cooper, Stefano Stabellini, Julien Grall, Volodymyr Babchuk

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



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

* [PATCH 2/7] x86/shim: Fix compilation at -Og
  2021-04-19 14:01 [PATCH 0/7] xen: Switch to using -Og for debug builds Andrew Cooper
  2021-04-19 14:01 ` [PATCH 1/7] xen/arm: Make make_cpus_node() compile at -Og Andrew Cooper
@ 2021-04-19 14:01 ` Andrew Cooper
  2021-04-19 14:01 ` [PATCH 3/7] x86/sysctl: Make arch_do_sysctl() compile " Andrew Cooper
                   ` (5 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Andrew Cooper @ 2021-04-19 14:01 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Wei Liu

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



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

* [PATCH 3/7] x86/sysctl: Make arch_do_sysctl() compile at -Og
  2021-04-19 14:01 [PATCH 0/7] xen: Switch to using -Og for debug builds Andrew Cooper
  2021-04-19 14:01 ` [PATCH 1/7] xen/arm: Make make_cpus_node() compile at -Og Andrew Cooper
  2021-04-19 14:01 ` [PATCH 2/7] x86/shim: Fix compilation " Andrew Cooper
@ 2021-04-19 14:01 ` Andrew Cooper
  2021-04-19 14:01 ` [PATCH 4/7] x86/irq: Make create_irq() " Andrew Cooper
                   ` (4 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Andrew Cooper @ 2021-04-19 14:01 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Wei Liu

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



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

* [PATCH 4/7] x86/irq: Make create_irq() compile at -Og
  2021-04-19 14:01 [PATCH 0/7] xen: Switch to using -Og for debug builds Andrew Cooper
                   ` (2 preceding siblings ...)
  2021-04-19 14:01 ` [PATCH 3/7] x86/sysctl: Make arch_do_sysctl() compile " Andrew Cooper
@ 2021-04-19 14:01 ` Andrew Cooper
  2021-04-19 14:01 ` [PATCH 5/7] xen/efi: Make efi_start() " Andrew Cooper
                   ` (3 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Andrew Cooper @ 2021-04-19 14:01 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné, Wei Liu

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



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

* [PATCH 5/7] xen/efi: Make efi_start() compile at -Og
  2021-04-19 14:01 [PATCH 0/7] xen: Switch to using -Og for debug builds Andrew Cooper
                   ` (3 preceding siblings ...)
  2021-04-19 14:01 ` [PATCH 4/7] x86/irq: Make create_irq() " Andrew Cooper
@ 2021-04-19 14:01 ` Andrew Cooper
  2021-04-19 14:01 ` [PATCH 6/7] x86/shadow: Make _shadow_prealloc() " Andrew Cooper
                   ` (2 subsequent siblings)
  7 siblings, 0 replies; 13+ messages in thread
From: Andrew Cooper @ 2021-04-19 14:01 UTC (permalink / raw)
  To: Xen-devel; +Cc: Andrew Cooper, Jan Beulich

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



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

* [PATCH 6/7] x86/shadow: Make _shadow_prealloc() compile at -Og
  2021-04-19 14:01 [PATCH 0/7] xen: Switch to using -Og for debug builds Andrew Cooper
                   ` (4 preceding siblings ...)
  2021-04-19 14:01 ` [PATCH 5/7] xen/efi: Make efi_start() " Andrew Cooper
@ 2021-04-19 14:01 ` Andrew Cooper
  2021-04-22  7:52   ` Tim Deegan
  2021-04-19 14:01 ` [PATCH 7/7] xen: Use -Og for debug builds when available Andrew Cooper
  2021-04-19 15:45 ` [PATCH 0/7] xen: Switch to using -Og for debug builds Jan Beulich
  7 siblings, 1 reply; 13+ messages in thread
From: Andrew Cooper @ 2021-04-19 14:01 UTC (permalink / raw)
  To: Xen-devel
  Cc: Andrew Cooper, Tim Deegan, Jan Beulich, Roger Pau Monné, Wei Liu

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



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

* [PATCH 7/7] xen: Use -Og for debug builds when available
  2021-04-19 14:01 [PATCH 0/7] xen: Switch to using -Og for debug builds Andrew Cooper
                   ` (5 preceding siblings ...)
  2021-04-19 14:01 ` [PATCH 6/7] x86/shadow: Make _shadow_prealloc() " Andrew Cooper
@ 2021-04-19 14:01 ` Andrew Cooper
  2021-04-19 15:45 ` [PATCH 0/7] xen: Switch to using -Og for debug builds Jan Beulich
  7 siblings, 0 replies; 13+ messages in thread
From: Andrew Cooper @ 2021-04-19 14:01 UTC (permalink / raw)
  To: Xen-devel
  Cc: Andrew Cooper, Jan Beulich, Roger Pau Monné,
	Wei Liu, Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	Juergen Gross

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



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

* Re: [PATCH 0/7] xen: Switch to using -Og for debug builds
  2021-04-19 14:01 [PATCH 0/7] xen: Switch to using -Og for debug builds Andrew Cooper
                   ` (6 preceding siblings ...)
  2021-04-19 14:01 ` [PATCH 7/7] xen: Use -Og for debug builds when available Andrew Cooper
@ 2021-04-19 15:45 ` Jan Beulich
  2021-04-21  9:31   ` Andrew Cooper
  7 siblings, 1 reply; 13+ messages in thread
From: Jan Beulich @ 2021-04-19 15:45 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Roger Pau Monné,
	Wei Liu, Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	Juergen Gross, Tim Deegan, Xen-devel

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


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

* Re: [PATCH 0/7] xen: Switch to using -Og for debug builds
  2021-04-19 15:45 ` [PATCH 0/7] xen: Switch to using -Og for debug builds Jan Beulich
@ 2021-04-21  9:31   ` Andrew Cooper
  2021-04-21  9:51     ` Jan Beulich
  0 siblings, 1 reply; 13+ messages in thread
From: Andrew Cooper @ 2021-04-21  9:31 UTC (permalink / raw)
  To: Jan Beulich
  Cc: Roger Pau Monné,
	Wei Liu, Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	Juergen Gross, Tim Deegan, Xen-devel

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


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

* Re: [PATCH 0/7] xen: Switch to using -Og for debug builds
  2021-04-21  9:31   ` Andrew Cooper
@ 2021-04-21  9:51     ` Jan Beulich
  0 siblings, 0 replies; 13+ messages in thread
From: Jan Beulich @ 2021-04-21  9:51 UTC (permalink / raw)
  To: Andrew Cooper
  Cc: Roger Pau Monné,
	Wei Liu, Stefano Stabellini, Julien Grall, Volodymyr Babchuk,
	Juergen Gross, Tim Deegan, Xen-devel

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


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

* Re: [PATCH 6/7] x86/shadow: Make _shadow_prealloc() compile at -Og
  2021-04-19 14:01 ` [PATCH 6/7] x86/shadow: Make _shadow_prealloc() " Andrew Cooper
@ 2021-04-22  7:52   ` Tim Deegan
  0 siblings, 0 replies; 13+ messages in thread
From: Tim Deegan @ 2021-04-22  7:52 UTC (permalink / raw)
  To: Andrew Cooper; +Cc: Xen-devel, Jan Beulich, Roger Pau Monné, Wei Liu

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>


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

* Re: [PATCH 1/7] xen/arm: Make make_cpus_node() compile at -Og
  2021-04-19 14:01 ` [PATCH 1/7] xen/arm: Make make_cpus_node() compile at -Og Andrew Cooper
@ 2021-04-22 18:20   ` Julien Grall
  0 siblings, 0 replies; 13+ messages in thread
From: Julien Grall @ 2021-04-22 18:20 UTC (permalink / raw)
  To: Andrew Cooper, Xen-devel; +Cc: Stefano Stabellini, Volodymyr Babchuk

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


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

end of thread, other threads:[~2021-04-22 18:21 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-19 14:01 [PATCH 0/7] xen: Switch to using -Og for debug builds Andrew Cooper
2021-04-19 14:01 ` [PATCH 1/7] xen/arm: Make make_cpus_node() compile at -Og Andrew Cooper
2021-04-22 18:20   ` Julien Grall
2021-04-19 14:01 ` [PATCH 2/7] x86/shim: Fix compilation " Andrew Cooper
2021-04-19 14:01 ` [PATCH 3/7] x86/sysctl: Make arch_do_sysctl() compile " Andrew Cooper
2021-04-19 14:01 ` [PATCH 4/7] x86/irq: Make create_irq() " Andrew Cooper
2021-04-19 14:01 ` [PATCH 5/7] xen/efi: Make efi_start() " Andrew Cooper
2021-04-19 14:01 ` [PATCH 6/7] x86/shadow: Make _shadow_prealloc() " Andrew Cooper
2021-04-22  7:52   ` Tim Deegan
2021-04-19 14:01 ` [PATCH 7/7] xen: Use -Og for debug builds when available Andrew Cooper
2021-04-19 15:45 ` [PATCH 0/7] xen: Switch to using -Og for debug builds Jan Beulich
2021-04-21  9:31   ` Andrew Cooper
2021-04-21  9:51     ` 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).