When building Xen with GCC 12 with UBSAN and PVH_GUEST both enabled the
compiler emits the following errors:

arch/x86/setup.c: In function '__start_xen':
arch/x86/setup.c:1504:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
 1504 |             end = consider_modules(s, e, reloc_size + mask,
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1505 |                                    bi->mods, bi->nr_modules, -1);
      |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/setup.c:1504:19: note: referencing argument 4 of type 'const struct boot_module[0]'
arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
  686 | static uint64_t __init consider_modules(
      |                        ^~~~~~~~~~~~~~~~
arch/x86/setup.c:1535:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
 1535 |             end = consider_modules(s, e, size, bi->mods,
      |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 1536 |                                    bi->nr_modules + relocated, j);
      |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
arch/x86/setup.c:1535:19: note: referencing argument 4 of type 'const struct boot_module[0]'
arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
  686 | static uint64_t __init consider_modules(
      |                        ^~~~~~~~~~~~~~~~

This seems to be the result of some function manipulation done by UBSAN
triggering GCC stringops related errors.  Placate the errors by declaring
the function parameter as `const struct *boot_module` instead of `const
struct boot_module[]`.

Note that GCC 13 seems to be fixed, and doesn't trigger the error when
using `[]`.

Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
---
 xen/arch/x86/setup.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
index 4a32d8491186..bde5d75ea6ab 100644
--- a/xen/arch/x86/setup.c
+++ b/xen/arch/x86/setup.c
@@ -684,7 +684,7 @@ static void __init noinline move_xen(void)
 #undef BOOTSTRAP_MAP_LIMIT
 
 static uint64_t __init consider_modules(
-    uint64_t s, uint64_t e, uint32_t size, const struct boot_module mods[],
+    uint64_t s, uint64_t e, uint32_t size, const struct boot_module *mods,
     unsigned int nr_mods, unsigned int this_mod)
 {
     unsigned int i;
-- 
2.48.1

On 13.03.2025 16:30, Roger Pau Monne wrote:
> When building Xen with GCC 12 with UBSAN and PVH_GUEST both enabled the
> compiler emits the following errors:
> 
> arch/x86/setup.c: In function '__start_xen':
> arch/x86/setup.c:1504:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
>  1504 |             end = consider_modules(s, e, reloc_size + mask,
>       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  1505 |                                    bi->mods, bi->nr_modules, -1);
>       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/setup.c:1504:19: note: referencing argument 4 of type 'const struct boot_module[0]'
> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
>   686 | static uint64_t __init consider_modules(
>       |                        ^~~~~~~~~~~~~~~~
> arch/x86/setup.c:1535:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
>  1535 |             end = consider_modules(s, e, size, bi->mods,
>       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  1536 |                                    bi->nr_modules + relocated, j);
>       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/setup.c:1535:19: note: referencing argument 4 of type 'const struct boot_module[0]'
> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
>   686 | static uint64_t __init consider_modules(
>       |                        ^~~~~~~~~~~~~~~~
> 
> This seems to be the result of some function manipulation done by UBSAN
> triggering GCC stringops related errors.  Placate the errors by declaring
> the function parameter as `const struct *boot_module` instead of `const
> struct boot_module[]`.
> 
> Note that GCC 13 seems to be fixed, and doesn't trigger the error when
> using `[]`.
> 
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> ---
>  xen/arch/x86/setup.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> index 4a32d8491186..bde5d75ea6ab 100644
> --- a/xen/arch/x86/setup.c
> +++ b/xen/arch/x86/setup.c
> @@ -684,7 +684,7 @@ static void __init noinline move_xen(void)
>  #undef BOOTSTRAP_MAP_LIMIT
>  
>  static uint64_t __init consider_modules(
> -    uint64_t s, uint64_t e, uint32_t size, const struct boot_module mods[],
> +    uint64_t s, uint64_t e, uint32_t size, const struct boot_module *mods,
>      unsigned int nr_mods, unsigned int this_mod)
>  {
>      unsigned int i;

While I'm okay-ish with the change, how are we going to make sure it won't be
re-introduced? Or something similar be introduced elsewhere?

Jan

On Fri, Mar 14, 2025 at 09:10:59AM +0100, Jan Beulich wrote:
> On 13.03.2025 16:30, Roger Pau Monne wrote:
> > When building Xen with GCC 12 with UBSAN and PVH_GUEST both enabled the
> > compiler emits the following errors:
> > 
> > arch/x86/setup.c: In function '__start_xen':
> > arch/x86/setup.c:1504:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
> >  1504 |             end = consider_modules(s, e, reloc_size + mask,
> >       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >  1505 |                                    bi->mods, bi->nr_modules, -1);
> >       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > arch/x86/setup.c:1504:19: note: referencing argument 4 of type 'const struct boot_module[0]'
> > arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
> >   686 | static uint64_t __init consider_modules(
> >       |                        ^~~~~~~~~~~~~~~~
> > arch/x86/setup.c:1535:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
> >  1535 |             end = consider_modules(s, e, size, bi->mods,
> >       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >  1536 |                                    bi->nr_modules + relocated, j);
> >       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> > arch/x86/setup.c:1535:19: note: referencing argument 4 of type 'const struct boot_module[0]'
> > arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
> >   686 | static uint64_t __init consider_modules(
> >       |                        ^~~~~~~~~~~~~~~~
> > 
> > This seems to be the result of some function manipulation done by UBSAN
> > triggering GCC stringops related errors.  Placate the errors by declaring
> > the function parameter as `const struct *boot_module` instead of `const
> > struct boot_module[]`.
> > 
> > Note that GCC 13 seems to be fixed, and doesn't trigger the error when
> > using `[]`.
> > 
> > Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> > ---
> >  xen/arch/x86/setup.c | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
> > 
> > diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> > index 4a32d8491186..bde5d75ea6ab 100644
> > --- a/xen/arch/x86/setup.c
> > +++ b/xen/arch/x86/setup.c
> > @@ -684,7 +684,7 @@ static void __init noinline move_xen(void)
> >  #undef BOOTSTRAP_MAP_LIMIT
> >  
> >  static uint64_t __init consider_modules(
> > -    uint64_t s, uint64_t e, uint32_t size, const struct boot_module mods[],
> > +    uint64_t s, uint64_t e, uint32_t size, const struct boot_module *mods,
> >      unsigned int nr_mods, unsigned int this_mod)
> >  {
> >      unsigned int i;
> 
> While I'm okay-ish with the change, how are we going to make sure it won't be
> re-introduced? Or something similar be introduced elsewhere?

I'm afraid I don't have a good response, as I don't even know exactly
why the error triggers.  We will rely on the CI to start doing
randconfig builds with UBSAN enabled (see patch 7/7).

Thanks, Roger.

On 14.03.2025 09:27, Roger Pau Monné wrote:
> On Fri, Mar 14, 2025 at 09:10:59AM +0100, Jan Beulich wrote:
>> On 13.03.2025 16:30, Roger Pau Monne wrote:
>>> When building Xen with GCC 12 with UBSAN and PVH_GUEST both enabled the
>>> compiler emits the following errors:
>>>
>>> arch/x86/setup.c: In function '__start_xen':
>>> arch/x86/setup.c:1504:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
>>>  1504 |             end = consider_modules(s, e, reloc_size + mask,
>>>       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>  1505 |                                    bi->mods, bi->nr_modules, -1);
>>>       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> arch/x86/setup.c:1504:19: note: referencing argument 4 of type 'const struct boot_module[0]'
>>> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
>>>   686 | static uint64_t __init consider_modules(
>>>       |                        ^~~~~~~~~~~~~~~~
>>> arch/x86/setup.c:1535:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
>>>  1535 |             end = consider_modules(s, e, size, bi->mods,
>>>       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>>  1536 |                                    bi->nr_modules + relocated, j);
>>>       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>>> arch/x86/setup.c:1535:19: note: referencing argument 4 of type 'const struct boot_module[0]'
>>> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
>>>   686 | static uint64_t __init consider_modules(
>>>       |                        ^~~~~~~~~~~~~~~~
>>>
>>> This seems to be the result of some function manipulation done by UBSAN
>>> triggering GCC stringops related errors.  Placate the errors by declaring
>>> the function parameter as `const struct *boot_module` instead of `const
>>> struct boot_module[]`.
>>>
>>> Note that GCC 13 seems to be fixed, and doesn't trigger the error when
>>> using `[]`.
>>>
>>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
>>> ---
>>>  xen/arch/x86/setup.c | 2 +-
>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
>>> index 4a32d8491186..bde5d75ea6ab 100644
>>> --- a/xen/arch/x86/setup.c
>>> +++ b/xen/arch/x86/setup.c
>>> @@ -684,7 +684,7 @@ static void __init noinline move_xen(void)
>>>  #undef BOOTSTRAP_MAP_LIMIT
>>>  
>>>  static uint64_t __init consider_modules(
>>> -    uint64_t s, uint64_t e, uint32_t size, const struct boot_module mods[],
>>> +    uint64_t s, uint64_t e, uint32_t size, const struct boot_module *mods,
>>>      unsigned int nr_mods, unsigned int this_mod)
>>>  {
>>>      unsigned int i;
>>
>> While I'm okay-ish with the change, how are we going to make sure it won't be
>> re-introduced? Or something similar be introduced elsewhere?
> 
> I'm afraid I don't have a good response, as I don't even know exactly
> why the error triggers.

One option might be to amend ./CODING_STYLE for dis-encourage [] notation
in function parameters. I wouldn't be happy about us doing so, as I think
that serves a documentation purpose, but compiler deficiencies getting in
the way is certainly higher priority here.

Trying to abstract this (vaguely along the lines of gcc11_wrap()), otoh,
wouldn't be desirable imo, as it would still lose the doc effect, at least
to some degree.

>  We will rely on the CI to start doing
> randconfig builds with UBSAN enabled (see patch 7/7).

Right. Just that randconfig is, well, random in what it covers.

Jan

On Fri, Mar 14, 2025 at 09:33:01AM +0100, Jan Beulich wrote:
> On 14.03.2025 09:27, Roger Pau Monné wrote:
> > On Fri, Mar 14, 2025 at 09:10:59AM +0100, Jan Beulich wrote:
> >> On 13.03.2025 16:30, Roger Pau Monne wrote:
> >>> When building Xen with GCC 12 with UBSAN and PVH_GUEST both enabled the
> >>> compiler emits the following errors:
> >>>
> >>> arch/x86/setup.c: In function '__start_xen':
> >>> arch/x86/setup.c:1504:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
> >>>  1504 |             end = consider_modules(s, e, reloc_size + mask,
> >>>       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>  1505 |                                    bi->mods, bi->nr_modules, -1);
> >>>       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>> arch/x86/setup.c:1504:19: note: referencing argument 4 of type 'const struct boot_module[0]'
> >>> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
> >>>   686 | static uint64_t __init consider_modules(
> >>>       |                        ^~~~~~~~~~~~~~~~
> >>> arch/x86/setup.c:1535:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
> >>>  1535 |             end = consider_modules(s, e, size, bi->mods,
> >>>       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>>  1536 |                                    bi->nr_modules + relocated, j);
> >>>       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> >>> arch/x86/setup.c:1535:19: note: referencing argument 4 of type 'const struct boot_module[0]'
> >>> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
> >>>   686 | static uint64_t __init consider_modules(
> >>>       |                        ^~~~~~~~~~~~~~~~
> >>>
> >>> This seems to be the result of some function manipulation done by UBSAN
> >>> triggering GCC stringops related errors.  Placate the errors by declaring
> >>> the function parameter as `const struct *boot_module` instead of `const
> >>> struct boot_module[]`.
> >>>
> >>> Note that GCC 13 seems to be fixed, and doesn't trigger the error when
> >>> using `[]`.
> >>>
> >>> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>
> >>> ---
> >>>  xen/arch/x86/setup.c | 2 +-
> >>>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>>
> >>> diff --git a/xen/arch/x86/setup.c b/xen/arch/x86/setup.c
> >>> index 4a32d8491186..bde5d75ea6ab 100644
> >>> --- a/xen/arch/x86/setup.c
> >>> +++ b/xen/arch/x86/setup.c
> >>> @@ -684,7 +684,7 @@ static void __init noinline move_xen(void)
> >>>  #undef BOOTSTRAP_MAP_LIMIT
> >>>  
> >>>  static uint64_t __init consider_modules(
> >>> -    uint64_t s, uint64_t e, uint32_t size, const struct boot_module mods[],
> >>> +    uint64_t s, uint64_t e, uint32_t size, const struct boot_module *mods,
> >>>      unsigned int nr_mods, unsigned int this_mod)
> >>>  {
> >>>      unsigned int i;
> >>
> >> While I'm okay-ish with the change, how are we going to make sure it won't be
> >> re-introduced? Or something similar be introduced elsewhere?
> > 
> > I'm afraid I don't have a good response, as I don't even know exactly
> > why the error triggers.
> 
> One option might be to amend ./CODING_STYLE for dis-encourage [] notation
> in function parameters. I wouldn't be happy about us doing so, as I think
> that serves a documentation purpose, but compiler deficiencies getting in
> the way is certainly higher priority here.
> 
> Trying to abstract this (vaguely along the lines of gcc11_wrap()), otoh,
> wouldn't be desirable imo, as it would still lose the doc effect, at least
> to some degree.

This is a very specific case, I don't think we should change our
coding style based on it.  I think our only option is to deal with
such compiler bugs when we detect them.

Thanks, Roger.

On 13/03/2025 3:30 pm, Roger Pau Monne wrote:
> When building Xen with GCC 12 with UBSAN and PVH_GUEST both enabled the
> compiler emits the following errors:
>
> arch/x86/setup.c: In function '__start_xen':
> arch/x86/setup.c:1504:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
>  1504 |             end = consider_modules(s, e, reloc_size + mask,
>       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  1505 |                                    bi->mods, bi->nr_modules, -1);
>       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/setup.c:1504:19: note: referencing argument 4 of type 'const struct boot_module[0]'
> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
>   686 | static uint64_t __init consider_modules(
>       |                        ^~~~~~~~~~~~~~~~
> arch/x86/setup.c:1535:19: error: 'consider_modules' reading 40 bytes from a region of size 4 [-Werror=stringop-overread]
>  1535 |             end = consider_modules(s, e, size, bi->mods,
>       |                   ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>  1536 |                                    bi->nr_modules + relocated, j);
>       |                                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> arch/x86/setup.c:1535:19: note: referencing argument 4 of type 'const struct boot_module[0]'
> arch/x86/setup.c:686:24: note: in a call to function 'consider_modules'
>   686 | static uint64_t __init consider_modules(
>       |                        ^~~~~~~~~~~~~~~~
>
> This seems to be the result of some function manipulation done by UBSAN
> triggering GCC stringops related errors.  Placate the errors by declaring
> the function parameter as `const struct *boot_module` instead of `const
> struct boot_module[]`.
>
> Note that GCC 13 seems to be fixed, and doesn't trigger the error when
> using `[]`.
>
> Signed-off-by: Roger Pau Monné <roger.pau@citrix.com>

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

(I swear I've seen this before, and already fixed it once by switching
to a pointer...)