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

[tambre]

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

[tambre]

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

[ilinpv]

In D93211#2452123, @tambre wrote:

@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.

[azharudd]

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]

In D93211#2452300, @azharudd wrote:

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

Reverted.

In D93211#2452242, @ilinpv wrote:

In D93211#2452123, @tambre wrote:

@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.

[tambre]

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

[ilinpv]

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]

In D93211#2452405, @tambre wrote:

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]

In D93211#2454859, @epastor wrote:

In D93211#2452405, @tambre wrote:

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.

[scott.linder]

In D93211#2454945, @tambre wrote:

In D93211#2454859, @epastor wrote:

In D93211#2452405, @tambre wrote:

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

[MaskRay]

In D93211#2452404, @tambre wrote:

In D93211#2452300, @azharudd wrote:

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

Reverted.

In D93211#2452242, @ilinpv wrote:

In D93211#2452123, @tambre wrote:

@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.

[tambre]

In D93211#2456054, @MaskRay wrote:

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.

In D93211#2456054, @MaskRay wrote:

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.

In D93211#2455766, @scott.linder wrote:

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]

In D93211#2457038, @tambre wrote:

In D93211#2456054, @MaskRay wrote:

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.

In D93211#2456054, @MaskRay wrote:

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.

In D93211#2455766, @scott.linder wrote:

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]

@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

[MaskRay]

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.

compiler-rt/cmake/Modules/CompilerRTDarwinUtils.cmake

 macro(darwin_add_builtin_libraries)
   set(DARWIN_EXCLUDE_DIR ${CMAKE_CURRENT_SOURCE_DIR}/Darwin-excludes)
 
   set(CFLAGS "-fPIC -O3 -fvisibility=hidden -DVISIBILITY_HIDDEN -Wall -fomit-frame-pointer")
   set(CMAKE_C_FLAGS "")
   set(CMAKE_CXX_FLAGS "")
   set(CMAKE_ASM_FLAGS "")
 
-  set(PROFILE_SOURCES ../profile/InstrProfiling 
+  append_string_if(COMPILER_RT_HAS_ASM_LSE " -DHAS_ASM_LSE" CFLAGS)
+
+  set(PROFILE_SOURCES ../profile/InstrProfiling
                       ../profile/InstrProfilingBuffer
                       ../profile/InstrProfilingPlatformDarwin
                       ../profile/InstrProfilingWriter
                       ../profile/InstrProfilingInternal
                       ../profile/InstrProfilingVersionVar)
   foreach (os ${ARGN})
     list_intersect(DARWIN_BUILTIN_ARCHS DARWIN_${os}_BUILTIN_ARCHS BUILTIN_SUPPORTED_ARCH)
     foreach (arch ${DARWIN_BUILTIN_ARCHS})

compiler-rt/lib/builtins/CMakeLists.txt

 set(aarch64_SOURCES
   ${GENERIC_TF_SOURCES}
   ${GENERIC_SOURCES}
   cpu_model.c
   aarch64/fp_mode.c
 )
 
 # Generate outline atomics helpers from lse.S base
-set(CUSTOM_FLAGS ${CMAKE_C_FLAGS})
-if(NOT ANDROID)
-  append_list_if(COMPILER_RT_HAS_VISIBILITY_HIDDEN_FLAG -DVISIBILITY_HIDDEN CUSTOM_FLAGS)
-endif()
-append_list_if(COMPILER_RT_HAS_ASM_LSE -DHAS_ASM_LSE CUSTOM_FLAGS)
-string(REPLACE " " "\t" CUSTOM_FLAGS "${CUSTOM_FLAGS}")
 set(OA_HELPERS_DIR "${CMAKE_CURRENT_BINARY_DIR}/outline_atomic_helpers.dir")
-file(MAKE_DIRECTORY ${OA_HELPERS_DIR})
+file(MAKE_DIRECTORY "${OA_HELPERS_DIR}")
 
 foreach(pat cas swp ldadd ldclr ldeor ldset)
   foreach(size 1 2 4 8 16)
     foreach(model 1 2 3 4)
       if(pat STREQUAL "cas" OR NOT size STREQUAL "16")
-        set(helper_asm ${OA_HELPERS_DIR}/outline_atomic_${pat}${size}_${model}.S)
+        set(helper_asm "${OA_HELPERS_DIR}/outline_atomic_${pat}${size}_${model}.S")
         add_custom_command(
           OUTPUT ${helper_asm}
-          COMMAND ${CMAKE_C_COMPILER} -E ${CUSTOM_FLAGS} -DL_${pat} -DSIZE=${size} -DMODEL=${model}
-                  ${CMAKE_CURRENT_SOURCE_DIR}/aarch64/lse.S -o ${helper_asm}
-          DEPENDS aarch64/lse.S assembly.h
+          COMMAND ${CMAKE_COMMAND} -E create_symlink "${CMAKE_CURRENT_SOURCE_DIR}/aarch64/lse.S" "${helper_asm}"
         )
-        set_source_files_properties(${helper_asm} PROPERTIES GENERATED TRUE)
-        set(aarch64_SOURCES
-          ${aarch64_SOURCES}
-          ${helper_asm}
+        set_source_files_properties("${helper_asm}"
+          PROPERTIES
+          COMPILE_DEFINITIONS "L_${pat};SIZE=${size};MODEL=${model}"
+          INCLUDE_DIRECTORIES "${CMAKE_CURRENT_SOURCE_DIR}"
         )
+        list(APPEND aarch64_SOURCES "${helper_asm}")
       endif()
     endforeach(model)
   endforeach(size)
 endforeach(pat)
 
 if (MINGW)
   set(aarch64_SOURCES
     ${aarch64_SOURCES}
else ()
   endif()
 
   set(BUILTIN_DEFS "")
 
   if(NOT ANDROID)
     append_list_if(COMPILER_RT_HAS_VISIBILITY_HIDDEN_FLAG VISIBILITY_HIDDEN BUILTIN_DEFS)
   endif()
 
+  append_list_if(COMPILER_RT_HAS_ASM_LSE HAS_ASM_LSE BUILTIN_DEFS)
+
   foreach (arch ${BUILTIN_SUPPORTED_ARCH})
     if (CAN_TARGET_${arch})
       # For ARM archs, exclude any VFP builtins if VFP is not supported
       if (${arch} MATCHES "^(arm|armhf|armv7|armv7s|armv7k|armv7m|armv7em)$")
         string(REPLACE ";" " " _TARGET_${arch}_CFLAGS "${TARGET_${arch}_CFLAGS}")
         check_compile_definition(__ARM_FP "${CMAKE_C_FLAGS} ${_TARGET_${arch}_CFLAGS}" COMPILER_RT_HAS_${arch}_VFP)
         if(NOT COMPILER_RT_HAS_${arch}_VFP)
           list(REMOVE_ITEM ${arch}_SOURCES ${arm_Thumb1_VFPv2_DP_SOURCES} ${arm_Thumb1_VFPv2_SP_SOURCES} ${arm_Thumb1_SjLj_EH_SOURCES})

compiler-rt/lib/builtins/aarch64/lse.S

 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 
-#include "../assembly.h"
+#include "assembly.h"
 
 // Out-of-line LSE atomics helpers. Ported from libgcc library.
 // N = {1, 2, 4, 8}
 // M = {1, 2, 4, 8, 16}
 // ORDER = {'relax', 'acq', 'rel', 'acq_rel'}
 // Routines implemented:
 //
 //  iM __aarch64_casM_ORDER(iM expected, iM desired, iM *ptr)