[PATCH] ARM: replace the sole use of a symbol with its definition

[PATCH] ARM: replace the sole use of a symbol with its definition

[Jian Cai]

ALT_UP_B macro sets symbol up_b_offset via .equ to an expression
involving another symbol. The macro gets expanded twice when
arch/arm/kernel/sleep.S is assembled, creating a scenario where
up_b_offset is set to another expression involving symbols while its
current value is based on symbols. LLVM integrated assembler does not
allow such cases, and based on the documentation of binutils, "Values
that are based on expressions involving other symbols are allowed, but
some targets may restrict this to only being done once per assembly", so
it may be better to avoid such cases as it is not clearly stated which
targets should support or disallow them. The fix in this case is simple,
as up_b_offset has only one use, so we can replace the use with the
definition and get rid of up_b_offset.

Signed-off-by: Jian Cai <caij2003@gmail.com>
---
 arch/arm/include/asm/assembler.h | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
index 99929122dad7..adee13126c62 100644
--- a/arch/arm/include/asm/assembler.h
+++ b/arch/arm/include/asm/assembler.h
@@ -269,10 +269,9 @@
 	.endif							;\
 	.popsection
 #define ALT_UP_B(label)					\
-	.equ	up_b_offset, label - 9998b			;\
 	.pushsection ".alt.smp.init", "a"			;\
 	.long	9998b						;\
-	W(b)	. + up_b_offset					;\
+	W(b)	. + (label - 9998b)					;\
 	.popsection
 #else
 #define ALT_SMP(instr...)
-- 
2.26.0.292.g33ef6b2f38-goog

Re: [PATCH] ARM: replace the sole use of a symbol with its definition

[Nick Desaulniers]

On Tue, Apr 7, 2020 at 12:09 PM Jian Cai <caij2003@gmail.com> wrote:
>
> ALT_UP_B macro sets symbol up_b_offset via .equ to an expression
> involving another symbol. The macro gets expanded twice when
> arch/arm/kernel/sleep.S is assembled, creating a scenario where
> up_b_offset is set to another expression involving symbols while its
> current value is based on symbols. LLVM integrated assembler does not
> allow such cases, and based on the documentation of binutils, "Values
> that are based on expressions involving other symbols are allowed, but
> some targets may restrict this to only being done once per assembly", so
> it may be better to avoid such cases as it is not clearly stated which
> targets should support or disallow them. The fix in this case is simple,
> as up_b_offset has only one use, so we can replace the use with the
> definition and get rid of up_b_offset.
>
> Signed-off-by: Jian Cai <caij2003@gmail.com>

Probably didn't need the extra parens, but it's fine (unless another
reviewer would like a v2).  Maybe Stefan has some thoughts?
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

Please add Link tags if these correspond to issues in our link
tracker, they help us track when and where patches land.
Link: https://github.com/ClangBuiltLinux/linux/issues/920

> ---
>  arch/arm/include/asm/assembler.h | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
> index 99929122dad7..adee13126c62 100644
> --- a/arch/arm/include/asm/assembler.h
> +++ b/arch/arm/include/asm/assembler.h
> @@ -269,10 +269,9 @@
>         .endif                                                  ;\
>         .popsection
>  #define ALT_UP_B(label)                                        \
> -       .equ    up_b_offset, label - 9998b                      ;\
>         .pushsection ".alt.smp.init", "a"                       ;\
>         .long   9998b                                           ;\
> -       W(b)    . + up_b_offset                                 ;\
> +       W(b)    . + (label - 9998b)                                     ;\
>         .popsection
>  #else
>  #define ALT_SMP(instr...)
> --
> 2.26.0.292.g33ef6b2f38-goog
>


-- 
Thanks,
~Nick Desaulniers

Re: [PATCH] ARM: replace the sole use of a symbol with its definition

[Stefan Agner]

On 2020-04-13 20:29, Nick Desaulniers wrote:
> On Tue, Apr 7, 2020 at 12:09 PM Jian Cai <caij2003@gmail.com> wrote:
>>
>> ALT_UP_B macro sets symbol up_b_offset via .equ to an expression
>> involving another symbol. The macro gets expanded twice when
>> arch/arm/kernel/sleep.S is assembled, creating a scenario where
>> up_b_offset is set to another expression involving symbols while its
>> current value is based on symbols. LLVM integrated assembler does not
>> allow such cases, and based on the documentation of binutils, "Values
>> that are based on expressions involving other symbols are allowed, but
>> some targets may restrict this to only being done once per assembly", so
>> it may be better to avoid such cases as it is not clearly stated which
>> targets should support or disallow them. The fix in this case is simple,
>> as up_b_offset has only one use, so we can replace the use with the
>> definition and get rid of up_b_offset.
>>
>> Signed-off-by: Jian Cai <caij2003@gmail.com>

Thanks for tackling this!

> 
> Probably didn't need the extra parens, but it's fine (unless another
> reviewer would like a v2).  Maybe Stefan has some thoughts?

Since this is a processor macro I actually prefer to have parentheses
here. All use sites of ALT_UP_B pass just a label, but still, just to be
on the safe side.

I was wondering why equ has been used in first place. I don't see an
advantage other than having a symbol which can be checked. But given
that this code is stable and don't really need debugging at this point,
I am fine replacing this to make it work for clang.


> Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>
> 
> Please add Link tags if these correspond to issues in our link
> tracker, they help us track when and where patches land.
> Link: https://github.com/ClangBuiltLinux/linux/issues/920

Agreed, please add the link. You can add this when submitting.

With that:
Reviewed-by: Stefan Agner <stefan@agner.ch>

--
Stefan

> 
>> ---
>>  arch/arm/include/asm/assembler.h | 3 +--
>>  1 file changed, 1 insertion(+), 2 deletions(-)
>>
>> diff --git a/arch/arm/include/asm/assembler.h b/arch/arm/include/asm/assembler.h
>> index 99929122dad7..adee13126c62 100644
>> --- a/arch/arm/include/asm/assembler.h
>> +++ b/arch/arm/include/asm/assembler.h
>> @@ -269,10 +269,9 @@
>>         .endif                                                  ;\
>>         .popsection
>>  #define ALT_UP_B(label)                                        \
>> -       .equ    up_b_offset, label - 9998b                      ;\
>>         .pushsection ".alt.smp.init", "a"                       ;\
>>         .long   9998b                                           ;\
>> -       W(b)    . + up_b_offset                                 ;\
>> +       W(b)    . + (label - 9998b)                                     ;\
>>         .popsection
>>  #else
>>  #define ALT_SMP(instr...)
>> --
>> 2.26.0.292.g33ef6b2f38-goog
>>