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

[Rafael Avila de Espindola via llvm-commits]

Peter Smith via Phabricator <reviews at reviews.llvm.org> writes:

> 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