Page MenuHomePhabricator

Re-apply "[CMake][compiler-rt][AArch64] Avoid preprocessing LSE builtins separately"
ClosedPublic

Authored by tambre on Dec 14 2020, 6:43 AM.

Details

Summary

aa772fc85e0f526615c78b9c3979c2be945a754c (D92530) has landed fixing Apple builds.
Previous quick-fix d9697c2e6b153ac7dc40a69450d9b672f71b1029 (D93198) included in this commit.

Invoking the preprocessor ourselves is fragile and would require us to replicate CMake's handling of definitions, compiler flags, etc for proper compatibility.
In my toolchain builds this notably resulted in a bunch of warnings from unused flags as my CMAKE_C_FLAGS includes CPU-specific optimization options.
Notably this part was already duplicating the logic for VISIBILITY_HIDDEN define.

Instead, symlink the files and set the proper set of defines on each.
This should also be faster as we avoid invoking the compiler multiple times.

Fixes https://llvm.org/PR48494

Diff Detail

Event Timeline

tambre created this revision.Dec 14 2020, 6:43 AM
tambre requested review of this revision.Dec 14 2020, 6:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 14 2020, 6:43 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript
This revision was not accepted when it landed; it landed in state Needs Review.Dec 14 2020, 6:45 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.

@ilinpv D92530 helped, but CFI start and end directives still seem to cause problems. Any ideas?

ilinpv added a comment.EditedDec 14 2020, 8:24 AM

@ilinpv D92530 helped, but CFI start and end directives still seem to cause problems. Any ideas?

I could be wrong 'if defined' for Apple target in compiler-rt/lib/builtins/assembly.h

#if defined(__clang__) || defined(__GCC_HAVE_DWARF2_CFI_ASM)
#define CFI_START .cfi_startproc
#define CFI_END .cfi_endproc
#else
#define CFI_START
#define CFI_END
#endif

If you provide details how to reproduce this fails I will fix them.

Could we revert this until we have a fix for the errors showing up on Darwin?

FAILED: lib/builtins/CMakeFiles/clang_rt.cc_kext_arm64e_ios.dir/outline_atomic_helpers.dir/outline_atomic_cas1_1.S.o 
/Users/buildslave/jenkins/workspace/clang-stage1-RA/clang-build/./bin/clang -DKERNEL_USE -DL_cas -DMODEL=1 -DSIZE=1 -I/Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/compiler-rt/lib/builtins  -I/Users/buildslave/jenkins/workspace/clang-stage1-RA/llvm-project/compiler-rt/lib/builtins/../../include -O2 -g -DNDEBUG -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/MacOSX.platform/Developer/SDKs/MacOSX10.15.sdk    -isysroot /Applications/Xcode.app/Contents/Developer/Platforms/iPhoneOS.platform/Developer/SDKs/iPhoneOS13.5.sdk -miphoneos-version-min=6.0 -fPIC -O3 -fvisibility=hidden -DVISIBILITY_HIDDEN -Wall -fomit-frame-pointer -arch arm64e -mkernel -MD -MT lib/builtins/CMakeFiles/clang_rt.cc_kext_arm64e_ios.dir/outline_atomic_helpers.dir/outline_atomic_cas1_1.S.o -MF lib/builtins/CMakeFiles/clang_rt.cc_kext_arm64e_ios.dir/outline_atomic_helpers.dir/outline_atomic_cas1_1.S.o.d -o lib/builtins/CMakeFiles/clang_rt.cc_kext_arm64e_ios.dir/outline_atomic_helpers.dir/outline_atomic_cas1_1.S.o -c lib/builtins/outline_atomic_helpers.dir/outline_atomic_cas1_1.S
lib/builtins/outline_atomic_helpers.dir/outline_atomic_cas1_1.S:160:1: error: this directive must appear between .cfi_startproc and .cfi_endproc directives
.cfi_endproc
^
tambre added a comment.EditedDec 14 2020, 9:10 AM

Could we revert this until we have a fix for the errors showing up on Darwin?

Reverted.

@ilinpv D92530 helped, but CFI start and end directives still seem to cause problems. Any ideas?

I could be wrong 'if defined' for Apple target in compiler-rt/lib/builtins/assembly.h

#if defined(__clang__) || defined(__GCC_HAVE_DWARF2_CFI_ASM)
#define CFI_START .cfi_startproc
#define CFI_END .cfi_endproc
#else
#define CFI_START
#define CFI_END
#endif

