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

[Heiko Stübner]

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 at 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 at 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