[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

25

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

40

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

... 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/Inputs/arm-vfp-arg-vfp-vendor.yaml

+# Variant form of the object compiled from arm-vfp-arg-vfp.s which
+# adds a vendor attribute subsection in front of the "aeabi" one. LLD
+# ought to skip over it and still parse the "aeabi" subsection beyond
+# it.
+#
+# Used by lld/test/ELF/arm-tag-vfp-args-errs.s .
+#
+# This file was generated by building arm-vfp-arg-vfp.s, running
+# yaml2obj over it, deleting parts not needed by the test (the .text
+# section and the symbols), and manually inserting the hex string
+# 2000000053686F756C64426549676E6F72656400FFFFFFFFFFFFFFFFFFFFFFFF
+# after the initial version byte of the attributes section, which
+# unpacks as a 32-bit length word, the zero-terminated vendor name
+# string "ShouldBeIgnored", and dummy data that's all FFs (since the
+# interior of a vendor subsection is not required to adhere to any
+# standard format).
+
+--- !ELF
+FileHeader:
+  Class:           ELFCLASS32
+  Data:            ELFDATA2LSB
+  Type:            ET_REL
+  Machine:         EM_ARM
+  Flags:           [ EF_ARM_EABI_VER5 ]
+  SectionHeaderStringTable: .strtab
+Sections:
+  - Name:            .ARM.attributes
+    Type:            SHT_ARM_ATTRIBUTES
+    AddressAlign:    0x1
+    Content:         412000000053686F756C64426549676E6F72656400FFFFFFFFFFFFFFFFFFFFFFFF30000000616561626900012600000005372D4100060A0741080109021204140115011703180119011A021C011E062201
+  - Type:            SectionHeaderTable
+    Sections:
+      - Name:            .strtab
+      - Name:            .ARM.attributes
+...

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: yaml2obj %S/Inputs/arm-vfp-arg-vfp-vendor.yaml -o %tvfpvendor.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 %tvfpvendor.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
 
 // CHECK: incompatible Tag_ABI_VFP_args
   .arch armv7-a

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();