[ELF] Handle SHT_RISCV_ATTRIBUTES similarly to SHT_ARM_ATTRIBUTES

[jrtc27]

Summary

Currently we treat SHT_RISCV_ATTRIBUTES like a normal section and
concatenate all such input sections, yielding invalid output unless only
a single attributes section is present in the input. Instead, pick the
first as with SHT_ARM_ATTRIBUTES. We do not currently need to condition
our behaviour on the contents, unlike Arm. In future, we should both do
stricter validation of the input and merge all sections together to
ensure we have, for example, the full arch string requirement, but this
rudimentary implementation is good enough for most common cases.

[jrtc27]

lld/ELF/InputFiles.cpp

871

NB: There is no default case for the switch statement so hoisting these out (required since SHT_ARM_ATTRIBUTES and SHT_RISCV_ATTRIBUTES use the same encoding in the arch-specific range) and thus losing the ability to break to the end of the switch shouldn't be a change of behaviour.

[jrtc27]

Fixed stray non-breaking spaces

[MaskRay]

lld/ELF/InputFiles.cpp

869

Keeping the switch may be fine.

case SHT_ARM_ATTRIBUTES:
  assert(SHT_ARM_ATTRIBUTES == SHT_RISCV_ATTRIBUTES);
  if (EM_ARM) {
  } else if (EM_RISCV) {
  } else
    break;

Adding assert(SHT_ARM_ATTRIBUTES == SHT_RISCV_ATTRIBUTES); should be sufficiently clear.

lld/test/ELF/riscv-attributes.s

6

Does llvm-readelf --arch-specific work?

[jrtc27]

Use llvm-readelf rather than llvm-readobj

They both have the same format for BuildAttributes, but llvm-readelf only emits
that whereas llvm-readobj emits additional information about the file we don't
care about.

[jrtc27]

lld/ELF/InputFiles.cpp

869

I considered that but it just feels too dirty... and what happens if another architecture allocates LOPROC+3 to a different kind of section? Then you have completely unrelated code under SHT_ARM_ATTRIBUTES.

[MaskRay]

LGTM.

lld/ELF/InputFiles.cpp

869

what happens if another architecture allocates LOPROC+3 to a different kind of section?

This is probably unlikely. The logic is a bit complex now. So moving it outside can reduce indentation, which seems fine.

lld/ELF/InputFiles.cpp

 #include "llvm/IR/Module.h"
 #include "llvm/LTO/LTO.h"
 #include "llvm/MC/StringTableBuilder.h"
 #include "llvm/Object/ELFObjectFile.h"
 #include "llvm/Support/ARMAttributeParser.h"
 #include "llvm/Support/ARMBuildAttributes.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/Path.h"
+#include "llvm/Support/RISCVAttributeParser.h"
 #include "llvm/Support/TarWriter.h"
 #include "llvm/Support/raw_ostream.h"
 
 using namespace llvm;
 using namespace llvm::ELF;
 using namespace llvm::object;
 using namespace llvm::sys;
 using namespace llvm::sys::fs;
