[ELFAttributeParser] Skip unknown vendor subsections.

[simon_tatham]

Summary

An .ARM.attributes section is divided into subsections, each labelled
with a vendor name. There is one standardised vendor name, which must
be used for all attributes that affect compatibility. Subsections
labelled with other vendor names can be used for optimisation
purposes, but it has to be safe for an object file consumer to ignore
them if it doesn't recognise the vendor name.

LLD currently terminates parsing of the whole attributes section as
soon as it encounters a subsection with a vendor name it doesn't
recognise (which is anything other than the standard one). This can
prevent it from detecting compatibility issues, if a standard
subsection followed the vendor-specific one.

This patch modifies the attribute parser so that unrecognised vendor
subsections are silently skipped, and the subsections beyond them are
still processed.

[simon_tatham]

@HsiangKai, @zixuan-wu, @jozefl: I've included you as reviewers because you each added a use of ELFAttributeParser for an architecture other than Arm. Are your architectures' use of attributes OK with this change, or will it need to be conditionalized so that it only applies to the Arm case?

[peter.smith]

This looks correct to me. You may be able to reproduce the same behaviour with a more minimal yaml file.

A link to the relevant part of the Arm specification: https://github.com/ARM-software/abi-aa/blob/main/addenda32/addenda32.rst#325conformance-constraints . The compatibility is meant to be handled by https://github.com/ARM-software/abi-aa/blob/main/addenda32/addenda32.rst#3372generic-compatibility-tag

There is a list of registered vendors at https://github.com/ARM-software/abi-aa/blob/main/aaelf32/aaelf32.rst#registered-vendor-names but I think on the general principle that the ABI doesn't require compatibility checks for this section then it isn't worth checking that the vendor be part of the registered list.

lld/test/ELF/Inputs/arm-vfp-arg-vfp-vendor.yaml

24 ↗

