[v2,2/5] powerpc/boot: Fix crt0.S syntax for clang

Message ID 20180914040649.1794-3-joel@jms.id.au
State Rejected
Headers show
Series
  • powerpc: Clang build fixes
Related show

Checks

Context Check Description
snowpatch_ozlabs/checkpatch success Test checkpatch on branch next
snowpatch_ozlabs/apply_patch success next/apply_patch Successfully applied

Commit Message

Joel Stanley Sept. 14, 2018, 4:06 a.m.
Clang's assembler does not like the syntax of the cmpdi:

 arch/powerpc/boot/crt0.S:168:22: error: unexpected modifier on variable reference
         cmpdi   12,RELACOUNT@l
                              ^
 arch/powerpc/boot/crt0.S:168:11: error: unknown operand
         cmpdi   12,RELACOUNT@l
                   ^
Enclosing RELACOUNT in () makes it is happy. Tested with GCC 8 and Clang
8 (trunk).

Reported to clang as https://bugs.llvm.org/show_bug.cgi?id=38945

Signed-off-by: Joel Stanley <joel@jms.id.au>
---
v2: Fix for !powerpc64 too, add bug link to commit message
---
 arch/powerpc/boot/crt0.S | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Nick Desaulniers Sept. 14, 2018, 5:47 p.m. | #1
On Thu, Sep 13, 2018 at 9:07 PM Joel Stanley <joel@jms.id.au> wrote:
>
> Clang's assembler does not like the syntax of the cmpdi:
>
>  arch/powerpc/boot/crt0.S:168:22: error: unexpected modifier on variable reference
>          cmpdi   12,RELACOUNT@l
>                               ^
>  arch/powerpc/boot/crt0.S:168:11: error: unknown operand
>          cmpdi   12,RELACOUNT@l
>                    ^
> Enclosing RELACOUNT in () makes it is happy. Tested with GCC 8 and Clang
> 8 (trunk).
>
> Reported to clang as https://bugs.llvm.org/show_bug.cgi?id=38945
>
> Signed-off-by: Joel Stanley <joel@jms.id.au>
> ---
> v2: Fix for !powerpc64 too, add bug link to commit message
> ---
>  arch/powerpc/boot/crt0.S | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/arch/powerpc/boot/crt0.S b/arch/powerpc/boot/crt0.S
> index dcf2f15e6797..e453e011d7e7 100644
> --- a/arch/powerpc/boot/crt0.S
> +++ b/arch/powerpc/boot/crt0.S
> @@ -80,7 +80,7 @@ p_base:       mflr    r10             /* r10 now points to runtime addr of p_base */
>         lwz     r9,4(r12)       /* get RELA pointer in r9 */
>         b       12f
>  11:    addis   r8,r8,(-RELACOUNT)@ha
> -       cmpwi   r8,RELACOUNT@l
> +       cmpwi   r8,(RELACOUNT)@l
>         bne     12f
>         lwz     r0,4(r12)       /* get RELACOUNT value in r0 */
>  12:    addi    r12,r12,8
> @@ -165,7 +165,7 @@ p_base:     mflr    r10             /* r10 now points to runtime addr of p_base */
>         ld      r13,8(r11)       /* get RELA pointer in r13 */
>         b       11f
>  10:    addis   r12,r12,(-RELACOUNT)@ha
> -       cmpdi   r12,RELACOUNT@l
> +       cmpdi   r12,(RELACOUNT)@l

Yep, as we can see above, when RELACOUNT is negated, it's wrapped in
parens.  It's important for Clang's assembler to match GAS eventually,
but for now, this change simply cononicalizes all of the the
references to RELACOUNT in this source file.
Reviewed-by: Nick Desaulniers <ndesaulniers@google.com>

>         bne     11f
>         ld      r8,8(r11)       /* get RELACOUNT value in r8 */
>  11:    addi    r11,r11,16
> --
> 2.17.1
>
Segher Boessenkool Sept. 14, 2018, 9:08 p.m. | #2
On Fri, Sep 14, 2018 at 10:47:08AM -0700, Nick Desaulniers wrote:
> On Thu, Sep 13, 2018 at 9:07 PM Joel Stanley <joel@jms.id.au> wrote:
> >  10:    addis   r12,r12,(-RELACOUNT)@ha
> > -       cmpdi   r12,RELACOUNT@l
> > +       cmpdi   r12,(RELACOUNT)@l
> 
> Yep, as we can see above, when RELACOUNT is negated, it's wrapped in
> parens.

The only thing that does is make it easier for humans to read; it means
exactly the same thing.


