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

Heiko Stübner
2023-01-04T15:14:29Z

Hi again,

scratch this mail, I just saw the other one from 15:08 :-)

Heiko

Am Mittwoch, 4. Januar 2023, 15:11:38 CET schrieb 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@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