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....@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.
