[PATCH] s390: Add assembler-level include guard to insn-common-asm.h

[PATCH] s390: Add assembler-level include guard to insn-common-asm.h

[Steffen Eiden]

The CPP include guard of insn-common-asm.h is ineffective when the file
is read via a gas-level .include from inline asm. The assembler ignores
all '#' lines, so every .include redefines GR_NUM/VX_NUM and fails with
"Macro was already defined" if a translation unit emits more than one
such include. This is not the case right now but may lead to future
build issues.

Add a .ifndef guard with an L-prefixed (local, non-emitted) symbol so
the file can be include multiple times per assembly unit.

Fixes: 495bc70aa3225 ("s390/fpu: Move GR_NUM / VX_NUM macros to separate header file")
Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
---
 arch/s390/include/asm/insn-common-asm.h | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/s390/include/asm/insn-common-asm.h b/arch/s390/include/asm/insn-common-asm.h
index fd9b3cacb7c5..060116719846 100644
--- a/arch/s390/include/asm/insn-common-asm.h
+++ b/arch/s390/include/asm/insn-common-asm.h
@@ -9,6 +9,10 @@
 
 #ifdef __ASSEMBLER__
 
+/* Prevent duplicated GAS macro definitions. CPP guards have no effect for GAS.*/
+.ifndef .Lasm_s390_insn_common_asm_h
+.Lasm_s390_insn_common_asm_h = 1
+
 /*
  * GR_NUM - Retrieve general-purpose register number
  *
@@ -49,5 +53,7 @@
 	.endif
 .endm
 
+.endif	/* .Lasm_s390_insn_common_asm_h */
+
 #endif	/* __ASSEMBLER__ */
 #endif	/* __ASM_S390_INSN_COMMON_ASM_H */

Re: [PATCH] s390: Add assembler-level include guard to insn-common-asm.h

[sashiko-bot]

> The CPP include guard of insn-common-asm.h is ineffective when the file
> is read via a gas-level .include from inline asm. The assembler ignores
> all '#' lines, so every .include redefines GR_NUM/VX_NUM and fails with
> "Macro was already defined" if a translation unit emits more than one
> such include. This is not the case right now but may lead to future
> build issues.
> 
> Add a .ifndef guard with an L-prefixed (local, non-emitted) symbol so
> the file can be include multiple times per assembly unit.
> 
> Fixes: 495bc70aa3225 ("s390/fpu: Move GR_NUM / VX_NUM macros to separate header file")
> Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>

Sashiko has reviewed this patch and found no issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-insn-v1-1-df048a0be0d5@linux.ibm.com?part=1

Re: [PATCH] s390: Add assembler-level include guard to insn-common-asm.h

[Alexander Gordeev]

On Mon, Nov 02, 2026 at 05:51:03PM +0100, Steffen Eiden wrote:

Hi Steffen,

> The CPP include guard of insn-common-asm.h is ineffective when the file
> is read via a gas-level .include from inline asm. The assembler ignores
> all '#' lines, so every .include redefines GR_NUM/VX_NUM and fails with
> "Macro was already defined" if a translation unit emits more than one
> such include. This is not the case right now but may lead to future
> build issues.
> 
> Add a .ifndef guard with an L-prefixed (local, non-emitted) symbol so
> the file can be include multiple times per assembly unit.
> 
> Fixes: 495bc70aa3225 ("s390/fpu: Move GR_NUM / VX_NUM macros to separate header file")

I have a problem with Fixes here:
- as you noted above nothing fails right now
- the same behaviour was there before 495bc70aa3225 commit

Is there a real problem this patch fixes?

Do you happen to know why some other archs use .purgem in similar cases
(aka though appears ugly, should not we use .purgem too)?

> Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
> ---
>  arch/s390/include/asm/insn-common-asm.h | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/arch/s390/include/asm/insn-common-asm.h b/arch/s390/include/asm/insn-common-asm.h
> index fd9b3cacb7c5..060116719846 100644
> --- a/arch/s390/include/asm/insn-common-asm.h
> +++ b/arch/s390/include/asm/insn-common-asm.h
> @@ -9,6 +9,10 @@
>  
>  #ifdef __ASSEMBLER__
>  
> +/* Prevent duplicated GAS macro definitions. CPP guards have no effect for GAS.*/
> +.ifndef .Lasm_s390_insn_common_asm_h
> +.Lasm_s390_insn_common_asm_h = 1
> +
>  /*
>   * GR_NUM - Retrieve general-purpose register number
>   *
> @@ -49,5 +53,7 @@
>  	.endif
>  .endm
>  
> +.endif	/* .Lasm_s390_insn_common_asm_h */
> +
>  #endif	/* __ASSEMBLER__ */
>  #endif	/* __ASM_S390_INSN_COMMON_ASM_H */

Thanks!

Re: [PATCH] s390: Add assembler-level include guard to insn-common-asm.h

[Steffen Eiden]

On Tue, Nov 03, 2026 at 05:39:19PM +0100, Alexander Gordeev wrote:
> On Mon, Nov 02, 2026 at 05:51:03PM +0100, Steffen Eiden wrote:
> 
> Hi Steffen,
> 
> > The CPP include guard of insn-common-asm.h is ineffective when the file
> > is read via a gas-level .include from inline asm. The assembler ignores
> > all '#' lines, so every .include redefines GR_NUM/VX_NUM and fails with
> > "Macro was already defined" if a translation unit emits more than one
> > such include. This is not the case right now but may lead to future
> > build issues.
> > 
> > Add a .ifndef guard with an L-prefixed (local, non-emitted) symbol so
> > the file can be include multiple times per assembly unit.
> > 
> > Fixes: 495bc70aa3225 ("s390/fpu: Move GR_NUM / VX_NUM macros to separate header file")
> 
> I have a problem with Fixes here:
> - as you noted above nothing fails right now
> - the same behaviour was there before 495bc70aa3225 commit
>

I have no problem with removing it. You are right this does not fix a
problem right now but prevents future issues. Let's remove it.

> Is there a real problem this patch fixes?
>

Yes. The second batch of arm-on-s390 patches use the GR_NUM macro as
well. I'll send an update for this series soon. And it happens that the
same compilation unit that uses the GR_NUM macro for creating a new
instruction format (not supported by binutils) also uses some FPU stuff.
So the asm marco will be defined twice. That's how I found this issue in
the first place.

I sent this as a separate patch in the hope that this fix gets into 7.4

> Do you happen to know why some other archs use .purgem in similar cases
> (aka though appears ugly, should not we use .purgem too)?
> 

.purgem can be used to undefine a macro for when you want to use it once.
That's how the other arch use it. Define a macro, use it once, purge it.
All in the same file.

We have a different use case here. We defined a reusable macro in a
separate header. So if we want to use .purgem instead the users of the
macro would need to purge it after they used it. This seems to britle
for me as purging it can be forgotten and the issue can manifest way
later in a complete separate patch.

	Steffen

> > Signed-off-by: Steffen Eiden <seiden@linux.ibm.com>
> > ---
> >  arch/s390/include/asm/insn-common-asm.h | 6 ++++++
> >  1 file changed, 6 insertions(+)
> > 
> > diff --git a/arch/s390/include/asm/insn-common-asm.h b/arch/s390/include/asm/insn-common-asm.h
> > index fd9b3cacb7c5..060116719846 100644
> > --- a/arch/s390/include/asm/insn-common-asm.h
> > +++ b/arch/s390/include/asm/insn-common-asm.h
> > @@ -9,6 +9,10 @@
> >  
> >  #ifdef __ASSEMBLER__
> >  
> > +/* Prevent duplicated GAS macro definitions. CPP guards have no effect for GAS.*/
> > +.ifndef .Lasm_s390_insn_common_asm_h
> > +.Lasm_s390_insn_common_asm_h = 1
> > +
> >  /*
> >   * GR_NUM - Retrieve general-purpose register number
> >   *
> > @@ -49,5 +53,7 @@
> >  	.endif
> >  .endm
> >  
> > +.endif	/* .Lasm_s390_insn_common_asm_h */
> > +
> >  #endif	/* __ASSEMBLER__ */
> >  #endif	/* __ASM_S390_INSN_COMMON_ASM_H */
> 
> Thanks!