If you provide details how to reproduce this fails I will fix them.

I doubt that's the case, as the buildbot uses Clang.
To reproduce use this command line in the repository root:

root@cl701g1-dev01-xinux:/opt/llvm# clang -DL_cas -DMODEL=3 -DSIZE=4 -Icompiler-rt/lib/builtins -target arm64-apple-darwin -c compiler-rt/lib/builtins/aarch64/lse.S
compiler-rt/lib/builtins/aarch64/lse.S:160:1: error: this directive must appear between .cfi_startproc and .cfi_endproc directives
.cfi_endproc
^

When pre-processed it's this:

# 1 "compiler-rt/lib/builtins/aarch64/lse.S"
# 1 "<built-in>" 1
# 1 "compiler-rt/lib/builtins/aarch64/lse.S" 2




# 1 "compiler-rt/lib/builtins/aarch64/../assembly.h" 1
# 6 "compiler-rt/lib/builtins/aarch64/lse.S" 2
# 28 "compiler-rt/lib/builtins/aarch64/lse.S"
.arch armv8-a


.private_extern __aarch64_have_lse_atomics
# 104 "compiler-rt/lib/builtins/aarch64/lse.S"
.macro JUMP_IF_NOT_LSE label




        adrp x16, __aarch64_have_lse_atomics@page
        ldrb w16, [x16, __aarch64_have_lse_atomics@pageoff]

        cbz w16, \label
.endm


 .text ; .balign 16 ; .globl __aarch64_cas4_rel ; ; ; .cfi_startproc ; __aarch64_cas4_rel: ;
        JUMP_IF_NOT_LSE 8f






        .inst 0x08a07c41 + 0x80000000 + 0x008000
        ret
8:
        mov w16, w0
0:
        ldxr w0, [x2]
        cmp w0, w16
        bne 1f
        stlxr w17, w1, [x2]
        cbnz w17, 0b
1:
        ret
# 160 "compiler-rt/lib/builtins/aarch64/lse.S"
.cfi_endproc

Putting .cfi_startproc on a new line solves the issue. Looking at the relevant code I think the issue is caused by a TODO in MasmParser::parseDirectiveCFIStartProc():

// TODO(kristina): Deal with a corner case of incorrect diagnostic context
// being produced if this directive is emitted as part of preprocessor macro
// expansion which can *ONLY* happen if Clang's cc1as is the API consumer.
// Tools like llvm-mc on the other hand are not affected by it, and report
// correct context information.

That comment also explains why this wasn't showing up prior to my changes. Previously we'd preprocess separately before assembling. Now both are done with a single frontend invocation.

It'd be great if you could figure out a way to have the macro expand in a way that doesn't hit the assembly parser bug.

It'd be great if you could figure out a way to have the macro expand in a way that doesn't hit the assembly parser bug.

Newline for .cfi_startproc https://reviews.llvm.org/D93236

epastor added a comment.EditedDec 15 2020, 6:47 AM

CC'ing @epastor as a FYI for the assembly parser bug.

Pretty sure the issue you're hitting will be in AsmParser, not MasmParser - you're not using MASM here, you're using GNU-style assembly.

But - the bug will be the same regardless. I'm not sure who the right expert is on AsmParser at the moment?

tambre added a subscriber: scott.linder.EditedDec 15 2020, 7:39 AM

CC'ing @epastor as a FYI for the assembly parser bug.

Pretty sure the issue you're hitting will be in AsmParser, not MasmParser - you're not using MASM here, you're using GNU-style assembly.

But - the bug will be the same regardless. I'm not sure who the right expert is on AsmParser at the moment?

Code ownership in LLVM generally feels hazy to me. CODE_OWNERS.txt seems to be mostly outdated.

@scott.linder You recently touched CFI assembly parsing, maybe you'd be interested in taking a look at the bug we encountered here too.

CC'ing @epastor as a FYI for the assembly parser bug.

Pretty sure the issue you're hitting will be in AsmParser, not MasmParser - you're not using MASM here, you're using GNU-style assembly.

But - the bug will be the same regardless. I'm not sure who the right expert is on AsmParser at the moment?

Code ownership in LLVM generally feels hazy to me. CODE_OWNERS.txt seems to be mostly outdated.

@scott.linder You recently touched CFI assembly parsing, maybe you'd be interested in taking a look at the bug we encountered here too.

I'm by no means someone to ask about AsmParser, I just noticed spurious newlines being inserted when MC emitted assembly text in some cases and guessed the parser wasn't consuming newlines eagerly enough.