(On Diff #532885)

May be able to remove the .text section and have no effect on the test.

39 ↗

(On Diff #532885)

Could probably get away with removing this part.

[simon_tatham]

Deleted unnecessary parts from the YAML input file (and, while I'm there, added a comment naming the test it's used by).

I think on the general principle that the ABI doesn't require compatibility checks for this section then it isn't worth checking that the vendor be part of the registered list.

Not to mention that it would require constant LLVM updates whenever the registered list changes!

[MaskRay]

Code looks good to me. For the test, GNU readelf gives a warning.

% readelf -A avfpvendor.o
Attribute Section: ShouldBeIgnored
readelf: Error: Bad subsection length (4294967295 > 12)
Unknown tag: 255
  Unknown attribute:
  0x00000000 ffffffff ffffff                     .......

Can we use llvm-mc -filetype=obj to construct the test? It will likely be more readable than the yaml file. Then just implement it as an extra file with split-file as well.

.section .ARM.attributes,""
...    // directives with comments
...

lld/test/ELF/Inputs/arm-vfp-arg-vfp-vendor.yaml

35 ↗

(On Diff #532943)

... is optional and can be removed.

[simon_tatham]

Rewritten the test to use llvm-mc instead of yaml2obj.

[simon_tatham]

In D153335#4446671, @MaskRay wrote:

For the test, GNU readelf gives a warning.

That seems like a bug in GNU readelf to me! It looks as if GNU readelf is expecting every subsection of .ARM.attributes to have the same structure as the standardized "aeabi" section: it's complaining here that my custom subsection with vendor="ShouldBeIgnored" is not divided into internal sub-subsections indicating file, section or symbol scope, but instead is full of nothing but FFFFFFFF padding.

But ADDENDA32 seems very clear on this: that internal structure is specific to "aeabi", and other vendors can define whatever format they like inside their own subsections. Of course some vendor subsections might choose to copy the "aeabi" internal structure, but they don't have to.

Can we use llvm-mc -filetype=obj to construct the test? It will likely be more readable than the yaml file.

I'd guessed that MC might try to write its own attributes section and fight with a manually specified one, but in fact, yes, that works fine and I agree it's more readable.

Then just implement it as an extra file with split-file as well.

I'm not sure about that. This particular test keeps its existing three sub-files in lld/test/ELF/Inputs. It would seem strange to use split-file for just this one extra one. Or do you mean you'd like me to reorganize the existing parts of the test while I'm here?

[MaskRay]

In D153335#4448302, @simon_tatham wrote:

In D153335#4446671, @MaskRay wrote:

Then just implement it as an extra file with split-file as well.

I'm not sure about that. This particular test keeps its existing three sub-files in lld/test/ELF/Inputs. It would seem strange to use split-file for just this one extra one. Or do you mean you'd like me to reorganize the existing parts of the test while I'm here?

The new file can

The existing llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %s -o %t.o can be changed to a part of split-file.
Inputs/arm-vfp-arg-vfp-vendor.s can be added as the second part.
The existing 3 Inputs/arm-* files are also used by arm-tag-vfp-args-errs.s. We don't need to convert them.

[simon_tatham]

In D153335#4449486, @MaskRay wrote:

The existing 3 Inputs/arm-* files are also used by arm-tag-vfp-args-errs.s. We don't need to convert them.

Ah, that's the part I hadn't spotted. Thanks; I've made your suggested change in the final version of the patch.

[hokein]

We seem to miss updating the unittest ELFAttributeParserTest.cpp, I landed a fix https://github.com/llvm/llvm-project/commit/975f71faa72aaaaf9c82b93a32bd428503722bbf.

lld/test/ELF/arm-tag-vfp-args-errs.s

 // REQUIRES:arm
-// RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %S/Inputs/arm-vfp-arg-base.s -o %tbase.o
-// RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %S/Inputs/arm-vfp-arg-vfp.s -o %tvfp.o
-// RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %S/Inputs/arm-vfp-arg-toolchain.s -o %ttoolchain.o
-// RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %s -o %t.o
-// RUN: not ld.lld %t.o %tbase.o %tvfp.o -o%t 2>&1 | FileCheck %s
-// RUN: not ld.lld %t.o %tbase.o %ttoolchain.o -o%t 2>&1 | FileCheck %s
-// RUN: not ld.lld %t.o %tvfp.o %tbase.o -o%t 2>&1 | FileCheck %s
-// RUN: not ld.lld %t.o %tvfp.o %ttoolchain.o -o%t 2>&1 | FileCheck %s
-// RUN: not ld.lld %t.o %ttoolchain.o %tbase.o -o%t 2>&1 | FileCheck %s
-// RUN: not ld.lld %t.o %ttoolchain.o %tvfp.o -o%t 2>&1 | FileCheck %s
+
+// RUN: rm -rf %t
+// RUN: split-file %s %t
+
+// RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %S/Inputs/arm-vfp-arg-base.s -o %t/base.o
+// RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %S/Inputs/arm-vfp-arg-vfp.s -o %t/vfp.o
+// RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %S/Inputs/arm-vfp-arg-toolchain.s -o %t/toolchain.o
+// RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %t/main.s -o %t/main.o
+// RUN: llvm-mc -filetype=obj -triple=armv7a-none-linux-gnueabi %t/vendor.s -o %t/vendor.o
+// RUN: not ld.lld %t/main.o %t/base.o %t/vfp.o -o%t/a.out 2>&1 | FileCheck %s
+// RUN: not ld.lld %t/main.o %t/base.o %t/vendor.o -o%t/a.out 2>&1 | FileCheck %s
+// RUN: not ld.lld %t/main.o %t/base.o %t/toolchain.o -o%t/a.out 2>&1 | FileCheck %s
+// RUN: not ld.lld %t/main.o %t/vfp.o %t/base.o -o%t/a.out 2>&1 | FileCheck %s
+// RUN: not ld.lld %t/main.o %t/vfp.o %t/toolchain.o -o%t/a.out 2>&1 | FileCheck %s
+// RUN: not ld.lld %t/main.o %t/toolchain.o %t/base.o -o%t/a.out 2>&1 | FileCheck %s
+// RUN: not ld.lld %t/main.o %t/toolchain.o %t/vfp.o -o%t/a.out 2>&1 | FileCheck %s
 
 // CHECK: incompatible Tag_ABI_VFP_args
+
+//--- main.s
+
   .arch armv7-a
   .eabi_attribute 20, 1
   .eabi_attribute 21, 1
   .eabi_attribute 23, 3
   .eabi_attribute 24, 1
   .eabi_attribute 25, 1
   .eabi_attribute 26, 2
   .eabi_attribute 30, 6
   .eabi_attribute 34, 1
   .eabi_attribute 18, 4
         .eabi_attribute 28, 3 // Tag_ABI_VFP_args = 3 (Compatible with all)
 
         .syntax unified
         .globl _start
         .type _start, %function
 _start: bx lr
+
+//--- vendor.s
+
+        .syntax unified
+
+        // Manually construct a custom .ARM.attributes section
+        .section .ARM.attributes,"",%0x70000003 // SHT_ARM_ATTRIBUTES
+
+        // Initial byte giving the section format version
+        .byte 'A'
+
+        // Subsection with a name that won't be recognised as a known vendor
+vendor_subsect_start:
+        .word vendor_subsect_end - vendor_subsect_start // subsection length
+        .asciz "ShouldBeIgnored" // vendor name
+        .dcb.b 64, 0xff // dummy vendor section contents
+vendor_subsect_end:
+
+        // Subsection that should be identical to the attributes defined by
+        // Inputs/arm-vfp-arg-vfp.s
+aeabi_subsect_start:
+        .word aeabi_subsect_end - aeabi_subsect_start
+        .asciz "aeabi" // vendor name indicating the standard subsection
+file_subsubsect_start:
+        .byte 1 // introduce sub-subsection of attributes for the whole file
+        .word file_subsubsect_end - file_subsubsect_start // sub-subsection len
+        .byte 5 // CPU_name
+        .asciz "7-A"
+        .byte 6, 10 // CPU_arch = ARM v7
+        .byte 7, 'A' // CPU_arch_profile = Application
+        .byte 8, 1 // ARM_ISA_use = Permitted
+        .byte 9, 2 // THUMB_ISA_use = Thumb-2
+        .byte 18, 4 // ABI_PCS_wchar_t = 4-byte
+        .byte 20, 1 // ABI_FP_denormal = IEEE-754
+        .byte 21, 1 // ABI_FP_exceptions = IEEE-754
+        .byte 23, 3 // ABI_FP_number_model = IEEE-754
+        .byte 24, 1 // ABI_align_needed = 8-byte alignment
+        .byte 25, 1 // ABI_align_preserved = 8-byte data alignment
+        .byte 26, 2 // ABI_enum_size = Int32
+        .byte 28, 1 // ABI_VFP_args = AAPCS VFP
+        .byte 30, 6 // ABI_optimization_goals = Best Debugging
+        .byte 34, 1 // CPU_unaligned_access = v6-style
+file_subsubsect_end:
+aeabi_subsect_end:
+
+        .text
+        .global f1
+        .type f1, %function
+f1:     bx lr

llvm/lib/Support/ELFAttributeParser.cpp

 Error ELFAttributeParser::parseSubsection(uint32_t length) {
   uint64_t end = cursor.tell() - sizeof(length) + length;
   StringRef vendorName = de.getCStrRef(cursor);
   if (sw) {
     sw->printNumber("SectionLength", length);
     sw->printString("Vendor", vendorName);
   }
 
-  // Ignore unrecognized vendor-name.
-  if (vendorName.lower() != vendor)
-    return createStringError(errc::invalid_argument,
-                             "unrecognized vendor-name: " + vendorName);
+  // Handle a subsection with an unrecognized vendor-name by skipping
+  // over it to the next subsection. ADDENDA32 in the Arm ABI defines
+  // that vendor attribute sections must not affect compatibility, so
+  // this should always be safe.
+  if (vendorName.lower() != vendor) {
+    cursor.seek(end);
+    return Error::success();
+  }
 
   while (cursor.tell() < end) {
     /// Tag_File | Tag_Section | Tag_Symbol   uleb128:byte-size
     uint8_t tag = de.getU8(cursor);
     uint32_t size = de.getU32(cursor);
     if (!cursor)
       return cursor.takeError();