compiler-rt builtins are broken for thumb

[Quuxplusone]

Bugzilla Link	PR34715
Status	RESOLVED FIXED
Importance	P enhancement
Reported by	Manoj Gupta (manojgupta@google.com)
Reported on	2017-09-24 16:11:22 -0700
Last modified on	2017-10-02 20:51:30 -0700
Version	unspecified
Hardware	PC All
CC	compnerd@compnerd.org, efriedma@quicinc.com, llozano@chromium.org, llvm-bugs@lists.llvm.org, pirama@google.com, smithp352@googlemail.com, srhines@google.com, weimingz@codeaurora.org
Fixed by commit(s)
Attachments
Blocks
Blocked by	PR34768
See also

Many Compiler-rt ARM asm builtins are missing .thumb declaration. As a result,
they are compiled in ARM mode.

r310884 forced function declaration to be thumb when the thumb state is
supported.
This created a mismatch in generated code where the encoding is done in ARM
mode but function is marked as a thumb function.

#define DEFINE_COMPILERRT_FUNCTION(name)                                       \
   FILE_LEVEL_DIRECTIVE SEPARATOR                                               \
    .globl SYMBOL_NAME(name) SEPARATOR                                           \
    SYMBOL_IS_FUNC(SYMBOL_NAME(name)) SEPARATOR                                  \
    DECLARE_SYMBOL_VISIBILITY(name)                                              \
 +  DECLARE_FUNC_ENCODING      // This forces function to be declared as a thumb function                                                  \
    SYMBOL_NAME(name):

e.g.

$ clang -Qunused-arguments -pie -mthumb -DVISIBILITY_HIDDEN -O2 -pipe -
rtlib=libgcc -fomit-frame-pointer -mfpu=vfpv3 -march=armv7-a -mfloat-abi=hard -
fno-lto -std=c11 -fPIC -fno-builtin -fvisibility=hidden -
DCOMPILER_RT_ARMHF_TARGET -o adddf3vfp.S.o -c  work/compiler-
rt/lib/builtins/arm/adddf3vfp.S -target armv7a-cros-linux-gnueabihf -mfloat-
abi=hard

$ arm-linux-gnueabihf-objdump -d adddf3vfp.S.o

Disassembly of section .text:

00000000 <__adddf3vfp>:
   0:   ee300b01    vadd.f64    d0, d0, d1 // These are ARM instructions, not thumb/thumb2
   4:   e12fff1e    bx  lr

$ readelf -s adddf3vfp.S.o

Previous:
Symbol table '.symtab' contains 3 entries:
   Num:    Value  Size Type    Bind   Vis      Ndx Name
     0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND
     1: 00000000     0 NOTYPE  LOCAL  DEFAULT    2 $a.0
     2: 00000000     8 FUNC    GLOBAL HIDDEN     2 __adddf3vfp

After r310884:
Symbol table '.symtab' contains 3 entries:
   Num:    Value  Size Type    Bind   Vis      Ndx Name
     0: 00000000     0 NOTYPE  LOCAL  DEFAULT  UND
     1: 00000000     0 NOTYPE  LOCAL  DEFAULT    2 $a.0
     2: 00000001     8 FUNC    GLOBAL HIDDEN     2 __adddf3vfp // Note the difference in the field Value

Because of this, linker think __adddf3vfp is a thumb function but the encoding
uses arm format.

[Quuxplusone]

Most likely, the issue can be fixed by adding .thumb decl to functions. Will test and send it for review.

[Quuxplusone]

This may also be somewhat related to https://bugs.llvm.org/show_bug.cgi?id=34519 which in summary is that clang doesn't assemble a file as Thumb when -mthumb or -Wa,-mthumb is given. For the record gcc requires the -mthumb option to be passed through to the assembler with -Wa, however this might not be the right answer for clang.

The majority of Arm Unified Assembly Language files should assemble to either Arm or Thumb dependent on a command line option. Putting a .thumb directive in would work, but would force thumb even if the build options where -marm. It may be better to find a fix for pr34519.

[Quuxplusone]

In assembly.h, there're checks:

#if defined(__thumb2__) || defined(__thumb__)
#define DEFINE_CODE_STATE .thumb
#define DECLARE_FUNC_ENCODING    .thumb_func SEPARATOR
#if defined(__thumb2__)

[Quuxplusone]

Maybe we can make "DEFINE_CODE_STATE" part of "DEFINE_COMPILERRT_FUNCTION"?

[Quuxplusone]

what can be done to discover issues like this earlier?
It is surprising that there is no testing for compiler-rt that catches this bug.

[Quuxplusone]

https://reviews.llvm.org/D38227

@Peter, The clang should be fixed as well but I believe this one can be fixed relatively easily without having to wait for that.

[Quuxplusone]

(In reply to Luis Lozano from comment #5)
> what can be done to discover issues like this earlier?
> It is surprising that there is no testing for compiler-rt that catches this
> bug.

I've not got a lot of experience in this area, but from what I've seen so far:
- There are a large number of possible ways to build compiler-rt, especially
for Arm, given the {A, R, M profile} * {Arm, Thumb} * {Hardfp, Softfp, Soft} *
{Big, Little Endian}. As a result I think that people build and test the
variant of compiler-rt that they need, but we haven't got coverage of all of
them.
- We are quite some way from a user-friendly way of building multiple
configurations of compiler-rt and then testing them. As far as I know:
-- By putting compiler-rt in runtimes rather than projects some extra cmake
build options are enabled that can build multiple configurations, but I don't
think that there are ways of feeding through all the necessary test
configuration options that are needed.
-- There is some hard-coded stuff that triggers off armhf of the default target
of the compiler that probably needs reworking.
-- Testing each configuration can often need some extra tool like qemu.

I've thought that some extra static checking scripts might be useful, for
example some scripts to catch Arm/Thumb mismatches like this.

[Quuxplusone]

Phab isn't sending out review mails for some reason.

I have split out the assembly files into their separate reviews and updated the
review for thumb. The URLs are:

https://reviews.llvm.org/D38227 (Thumb fix for all files)

https://reviews.llvm.org/D38268 (msr instruction in thumb2)
https://reviews.llvm.org/D38271 (p2align for memclr)

[Quuxplusone]

Fixed in https://reviews.llvm.org/rL314718