@MaskRay was very responsive and helpful in the review of that patch, maybe they have a better idea what may be wrong?

The FIXME you mention seems to be talking about how the diagnostic is presented (i.e. what source context it presents), not whether a diagnostic occurs, so it doesn't seem related to me on first blush.

Do you have a reduced reproducer? Is it not reproducible with llvm-mc using the pre-processed assembly text? I can't reproduce it with, e.g.

$ printf " .text ; .balign 16 ; .globl __aarch64_cas4_rel ; ; ; .cfi_startproc ; __aarch64_cas4_rel: ;\n.cfi_endproc" | release/bin/llvm-mc --arch=aarch64 -
        .text
        .p2align        4
        .globl  __aarch64_cas4_rel
        .cfi_startproc
__aarch64_cas4_rel:

        .cfi_endproc

Or even with everything on one line:

$ printf " .text ; .balign 16 ; .globl __aarch64_cas4_rel ; ; ; .cfi_startproc ; __aarch64_cas4_rel: ; .cfi_endproc" | release/bin/llvm-mc --arch=aarch64 -
        .text
        .p2align        4
        .globl  __aarch64_cas4_rel
        .cfi_startproc
__aarch64_cas4_rel:
        .cfi_endproc

Could we revert this until we have a fix for the errors showing up on Darwin?

Reverted.

@ilinpv D92530 helped, but CFI start and end directives still seem to cause problems. Any ideas?

I could be wrong 'if defined' for Apple target in compiler-rt/lib/builtins/assembly.h

#if defined(__clang__) || defined(__GCC_HAVE_DWARF2_CFI_ASM)
#define CFI_START .cfi_startproc
#define CFI_END .cfi_endproc
#else
#define CFI_START
#define CFI_END
#endif

If you provide details how to reproduce this fails I will fix them.

I doubt that's the case, as the buildbot uses Clang.
To reproduce use this command line in the repository root:

root@cl701g1-dev01-xinux:/opt/llvm# clang -DL_cas -DMODEL=3 -DSIZE=4 -Icompiler-rt/lib/builtins -target arm64-apple-darwin -c compiler-rt/lib/builtins/aarch64/lse.S
compiler-rt/lib/builtins/aarch64/lse.S:160:1: error: this directive must appear between .cfi_startproc and .cfi_endproc directives
.cfi_endproc
^

When pre-processed it's this:

# 1 "compiler-rt/lib/builtins/aarch64/lse.S"
# 1 "<built-in>" 1
# 1 "compiler-rt/lib/builtins/aarch64/lse.S" 2




# 1 "compiler-rt/lib/builtins/aarch64/../assembly.h" 1
# 6 "compiler-rt/lib/builtins/aarch64/lse.S" 2
# 28 "compiler-rt/lib/builtins/aarch64/lse.S"
.arch armv8-a


.private_extern __aarch64_have_lse_atomics
# 104 "compiler-rt/lib/builtins/aarch64/lse.S"
.macro JUMP_IF_NOT_LSE label




        adrp x16, __aarch64_have_lse_atomics@page
        ldrb w16, [x16, __aarch64_have_lse_atomics@pageoff]

        cbz w16, \label
.endm


 .text ; .balign 16 ; .globl __aarch64_cas4_rel ; ; ; .cfi_startproc ; __aarch64_cas4_rel: ;
        JUMP_IF_NOT_LSE 8f






        .inst 0x08a07c41 + 0x80000000 + 0x008000
        ret
8:
        mov w16, w0
0:
        ldxr w0, [x2]
        cmp w0, w16
        bne 1f
        stlxr w17, w1, [x2]
        cbnz w17, 0b
1:
        ret
# 160 "compiler-rt/lib/builtins/aarch64/lse.S"
.cfi_endproc

Putting .cfi_startproc on a new line solves the issue. Looking at the relevant code I think the issue is caused by a TODO in MasmParser::parseDirectiveCFIStartProc():

// TODO(kristina): Deal with a corner case of incorrect diagnostic context
// being produced if this directive is emitted as part of preprocessor macro
// expansion which can *ONLY* happen if Clang's cc1as is the API consumer.
// Tools like llvm-mc on the other hand are not affected by it, and report
// correct context information.

That comment also explains why this wasn't showing up prior to my changes. Previously we'd preprocess separately before assembling. Now both are done with a single frontend invocation.

It'd be great if you could figure out a way to have the macro expand in a way that doesn't hit the assembly parser bug.

