This is an archive of the discontinued LLVM Phabricator instance.

[builtins] Do not force thumb mode directive in arm/sync-ops.h
ClosedPublic

Authored by raj.khem on Jun 12 2021, 9:09 AM.

Details

Summary

.thumb_func was not switching mode until [1]
so it did not show up but now that .thumb_func (without argument) is
switching mode, its causing build failures on armv6 ( rpi0 ) even when
build is explicitly asking for this file to be built with -marm (ARM
mode), therefore use DEFINE_COMPILERRT_FUNCTION macro to add function
header which considers arch and mode from compiler cmdline to decide if
the function is built using thumb mode or arm mode.

[1] https://reviews.llvm.org/D101975

Note that it also needs

https://reviews.llvm.org/D99282

Diff Detail

Event Timeline

raj.khem created this revision.Jun 12 2021, 9:09 AM
raj.khem requested review of this revision.Jun 12 2021, 9:09 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2021, 9:09 AM
Herald added a subscriber: Restricted Project. · View Herald Transcript

Some platforms (e.g. WoA) do require thumb-only compilation. Can we conditionalize this on whether we are building for ARMv6 or not?

Some platforms (e.g. WoA) do require thumb-only compilation. Can we conditionalize this on whether we are building for ARMv6 or not?

for such platforms I think passing -mthumb via cmdline would be required which then will work just fine here since DEFINE_COMPILERRT_FUNCTION
will add needed directives automatically.

For ELF targets I think this should work fine. However looking at the previous change I think this may break the Darwin build. This change looks like it reverts:

commit 84610ba9b38d917317ac997074acacd99962d767
Author: Steven Wu <stevenwu@apple.com>
Date:   Sat Oct 4 00:18:59 2014 +0000

    Fix the armv7 thumb builtins on darwin

    The arm builtins converted into thumb in r213481 are not working
    on darwin. On apple platforms, .thumb_func directive is required
    to generated correct symbols for thumb functions.

    <rdar://problem/18523605>

    llvm-svn: 219040

I think we'll need a way to get .thumb_func added for Darwin or potentially call DEFINE_COMPILERRT_FUNCTION for Armv6.

For ELF targets I think this should work fine. However looking at the previous change I think this may break the Darwin build. This change looks like it reverts:

commit 84610ba9b38d917317ac997074acacd99962d767
Author: Steven Wu <stevenwu@apple.com>
Date:   Sat Oct 4 00:18:59 2014 +0000

    Fix the armv7 thumb builtins on darwin

    The arm builtins converted into thumb in r213481 are not working
    on darwin. On apple platforms, .thumb_func directive is required
    to generated correct symbols for thumb functions.

    <rdar://problem/18523605>

    llvm-svn: 219040

I think we'll need a way to get .thumb_func added for Darwin or potentially call DEFINE_COMPILERRT_FUNCTION for Armv6.

I looked at snapshot of code when this change was made and we did not have DECLARE_FUNC_ENCODING defined back then

#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)                                              \
  SYMBOL_NAME(name):

#define DEFINE_COMPILERRT_THUMB_FUNCTION(name)                                 \
  FILE_LEVEL_DIRECTIVE SEPARATOR                                               \
  .globl SYMBOL_NAME(name) SEPARATOR                                           \
  THUMB_FUNC                                                                   \
  SYMBOL_IS_FUNC(SYMBOL_NAME(name)) SEPARATOR                                  \
  DECLARE_SYMBOL_VISIBILITY(name)                                              \
  SYMBOL_NAME(name):

now we use DECLARE_FUNC_ENCODING to define thumb/arm attribute for the function in DEFINE_COMPILERRT_FUNCTION macro

which is defined like this

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

#else // !defined(__thumb2__) && !defined(__thumb__)
#define DEFINE_CODE_STATE .arm SEPARATOR
#define DECLARE_FUNC_ENCODING
....

so perhaps it will work ok for darwin too maybe ?

peter.smith accepted this revision.Jun 16 2021, 2:17 AM

OK with DECLARE_FUNC_ENCODING introduced in D31220, along with many similar changes of DEFINE_COMPILERRT_THUMB_FUNCTION to DEFINE_COMPILERRT_FUNCTION, if the file is compiled for Thumb DEFINE_COMPILERRT_FUNCTION will produce .thumb_func before the symbol as DEFINE_COMPILERRT_THUMB_FUNCTION. It seems like the only remaining use case of DEFINE_COMPILER_RT_THUMB_FUNCTION is to force Thumb state.

With this change it looks like the only remaining use of DEFINE_COMPILERRT_THUMB_FUNCTION is in addsf3.S (D29485) the function is written with Thumb (does not require Thumb 2) in mind, although it should compile with Arm if needed. At some point it may be worth removing converting it to DEFINE_COMPILERRT_FUNCTION and removing DEFINE_COMPILERRT_THUMB_FUNCTION.

I've set approved, but please wait a few days to see if there are any objections.

This revision is now accepted and ready to land.Jun 16 2021, 2:17 AM

@peter.smith Can you help landing this changeset ? there has been no objections on it thus far.

Herald added a project: Restricted Project. · View Herald TranscriptMar 10 2022, 7:19 PM

arc patch D104183 fails for me. You need to rebase on origin/main and upload a new version with arc diff --head=HEAD 'HEAD^'.

This revision was landed with ongoing or failed builds.Mar 11 2022, 4:25 PM
This revision was automatically updated to reflect the committed changes.
MaskRay retitled this revision from [compiler-rt] Do not force thumb mode directive in arm/sync-ops.h to [builtins] Do not force thumb mode directive in arm/sync-ops.h.Mar 11 2022, 4:26 PM
MaskRay removed a project: Restricted Project.
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2022, 4:26 PM