On Thu, Nov 26, 2020, 5:03 PM Michael Ellerman [off-list ref] wrote:

Bill Wendling [off-list ref] writes:

quoted

On Mon, Nov 23, 2020 at 7:44 PM Michael Ellerman [off-list ref]

wrote:

quoted

quoted

Bill Wendling [off-list ref] writes:

quoted

On Mon, Nov 23, 2020 at 12:10 PM Segher Boessenkool
[off-list ref] wrote:

quoted

On Mon, Nov 23, 2020 at 12:01:01PM -0800, Bill Wendling wrote:

quoted

On Mon, Nov 23, 2020 at 11:58 AM Segher Boessenkool
[off-list ref] wrote:

quoted

quoted

On Sun, Nov 22, 2020 at 10:36 PM Segher Boessenkool
[off-list ref] wrote:

quoted

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

quoted

What Segher said. :-) Also, if you reverse the comparison,

you'll get

quoted

quoted

quoted

quoted

quoted

quoted

quoted

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)); \

quoted

quoted

quoted

quoted

quoted

quoted

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

quoted

quoted

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:

\

quoted

quoted

        .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

quoted

quoted

+ * expression being non-absolute when the code appears in an inline

assembly

quoted

quoted

+ * statement.
+ * As a workaround use an .org directive that has no effect if the

else case

quoted

quoted

+ * 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:

         \

quoted

quoted

        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.

quoted

quoted

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

quoted

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