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

["Heiko Stübner" <heiko@sntech.de>]

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





_______________________________________________
linux-riscv mailing list
linux-riscv@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-riscv