I am confused. Did you mean that with this patch clang -S generated assembly has a straying .cfi_endproc ?

I don't have Darwin so I don't know how this affects the clang command line.
clang -DL_cas -DMODEL=3 -DSIZE=4 -Icompiler-rt/lib/builtins -target arm64-apple-darwin -c compiler-rt/lib/builtins/aarch64/lse.S currently works fine.

I am confused. Did you mean that with this patch clang -S generated assembly has a straying .cfi_endproc ?

Not quite. With this patch the compilation would fail because of what seems to like an assembly parser bug to me.

I don't have Darwin so I don't know how this affects the clang command line.
clang -DL_cas -DMODEL=3 -DSIZE=4 -Icompiler-rt/lib/builtins -target arm64-apple-darwin -c compiler-rt/lib/builtins/aarch64/lse.S currently works fine.

D93236 added a workaround so that works now.

I'm by no means someone to ask about AsmParser, I just noticed spurious newlines being inserted when MC emitted assembly text in some cases and guessed the parser wasn't consuming newlines eagerly enough.

@MaskRay was very responsive and helpful in the review of that patch, maybe they have a better idea what may be wrong?

The FIXME you mention seems to be talking about how the diagnostic is presented (i.e. what source context it presents), not whether a diagnostic occurs, so it doesn't seem related to me on first blush.

Do you have a reduced reproducer? Is it not reproducible with llvm-mc using the pre-processed assembly text? I can't reproduce it with, e.g.

Repro:

.text; .cfi_startproc;
.cfi_endproc

clang -target arm64-apple-darwin bug.s:

bug.s:2:1: error: this directive must appear between .cfi_startproc and .cfi_endproc directives
.cfi_endproc
^

Putting .cfi_startproc; on a new line fixes the issue.
This doesn't reproduce with llvm-mc --arch arm64 bug.s.

MaskRay added a comment.EditedDec 15 2020, 11:42 PM

I am confused. Did you mean that with this patch clang -S generated assembly has a straying .cfi_endproc ?

Not quite. With this patch the compilation would fail because of what seems to like an assembly parser bug to me.

I don't have Darwin so I don't know how this affects the clang command line.
clang -DL_cas -DMODEL=3 -DSIZE=4 -Icompiler-rt/lib/builtins -target arm64-apple-darwin -c compiler-rt/lib/builtins/aarch64/lse.S currently works fine.

D93236 added a workaround so that works now.

I'm by no means someone to ask about AsmParser, I just noticed spurious newlines being inserted when MC emitted assembly text in some cases and guessed the parser wasn't consuming newlines eagerly enough.

@MaskRay was very responsive and helpful in the review of that patch, maybe they have a better idea what may be wrong?

The FIXME you mention seems to be talking about how the diagnostic is presented (i.e. what source context it presents), not whether a diagnostic occurs, so it doesn't seem related to me on first blush.

Do you have a reduced reproducer? Is it not reproducible with llvm-mc using the pre-processed assembly text? I can't reproduce it with, e.g.

Repro:

.text; .cfi_startproc;
.cfi_endproc

clang -target arm64-apple-darwin bug.s:

bug.s:2:1: error: this directive must appear between .cfi_startproc and .cfi_endproc directives
.cfi_endproc
^

Putting .cfi_startproc; on a new line fixes the issue.
This doesn't reproduce with llvm-mc --arch arm64 bug.s.

Thanks for the reproduce. It is a issue that DarwinAsmParser.cpp parseSectionDirectiveText eats the next token after .text (I guess it intended to eat the newline). If we agree that this is an issue, this should be straightforward to fix. I can do that tomorrow.

The GNU style parser can handle .text; .cfi_startproc properly. I'll check binutils's darwin port as well

MaskRay added a comment.EditedDec 15 2020, 11:50 PM

@tambre Sorry, my previous comment was wrong. The problem was simply because ; is a comment marker for AArch64MCAsmInfoDarwin (see AArch64MCAsmInfo.cpp:41)

For aarch64-*-darwin, you have to use %% instead of ; as a statement separator.
Also see the definition of SEPARATOR in libunwind/src/assembly.h

In any case aarch64-*-darwin ; as a comment marker is a rather unfortunate choice to me, it unnecessarily diverges from x86_64-*-darwin and *-elf

The issue was due to a confusion about the comment marker. The root cause should have been fixed by D93378 and these workarounds can be deleted. aarch64-*-darwin diverges from most other targets.... which is weird.