Re: [PATCH] sparc: mdesc: Fix compile error seen with gcc 11.x

[Guenter Roeck]

On 9/14/21 7:17 AM, David Laight wrote:
> > -----Original Message-----
> > From: Guenter Roeck <groeck7@xxxxxxxxx> On Behalf Of Guenter Roeck
> > Sent: 13 September 2021 19:53
> > To: Sam Ravnborg <sam@xxxxxxxxxxxx>
> > Cc: David S . Miller <davem@xxxxxxxxxxxxx>; sparclinux@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx;
> > Arnd Bergmann <arnd@xxxxxxxxxx>
> > Subject: Re: [PATCH] sparc: mdesc: Fix compile error seen with gcc 11.x
> >
> > On 9/13/21 11:02 AM, Sam Ravnborg wrote:
> > > Hi Guenter,
> > >
> > > On Mon, Sep 13, 2021 at 09:37:12AM -0700, Guenter Roeck wrote:
> > > > sparc64 images fail to compile with gcc 11.x, reporting the following
> > > > errors.
> > > >
> > > > arch/sparc/kernel/mdesc.c:647:22: error:
> > > > 	'strcmp' reading 1 or more bytes from a region of size 0
> > > > arch/sparc/kernel/mdesc.c:692:22: error:
> > > > 	'strcmp' reading 1 or more bytes from a region of size 0
> > > > arch/sparc/kernel/mdesc.c:719:21:
> > > > 	error: 'strcmp' reading 1 or more bytes from a region of size 0
> > > >
> > > > The underlying problem is that node_block() returns a pointer beyond
> > > > the end of struct mdesc_hdr. gcc 11.x detects that and reports the error.
> > > > Adding an additional zero-length field to struct mdesc_hdr and pointing
> > > > to that field fixes the problem.
> > > >
> > > > Cc: Arnd Bergmann <arnd@xxxxxxxxxx>
> > > > Signed-off-by: Guenter Roeck <linux@xxxxxxxxxxxx>
> > > > ---
> > > > My apologies if a similar patch was submitted already; I was unable to find it.
> > > > I did find the following patch:
> > > >       https://git.busybox.net/buildroot/commit/?id=6e1106b4a9aee25d1556310d5cd1cb6dde2e6e3f
> > > > but I failed to find it in patchwork or on lore.kernel.org, and it
> > > > seems to be more expensive than the solution suggested here.
> > > >
> > > >    arch/sparc/kernel/mdesc.c | 3 ++-
> > > >    1 file changed, 2 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/arch/sparc/kernel/mdesc.c b/arch/sparc/kernel/mdesc.c
> > > > index 8e645ddac58e..c67bdcc23727 100644
> > > > --- a/arch/sparc/kernel/mdesc.c
> > > > +++ b/arch/sparc/kernel/mdesc.c
> > > > @@ -39,6 +39,7 @@ struct mdesc_hdr {
> > > >    	u32	node_sz; /* node block size */
> > > >    	u32	name_sz; /* name block size */
> > > >    	u32	data_sz; /* data block size */
> > > > +	char	data[0];
> > > >    } __attribute__((aligned(16)));
> > >
> > > I do not think this will works.
> > > See following comment:
> > >    * mdesc_hdr and mdesc_elem describe the layout of the data structure
> > >    * we get from the Hypervisor.
> > >
> > > With the above change you increased the size from 16 to 32 bytes,
> > > and any code using sizeof(struct mdesc_hdr) will now point too far in
> > > memory for the second and subsequent entries.
> > >
> > > I did not take any closer look, but this was from a quick analysis.
> >
> > Sorry, I didn't realize that a field of size 0 increases the structure size
> > on sparc. I had checked the size of the old and the new structure with gcc
> > on x86_64 and didn't see a field size increase.
>
> clang output doesn't change:
>
> https://godbolt.org/z/bTeeq19j1
>
> gcc ought to generate the same size.
>
> It ought to be 'char data[];' though.

I am never sure if [] or [0] is "correct". Anyway, is there agreement that this
is an acceptable solution ? I'll be happy to resend if that is the case.

Guenter