[PATCH] D36823: [LLD][ELF] Read ARM BuildAttributes section to determine supported features.
Rafael Avila de Espindola via llvm-commits
llvm-commits at lists.llvm.org
Fri Nov 24 16:09:30 PST 2017
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
More information about the llvm-commits
mailing list