Re: [PATCH] s390: Add assembler-level include guard to insn-common-asm.h

[Andreas Grapentin]

On Nov 04 26, Steffen Eiden wrote:
> On Tue, Nov 03, 2026 at 05:39:19PM +0100, Alexander Gordeev wrote:
> > On Mon, Nov 02, 2026 at 05:51:03PM +0100, Steffen Eiden wrote:
> > 
> > Hi Steffen,
> > 
> > > The CPP include guard of insn-common-asm.h is ineffective when the file
> > > is read via a gas-level .include from inline asm. The assembler ignores
> > > all '#' lines, so every .include redefines GR_NUM/VX_NUM and fails with
> > > "Macro was already defined" if a translation unit emits more than one
> > > such include. This is not the case right now but may lead to future
> > > build issues.
> > > 
> > > Add a .ifndef guard with an L-prefixed (local, non-emitted) symbol so
> > > the file can be include multiple times per assembly unit.
> > > 
> > > Fixes: 495bc70aa3225 ("s390/fpu: Move GR_NUM / VX_NUM macros to separate header file")
> > 
> > Is there a real problem this patch fixes?
> 
> Yes. The second batch of arm-on-s390 patches use the GR_NUM macro as
> well. I'll send an update for this series soon. And it happens that the
> same compilation unit that uses the GR_NUM macro for creating a new
> instruction format (not supported by binutils) also uses some FPU stuff.
> So the asm marco will be defined twice. That's how I found this issue in
> the first place.

I just tested this and can confirm the problem. the fpu and sae headers
both asm(.include...) insn-common-asm.h which bypasses the cpp include
guards and breaks GAS because the macros become doubly-defined.

> > Do you happen to know why some other archs use .purgem in similar cases
> > (aka though appears ugly, should not we use .purgem too)?
> > 
> 
> .purgem can be used to undefine a macro for when you want to use it once.
> That's how the other arch use it. Define a macro, use it once, purge it.
> All in the same file.
> 
> We have a different use case here. We defined a reusable macro in a
> separate header. So if we want to use .purgem instead the users of the
> macro would need to purge it after they used it. This seems to britle
> for me as purging it can be forgotten and the issue can manifest way
> later in a complete separate patch.

also, .purgem itself errors when the macro is not already defined, so
won't help us here anyway.

However, if the include guards in insn-common-asm.h are too ugly, we
could also guard the asm(.include...) itself:

+#ifndef __ASM_S390_INSN_COMMON_ASM_H
 asm(".include \"asm/insn-common-asm.h\"\n");
+#endif

The same should also work if we wrap the asm(.include...) in another
header.

Best,
-A

Re: [PATCH] s390: Add assembler-level include guard to insn-common-asm.h

[Andreas Grapentin]

On Nov 05 26, Andreas Grapentin wrote:
> The same should also work if we wrap the asm(.include...) in another
> header.

Something like this (compile tested):

diff --git a/arch/s390/include/asm/fpu-insn.h b/arch/s390/include/asm/fpu-insn.h
index eeb0a233d5c7..f1167aa2ab45 100644
--- a/arch/s390/include/asm/fpu-insn.h
+++ b/arch/s390/include/asm/fpu-insn.h
@@ -15,7 +15,7 @@
 #include <linux/kmsan.h>
 #include <asm/asm-extable.h>

-asm(".include \"asm/insn-common-asm.h\"\n");
+#include <asm/insn-common-asm-emit.h>
 asm(".include \"asm/fpu-insn-asm.h\"\n");

 /*
diff --git a/arch/s390/include/asm/insn-common-asm-emit.h b/arch/s390/include/asm/insn-common-asm-emit.h
new file mode 100644
index 000000000000..26a31e8439bf
--- /dev/null
+++ b/arch/s390/include/asm/insn-common-asm-emit.h
@@ -0,0 +1,8 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+
+#ifndef __ASM_S390_INSN_COMMON_ASM_EMIT_H
+#define __ASM_S390_INSN_COMMON_ASM_EMIT_H
+
+asm(".include \"asm/insn-common-asm.h\"\n");
+
+#endif /* __ASM_S390_INSN_COMMON_ASM_EMIT_H */