| Message ID | 20201120224034.191382-4-morbo@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | f47462c9d8af437ae7d3ef410cf11513f5e3714c |
| Headers | show |
| Series | PPC: fixes for clang support | expand |
Hi Bill, Bill Wendling <morbo@google.com> writes: > The clang toolchain treats inline assembly a bit differently than > straight assembly code. In particular, inline assembly doesn't have the > complete context available to resolve expressions. This is intentional > to avoid divergence in the resulting assembly code. > > We can work around this issue by borrowing a workaround done for ARM, > i.e. not directly testing the labels themselves, but by moving the > current output pointer by a value that should always be zero. If this > value is not null, then we will trigger a backward move, which is > explicitly forbidden. > > Signed-off-by: Bill Wendling <morbo@google.com> > --- > arch/powerpc/include/asm/feature-fixups.h | 17 +++++++++++++---- > 1 file changed, 13 insertions(+), 4 deletions(-) > > diff --git a/arch/powerpc/include/asm/feature-fixups.h b/arch/powerpc/include/asm/feature-fixups.h > index b0af97add751..f81036518edb 100644 > --- a/arch/powerpc/include/asm/feature-fixups.h > +++ b/arch/powerpc/include/asm/feature-fixups.h > @@ -36,6 +36,18 @@ label##2: \ > .align 2; \ > label##3: > > +/* > + * If the .org directive fails, it means that the feature instructions > + * are smaller than the alternate instructions. This used to be written > + * as > + * > + * .ifgt (label##4b-label##3b) - (label##2b-label##1b) > + * .error "Feature section else case larger than body" > + * .endif > + * > + * but clang's assembler complains about the expression being non-absolute > + * when the code appears in an inline assembly statement. > + */ > #define MAKE_FTR_SECTION_ENTRY(msk, val, label, sect) \ > label##4: \ > .popsection; \ > @@ -48,12 +60,9 @@ label##5: \ > FTR_ENTRY_OFFSET label##2b-label##5b; \ > FTR_ENTRY_OFFSET label##3b-label##5b; \ > FTR_ENTRY_OFFSET label##4b-label##5b; \ > - .ifgt (label##4b- label##3b)-(label##2b- label##1b); \ > - .error "Feature section else case larger than body"; \ > - .endif; \ > + .org . - ((label##4b-label##3b) > (label##2b-label##1b)); \ > .popsection; When I have an oversize alt section this doesn't seem to give me any error using binutils? If I hard code: .org . - (1); It fails as expected. But if I hard code: .org . - (1 > 0); It builds? cheers
On Mon, Nov 23, 2020 at 04:44:56PM +1100, Michael Ellerman wrote: > If I hard code: > > .org . - (1); > > It fails as expected. > > But if I hard code: > > .org . - (1 > 0); > > It builds? "true" (as a result of a comparison) in as is -1, not 1. Segher
What Segher said. :-) Also, if you reverse the comparison, you'll get a build error. On Sun, Nov 22, 2020 at 10:36 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > On Mon, Nov 23, 2020 at 04:44:56PM +1100, Michael Ellerman wrote: > > If I hard code: > > > > .org . - (1); > > > > It fails as expected. > > > > But if I hard code: > > > > .org . - (1 > 0); > > > > It builds? > > "true" (as a result of a comparison) in as is -1, not 1. > > > Segher
After looking at this, I suspect that the correct change should be: .org . + ((label##4b-label##3b) > (label##2b-label##1b)); I'm sorry about that. I can submit another version of the patch. -bw On Mon, Nov 23, 2020 at 11:43 AM Bill Wendling <morbo@google.com> wrote: > > What Segher said. :-) Also, if you reverse the comparison, you'll get > a build error. > > On Sun, Nov 22, 2020 at 10:36 PM Segher Boessenkool > <segher@kernel.crashing.org> wrote: > > > > On Mon, Nov 23, 2020 at 04:44:56PM +1100, Michael Ellerman wrote: > > > If I hard code: > > > > > > .org . - (1); > > > > > > It fails as expected. > > > > > > But if I hard code: > > > > > > .org . - (1 > 0); > > > > > > It builds? > > > > "true" (as a result of a comparison) in as is -1, not 1. > > > > > > Segher
(Please don't top-post.) > On Sun, Nov 22, 2020 at 10:36 PM Segher Boessenkool > <segher@kernel.crashing.org> wrote: > > "true" (as a result of a comparison) in as is -1, not 1. On Mon, Nov 23, 2020 at 11:43:11AM -0800, Bill Wendling wrote: > What Segher said. :-) Also, if you reverse the comparison, you'll get > a build error. But that means your patch is the wrong way around? - .ifgt (label##4b- label##3b)-(label##2b- label##1b); \ - .error "Feature section else case larger than body"; \ - .endif; \ + .org . - ((label##4b-label##3b) > (label##2b-label##1b)); \ It should be a + in that last line, not a -. Was this tested? Segher
On Mon, Nov 23, 2020 at 11:58 AM Segher Boessenkool <segher@kernel.crashing.org> wrote: > > > On Sun, Nov 22, 2020 at 10:36 PM Segher Boessenkool > > <segher@kernel.crashing.org> wrote: > > > "true" (as a result of a comparison) in as is -1, not 1. > > On Mon, Nov 23, 2020 at 11:43:11AM -0800, Bill Wendling wrote: > > What Segher said. :-) Also, if you reverse the comparison, you'll get > > a build error. > > But that means your patch is the wrong way around? > > - .ifgt (label##4b- label##3b)-(label##2b- label##1b); \ > - .error "Feature section else case larger than body"; \ > - .endif; \ > + .org . - ((label##4b-label##3b) > (label##2b-label##1b)); \ > > It should be a + in that last line, not a -. I said so in a follow up email. > Was this tested? > Please don't be insulting. Anyone can make an error. -bw
On Mon, Nov 23, 2020 at 12:01:01PM -0800, Bill Wendling wrote: > On Mon, Nov 23, 2020 at 11:58 AM Segher Boessenkool > <segher@kernel.crashing.org> wrote: > > > On Sun, Nov 22, 2020 at 10:36 PM Segher Boessenkool > > > <segher@kernel.crashing.org> wrote: > > > > "true" (as a result of a comparison) in as is -1, not 1. > > > > On Mon, Nov 23, 2020 at 11:43:11AM -0800, Bill Wendling wrote: > > > What Segher said. :-) Also, if you reverse the comparison, you'll get > > > a build error. > > > > But that means your patch is the wrong way around? > > > > - .ifgt (label##4b- label##3b)-(label##2b- label##1b); \ > > - .error "Feature section else case larger than body"; \ > > - .endif; \ > > + .org . - ((label##4b-label##3b) > (label##2b-label##1b)); \ > > > > It should be a + in that last line, not a -. > > I said so in a follow up email. Yeah, and that arrived a second after I pressed "send" :-) > > Was this tested? > > > Please don't be insulting. Anyone can make an error. Absolutely, but it is just a question. It seems you could improve that testing! It helps you yourself most of all ;-) Segher
On Mon, Nov 23, 2020 at 12:10 PM Segher Boessenkool <segher@kernel.crashing.org> wrote: > On Mon, Nov 23, 2020 at 12:01:01PM -0800, Bill Wendling wrote: > > On Mon, Nov 23, 2020 at 11:58 AM Segher Boessenkool > > <segher@kernel.crashing.org> wrote: > > > > On Sun, Nov 22, 2020 at 10:36 PM Segher Boessenkool > > > > <segher@kernel.crashing.org> wrote: > > > > > "true" (as a result of a comparison) in as is -1, not 1. > > > > > > On Mon, Nov 23, 2020 at 11:43:11AM -0800, Bill Wendling wrote: > > > > What Segher said. :-) Also, if you reverse the comparison, you'll get > > > > a build error. > > > > > > But that means your patch is the wrong way around? > > > > > > - .ifgt (label##4b- label##3b)-(label##2b- label##1b); \ > > > - .error "Feature section else case larger than body"; \ > > > - .endif; \ > > > + .org . - ((label##4b-label##3b) > (label##2b-label##1b)); \ > > > > > > It should be a + in that last line, not a -. > > > > I said so in a follow up email. > > Yeah, and that arrived a second after I pressed "send" :-) > Michael, I apologize for the churn with these patches. I believe the policy is to resend the match as "v4", correct? I ran tests with the change above. It compiled with no error. If I switch the labels around to ".org . + ((label##2b-label##1b) > (label##4b-label##3b))", then it fails as expected. -bw
Bill Wendling <morbo@google.com> writes: > On Mon, Nov 23, 2020 at 12:10 PM Segher Boessenkool > <segher@kernel.crashing.org> wrote: >> On Mon, Nov 23, 2020 at 12:01:01PM -0800, Bill Wendling wrote: >> > On Mon, Nov 23, 2020 at 11:58 AM Segher Boessenkool >> > <segher@kernel.crashing.org> wrote: >> > > > On Sun, Nov 22, 2020 at 10:36 PM Segher Boessenkool >> > > > <segher@kernel.crashing.org> wrote: >> > > > > "true" (as a result of a comparison) in as is -1, not 1. >> > > >> > > On Mon, Nov 23, 2020 at 11:43:11AM -0800, Bill Wendling wrote: >> > > > What Segher said. :-) Also, if you reverse the comparison, you'll get >> > > > a build error. >> > > >> > > But that means your patch is the wrong way around? >> > > >> > > - .ifgt (label##4b- label##3b)-(label##2b- label##1b); \ >> > > - .error "Feature section else case larger than body"; \ >> > > - .endif; \ >> > > + .org . - ((label##4b-label##3b) > (label##2b-label##1b)); \ >> > > >> > > It should be a + in that last line, not a -. >> > >> > I said so in a follow up email. >> >> Yeah, and that arrived a second after I pressed "send" :-) >> > Michael, I apologize for the churn with these patches. I believe the > policy is to resend the match as "v4", correct? > > I ran tests with the change above. It compiled with no error. If I > switch the labels around to ".org . + ((label##2b-label##1b) > > (label##4b-label##3b))", then it fails as expected. I wanted to retain the nicer error reporting for gcc builds, so I did it like this: diff --git a/arch/powerpc/include/asm/feature-fixups.h b/arch/powerpc/include/asm/feature-fixups.h index b0af97add751..c4ad33074df5 100644 --- a/arch/powerpc/include/asm/feature-fixups.h +++ b/arch/powerpc/include/asm/feature-fixups.h @@ -36,6 +36,24 @@ label##2: \ .align 2; \ label##3: + +#ifndef CONFIG_CC_IS_CLANG +#define CHECK_ALT_SIZE(else_size, body_size) \ + .ifgt (else_size) - (body_size); \ + .error "Feature section else case larger than body"; \ + .endif; +#else +/* + * If we use the ifgt syntax above, clang's assembler complains about the + * expression being non-absolute when the code appears in an inline assembly + * statement. + * As a workaround use an .org directive that has no effect if the else case + * instructions are smaller than the body, but fails otherwise. + */ +#define CHECK_ALT_SIZE(else_size, body_size) \ + .org . + ((else_size) > (body_size)); +#endif + #define MAKE_FTR_SECTION_ENTRY(msk, val, label, sect) \ label##4: \ .popsection; \ @@ -48,9 +66,7 @@ label##5: \ FTR_ENTRY_OFFSET label##2b-label##5b; \ FTR_ENTRY_OFFSET label##3b-label##5b; \ FTR_ENTRY_OFFSET label##4b-label##5b; \ - .ifgt (label##4b- label##3b)-(label##2b- label##1b); \ - .error "Feature section else case larger than body"; \ - .endif; \ + CHECK_ALT_SIZE((label##4b-label##3b), (label##2b-label##1b)); \ .popsection; I've pushed a branch with all your patches applied to: https://github.com/linuxppc/linux/commits/next-test Are you able to give that a quick test? It builds clean with clang for me, but we must be using different versions of clang because my branch already builds clean for me even without your patches. cheers
On Mon, Nov 23, 2020 at 7:44 PM Michael Ellerman <mpe@ellerman.id.au> wrote: > Bill Wendling <morbo@google.com> writes: > > On Mon, Nov 23, 2020 at 12:10 PM Segher Boessenkool > > <segher@kernel.crashing.org> wrote: > >> On Mon, Nov 23, 2020 at 12:01:01PM -0800, Bill Wendling wrote: > >> > On Mon, Nov 23, 2020 at 11:58 AM Segher Boessenkool > >> > <segher@kernel.crashing.org> wrote: > >> > > > On Sun, Nov 22, 2020 at 10:36 PM Segher Boessenkool > >> > > > <segher@kernel.crashing.org> wrote: > >> > > > > "true" (as a result of a comparison) in as is -1, not 1. > >> > > > >> > > On Mon, Nov 23, 2020 at 11:43:11AM -0800, Bill Wendling wrote: > >> > > > What Segher said. :-) Also, if you reverse the comparison, you'll get > >> > > > a build error. > >> > > > >> > > But that means your patch is the wrong way around? > >> > > > >> > > - .ifgt (label##4b- label##3b)-(label##2b- label##1b); \ > >> > > - .error "Feature section else case larger than body"; \ > >> > > - .endif; \ > >> > > + .org . - ((label##4b-label##3b) > (label##2b-label##1b)); \ > >> > > > >> > > It should be a + in that last line, not a -. > >> > > >> > I said so in a follow up email. > >> > >> Yeah, and that arrived a second after I pressed "send" :-) > >> > > Michael, I apologize for the churn with these patches. I believe the > > policy is to resend the match as "v4", correct? > > > > I ran tests with the change above. It compiled with no error. If I > > switch the labels around to ".org . + ((label##2b-label##1b) > > > (label##4b-label##3b))", then it fails as expected. > > I wanted to retain the nicer error reporting for gcc builds, so I did it > like this: > > diff --git a/arch/powerpc/include/asm/feature-fixups.h b/arch/powerpc/include/asm/feature-fixups.h > index b0af97add751..c4ad33074df5 100644 > --- a/arch/powerpc/include/asm/feature-fixups.h > +++ b/arch/powerpc/include/asm/feature-fixups.h > @@ -36,6 +36,24 @@ label##2: \ > .align 2; \ > label##3: > > + > +#ifndef CONFIG_CC_IS_CLANG > +#define CHECK_ALT_SIZE(else_size, body_size) \ > + .ifgt (else_size) - (body_size); \ > + .error "Feature section else case larger than body"; \ > + .endif; > +#else > +/* > + * If we use the ifgt syntax above, clang's assembler complains about the > + * expression being non-absolute when the code appears in an inline assembly > + * statement. > + * As a workaround use an .org directive that has no effect if the else case > + * instructions are smaller than the body, but fails otherwise. > + */ > +#define CHECK_ALT_SIZE(else_size, body_size) \ > + .org . + ((else_size) > (body_size)); > +#endif > + > #define MAKE_FTR_SECTION_ENTRY(msk, val, label, sect) \ > label##4: \ > .popsection; \ > @@ -48,9 +66,7 @@ label##5: \ > FTR_ENTRY_OFFSET label##2b-label##5b; \ > FTR_ENTRY_OFFSET label##3b-label##5b; \ > FTR_ENTRY_OFFSET label##4b-label##5b; \ > - .ifgt (label##4b- label##3b)-(label##2b- label##1b); \ > - .error "Feature section else case larger than body"; \ > - .endif; \ > + CHECK_ALT_SIZE((label##4b-label##3b), (label##2b-label##1b)); \ > .popsection; > > > > I've pushed a branch with all your patches applied to: > > https://github.com/linuxppc/linux/commits/next-test > This works for me. Thanks! > Are you able to give that a quick test? It builds clean with clang for > me, but we must be using different versions of clang because my branch > already builds clean for me even without your patches. > You may need to set LLVM_IAS=1 to get the behavior I'm seeing. That turns on clang's integrated assembler, which I think is disabled by default. Note that with clang's integrated assembler, arch/powerpc/boot/util.S fails to compile. Alan Modra mentioned that he sent you a patch to "modernize" the file so that clang can compile it. -bw
Bill Wendling <morbo@google.com> writes: > On Mon, Nov 23, 2020 at 7:44 PM Michael Ellerman <mpe@ellerman.id.au> wrote: >> Bill Wendling <morbo@google.com> writes: >> > On Mon, Nov 23, 2020 at 12:10 PM Segher Boessenkool >> > <segher@kernel.crashing.org> wrote: >> >> On Mon, Nov 23, 2020 at 12:01:01PM -0800, Bill Wendling wrote: >> >> > On Mon, Nov 23, 2020 at 11:58 AM Segher Boessenkool >> >> > <segher@kernel.crashing.org> wrote: >> >> > > > On Sun, Nov 22, 2020 at 10:36 PM Segher Boessenkool >> >> > > > <segher@kernel.crashing.org> wrote: >> >> > > > > "true" (as a result of a comparison) in as is -1, not 1. >> >> > > >> >> > > On Mon, Nov 23, 2020 at 11:43:11AM -0800, Bill Wendling wrote: >> >> > > > What Segher said. :-) Also, if you reverse the comparison, you'll get >> >> > > > a build error. >> >> > > >> >> > > But that means your patch is the wrong way around? >> >> > > >> >> > > - .ifgt (label##4b- label##3b)-(label##2b- label##1b); \ >> >> > > - .error "Feature section else case larger than body"; \ >> >> > > - .endif; \ >> >> > > + .org . - ((label##4b-label##3b) > (label##2b-label##1b)); \ >> >> > > >> >> > > It should be a + in that last line, not a -. >> >> > >> >> > I said so in a follow up email. >> >> >> >> Yeah, and that arrived a second after I pressed "send" :-) >> >> >> > Michael, I apologize for the churn with these patches. I believe the >> > policy is to resend the match as "v4", correct? >> > >> > I ran tests with the change above. It compiled with no error. If I >> > switch the labels around to ".org . + ((label##2b-label##1b) > >> > (label##4b-label##3b))", then it fails as expected. >> >> I wanted to retain the nicer error reporting for gcc builds, so I did it >> like this: >> >> diff --git a/arch/powerpc/include/asm/feature-fixups.h b/arch/powerpc/include/asm/feature-fixups.h >> index b0af97add751..c4ad33074df5 100644 >> --- a/arch/powerpc/include/asm/feature-fixups.h >> +++ b/arch/powerpc/include/asm/feature-fixups.h >> @@ -36,6 +36,24 @@ label##2: \ >> .align 2; \ >> label##3: >> >> + >> +#ifndef CONFIG_CC_IS_CLANG >> +#define CHECK_ALT_SIZE(else_size, body_size) \ >> + .ifgt (else_size) - (body_size); \ >> + .error "Feature section else case larger than body"; \ >> + .endif; >> +#else >> +/* >> + * If we use the ifgt syntax above, clang's assembler complains about the >> + * expression being non-absolute when the code appears in an inline assembly >> + * statement. >> + * As a workaround use an .org directive that has no effect if the else case >> + * instructions are smaller than the body, but fails otherwise. >> + */ >> +#define CHECK_ALT_SIZE(else_size, body_size) \ >> + .org . + ((else_size) > (body_size)); >> +#endif >> + >> #define MAKE_FTR_SECTION_ENTRY(msk, val, label, sect) \ >> label##4: \ >> .popsection; \ >> @@ -48,9 +66,7 @@ label##5: \ >> FTR_ENTRY_OFFSET label##2b-label##5b; \ >> FTR_ENTRY_OFFSET label##3b-label##5b; \ >> FTR_ENTRY_OFFSET label##4b-label##5b; \ >> - .ifgt (label##4b- label##3b)-(label##2b- label##1b); \ >> - .error "Feature section else case larger than body"; \ >> - .endif; \ >> + CHECK_ALT_SIZE((label##4b-label##3b), (label##2b-label##1b)); \ >> .popsection; >> >> >> >> I've pushed a branch with all your patches applied to: >> >> https://github.com/linuxppc/linux/commits/next-test >> > This works for me. Thanks! Great. >> Are you able to give that a quick test? It builds clean with clang for >> me, but we must be using different versions of clang because my branch >> already builds clean for me even without your patches. >> > You may need to set LLVM_IAS=1 to get the behavior I'm seeing. That > turns on clang's integrated assembler, which I think is disabled by > default. Yep that does it. But then I get: clang: error: unsupported argument '-mpower4' to option 'Wa,' clang: error: unsupported argument '-many' to option 'Wa,' So I guess I'm still missing something? > Note that with clang's integrated assembler, arch/powerpc/boot/util.S > fails to compile. Alan Modra mentioned that he sent you a patch to > "modernize" the file so that clang can compile it. Ah you're right he did, it didn't go to patchwork so I missed it. Have grabbed it now. cheers
On Thu, Nov 26, 2020, 5:03 PM Michael Ellerman <mpe@ellerman.id.au> wrote: > Bill Wendling <morbo@google.com> writes: > > On Mon, Nov 23, 2020 at 7:44 PM Michael Ellerman <mpe@ellerman.id.au> > wrote: > >> Bill Wendling <morbo@google.com> writes: > >> > On Mon, Nov 23, 2020 at 12:10 PM Segher Boessenkool > >> > <segher@kernel.crashing.org> wrote: > >> >> On Mon, Nov 23, 2020 at 12:01:01PM -0800, Bill Wendling wrote: > >> >> > On Mon, Nov 23, 2020 at 11:58 AM Segher Boessenkool > >> >> > <segher@kernel.crashing.org> wrote: > >> >> > > > On Sun, Nov 22, 2020 at 10:36 PM Segher Boessenkool > >> >> > > > <segher@kernel.crashing.org> wrote: > >> >> > > > > "true" (as a result of a comparison) in as is -1, not 1. > >> >> > > > >> >> > > On Mon, Nov 23, 2020 at 11:43:11AM -0800, Bill Wendling wrote: > >> >> > > > What Segher said. :-) Also, if you reverse the comparison, > you'll get > >> >> > > > a build error. > >> >> > > > >> >> > > But that means your patch is the wrong way around? > >> >> > > > >> >> > > - .ifgt (label##4b- label##3b)-(label##2b- label##1b); \ > >> >> > > - .error "Feature section else case larger than body"; \ > >> >> > > - .endif; \ > >> >> > > + .org . - ((label##4b-label##3b) > > (label##2b-label##1b)); \ > >> >> > > > >> >> > > It should be a + in that last line, not a -. > >> >> > > >> >> > I said so in a follow up email. > >> >> > >> >> Yeah, and that arrived a second after I pressed "send" :-) > >> >> > >> > Michael, I apologize for the churn with these patches. I believe the > >> > policy is to resend the match as "v4", correct? > >> > > >> > I ran tests with the change above. It compiled with no error. If I > >> > switch the labels around to ".org . + ((label##2b-label##1b) > > >> > (label##4b-label##3b))", then it fails as expected. > >> > >> I wanted to retain the nicer error reporting for gcc builds, so I did it > >> like this: > >> > >> diff --git a/arch/powerpc/include/asm/feature-fixups.h > b/arch/powerpc/include/asm/feature-fixups.h > >> index b0af97add751..c4ad33074df5 100644 > >> --- a/arch/powerpc/include/asm/feature-fixups.h > >> +++ b/arch/powerpc/include/asm/feature-fixups.h > >> @@ -36,6 +36,24 @@ label##2: > \ > >> .align 2; \ > >> label##3: > >> > >> + > >> +#ifndef CONFIG_CC_IS_CLANG > >> +#define CHECK_ALT_SIZE(else_size, body_size) \ > >> + .ifgt (else_size) - (body_size); \ > >> + .error "Feature section else case larger than body"; \ > >> + .endif; > >> +#else > >> +/* > >> + * If we use the ifgt syntax above, clang's assembler complains about > the > >> + * expression being non-absolute when the code appears in an inline > assembly > >> + * statement. > >> + * As a workaround use an .org directive that has no effect if the > else case > >> + * instructions are smaller than the body, but fails otherwise. > >> + */ > >> +#define CHECK_ALT_SIZE(else_size, body_size) \ > >> + .org . + ((else_size) > (body_size)); > >> +#endif > >> + > >> #define MAKE_FTR_SECTION_ENTRY(msk, val, label, sect) \ > >> label##4: \ > >> .popsection; \ > >> @@ -48,9 +66,7 @@ label##5: > \ > >> FTR_ENTRY_OFFSET label##2b-label##5b; \ > >> FTR_ENTRY_OFFSET label##3b-label##5b; \ > >> FTR_ENTRY_OFFSET label##4b-label##5b; \ > >> - .ifgt (label##4b- label##3b)-(label##2b- label##1b); \ > >> - .error "Feature section else case larger than body"; \ > >> - .endif; \ > >> + CHECK_ALT_SIZE((label##4b-label##3b), (label##2b-label##1b)); \ > >> .popsection; > >> > >> > >> > >> I've pushed a branch with all your patches applied to: > >> > >> https://github.com/linuxppc/linux/commits/next-test > >> > > This works for me. Thanks! > > Great. > > >> Are you able to give that a quick test? It builds clean with clang for > >> me, but we must be using different versions of clang because my branch > >> already builds clean for me even without your patches. > >> > > You may need to set LLVM_IAS=1 to get the behavior I'm seeing. That > > turns on clang's integrated assembler, which I think is disabled by > > default. > > Yep that does it. > > But then I get: > clang: error: unsupported argument '-mpower4' to option 'Wa,' > clang: error: unsupported argument '-many' to option 'Wa,' > > So I guess I'm still missing something? > I've seen that too. I'm not entirely sure what's causing it, but I'll look into it. I've got a backlog of things to work on still. :-) It's probably a clang issue. There's another one that came up having to do with the format of some PPC instructions. I have a clang fix on review for those. > Note that with clang's integrated assembler, arch/powerpc/boot/util.S > > fails to compile. Alan Modra mentioned that he sent you a patch to > > "modernize" the file so that clang can compile it. > > Ah you're right he did, it didn't go to patchwork so I missed it. Have > grabbed it now. > Thanks! -bw >
On Thu, Nov 26, 2020 at 5:03 PM Michael Ellerman <mpe@ellerman.id.au> wrote: > > Bill Wendling <morbo@google.com> writes: > > On Mon, Nov 23, 2020 at 7:44 PM Michael Ellerman <mpe@ellerman.id.au> wrote: > >> Bill Wendling <morbo@google.com> writes: > >> > On Mon, Nov 23, 2020 at 12:10 PM Segher Boessenkool > >> > <segher@kernel.crashing.org> wrote: > >> >> On Mon, Nov 23, 2020 at 12:01:01PM -0800, Bill Wendling wrote: > >> >> > On Mon, Nov 23, 2020 at 11:58 AM Segher Boessenkool > >> >> > <segher@kernel.crashing.org> wrote: > >> >> > > > On Sun, Nov 22, 2020 at 10:36 PM Segher Boessenkool > >> >> > > > <segher@kernel.crashing.org> wrote: > >> >> > > > > "true" (as a result of a comparison) in as is -1, not 1. > >> >> > > > >> >> > > On Mon, Nov 23, 2020 at 11:43:11AM -0800, Bill Wendling wrote: > >> >> > > > What Segher said. :-) Also, if you reverse the comparison, you'll get > >> >> > > > a build error. > >> >> > > > >> >> > > But that means your patch is the wrong way around? > >> >> > > > >> >> > > - .ifgt (label##4b- label##3b)-(label##2b- label##1b); \ > >> >> > > - .error "Feature section else case larger than body"; \ > >> >> > > - .endif; \ > >> >> > > + .org . - ((label##4b-label##3b) > (label##2b-label##1b)); \ > >> >> > > > >> >> > > It should be a + in that last line, not a -. > >> >> > > >> >> > I said so in a follow up email. > >> >> > >> >> Yeah, and that arrived a second after I pressed "send" :-) > >> >> > >> > Michael, I apologize for the churn with these patches. I believe the > >> > policy is to resend the match as "v4", correct? > >> > > >> > I ran tests with the change above. It compiled with no error. If I > >> > switch the labels around to ".org . + ((label##2b-label##1b) > > >> > (label##4b-label##3b))", then it fails as expected. > >> > >> I wanted to retain the nicer error reporting for gcc builds, so I did it > >> like this: > >> > >> diff --git a/arch/powerpc/include/asm/feature-fixups.h b/arch/powerpc/include/asm/feature-fixups.h > >> index b0af97add751..c4ad33074df5 100644 > >> --- a/arch/powerpc/include/asm/feature-fixups.h > >> +++ b/arch/powerpc/include/asm/feature-fixups.h > >> @@ -36,6 +36,24 @@ label##2: \ > >> .align 2; \ > >> label##3: > >> > >> + > >> +#ifndef CONFIG_CC_IS_CLANG > >> +#define CHECK_ALT_SIZE(else_size, body_size) \ > >> + .ifgt (else_size) - (body_size); \ > >> + .error "Feature section else case larger than body"; \ > >> + .endif; > >> +#else > >> +/* > >> + * If we use the ifgt syntax above, clang's assembler complains about the > >> + * expression being non-absolute when the code appears in an inline assembly > >> + * statement. > >> + * As a workaround use an .org directive that has no effect if the else case > >> + * instructions are smaller than the body, but fails otherwise. > >> + */ > >> +#define CHECK_ALT_SIZE(else_size, body_size) \ > >> + .org . + ((else_size) > (body_size)); > >> +#endif > >> + > >> #define MAKE_FTR_SECTION_ENTRY(msk, val, label, sect) \ > >> label##4: \ > >> .popsection; \ > >> @@ -48,9 +66,7 @@ label##5: \ > >> FTR_ENTRY_OFFSET label##2b-label##5b; \ > >> FTR_ENTRY_OFFSET label##3b-label##5b; \ > >> FTR_ENTRY_OFFSET label##4b-label##5b; \ > >> - .ifgt (label##4b- label##3b)-(label##2b- label##1b); \ > >> - .error "Feature section else case larger than body"; \ > >> - .endif; \ > >> + CHECK_ALT_SIZE((label##4b-label##3b), (label##2b-label##1b)); \ > >> .popsection; > >> > >> > >> > >> I've pushed a branch with all your patches applied to: > >> > >> https://github.com/linuxppc/linux/commits/next-test > >> > > This works for me. Thanks! > > Great. > > >> Are you able to give that a quick test? It builds clean with clang for > >> me, but we must be using different versions of clang because my branch > >> already builds clean for me even without your patches. > >> > > You may need to set LLVM_IAS=1 to get the behavior I'm seeing. That > > turns on clang's integrated assembler, which I think is disabled by > > default. > > Yep that does it. > > But then I get: > clang: error: unsupported argument '-mpower4' to option 'Wa,' > clang: error: unsupported argument '-many' to option 'Wa,' > > So I guess I'm still missing something? > [Resent, because my previous email went out as non-plain text.] I've seen that too. I'm not entirely sure what's causing it, but I'll look into it. I've got a backlog of things to work on still. :-) It's probably a clang issue. There's another one that came up having to do with the format of some PPC instructions. I have a clang fix on review for those. > > Note that with clang's integrated assembler, arch/powerpc/boot/util.S > > fails to compile. Alan Modra mentioned that he sent you a patch to > > "modernize" the file so that clang can compile it. > > Ah you're right he did, it didn't go to patchwork so I missed it. Have > grabbed it now. > Thanks! -bw
diff --git a/arch/powerpc/include/asm/feature-fixups.h b/arch/powerpc/include/asm/feature-fixups.h index b0af97add751..f81036518edb 100644 --- a/arch/powerpc/include/asm/feature-fixups.h +++ b/arch/powerpc/include/asm/feature-fixups.h @@ -36,6 +36,18 @@ label##2: \ .align 2; \ label##3: +/* + * If the .org directive fails, it means that the feature instructions + * are smaller than the alternate instructions. This used to be written + * as + * + * .ifgt (label##4b-label##3b) - (label##2b-label##1b) + * .error "Feature section else case larger than body" + * .endif + * + * but clang's assembler complains about the expression being non-absolute + * when the code appears in an inline assembly statement. + */ #define MAKE_FTR_SECTION_ENTRY(msk, val, label, sect) \ label##4: \ .popsection; \ @@ -48,12 +60,9 @@ label##5: \ FTR_ENTRY_OFFSET label##2b-label##5b; \ FTR_ENTRY_OFFSET label##3b-label##5b; \ FTR_ENTRY_OFFSET label##4b-label##5b; \ - .ifgt (label##4b- label##3b)-(label##2b- label##1b); \ - .error "Feature section else case larger than body"; \ - .endif; \ + .org . - ((label##4b-label##3b) > (label##2b-label##1b)); \ .popsection; - /* CPU feature dependent sections */ #define BEGIN_FTR_SECTION_NESTED(label) START_FTR_SECTION(label) #define BEGIN_FTR_SECTION START_FTR_SECTION(97)
The clang toolchain treats inline assembly a bit differently than straight assembly code. In particular, inline assembly doesn't have the complete context available to resolve expressions. This is intentional to avoid divergence in the resulting assembly code. We can work around this issue by borrowing a workaround done for ARM, i.e. not directly testing the labels themselves, but by moving the current output pointer by a value that should always be zero. If this value is not null, then we will trigger a backward move, which is explicitly forbidden. Signed-off-by: Bill Wendling <morbo@google.com> --- arch/powerpc/include/asm/feature-fixups.h | 17 +++++++++++++---- 1 file changed, 13 insertions(+), 4 deletions(-)