[LLD][ELF] Read ARM BuildAttributes section to determine supported features.

[peter.smith]

Summary

lld assumes some ARM features that are not available in all Arm processors. In particular:

• The blx instruction present for interworking.
• The movt/movw instructions are used in Thunks.
• The J1=1 J2=1 encoding of branch immediates to improve Thumb wide branch range are assumed to be present.

This patch reads the ARM Attributes section to check for the architecture the object file was compiled with. If none of the objects have an architecture that supports these features a warning will be given. These warnings are most likely to affect people compiling for Armv6 as used in the first Raspberry Pi as this is the only mass-market device capable of running linux or BSD that isn't Armv7 or higher.

For completeness I would like to implement support for Armv6 by providing alternative implementations that don't use these features. These can come in later patches.

[emaste]

Thank you, this will be quite useful for FreeBSD and the change seems reasonable to me.

In FreeBSD we target the original RPi, and currently use TARGET_ARCH=armv6 across the majority of our supported 32-bit platforms (but are in the process of introducing TARGET_ARCH=armv7). We do support a small number of Armv4/v5 devices as well.

ELF/Config.h

112–114 ↗

(On Diff #111489)

Should these now be ArmHas... etc.? (i.e., new capitalization as mentioned on llvm-weekly)

ELF/Driver.cpp

1073–1075 ↗

(On Diff #111489)

As a general principle I'd think these should really default to false, but until lld can generate proper output for the false cases I'd say of course this is preferred. (Or, is it the case that we expect tool chains to always produce an attributes section?)

Once it can generate proper output (and tests cover both cases) and the warnings below disappear perhaps this case for lack of attribute sections can become a warning?

[peter.smith]

ELF/Config.h

112–114 ↗

(On Diff #111489)

I haven't got a good answer for you unfortunately. The Arm form is correct according to the new capitalization, but ARM is more consistent with the names throughout the rest of the code. My guess was that consistency would be preferred although I'm very happy to change to use Arm.

ELF/Driver.cpp

1073–1075 ↗

(On Diff #111489)

The build attributes section is not required by the ABI, however in practice all widely used ARM C/C++ compilers provide one as the intersection of the subsets of Arm/Thumb that are supported by all the different Arm CPUs is very restrictive. The assembler won't automatically generate a build attributes section unless it has at least one .eabi_attribute directive, although links using exclusively assembler produced objects tend to be restricted to assembler and linker tests.

I think it would be worthwhile switching the defaults to false when more support for older Arm architectures is added, it would mean adding .eabi_attribute directives to some of the existing tests, but I don't think that this will be too much effort. I also agree a warning for no build attributes is sensible if the defaults go to false as it is usually a mistake by the user.

[peter.smith]

Ping for the build attributes scanning and warning patch. I have some follow up patches that implement support for Arm architectures v5 and v6 (1st revision Raspberry Pi with an Arm1176) that rely on the detection of architecture to restrict the Thumb branch range to 4 megabytes and to not use alternative code sequences in Thunks that avoid use of movt and movw instructions.

[peter.smith]

Ping for the build attributes scanning and warning patch. As mentioned previously, I've got some follow up patches to add Arm V5 and Arm V6 support that rely on detecting (lack of support) for features used by lld.

[grimar]

Few non-strong suggestions from me.

ELF/Config.h

112–114 ↗

(On Diff #111489)

So you use this 3 only for producing a warning currently. And them are non a command line options,
but target specific flags, so I wonder will it be better to make them be a part of ARM target class ?
Or/and may be group them in a struct like ARMFeatures ?

ELF/InputFiles.cpp

358 ↗

(On Diff #111489)

Should this be in ARM.cpp ? Probably a method of ARM target (non-virtual) ?

372 ↗

(On Diff #111489)

Is v6T2 intentionally missing from this list ?

[peter.smith]

Thanks very much for the comments.

I initially thought that putting the Arm specific parts of the implementation into Arch/ARM, however it looks like all of the Targets in Arch are just an implementation of Target.h and aren't directly accessed from the rest of lld, for example there is no ARM.h to include so it looked like I was breaking someone's abstraction. I'm happy to move them if it is the consensus that it is worth separating them.

I'm also happy to move the command line options into ArmFeatures, although I noted that the Mips specific features weren't isolated either, again if there is a consensus that this is the right thing to do I'll do it.

ELF/InputFiles.cpp

372 ↗

(On Diff #111489)

Yes it is. The ARMBuildAttrs v6T2 should be covered by the default: case. I've attempted to explain this with the comment:

// Architectures used in pre-Cortex processors do not support
// The J1 = 1 J2 = 1 Thumb branch range extension, with the exception
// of Architecture v6T2 (arm1156t2-s and arm1156t2f-s) that do.

If that isn't clear? I can reword it.

[grimar]

ELF/InputFiles.cpp

372 ↗

(On Diff #111489)

Ah I see now, sorry for noise here. Comment looks fine.

[peter.smith]

Rebased, to account for changes in Driver.c. Is there anything I can do to progress this patch? At present lld may silently use instructions that will break on older Arm CPUs that don't support them. This patch gives a warning, and I have follow up patches that add compatible alternatives.

[peter.smith]

Ping for review. I would like to get this change in as currently lld may produce output that is incompatible with older Arm CPUs. I have some follow up patches that add support for older Arm CPUs meaning that the warnings can be removed. They all depend on detecting the Architecture via the build attributes though.

[peter.smith]

Putting comment in Phabricator from llvm-commits

Index: test/ELF/arm-bl-v6.s

• /dev/null

+++ test/ELF/arm-bl-v6.s
@@ -0,0 +1,50 @@
+ RUN: llvm-mc -filetype=obj -triple=arm-none-linux-gnueabi %s -o %t
+ RUN: ld.lld %t -o %t2 2>&1 | FileCheck %s
+ Requires: arm
+
+ On Arm v6 the range of a Thumb BL instruction is only 4 megabytes as the
+ extended range encoding is not supported. The following example has a Thumb
+ BL that is out of range on ARM v6 and requires a range extension thunk.
+ As v6 does not support MOVT or MOVW instructions the Thunk must not
+ use these instructions either. At present we don't support v6 so we give a
+ warning for unsupported features.
+
+ CHECK: warning: lld uses extended branch encoding, no object with architecture supporting feature detected.
+// CHECK-NEXT: warning: lld may use movt/movw, no object with architecture supporting feature detected.

Don't you want to check that the other warning is not issued?

+ The ARM support in lld makes some use of instructions that are not available
+ on all ARM architectures. Namely:
+ - Use of BLX instruction for interworking between ARM and Thumb state.
+ - Use of the extended Thumb branch encoding in relocation.
+ - Use of the MOVT/MOVW instructions in Thumb Thunks.
+ The ARM Attributes section contains information about the architecture chosen
+ at compile time. We follow the convention that if at least one input object
+ is compiled with an architecture that supports these features then lld is
+// permitted to use them.

That is the same heuristic used by bfd and gold?

+static void updateSupportedARMFeatures(const ARMAttributeParser &Attributes) {
+ if (Attributes.hasAttribute(ARMBuildAttrs::CPU_arch)) {

Early return maybe?

+ auto Arch = Attributes.getAttributeValue(ARMBuildAttrs::CPU_arch);
+ switch (Arch) {
+ case ARMBuildAttrs::Pre_v4:
+ case ARMBuildAttrs::v4:
+ case ARMBuildAttrs::v4T:
+ Architectures prior to v5 do not support BLX instruction
+ break;
+ case ARMBuildAttrs::v5T:
+ case ARMBuildAttrs::v5TE:
+ case ARMBuildAttrs::v5TEJ:
+ case ARMBuildAttrs::v6:
+ case ARMBuildAttrs::v6KZ:
+ case ARMBuildAttrs::v6K:
+ Config->ARMHasBlx = true;
+ Architectures used in pre-Cortex processors do not support
+ The J1 = 1 J2 = 1 Thumb branch range extension, with the exception
+ of Architecture v6T2 (arm1156t2-s and arm1156t2f-s) that do.
+ break;
+ default:
+ All other Architectures have BLX and extended branch encoding
+ Config->ARMHasBlx = true;
+ Config->ARMJ1J2BranchEncoding = true;
+ if (Arch != ARMBuildAttrs::v6_M && Arch != ARMBuildAttrs::v6S_M)
+ All Architectures used in Cortex processors with the exception
+ // of v6-M and v6S-M have the MOVT and MOVW instructions.
+ Config->ARMHasMovtMovw = true;
+ }
+ }

If you invert switch order you can use a fallthrough:

default: //asumed newer than anything listed explicitly.
case newest:

feature_a = true;

case not_so_new:

feture_b = true;

...

Not sure if it is actually better. Up to you.

+}
+
template <class ELFT>
InputSectionBase *ObjFile<ELFT>::getRelocTarget(const Elf_Shdr &Sec) {

uint32_t Idx = Sec.sh_info;

@@ -381,16 +424,20 @@

StringRef Name = getSectionName(Sec);

switch (Sec.sh_type) {

• case SHT_ARM_ATTRIBUTES:
• // FIXME: ARM meta-data section. Retain the first attribute section
• // we see. The eglibc ARM dynamic loaders require the presence of an
• // attribute section for dlopen to work.
• // In a full implementation we would merge all attribute sections.

+ case SHT_ARM_ATTRIBUTES: {
+ ARMAttributeParser Attributes;
+ ArrayRef<uint8_t> Contents = check(this->getObj().getSectionContents(&Sec));
+ Attributes.Parse(Contents, true);

Add a comment saying what "true" is:

Attributes.Parse(Contents, /*foobar*/ true);

Index: ELF/Driver.cpp

• ELF/Driver.cpp

+++ ELF/Driver.cpp
@@ -1083,6 +1083,28 @@

if (Config->EMachine == EM_MIPS)
  Config->MipsEFlags = calcMipsEFlags<ELFT>();

+ if (Config->EMachine == EM_ARM) {
+ if (InX::ARMAttributes == nullptr) {
+ No ARM attributes section present (assembler without .eabi_attribute
+ directives, like lld tests), assume support for all features
+ Config->ARMHasBlx = true;
+ Config->ARMHasMovtMovw = true;
+ Config->ARMJ1J2BranchEncoding = true;
+ }

The tests don't fail because of a warning, right? So you could probably
just remove this if.

So LGTM with some nits.

Cheers,
Rafael

[peter.smith]

Updated review diff incorporating review comments, I'll commit tomorrow if there are no more changes needed.

To answer a couple of questions in more detail:

+ The ARM support in lld makes some use of instructions that are not available
+ on all ARM architectures. Namely:
+ - Use of BLX instruction for interworking between ARM and Thumb state.
+ - Use of the extended Thumb branch encoding in relocation.
+ - Use of the MOVT/MOVW instructions in Thumb Thunks.
+ The ARM Attributes section contains information about the architecture chosen
+ at compile time. We follow the convention that if at least one input object
+ is compiled with an architecture that supports these features then lld is
+// permitted to use them.

That is the same heuristic used by bfd and gold?

In a simplified form yes. Both bfd and gold have additional logic to detect incompatibilities between attributes, but both will choose the highest architecture if the two are compatible. In the absence of a linker -mcpu option the only other option is to fault inconsistencies (how do we know which of the two the target system is?), which I think would be frustrating.

The basic rules for combining build attributes are defined from the ABI (http://infocenter.arm.com/help/topic/com.arm.doc.ihi0045e/IHI0045E_ABI_addenda.pdf) search for "Combining Attribute Values". The intention is that the values for each Build Attributes tag such as Tag_CPU_arch form a partial order in terms of the demands that they make of the environment. It is up to the tool to determine what it wants to do with that ordering information.

+ if (Config->EMachine == EM_ARM) {
+ if (InX::ARMAttributes == nullptr) {
+ No ARM attributes section present (assembler without .eabi_attribute
+ directives, like lld tests), assume support for all features
+ Config->ARMHasBlx = true;
+ Config->ARMHasMovtMovw = true;
+ Config->ARMJ1J2BranchEncoding = true;
+ }

The tests don't fail because of a warning, right? So you could probably
just remove this if.

I've removed it in the updated diff. This has no effect right now. I've got some follow up patches to post that add support for v5 and v6 Arm, when these are added I'll need to add a .eabi_attribute 6, 10 @ Tag_CPU_arch for v7; for the existing branch range tests to make sure they use the v7 branch range.

lld/trunk/ELF/Config.h

struct Configuration {
   std::vector<llvm::StringRef> SymbolOrderingFile;
   std::vector<llvm::StringRef> Undefined;
   std::vector<SymbolVersion> DynamicList;
   std::vector<SymbolVersion> VersionScriptGlobals;
   std::vector<SymbolVersion> VersionScriptLocals;
   std::vector<uint8_t> BuildIdVector;
   bool AllowMultipleDefinition;
   bool AndroidPackDynRelocs = false;
+  bool ARMHasBlx = false;
+  bool ARMHasMovtMovw = false;
+  bool ARMJ1J2BranchEncoding = false;
   bool AsNeeded = false;
   bool Bsymbolic;
   bool BsymbolicFunctions;
   bool CompressDebugSections;
   bool DefineCommon;
   bool Demangle = true;
   bool DisableVerify;
   bool EhFrameHdr;

lld/trunk/ELF/Driver.cpp

template <class ELFT> void LinkerDriver::link(opt::InputArgList &Args) {
   // or -strip-debug are given.
   if (Config->Strip != StripPolicy::None)
     llvm::erase_if(InputSections, [](InputSectionBase *S) {
       return S->Name.startswith(".debug") || S->Name.startswith(".zdebug");
     });
 
   Config->EFlags = Target->calcEFlags();
 
+  if (Config->EMachine == EM_ARM) {
+    // FIXME: These warnings can be removed when lld only uses these features
+    // when the input objects have been compiled with an architecture that
+    // supports them.
+    if (Config->ARMHasBlx == false)
+      warn("lld uses blx instruction, no object with architecture supporting "
+           "feature detected.");
+    if (Config->ARMJ1J2BranchEncoding == false)
+      warn("lld uses extended branch encoding, no object with architecture "
+           "supporting feature detected.");
+    if (Config->ARMHasMovtMovw == false)
+      warn("lld may use movt/movw, no object with architecture supporting "
+           "feature detected.");
+  }
+
   // This adds a .comment section containing a version string. We have to add it
   // before decompressAndMergeSections because the .comment section is a
   // mergeable section.
   if (!Config->Relocatable)
     InputSections.push_back(createCommentSection<ELFT>());
 
   // Do size optimizations: garbage collection, merging of SHF_MERGE sections
   // and identical code folding.

lld/trunk/ELF/InputFiles.cpp

 #include "llvm/ADT/STLExtras.h"
 #include "llvm/CodeGen/Analysis.h"
 #include "llvm/DebugInfo/DWARF/DWARFContext.h"
 #include "llvm/IR/LLVMContext.h"
 #include "llvm/IR/Module.h"
 #include "llvm/LTO/LTO.h"
 #include "llvm/MC/StringTableBuilder.h"
 #include "llvm/Object/ELFObjectFile.h"
+#include "llvm/Support/ARMAttributeParser.h"
+#include "llvm/Support/ARMBuildAttributes.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/TarWriter.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 using namespace llvm::ELF;
 using namespace llvm::object;
 using namespace llvm::sys::fs;
if (Sec.sh_flags & SHF_LINK_ORDER) {
         fatal(toString(this) + ": invalid sh_link index: " +
               Twine(Sec.sh_link));
       this->Sections[Sec.sh_link]->DependentSections.push_back(
           cast<InputSection>(this->Sections[I]));
     }
   }
 }
 
+// The ARM support in lld makes some use of instructions that are not available
+// on all ARM architectures. Namely:
+// - Use of BLX instruction for interworking between ARM and Thumb state.
+// - Use of the extended Thumb branch encoding in relocation.
+// - Use of the MOVT/MOVW instructions in Thumb Thunks.
+// The ARM Attributes section contains information about the architecture chosen
+// at compile time. We follow the convention that if at least one input object
+// is compiled with an architecture that supports these features then lld is
+// permitted to use them.
+static void updateSupportedARMFeatures(const ARMAttributeParser &Attributes) {
+  if (!Attributes.hasAttribute(ARMBuildAttrs::CPU_arch))
+    return;
+  auto Arch = Attributes.getAttributeValue(ARMBuildAttrs::CPU_arch);
+  switch (Arch) {
+  case ARMBuildAttrs::Pre_v4:
+  case ARMBuildAttrs::v4:
+  case ARMBuildAttrs::v4T:
+    // Architectures prior to v5 do not support BLX instruction
+    break;
+  case ARMBuildAttrs::v5T:
+  case ARMBuildAttrs::v5TE:
+  case ARMBuildAttrs::v5TEJ:
+  case ARMBuildAttrs::v6:
+  case ARMBuildAttrs::v6KZ:
+  case ARMBuildAttrs::v6K:
+    Config->ARMHasBlx = true;
+    // Architectures used in pre-Cortex processors do not support
+    // The J1 = 1 J2 = 1 Thumb branch range extension, with the exception
+    // of Architecture v6T2 (arm1156t2-s and arm1156t2f-s) that do.
+    break;
+  default:
+    // All other Architectures have BLX and extended branch encoding
+    Config->ARMHasBlx = true;
+    Config->ARMJ1J2BranchEncoding = true;
+    if (Arch != ARMBuildAttrs::v6_M && Arch != ARMBuildAttrs::v6S_M)
+      // All Architectures used in Cortex processors with the exception
+      // of v6-M and v6S-M have the MOVT and MOVW instructions.
+      Config->ARMHasMovtMovw = true;
+    break;
+  }
+}
+
 template <class ELFT>
 InputSectionBase *ObjFile<ELFT>::getRelocTarget(const Elf_Shdr &Sec) {
   uint32_t Idx = Sec.sh_info;
   if (Idx >= this->Sections.size())
     fatal(toString(this) + ": invalid relocated section index: " + Twine(Idx));
   InputSectionBase *Target = this->Sections[Idx];
 
   // Strictly speaking, a relocation section must be included in the
InputSectionBase *toRegularSection(MergeInputSection *Sec) {
   return Ret;
 }
 
 template <class ELFT>
 InputSectionBase *ObjFile<ELFT>::createInputSection(const Elf_Shdr &Sec) {
   StringRef Name = getSectionName(Sec);
 
   switch (Sec.sh_type) {
-  case SHT_ARM_ATTRIBUTES:
-    // FIXME: ARM meta-data section. Retain the first attribute section
-    // we see. The eglibc ARM dynamic loaders require the presence of an
-    // attribute section for dlopen to work.
-    // In a full implementation we would merge all attribute sections.
+  case SHT_ARM_ATTRIBUTES: {
+    ARMAttributeParser Attributes;
+    ArrayRef<uint8_t> Contents = check(this->getObj().getSectionContents(&Sec));
+    Attributes.Parse(Contents, /*isLittle*/Config->EKind == ELF32LEKind);
+    updateSupportedARMFeatures(Attributes);
+    // FIXME: Retain the first attribute section we see. The eglibc ARM
+    // dynamic loaders require the presence of an attribute section for dlopen
+    // to work. In a full implementation we would merge all attribute sections.
     if (InX::ARMAttributes == nullptr) {
       InX::ARMAttributes = make<InputSection>(this, &Sec, Name);
       return InX::ARMAttributes;
     }
     return &InputSection::Discarded;
+  }
   case SHT_RELA:
   case SHT_REL: {
     // Find the relocation target section and associate this
     // section with it. Target can be discarded, for example
     // if it is a duplicated member of SHT_GROUP section, we
     // do not create or proccess relocatable sections then.
     InputSectionBase *Target = getRelocTarget(Sec);
     if (!Target)

lld/trunk/test/ELF/arm-bl-v6.s

+// RUN: llvm-mc -filetype=obj -triple=arm-none-linux-gnueabi %s -o %t
+// RUN: ld.lld %t -o %t2 2>&1 | FileCheck %s
+// Requires: arm
+
+// On Arm v6 the range of a Thumb BL instruction is only 4 megabytes as the
+// extended range encoding is not supported. The following example has a Thumb
+// BL that is out of range on ARM v6 and requires a range extension thunk.
+// As v6 does not support MOVT or MOVW instructions the Thunk must not
+// use these instructions either. At present we don't support v6 so we give a
+// warning for unsupported features.
+
+// CHECK: warning: lld uses extended branch encoding, no object with architecture supporting feature detected.
+// CHECK-NEXT: warning: lld may use movt/movw, no object with architecture supporting feature detected.
+// ARM v6 supports blx so we shouldn't see the blx not supported warning.
+// CHECK-NOT: warning: lld uses blx instruction, no object with architecture supporting feature detected.
+ .text
+ .syntax unified
+ .cpu    arm1176jzf-s
+ .eabi_attribute 6, 6    @ Tag_CPU_arch
+ .globl _start
+ .type   _start,%function
+ .balign 0x1000
+_start:
+  bl thumbfunc
+  bx lr
+
+ .thumb
+ .section .text.2, "ax", %progbits
+ .globl thumbfunc
+ .type thumbfunc,%function
+thumbfunc:
+ bl farthumbfunc
+
+// 6 Megabytes, enough to make farthumbfunc out of range of caller on a v6
+// Arm, but not on a v7 Arm.
+ .section .text.3, "ax", %progbits
+ .space 0x200000
+
+ .section .text.4, "ax", %progbits
+ .space 0x200000
+
+ .section .text.5, "ax", %progbits
+ .space 0x200000
+
+ .thumb
+ .section .text.6, "ax", %progbits
+ .balign 0x1000
+ .globl farthumbfunc
+ .type farthumbfunc,%function
+farthumbfunc:
+ bx lr

lld/trunk/test/ELF/arm-blx-v4t.s

+// RUN: llvm-mc -filetype=obj -triple=arm-none-linux-gnueabi %s -o %t
+// RUN: ld.lld %t -o %t2 2>&1 | FileCheck %s
+// REQUIRES: arm
+
+// On Arm v4t there is no blx instruction so all interworking must go via
+// a thunk. At present we don't support v4t so we give a warning for unsupported
+// features.
+
+// CHECK: warning: lld uses blx instruction, no object with architecture supporting feature detected.
+// CHECK-NEXT: warning: lld uses extended branch encoding, no object with architecture supporting feature detected.
+// CHECK-NEXT: warning: lld may use movt/movw, no object with architecture supporting feature detected.
+
+ .text
+ .syntax unified
+ .cpu   arm7tdmi
+ .eabi_attribute        6, 2    @ Tag_CPU_arch
+ .arm
+ .globl _start
+ .type   _start,%function
+ .p2align       2
+_start:
+  bl thumbfunc
+  bx lr
+
+ .thumb
+ .section .text.2, "ax", %progbits
+ .globl thumbfunc
+ .type thumbfunc,%function
+thumbfunc:
+ bx lr