[compiler-rt] CMake bug in building ARM builtins library

I noticed the compiler-rt/lib/builtins/CmakeLists.txt is not including the .S files in building clang_rt.builtins-arm.a

We need to tell the CMake build system that the .S files are also the source files.

Is there any intention behind leaving the .S files not to compile?

If not, let me know and I will push a patch.

–Sumanth G

Hi,

compiler-rt/lib/builtins/arm/*.S files are listed in arm_SOURCES variable, and therefore should make it into the builtins static library on ARM. Don’t they?

It probably was just me being silly. If they compile well and pass all
the tests, please, add them.

cheers,
--renato

I assume so... But I'm not an expert in CMake.

No. CMake has different way of treating the .S files
Refer to CMake Wiki has moved

I have a patch which will make CMake treat the .S files as source files.
But, how I can run the unit tests. I see from compiler-rt/test/CMakeLists.txt

# BlocksRuntime and builtins testsuites are not yet ported to lit.
# add_subdirectory(BlocksRuntime)
# add_subdirectory(builtins)

How do you run the unit tests for builtins?

--Sumanth G

No. CMake has different way of treating the .S files
Refer to CMake Wiki has moved

I have a patch which will make CMake treat the .S files as source files.

Let me guess, you should just call set_source_file_properties(<arm .S

PROPERTIES LANGUAGE C)?

But, how I can run the unit tests. I see from
compiler-rt/test/CMakeLists.txt

# BlocksRuntime and builtins testsuites are not yet ported to lit.
# add_subdirectory(BlocksRuntime)
# add_subdirectory(builtins)

How do you run the unit tests for builtins?

Well, there is ⚙ D4251 [compiler-rt][builtins] CTest to execute builtins testsuite which adds support for running
the tests under CTest, but it's not yet reviewed.
There's also a "test/builtins/Unit/test" script, but I doubt it will run
out of the box.

Yes, the fix is set_source_file_properties(<arm .S files> PROPERTIES LANGUAGE C)?

I will try using your patch for running builtins tests in my local set up.

Thanks

–Sumanth G

No. CMake has different way of treating the .S files
Refer to http://www.cmake.org/Wiki/CMake/Assembler

I have a patch which will make CMake treat the .S files as source files.

Let me guess, you should just call set_source_file_properties(<arm .S files> PROPERTIES LANGUAGE C)?

But, how I can run the unit tests. I see from compiler-rt/test/CMakeLists.txt

BlocksRuntime and builtins testsuites are not yet ported to lit.

add_subdirectory(BlocksRuntime)

add_subdirectory(builtins)

How do you run the unit tests for builtins?

Well, there is http://reviews.llvm.org/D4251 which adds support for running the tests under CTest, but it’s not yet reviewed.

There’s also a “test/builtins/Unit/test” script, but I doubt it will run out of the box.

> Is there any intention behind leaving the .S files not to compile?

It probably was just me being silly. If they compile well and pass all
the tests, please, add them.

Well, there should be some attention paid to this. Some of them are meant
for the soft ABI. Some of them are only applicable to EABI environments,
and some of them are only for SJLJ exceptions.

The comments in the outstanding Makefile change to enable Windows on ARM
build contains comments categorizing them.

I see a couple of issues here.

If I include .S files for ARM, the –no-integrated-as path complains about Assembler errors.

The integrated-as path works fine though.

Since I am cross compiling, I cannot use the CTest framework from http://reviews.llvm.org/D4251

I am writing the lit complaint files to run for cross-compiled unit tests. I will push one when I am done testing it.

–Sumanth G

Yes, the fix is set_source_file_properties(<arm .S files> PROPERTIES LANGUAGE C)?

I will try using your patch for running builtins tests in my local set up.

Thanks

–Sumanth G

No. CMake has different way of treating the .S files
Refer to http://www.cmake.org/Wiki/CMake/Assembler

I have a patch which will make CMake treat the .S files as source files.

Let me guess, you should just call set_source_file_properties(<arm .S files> PROPERTIES LANGUAGE C)?

But, how I can run the unit tests. I see from compiler-rt/test/CMakeLists.txt

BlocksRuntime and builtins testsuites are not yet ported to lit.

add_subdirectory(BlocksRuntime)

add_subdirectory(builtins)

How do you run the unit tests for builtins?

Well, there is http://reviews.llvm.org/D4251 which adds support for running the tests under CTest, but it’s not yet reviewed.

There’s also a “test/builtins/Unit/test” script, but I doubt it will run out of the box.

These are very likely just differences between the old ARM assembler syntax and the new 'Unified' syntax. Can you use an assembler that accepts UAL syntax?

Regards,
Jon

+Tim Northover

I added ".syntax unified" to the sync_fetch_* .S files and the
"-no-integrated-as" path moved ahead and failed with
Error: branch out of range

No Integrated asm path:

.p2align 2 ;
.thumb ;
.syntax unified ;
;
.globl __sync_fetch_and_add_8 ;
.type __sync_fetch_and_add_8,%function ;
__sync_fetch_and_add_8: push {r4, r5, r6, lr} ;
dmb ;
mov r12, r0 ;
.L_tryatomic_add_8: ldrexd r0, r1, [r12] ;
adds r4, r0, r2 ;
adc r5, r1, r3 ;
strexd r6, r4, r5, [r12] ;
cbnz r6, .L_tryatomic_add_8 ;
dmb ;
pop {r4, r5, r6, pc}

The ARM manual on CBNZ says backward branching is not possible.
Refer to
http://infocenter.arm.com/help/index.jsp?topic=/com.arm.doc.dui0489c/Cjaghef
c.html

Integrated asm path:
Surprisingly, integrated-as compiles the file and the objdump looks invalid

Disassembly of section .text:
00000000 <__sync_fetch_and_add_8>:
   0: b570 push {r4, r5, r6, lr}
   2: f3bf 8f5f dmb sy
   6: 4684 mov ip, r0
   8: e8dc 017f ldrexd r0, r1, [ip]
   c: 1884 adds r4, r0, r2
   e: eb41 0503 adc.w r5, r1, r3
  12: e8cc 4576 strexd r6, r4, r5, [ip]
  16: bbbe cbnz r6, 88 <__sync_fetch_and_add_8+0x88>
  18: f3bf 8f5f dmb sy
  1c: bd70 pop {r4, r5, r6, pc}

There is no <__sync_fetch_and_add_8+0x88> at least as I could tell

There are two issues here:
1. We need to update the sync_fetch_* files to generate proper assembly
2. How clang ARM MC is accepting the above assembly to compile.

--Sumanth G

Oh yeah, Tim and I discussed this exact issue on irc the other day...

I should have filed a PR. Oops.

Jon

I can file a bug :slight_smile:

2 bugs here:

1. We need to update the sync_fetch_* files to generate proper assembly

2. Clang ARM MC should emit an error on seeing a backward branch for CBNZ.

--Sumanth G

I have filed the 2 bugs

ARM MC assembler allows backward branching on CBNZ instruction
http://llvm.org/bugs/show_bug.cgi?id=20359

ARM builtins library has incompatible assembly
http://llvm.org/bugs/show_bug.cgi?id=20360

I have fixed PR20360 in http://reviews.llvm.org/D4610 (My first patch to
llvm :slight_smile: )
This also addressed the .S source file inclusion issue during bultins
library compilation.

--Sumanth G