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

[Peter Smith via Phabricator via llvm-commits]

peter.smith added a comment.

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 https://reviews.llvm.org/J1 = 1 https://reviews.llvm.org/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


https://reviews.llvm.org/D36823