[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

Re: [PATCH 3/7] x86/dom0: placate GCC 12 compile-time errors with UBSAN and PVH_GUEST



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



 


Rackspace

Lists.xenproject.org is hosted with RackSpace, monitoring our
servers 24x7x365 and backed by RackSpace's Fanatical Support®.