static InputSection *toRegularSection(MergeInputSection *sec) {
   return make<InputSection>(sec->file, sec->flags, sec->type, sec->alignment,
                             sec->data(), sec->name);
 }
 
 template <class ELFT>
 InputSectionBase *ObjFile<ELFT>::createInputSection(const Elf_Shdr &sec) {
   StringRef name = getSectionName(sec);
 
-  switch (sec.sh_type) {
-  case SHT_ARM_ATTRIBUTES: {
-    if (config->emachine != EM_ARM)
-      break;
+  if (config->emachine == EM_ARM && sec.sh_type == SHT_ARM_ATTRIBUTES) {
     ARMAttributeParser attributes;
     ArrayRef<uint8_t> contents = check(this->getObj().getSectionContents(&sec));
     if (Error e = attributes.parse(contents, config->ekind == ELF32LEKind
                                                  ? support::little
                                                  : support::big)) {
       auto *isec = make<InputSection>(*this, sec, name);
       warn(toString(isec) + ": " + llvm::toString(std::move(e)));
-      break;
-    }
+    } else {
       updateSupportedARMFeatures(attributes);
       updateARMVFPArgs(attributes, this);
 
       // FIXME: 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.
-    if (in.armAttributes == nullptr) {
-      in.armAttributes = make<InputSection>(*this, sec, name);
-      return in.armAttributes;
+      // to work. In a full implementation we would merge all attribute
+      // sections.
+      if (in.attributes == nullptr) {
+        in.attributes = make<InputSection>(*this, sec, name);
+        return in.attributes;
+      }
+      return &InputSection::discarded;
+    }
+  }
+
+  if (config->emachine == EM_RISCV && sec.sh_type == SHT_RISCV_ATTRIBUTES) {
+    RISCVAttributeParser attributes;
+    ArrayRef<uint8_t> contents = check(this->getObj().getSectionContents(&sec));
+    if (Error e = attributes.parse(contents, support::little)) {
+      auto *isec = make<InputSection>(*this, sec, name);
+      warn(toString(isec) + ": " + llvm::toString(std::move(e)));
+    } else {
+      // FIXME: Validate arch tag contains C if and only if EF_RISCV_RVC is
+      // present.
+
+      // FIXME: Retain the first attribute section we see. Tools such as
+      // llvm-objdump make use of the attribute section to determine which
+      // standard extensions to enable. In a full implementation we would merge
+      // all attribute sections.
+      if (in.attributes == nullptr) {
+        in.attributes = make<InputSection>(*this, sec, name);
+        return in.attributes;
       }
       return &InputSection::discarded;
     }
+  }
+
+  switch (sec.sh_type) {
   case SHT_LLVM_DEPENDENT_LIBRARIES: {
     if (config->relocatable)
       break;
     ArrayRef<char> data =
         CHECK(this->getObj().template getSectionContentsAsArray<char>(&sec), this);
     if (!data.empty() && data.back() != '\0') {
       error(toString(this) +
             ": corrupted dependent libraries section (unterminated string): " +

lld/ELF/SyntheticSections.h

 inline Partition &SectionBase::getPartition() const {
   assert(isLive());
   return partitions[partition - 1];
 }
 
 // Linker generated sections which can be used as inputs and are not specific to
 // a partition.
 struct InStruct {
-  InputSection *armAttributes;
+  InputSection *attributes;
   BssSection *bss;
   BssSection *bssRelRo;
   GotSection *got;
   GotPltSection *gotPlt;
   IgotPltSection *igotPlt;
   PPC64LongBranchTargetSection *ppc64LongBranchTarget;
   MipsGotSection *mipsGot;
   MipsRldMapSection *mipsRldMap;

lld/test/ELF/riscv-attributes.s

+# REQUIRES: riscv
+
+# RUN: llvm-mc -filetype=obj -triple=riscv64-unknown-elf -mattr=-relax %s -o %t.o
+#!RUN: ld.lld %t.o -o %t
+# RUN: llvm-readobj --arch-specific %t | FileCheck %s
+#!RUN: ld.lld %t.o %t.o -o %t2
+# RUN: llvm-readobj --arch-specific %t2 | FileCheck %s
+
+#!CHECK:      BuildAttributes {
+#!CHECK-NEXT:   FormatVersion: 0x41
+#!CHECK-NEXT:   Section 1 {
+#!CHECK-NEXT:     SectionLength: 52
+#!CHECK-NEXT:     Vendor: riscv
+#!CHECK-NEXT:     Tag: Tag_File (0x1)
+#!CHECK-NEXT:     Size: 42
+#!CHECK-NEXT:     FileAttributes {
+#!CHECK-NEXT:       Attribute {
+#!CHECK-NEXT:         Tag: 4
+#!CHECK-NEXT:         Value: 16
+#!CHECK-NEXT:         TagName: stack_align
+#!CHECK-NEXT:         Description: Stack alignment is 16-bytes
+#!CHECK-NEXT:       }
+#!CHECK-NEXT:       Attribute {
+#!CHECK-NEXT:         Tag: 5
+#!CHECK-NEXT:         TagName: arch
+#!CHECK-NEXT:         Value: rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0
+#!CHECK-NEXT:       }
+#!CHECK-NEXT:     }
+#!CHECK-NEXT:   }
+#!CHECK-NEXT: }
+
+.attribute 4, 16
+.attribute 5, "rv64i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"