Segher
Nick Desaulniers Sept. 17, 2018, 8:41 p.m. | #3
On Fri, Sep 14, 2018 at 2:08 PM Segher Boessenkool
<segher@kernel.crashing.org> wrote:
>
> On Fri, Sep 14, 2018 at 10:47:08AM -0700, Nick Desaulniers wrote:
> > On Thu, Sep 13, 2018 at 9:07 PM Joel Stanley <joel@jms.id.au> wrote:
> > >  10:    addis   r12,r12,(-RELACOUNT)@ha
> > > -       cmpdi   r12,RELACOUNT@l
> > > +       cmpdi   r12,(RELACOUNT)@l
> >
> > Yep, as we can see above, when RELACOUNT is negated, it's wrapped in
> > parens.

Looks like this was just fixed in Clang-8:
https://bugs.llvm.org/show_bug.cgi?id=38945
https://reviews.llvm.org/D52188
Joel Stanley Sept. 18, 2018, 1:08 a.m. | #4
On Tue, 18 Sep 2018 at 06:11, Nick Desaulniers <ndesaulniers@google.com> wrote:
>
> On Fri, Sep 14, 2018 at 2:08 PM Segher Boessenkool
> <segher@kernel.crashing.org> wrote:
> >
> > On Fri, Sep 14, 2018 at 10:47:08AM -0700, Nick Desaulniers wrote:
> > > On Thu, Sep 13, 2018 at 9:07 PM Joel Stanley <joel@jms.id.au> wrote:
> > > >  10:    addis   r12,r12,(-RELACOUNT)@ha
> > > > -       cmpdi   r12,RELACOUNT@l
> > > > +       cmpdi   r12,(RELACOUNT)@l
> > >
> > > Yep, as we can see above, when RELACOUNT is negated, it's wrapped in
> > > parens.
>
> Looks like this was just fixed in Clang-8:
> https://bugs.llvm.org/show_bug.cgi?id=38945
> https://reviews.llvm.org/D52188

Nice!

mpe, given we need the local references to labels fix which is also in
clang-8 I suggest we drop this patch.

Cheers,

Joel
Michael Ellerman Sept. 18, 2018, 10:53 a.m. | #5
Joel Stanley <joel@jms.id.au> writes:

> On Tue, 18 Sep 2018 at 06:11, Nick Desaulniers <ndesaulniers@google.com> wrote:
>>
>> On Fri, Sep 14, 2018 at 2:08 PM Segher Boessenkool
>> <segher@kernel.crashing.org> wrote:
>> >
>> > On Fri, Sep 14, 2018 at 10:47:08AM -0700, Nick Desaulniers wrote:
>> > > On Thu, Sep 13, 2018 at 9:07 PM Joel Stanley <joel@jms.id.au> wrote:
>> > > >  10:    addis   r12,r12,(-RELACOUNT)@ha
>> > > > -       cmpdi   r12,RELACOUNT@l
>> > > > +       cmpdi   r12,(RELACOUNT)@l
>> > >
>> > > Yep, as we can see above, when RELACOUNT is negated, it's wrapped in
>> > > parens.
>>
>> Looks like this was just fixed in Clang-8:
>> https://bugs.llvm.org/show_bug.cgi?id=38945
>> https://reviews.llvm.org/D52188
>
> Nice!
>
> mpe, given we need the local references to labels fix which is also in
> clang-8 I suggest we drop this patch.

OK, no worries.

cheers

Patch

diff --git a/arch/powerpc/boot/crt0.S b/arch/powerpc/boot/crt0.S
index dcf2f15e6797..e453e011d7e7 100644
--- a/arch/powerpc/boot/crt0.S
+++ b/arch/powerpc/boot/crt0.S
@@ -80,7 +80,7 @@  p_base:	mflr	r10		/* r10 now points to runtime addr of p_base */
 	lwz	r9,4(r12)	/* get RELA pointer in r9 */
 	b	12f
 11:	addis	r8,r8,(-RELACOUNT)@ha
-	cmpwi	r8,RELACOUNT@l
+	cmpwi	r8,(RELACOUNT)@l
 	bne	12f
 	lwz	r0,4(r12)	/* get RELACOUNT value in r0 */
 12:	addi	r12,r12,8
@@ -165,7 +165,7 @@  p_base:	mflr	r10		/* r10 now points to runtime addr of p_base */
 	ld	r13,8(r11)       /* get RELA pointer in r13 */
 	b	11f
 10:	addis	r12,r12,(-RELACOUNT)@ha
-	cmpdi	r12,RELACOUNT@l
+	cmpdi	r12,(RELACOUNT)@l
 	bne	11f
 	ld	r8,8(r11)       /* get RELACOUNT value in r8 */
 11:	addi	r11,r11,16