Re: [PATCH] RISC-V: fix compile error from decuplicated __ALTERNATIVE_CFG_2

Heiko Stübner
2023-01-04T15:13:48Z

Hi Andrew,

Am Mittwoch, 4. Januar 2023, 11:06:29 CET schrieb Andrew Jones:
> On Tue, Jan 03, 2023 at 10:42:28PM +0100, Heiko Stuebner wrote:
> > From: Heiko Stuebner <heiko.stuebner@vrull.eu>
> >
> > On the non-assembler-side wrapping alternative-macros inside other macros
> > to prevent duplication of code works, as the end result will just be a
> > string that gets fed to the asm instruction.
> >
> > In real assembler code, wrapping .macro blocks inside other .macro blocks
> > brings more restrictions on usage and the optimization done by
> > commit 2ba8c7dc71c0 ("riscv: Don't duplicate __ALTERNATIVE_CFG in __ALTERNATIVE_CFG_2")
> > results in a compile error like:
> >
> > ../arch/riscv/lib/strcmp.S: Assembler messages:
> > ../arch/riscv/lib/strcmp.S:15: Error: too many positional arguments
> > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:"
> > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "887:"
> > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:"
> > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "887:"
> > ../arch/riscv/lib/strcmp.S:15: Error: backward ref to unknown label "886:"
> > ../arch/riscv/lib/strcmp.S:15: Error: attempt to move .org backwards
>
> Ouch. I thought I had tested that, but looking now at my test code I see I
> only bothered to test ALTERNATIVE(), not ALTERNATIVE_2(). Adding the
> ALTERNATIVE_2() test, I can reproduce this.
>
> It appears the issue is that as macro arguments may be separated by commas
> or spaces, the old and new instruction macro arguments, which have spaces
> between their instructions and operands, get interpreted as extra macro
> arguments. There's probably no way to convince the macro otherwise,
> unfortunately.
>
> >
> > Going back to the original code for the non-assembler-part makes that
> > code work again. So this reverts the #ifdef ASSEMBLY part of that commit
> > to the previous variant with duplicated base.
> >
> > Fixes: 2ba8c7dc71c0 ("riscv: Don't duplicate __ALTERNATIVE_CFG in __ALTERNATIVE_CFG_2")
> > Signed-off-by: Heiko Stuebner <heiko.stuebner@vrull.eu>
> > ---
> > I was of two minds about either to revert the full patch, or doing just
> > this partial one for the ASSEMBLY part. I did go with this variant, as I
> > still like the idea of deduplicating as much as possible :-)
> >
> > arch/riscv/include/asm/alternative-macros.h | 13 ++++++++++++-
> > 1 file changed, 12 insertions(+), 1 deletion(-)
> >
> > diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
> > index 7226e2462584..e7bdb2a510a4 100644
> > --- a/arch/riscv/include/asm/alternative-macros.h
> > +++ b/arch/riscv/include/asm/alternative-macros.h
> > @@ -44,9 +44,20 @@
> > ALT_NEW_CONTENT \vendor_id, \errata_id, \enable, \new_c
> > .endm
> >
> > +/*
> > + * Using ALTERNATIVE_CFG inside ALTERNATIVE_CFG_2 results in compile errors.
> > + * So the common code needs to stay duplicated.
> > + */
> > .macro ALTERNATIVE_CFG_2 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1, \
> > new_c_2, vendor_id_2, errata_id_2, enable_2
> > - ALTERNATIVE_CFG \old_c, \new_c_1, \vendor_id_1, \errata_id_1, \enable_1
> > +886 :
> > + .option push
> > + .option norvc
> > + .option norelax
> > + \old_c
> > + .option pop
> > +887 :
>
> We could still share this by creating another macro which only takes old_c,
> and then invoke that from both ALTERNATIVE_CFG and ALTERNATIVE_CFG_2, I
> think.

hmm, so I was trying the change below, but then even ALTERNATIVE_CFG fails.
I was looking for quite a while for some sort of typo I may have in that
change, but so far haven't found any.

Did you have a different solution in mind that may work?

Heiko

------------- 8< -------------
diff --git a/arch/riscv/include/asm/alternative-macros.h b/arch/riscv/include/asm/alternative-macros.h
index e7bdb2a510a4..49c67fd94c57 100644
--- a/arch/riscv/include/asm/alternative-macros.h
+++ b/arch/riscv/include/asm/alternative-macros.h
@@ -33,13 +33,17 @@
.endif
.endm

-.macro ALTERNATIVE_CFG old_c, new_c, vendor_id, errata_id, enable
-886 :
+.macro ALT_OLD_CONTENT old_c
.option push
.option norvc
.option norelax
\old_c
.option pop
+.endm
+
+.macro ALTERNATIVE_CFG old_c, new_c, vendor_id, errata_id, enable
+886 :
+ ALT_OLD_CONTENT \old_c
887 :
ALT_NEW_CONTENT \vendor_id, \errata_id, \enable, \new_c
.endm
@@ -51,11 +55,7 @@
.macro ALTERNATIVE_CFG_2 old_c, new_c_1, vendor_id_1, errata_id_1, enable_1, \
new_c_2, vendor_id_2, errata_id_2, enable_2
886 :
- .option push
- .option norvc
- .option norelax
- \old_c
- .option pop
+ ALT_OLD_CONTENT \old_c
887 :
ALT_NEW_CONTENT \vendor_id_1, \errata_id_1, \enable_1, \new_c_1
ALT_NEW_CONTENT \vendor_id_2, \errata_id_2, \enable_2, \new_c_2