5.19-rc1 x86 build failure

5.19-rc1 x86 build failure

[Joe Damato]

Greetings:

My apologies if this is the incorrect place to report this, but I got a
build error when trying to compile the net-next 5.19-rc1 tree.

git bisect says that commit a1e2c031ec394 ("x86/mm: Simplify
RESERVE_BRK()") is responsible for the build issue I am hitting.

I am performing this build on an x86_64 system with GNU C11 (Ubuntu
5.4.0-6ubuntu1~16.04.12) version 5.4.0 20160609 (x86_64-linux-gnu).

The assembler outputs a cryptic error message:

/tmp/ccnGOKZ5.s: Assembler messages:
/tmp/ccnGOKZ5.s:98: Error: missing ')'
/tmp/ccnGOKZ5.s:98: Error: missing ')'
/tmp/ccnGOKZ5.s:98: Error: missing ')'
/tmp/ccnGOKZ5.s:98: Error: junk at end of line, first unrecognized
character is `U'
/tmp/ccnGOKZ5.s:99: Error: missing ')'
/tmp/ccnGOKZ5.s:99: Error: missing ')'
/tmp/ccnGOKZ5.s:99: Error: missing ')'
/tmp/ccnGOKZ5.s:99: Error: junk at end of line, first unrecognized
character is `U'

I've asked GCC to generate the assembly and output so I can see more
specifically where this issue is (via "-fverbose-asm -Wa,-adhln=output"):

  96                            .pushsection .brk_reservation,"aw",@nobits
  97                            .brk.early_pgt_alloc:
  98 ???? 00000000              .skip ((2 * 3) * ((1UL) << 12))
****  Error: missing ')'
****  Error: missing ')'
****  Error: missing ')'
****  Error: junk at end of line, first unrecognized character is `U'
  98      0000
 100                            .popsection

This comes from arch/x86/mm/init.c, which has the following code:

RESERVE_BRK(early_pgt_alloc, INIT_PGT_BUF_SIZE);

wherein INIT_PGT_BUF_SIZE (via PAGE_SIZE) has a "1UL" which makes the
assembler unhappy.

I don't really know what the correct way to fix this is; it seems that the
macro _AC should handle this if ASSEMBLY is defined, IIUC, but that does
not seem to be the case at this point in init.c.

Perhaps I am doing something incorrect during the build process causing
this to happen?

Thanks,
Joe

Re: 5.19-rc1 x86 build failure

[Andrew Cooper]

On 07/06/2022 13:19, Joe Damato wrote:
> Greetings:
>
> My apologies if this is the incorrect place to report this, but I got a
> build error when trying to compile the net-next 5.19-rc1 tree.
>
> git bisect says that commit a1e2c031ec394 ("x86/mm: Simplify
> RESERVE_BRK()") is responsible for the build issue I am hitting.
>
> I am performing this build on an x86_64 system with GNU C11 (Ubuntu
> 5.4.0-6ubuntu1~16.04.12) version 5.4.0 20160609 (x86_64-linux-gnu).
>
> The assembler outputs a cryptic error message:
>
> /tmp/ccnGOKZ5.s: Assembler messages:
> /tmp/ccnGOKZ5.s:98: Error: missing ')'
> /tmp/ccnGOKZ5.s:98: Error: missing ')'
> /tmp/ccnGOKZ5.s:98: Error: missing ')'
> /tmp/ccnGOKZ5.s:98: Error: junk at end of line, first unrecognized
> character is `U'
> /tmp/ccnGOKZ5.s:99: Error: missing ')'
> /tmp/ccnGOKZ5.s:99: Error: missing ')'
> /tmp/ccnGOKZ5.s:99: Error: missing ')'
> /tmp/ccnGOKZ5.s:99: Error: junk at end of line, first unrecognized
> character is `U'
>
> I've asked GCC to generate the assembly and output so I can see more
> specifically where this issue is (via "-fverbose-asm -Wa,-adhln=output"):
>
>   96                            .pushsection .brk_reservation,"aw",@nobits
>   97                            .brk.early_pgt_alloc:
>   98 ???? 00000000              .skip ((2 * 3) * ((1UL) << 12))
> ****  Error: missing ')'
> ****  Error: missing ')'
> ****  Error: missing ')'
> ****  Error: junk at end of line, first unrecognized character is `U'
>   98      0000
>  100                            .popsection
>
> This comes from arch/x86/mm/init.c, which has the following code:
>
> RESERVE_BRK(early_pgt_alloc, INIT_PGT_BUF_SIZE);
>
> wherein INIT_PGT_BUF_SIZE (via PAGE_SIZE) has a "1UL" which makes the
> assembler unhappy.
>
> I don't really know what the correct way to fix this is; it seems that the
> macro _AC should handle this if ASSEMBLY is defined, IIUC, but that does
> not seem to be the case at this point in init.c.
>
> Perhaps I am doing something incorrect during the build process causing
> this to happen?

The problem is that _AC() is evaluated in C context (so gains the UL/ULL
suffix), and the C'd string is fed directly into the assembler (where
older binutils doesn't tolerate the suffix).

Short of having a _PAGE_SIZE which is an explicitly non-AC()'d constant,
I'm not sure what to suggest.  Ideally, you'd want to temporarily define
__ASSEMBLY__ around the expansion of __stringify(), but I don't think
that's possible as RESERVE_BRK() is a macro itself.

~Andrew

Re: 5.19-rc1 x86 build failure

[Josh Poimboeuf]

On Tue, Jun 07, 2022 at 12:42:33PM +0000, Andrew Cooper wrote:
> On 07/06/2022 13:19, Joe Damato wrote:
> >   96                            .pushsection .brk_reservation,"aw",@nobits
> >   97                            .brk.early_pgt_alloc:
> >   98 ???? 00000000              .skip ((2 * 3) * ((1UL) << 12))
> > ****  Error: missing ')'
> > ****  Error: missing ')'
> > ****  Error: missing ')'
> > ****  Error: junk at end of line, first unrecognized character is `U'
> >   98      0000
> >  100                            .popsection
> >
> > This comes from arch/x86/mm/init.c, which has the following code:
> >
> > RESERVE_BRK(early_pgt_alloc, INIT_PGT_BUF_SIZE);
> >
> > wherein INIT_PGT_BUF_SIZE (via PAGE_SIZE) has a "1UL" which makes the
> > assembler unhappy.
> >
> > I don't really know what the correct way to fix this is; it seems that the
> > macro _AC should handle this if ASSEMBLY is defined, IIUC, but that does
> > not seem to be the case at this point in init.c.
> >
> > Perhaps I am doing something incorrect during the build process causing
> > this to happen?
> 
> The problem is that _AC() is evaluated in C context (so gains the UL/ULL
> suffix), and the C'd string is fed directly into the assembler (where
> older binutils doesn't tolerate the suffix).
> 
> Short of having a _PAGE_SIZE which is an explicitly non-AC()'d constant,
> I'm not sure what to suggest.  Ideally, you'd want to temporarily define
> __ASSEMBLY__ around the expansion of __stringify(), but I don't think
> that's possible as RESERVE_BRK() is a macro itself.

Joe, what version of binutils do you have?

We can fix this by taking a completely different approach: define the
variable in C and just do the "nobits" in the linker script, like below.
I can work up a proper patch tomorrow.


diff --git a/arch/x86/include/asm/setup.h b/arch/x86/include/asm/setup.h
index 7590ac2570b9..4704184a2d78 100644
--- a/arch/x86/include/asm/setup.h
+++ b/arch/x86/include/asm/setup.h
@@ -108,19 +108,11 @@ extern unsigned long _brk_end;
 void *extend_brk(size_t size, size_t align);
 
 /*
- * Reserve space in the brk section.  The name must be unique within the file,
+ * Reserve space in the .brk section.  The name must be unique within vmlinux,
  * and somewhat descriptive.  The size is in bytes.
- *
- * The allocation is done using inline asm (rather than using a section
- * attribute on a normal variable) in order to allow the use of @nobits, so
- * that it doesn't take up any space in the vmlinux file.
  */
 #define RESERVE_BRK(name, size)						\
-	asm(".pushsection .brk_reservation,\"aw\",@nobits\n\t"		\
-	    ".brk." #name ":\n\t"					\
-	    ".skip " __stringify(size) "\n\t"				\
-	    ".size .brk." #name ", " __stringify(size) "\n\t"		\
-	    ".popsection\n\t")
+	char __brk_##name[size] __section(".brk_reservation");
 
 extern void probe_roms(void);
 #ifdef __i386__
@@ -133,12 +125,16 @@ asmlinkage void __init x86_64_start_reservations(char *real_mode_data);
 
 #endif /* __i386__ */
 #endif /* _SETUP */
-#else
-#define RESERVE_BRK(name,sz)				\
-	.pushsection .brk_reservation,"aw",@nobits;	\
-.brk.name:						\
-1:	.skip sz;					\
-	.size .brk.name,.-1b;				\
+
+#else  /* __ASSEMBLY */
+
+#define RESERVE_BRK(name, size)				\
+	.pushsection .brk_reservation, "aw";		\
+__brk_##name:						\
+1:	.skip size;					\
+	.size __brk_##name, . - 1b;			\
 	.popsection
+
 #endif /* __ASSEMBLY__ */
+
 #endif /* _ASM_X86_SETUP_H */
diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
index 91831b9d8aa8..f105b8aa055e 100644
--- a/arch/x86/kernel/vmlinux.lds.S
+++ b/arch/x86/kernel/vmlinux.lds.S
@@ -392,7 +392,7 @@ SECTIONS
 	__end_of_kernel_reserve = .;
 
 	. = ALIGN(PAGE_SIZE);
-	.brk : AT(ADDR(.brk) - LOAD_OFFSET) {
+	.brk (NOLOAD) : AT(ADDR(.brk) - LOAD_OFFSET) {
 		__brk_base = .;
 		. += 64 * 1024;		/* 64k alignment slop space */
 		*(.brk_reservation)	/* areas brk users have reserved */

Re: 5.19-rc1 x86 build failure

[Joe Damato]

On Tue, Jun 07, 2022 at 05:59:56PM -0700, Josh Poimboeuf wrote:
> On Tue, Jun 07, 2022 at 12:42:33PM +0000, Andrew Cooper wrote:
> > On 07/06/2022 13:19, Joe Damato wrote:
> > >   96                            .pushsection .brk_reservation,"aw",@nobits
> > >   97                            .brk.early_pgt_alloc:
> > >   98 ???? 00000000              .skip ((2 * 3) * ((1UL) << 12))
> > > ****  Error: missing ')'
> > > ****  Error: missing ')'
> > > ****  Error: missing ')'
> > > ****  Error: junk at end of line, first unrecognized character is `U'
> > >   98      0000
> > >  100                            .popsection
> > >
> > > This comes from arch/x86/mm/init.c, which has the following code:
> > >
> > > RESERVE_BRK(early_pgt_alloc, INIT_PGT_BUF_SIZE);
> > >
> > > wherein INIT_PGT_BUF_SIZE (via PAGE_SIZE) has a "1UL" which makes the
> > > assembler unhappy.
> > >
> > > I don't really know what the correct way to fix this is; it seems that the
> > > macro _AC should handle this if ASSEMBLY is defined, IIUC, but that does
> > > not seem to be the case at this point in init.c.
> > >
> > > Perhaps I am doing something incorrect during the build process causing
> > > this to happen?
> >
> > The problem is that _AC() is evaluated in C context (so gains the UL/ULL
> > suffix), and the C'd string is fed directly into the assembler (where
> > older binutils doesn't tolerate the suffix).
> >
> > Short of having a _PAGE_SIZE which is an explicitly non-AC()'d constant,
> > I'm not sure what to suggest.  Ideally, you'd want to temporarily define
> > __ASSEMBLY__ around the expansion of __stringify(), but I don't think
> > that's possible as RESERVE_BRK() is a macro itself.
>
> Joe, what version of binutils do you have?

I am using binutils: 2.26.1-1ubuntu1~16.04.8+esm4.

> We can fix this by taking a completely different approach: define the
> variable in C and just do the "nobits" in the linker script, like below.
> I can work up a proper patch tomorrow.

The change below makes sense to me. I applied it locally and was able to
build the kernel again.

Let me know if you'd like me to try building the next patch you write up.
> diff --git a/arch/x86/include/asm/setup.h b/arch/x86/include/asm/setup.h
> index 7590ac2570b9..4704184a2d78 100644
> --- a/arch/x86/include/asm/setup.h
> +++ b/arch/x86/include/asm/setup.h
> @@ -108,19 +108,11 @@ extern unsigned long _brk_end;
>  void *extend_brk(size_t size, size_t align);
>
>  /*
> - * Reserve space in the brk section.  The name must be unique within the file,
> + * Reserve space in the .brk section.  The name must be unique within vmlinux,
>   * and somewhat descriptive.  The size is in bytes.
> - *
> - * The allocation is done using inline asm (rather than using a section
> - * attribute on a normal variable) in order to allow the use of @nobits, so
> - * that it doesn't take up any space in the vmlinux file.
>   */
>  #define RESERVE_BRK(name, size)                                              \
> -     asm(".pushsection .brk_reservation,\"aw\",@nobits\n\t"          \
> -         ".brk." #name ":\n\t"                                       \
> -         ".skip " __stringify(size) "\n\t"                           \
> -         ".size .brk." #name ", " __stringify(size) "\n\t"           \
> -         ".popsection\n\t")
> +     char __brk_##name[size] __section(".brk_reservation");
>
>  extern void probe_roms(void);
>  #ifdef __i386__
> @@ -133,12 +125,16 @@ asmlinkage void __init x86_64_start_reservations(char *real_mode_data);
>
>  #endif /* __i386__ */
>  #endif /* _SETUP */
> -#else
> -#define RESERVE_BRK(name,sz)                         \
> -     .pushsection .brk_reservation,"aw",@nobits;     \
> -.brk.name:                                           \
> -1:   .skip sz;                                       \
> -     .size .brk.name,.-1b;                           \
> +
> +#else  /* __ASSEMBLY */
> +
> +#define RESERVE_BRK(name, size)                              \
> +     .pushsection .brk_reservation, "aw";            \
> +__brk_##name:                                                \
> +1:   .skip size;                                     \
> +     .size __brk_##name, . - 1b;                     \
>       .popsection
> +
>  #endif /* __ASSEMBLY__ */
> +
>  #endif /* _ASM_X86_SETUP_H */
> diff --git a/arch/x86/kernel/vmlinux.lds.S b/arch/x86/kernel/vmlinux.lds.S
> index 91831b9d8aa8..f105b8aa055e 100644
> --- a/arch/x86/kernel/vmlinux.lds.S
> +++ b/arch/x86/kernel/vmlinux.lds.S
> @@ -392,7 +392,7 @@ SECTIONS
>       __end_of_kernel_reserve = .;
>
>       . = ALIGN(PAGE_SIZE);
> -     .brk : AT(ADDR(.brk) - LOAD_OFFSET) {
> +     .brk (NOLOAD) : AT(ADDR(.brk) - LOAD_OFFSET) {
>               __brk_base = .;
>               . += 64 * 1024;         /* 64k alignment slop space */
>               *(.brk_reservation)     /* areas brk users have reserved */

Re: 5.19-rc1 x86 build failure

[Andrew Cooper]

On 08/06/2022 01:59, Josh Poimboeuf wrote:
> @@ -133,12 +125,16 @@ asmlinkage void __init x86_64_start_reservations(char *real_mode_data);
>  
>  #endif /* __i386__ */
>  #endif /* _SETUP */
> -#else
> -#define RESERVE_BRK(name,sz)				\
> -	.pushsection .brk_reservation,"aw",@nobits;	\
> -.brk.name:						\
> -1:	.skip sz;					\
> -	.size .brk.name,.-1b;				\
> +
> +#else  /* __ASSEMBLY */
> +
> +#define RESERVE_BRK(name, size)				\
> +	.pushsection .brk_reservation, "aw";		\
> +__brk_##name:						\
> +1:	.skip size;					\
> +	.size __brk_##name, . - 1b;			\
>  	.popsection

While I think about it before you write the patch properly, you ought to
have a .type in here too, seeing as the C side now gets one.

~Andrew

Re: 5.19-rc1 x86 build failure

[Josh Poimboeuf]

On Wed, Jun 08, 2022 at 09:24:25AM +0000, Andrew Cooper wrote:
> On 08/06/2022 01:59, Josh Poimboeuf wrote:
> > @@ -133,12 +125,16 @@ asmlinkage void __init x86_64_start_reservations(char *real_mode_data);
> >  
> >  #endif /* __i386__ */
> >  #endif /* _SETUP */
> > -#else
> > -#define RESERVE_BRK(name,sz)				\
> > -	.pushsection .brk_reservation,"aw",@nobits;	\
> > -.brk.name:						\
> > -1:	.skip sz;					\
> > -	.size .brk.name,.-1b;				\
> > +
> > +#else  /* __ASSEMBLY */
> > +
> > +#define RESERVE_BRK(name, size)				\
> > +	.pushsection .brk_reservation, "aw";		\
> > +__brk_##name:						\
> > +1:	.skip size;					\
> > +	.size __brk_##name, . - 1b;			\
> >  	.popsection
> 
> While I think about it before you write the patch properly, you ought to
> have a .type in here too, seeing as the C side now gets one.

Good point, I'll just use the SYM_DATA_{START,END} macros.

-- 
Josh