[ELF][RISCV] Merge `.riscv.attributes` sections

[eklepilkina]

Summary

Now .riscv.attributes section from the first file is taken and placed to result elf file. This can cause problems with using GNU tools, e.g. GNU objdump looks at this section to get extensions and may fail to decode instructions from c extension if there is no c mentioned in riscv.attributes section.

[eklepilkina]

Gently ping

[kito-cheng]

lld/ELF/SyntheticSections.cpp

3758

I think this assertion should be removed or use error instead, but I realized one fact soon is: we'll check the version during RISCVISAInfo::parseArchString and it will error out if the version is unsupported, so the assertion should always true for this moment.

Anyway I am OK with that for now, but I would prefer that let @MaskRay to make the final call for that.

Could you add a TODO: to the comment, like TODO: Versions should be the same otherwise parser...

3781

This should just error if the stack alignment is different, see psABI for that https://github.com/riscv-non-isa/riscv-elf-psabi-doc/blob/master/riscv-elf.adoc#tag_riscv_stack_align-4-uleb128value

[eklepilkina]

• Fix merge rules for attributes based on documentation

[eklepilkina]

lld/ELF/SyntheticSections.cpp

3758

Yes, it's true I can remove completely.

3781

Thank you for link to documentation with merge rules. Fix merging attributes with these rules.

[MaskRay]

Can you add a link to the official ABI and the relevant chapter?

riscv.attributes sections

Change these to .riscv.attributes in the subject and description and comments.

lld/ELF/SyntheticSections.cpp

3689

The extra memcpy is wasteful.

3702

delete the blank line

3705

drop braces

3768

Does make_unique work?

3772

drop braces around single line simple statements.

3846

drop blank line.

3866

drop blank line.

[eklepilkina]

• Review fixes

[eklepilkina]

@MaskRay thank you for review. I fixed all comments, could you have a look again?

[eklepilkina]

Gently ping

[MaskRay]

Sorry for the delay. I think this still requires some changes.

lld/ELF/Driver.cpp

2821

"Combine RISCV attributes." and this comment have the same meaning. Just place the comment before if.

This block does not belong to generic code. Move it to Arch/RISCV.cpp

2826

This comment does not say beyond what the code explains. Remove it.

2832

ctx.inputSections[0] being an attribute section is possible, so the code should handle the case, instead of using an assert which would lead to a runtime crash (either assert or attributesSectionPlace).

lld/ELF/InputFiles.cpp

605

no braces around single line simple statements.

607

avoid the used-once variable

609

errorOrWarn so that users can ignore the error with --noinhibit-exec.

lld/ELF/SyntheticSections.cpp

3684

Delete comments when the function name is self-explanatory.

There are many places where a redundant comment is added. Please drop.

3705

not done

lld/ELF/SyntheticSections.h

1197

The comment is not useful.

1198

This variable can be a const. constexpr

1309

This must be reset in reset()

lld/test/ELF/riscv-merge-attributes.s

40

no trailing blank line

[MaskRay]

[ELF][RISCV] Merge riscv.attributes sections from all input files

.riscv.attributes

You may drop from all input files since it is not precise: we only merge sections from relocatable object files, e.g. not from shared objects.

Ideally, add a lld/test/ELF/lto/ test that an attribute section from a bitcode file (compiled by compileBitcodeFiles) is handled as well.

[eklepilkina]

• Review fixes
• Added test for lto case

[eklepilkina]

lld/ELF/Driver.cpp

2821

Replaced to copy_if, this comment was connected with creating container with all riscv attributes sections.

[MaskRay]

Some variable names are overly longer and are unlike the rest of lld/ELF.

lld/ELF/Arch/RISCV.cpp

824 ↗

(On Diff #476867)

The name is verbose and isn't like other lld code. Just sections is fine since we are in the context of combineRISCVAttributesSections

826 ↗

(On Diff #476867)

This with erase_if can be simplified as: llvm::erase_if(ctx.inputSections, [&](...) { if (!riscvattribute) return false; sections.push_back(...); return true; })

828 ↗

(On Diff #476867)

early return is more common

834 ↗

(On Diff #476867)

The name is verbose. Just place is fine.

lld/ELF/Driver.cpp

2818

drop braces

lld/ELF/SyntheticSections.cpp

3691

parser

3695

Just avoid the variable? It isn't much shorter than currentTag.attr.

You can use tag instead of currentTag

3721

Prefer lower-case diagnostics. See the coding standard about error messages.

3736

no trailing dot. avoid std::to_string. error(Twine(some_integer) + "...")

[eklepilkina]

• Review fixes

[MaskRay]

Thank you for the patch and sorry for my previous belated response. I took a careful look today. I think a very large architectural refactoring is needed.
We probably want to use an armExidx style to merge sections.
Merging attributes as we see an input section is probably more convenient than the current approach maintaining two DenseMap with DenseSet as values (minor size concern) and iterating over all possible values.
I'll try an alternative implementation.
(I'll be out of town for about 1 week starting from Thursday, so sorry for my possible belated response. Hopefully I can get a draft out before that.)

lld/ELF/Driver.cpp

2815

Prefer "RISC-V" in comments.

lld/ELF/InputFiles.cpp

597

C++17 allows if (Optional<StringRef> Attr = ...)

lld/ELF/SyntheticSections.cpp

3684

I'd use parseRISCVAttributesSections. "parse" implies input (we don't parse output).

3695

if (Optional<unsigned> value = ...)

3712

arch

3805

Delete the assert. The call site has an emptiness check and the next line accesses [0].

[MaskRay]

lld/ELF/InputFiles.cpp

596

config->eflags hasn't been computed at this point, so everything in this body won't fire.
I am rewriting this a la armExidx.

[eklepilkina]

• Review fixes

[MaskRay]

Alternative with more tests: D138550

[eklepilkina]

Abandoned in favor of D138550

lld/ELF/Driver.cpp

void LinkerDriver::link(opt::InputArgList &args) {
   invokeELFT(createSyntheticSections);
 
   // Some input sections that are used for exception handling need to be moved
   // into synthetic sections. Do that now so that they aren't assigned to
   // output sections in the usual way.
   if (!config->relocatable)
     combineEhSections();
 
+  // Combine RISCV attributes.
+  if (config->emachine == EM_RISCV) {
+    auto isRISCVAttributesSection = [](InputSectionBase *s) {
+      return s->type == SHT_RISCV_ATTRIBUTES;
+    };
+
+    // Collect all `riscv.attributes` sections.
+    SmallVector<InputSectionBase *, 0> attributesSections;
+    llvm::copy_if(ctx.inputSections, std::back_inserter(attributesSections),
+                  isRISCVAttributesSection);
+    if (!attributesSections.empty()) {
+      // Need to merge different attributes.
+      InputSectionBase *resultAttributesSection =
+          RISCVMergedAttributeSection::createRISCVMergedAttributesSection(
+              std::move(attributesSections));
+
+      // Replace all existing attributes sections with merged one.
+      assert(!ctx.inputSections.empty() &&
+             !isRISCVAttributesSection(ctx.inputSections[0]) &&
+             "`riscv.attributes` section is expected not to be the first one.");
+      auto attributesSectionPlace =
+          llvm::find_if(ctx.inputSections, isRISCVAttributesSection) - 1;
+      llvm::erase_if(ctx.inputSections, isRISCVAttributesSection);
+      ctx.inputSections.insert(std::next(attributesSectionPlace),
+                           resultAttributesSection);
+    }
+  }
+
   {
     llvm::TimeTraceScope timeScope("Assign sections");
 
     // Create output sections described by SECTIONS commands.
     script->processSectionCommands();
 
     // Linker scripts control how input sections are assigned to output
     // sections. Input sections that were not handled by scripts are called

lld/ELF/InputFiles.cpp

 #include "llvm/LTO/LTO.h"
 #include "llvm/Object/IRObjectFile.h"
 #include "llvm/Support/ARMAttributeParser.h"
 #include "llvm/Support/ARMBuildAttributes.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/FileSystem.h"
 #include "llvm/Support/Path.h"
 #include "llvm/Support/RISCVAttributeParser.h"
+#include "llvm/Support/RISCVISAInfo.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;
if (sec.sh_type == SHT_ARM_ATTRIBUTES && config->emachine == EM_ARM) {
       }
     }
 
     if (sec.sh_type == SHT_RISCV_ATTRIBUTES && config->emachine == EM_RISCV) {
       RISCVAttributeParser attributes;
       ArrayRef<uint8_t> contents =
           check(this->getObj().getSectionContents(sec));
       StringRef name = check(obj.getSectionName(sec, shstrtab));
-      this->sections[i] = &InputSection::discarded;
       if (Error e = attributes.parse(contents, support::little)) {
+        this->sections[i] = &InputSection::discarded;
         InputSection isec(*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 = std::make_unique<InputSection>(*this, sec, name);
-          this->sections[i] = in.attributes.get();
+        // Validate arch tag contains C if and only if EF_RISCV_RVC is present.
+        if (config->eflags & EF_RISCV_RVC) {
+          Optional<StringRef> Attr =
+              attributes.getAttributeString(RISCVAttrs::ARCH);
+          if (Attr) {
+            StringRef arch = *Attr;
+            auto parseResult = llvm::RISCVISAInfo::parseArchString(
+                arch, /*EnableExperimentalExtension=*/true,
+                /*ExperimentalExtensionVersionCheck=*/true);
+            if (!parseResult) {
+              error(std::string("Invalid arch name: ") + arch.str());
+            }
+            auto &isaInfo = *parseResult;
+            if (!isaInfo->hasExtension("c")) {
+              error(toString(this) +
+                    " has set set RISCV_RVC flag, but RISCV_ARCH tag from " +
+                    name.str() + " section doesn't contain C extension");
+            }
+          }
         }
+        this->sections[i] = makeThreadLocal<InputSection>(*this, sec, name);
       }
     }
 
     if (sec.sh_type != SHT_GROUP)
       continue;
     StringRef signature = getShtGroupSignature(objSections, sec);
     ArrayRef<Elf_Word> entries =
         CHECK(obj.template getSectionContentsAsArray<Elf_Word>(sec), this);

lld/ELF/SyntheticSections.h

 
 #ifndef LLD_ELF_SYNTHETIC_SECTIONS_H
 #define LLD_ELF_SYNTHETIC_SECTIONS_H
 
 #include "Config.h"
 #include "InputSection.h"
 #include "llvm/ADT/DenseSet.h"
 #include "llvm/ADT/MapVector.h"
+#include "llvm/ADT/StringRef.h"
 #include "llvm/MC/StringTableBuilder.h"
 #include "llvm/Support/Compiler.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/Parallel.h"
 #include "llvm/Support/Threading.h"
 
 namespace lld::elf {
 class Defined;
public:
   void finalizeContents() override { finalized = true; }
 
 private:
   SmallVector<std::pair<const Symbol *, int64_t>, 0> entries;
   llvm::DenseMap<std::pair<const Symbol *, int64_t>, uint32_t> entry_index;
   bool finalized = false;
 };
 
+// This section is `riscv.attributes` section with merged attributes value
+// from all same sections from input files.
+class RISCVMergedAttributeSection final : public SyntheticSection {
+public:
+  static InputSectionBase *createRISCVMergedAttributesSection(
+      llvm::SmallVector<InputSectionBase *, 0> &&attributesSections);
+  size_t getSize() const override;
+  void writeTo(uint8_t *buf) override;
+
+private:
+  const size_t tagHeaderSize = 1 + 4; // Tag + Tag Size
+  // Size of subsection with attributes values content.
+  size_t contentsSize = 0;
+  // Map with merged numerical attributes.
+  llvm::DenseMap<unsigned, unsigned> mergedNumericalAttributes;
+  // Map with merged string attributes.
+  llvm::DenseMap<unsigned, std::string> mergedStringAttributes;
+  // Current vendor.
+  llvm::StringRef currentVendor = "riscv";
+
+  RISCVMergedAttributeSection(uint64_t flags, uint32_t type,
+                              uint32_t alignment);
+  void calculateContentsSize();
+  void mergeAttributes(
+      llvm::DenseMap<unsigned, llvm::DenseSet<unsigned>> &attributes,
+      llvm::DenseMap<unsigned, llvm::DenseSet<llvm::StringRef>> &attributesStr);
+  std::string
+  mergeArchExtensions(const llvm::DenseSet<llvm::StringRef> &arches);
+  size_t writeULEB128IntValue(uint8_t *buf, uint64_t value);
+};
+
 template <typename ELFT>
 class PartitionElfHeaderSection : public SyntheticSection {
 public:
   PartitionElfHeaderSection();
   size_t getSize() const override;
   void writeTo(uint8_t *buf) override;
 };
 
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 {
   std::unique_ptr<InputSection> attributes;
+  std::unique_ptr<RISCVMergedAttributeSection> riscvAttributes;
   std::unique_ptr<BssSection> bss;
   std::unique_ptr<BssSection> bssRelRo;
   std::unique_ptr<GotSection> got;
   std::unique_ptr<GotPltSection> gotPlt;
   std::unique_ptr<IgotPltSection> igotPlt;
   std::unique_ptr<PPC64LongBranchTargetSection> ppc64LongBranchTarget;
   std::unique_ptr<SyntheticSection> mipsAbiFlags;
   std::unique_ptr<MipsGotSection> mipsGot;

lld/ELF/SyntheticSections.cpp

 #include "lld/Common/Strings.h"
 #include "lld/Common/Version.h"
 #include "llvm/ADT/STLExtras.h"
 #include "llvm/ADT/SetOperations.h"
 #include "llvm/ADT/StringExtras.h"
 #include "llvm/BinaryFormat/Dwarf.h"
 #include "llvm/BinaryFormat/ELF.h"
 #include "llvm/DebugInfo/DWARF/DWARFDebugPubTable.h"
+#include "llvm/Support/ELFAttributes.h"
 #include "llvm/Support/Endian.h"
 #include "llvm/Support/LEB128.h"
 #include "llvm/Support/Parallel.h"
+#include "llvm/Support/RISCVAttributeParser.h"
+#include "llvm/Support/RISCVAttributes.h"
+#include "llvm/Support/RISCVISAInfo.h"
 #include "llvm/Support/TimeProfiler.h"
 #include <cstdlib>
 
 using namespace llvm;
 using namespace llvm::dwarf;
 using namespace llvm::ELF;
 using namespace llvm::object;
 using namespace llvm::support;
bool PPC64LongBranchTargetSection::isNeeded() const {
   // is too early to determine if this section will be empty or not. We need
   // Finalized to keep the section alive until after thunk creation. Finalized
   // only gets set to true once `finalizeSections()` is called after thunk
   // creation. Because of this, if we don't create any long-branch thunks we end
   // up with an empty .branch_lt section in the binary.
   return !finalized || !entries.empty();
 }
 
+size_t RISCVMergedAttributeSection::writeULEB128IntValue(uint8_t *buf,
+                                                         uint64_t value) {
+  SmallString<128> tmp;
+  raw_svector_ostream OSE(tmp);
+  encodeULEB128(value, OSE);
+  memcpy(buf, OSE.str().data(), OSE.str().size());
+  return OSE.str().size();
+}
+
+// Parse `riscv.attributes` sections from input files.
+static void parseInputRISCVAttributesSections(
+    const SmallVector<InputSectionBase *, 0> &attributesSections,
+    DenseMap<unsigned, DenseSet<unsigned>> &attributes,
+    DenseMap<unsigned, DenseSet<StringRef>> &attributesStr) {
+  // Collect all tags values from attributes section.
+  auto attributesTags = RISCVAttrs::getRISCVAttributeTags();
+  for (const InputSectionBase *s : attributesSections) {
+    RISCVAttributeParser attributesParser;
+
+    if (Error e = attributesParser.parse(s->data(), support::little)) {
+      error(toString(s) + ": " + llvm::toString(std::move(e)));
+    }
+    for (const auto &currentTag : attributesTags) {
+      unsigned currentAttribute = currentTag.attr;
+      Optional<unsigned> value =
+          attributesParser.getAttributeValue(currentAttribute);
+      if (value) {
+        attributes[currentAttribute].insert(*value);
+      } else {
+        Optional<StringRef> stringValue =
+            attributesParser.getAttributeString(currentAttribute);
+        if (stringValue) {
+          attributesStr[currentAttribute].insert(*stringValue);
+        }
+      }
+    }
+  }
+}
+
+// Parse ARCH attributes value and merge all extensions from them.
+std::string RISCVMergedAttributeSection::mergeArchExtensions(
+    const DenseSet<StringRef> &arches) {
+  RISCVISAInfo::OrderedExtensionMap resultExtensions;
+  unsigned resultXlen;
+  for (const StringRef archValue : arches) {
+    // Parse arch string.
+    auto parseResult = llvm::RISCVISAInfo::parseArchString(
+        archValue, /*EnableExperimentalExtension=*/true,
+        /*ExperimentalExtensionVersionCheck=*/true);
+    if (!parseResult) {
+      error(std::string("Invalid arch name: ") + archValue.str() + " " +
+            llvm::toString(parseResult.takeError()));
+    }
+
+    // Merge extensions.
+    auto &isaInfo = *parseResult;
+    auto currentExtensions = isaInfo->getExtensions();
+    if (resultExtensions.empty()) {
+      resultExtensions = currentExtensions;
+      resultXlen = isaInfo->getXLen();
+    } else {
+      if (resultXlen != isaInfo->getXLen()) {
+        error(archValue.str() +
+              " arch isn't compatible with archs from other object files. "
+              "Expected rv" +
+              std::to_string(resultXlen) + " configuration.");
+      }
+      for (auto const &extension : currentExtensions) {
+        if (resultExtensions.count(extension.first) != 0) {
+          // TODO: Versions should be the same otherwise parser should report
+          // unsupported version error.
+          assert(resultExtensions[extension.first].MajorVersion ==
+                     extension.second.MajorVersion &&
+                 resultExtensions[extension.first].MinorVersion ==
+                     extension.second.MinorVersion);
+          continue;
+        }
+        // Add extension.
+        resultExtensions[extension.first] = extension.second;
+      }
+    }
+  }
+  // Check result set of merged extensions.
+  std::unique_ptr<RISCVISAInfo> isaInfo(
+      new RISCVISAInfo(resultXlen, resultExtensions));
+  auto checkResult = RISCVISAInfo::postProcessAndChecking(std::move(isaInfo));
+  if (!checkResult) {
+    error(llvm::toString(checkResult.takeError()));
+  }
+  auto &resultInfo = *checkResult;
+  return resultInfo->toString();
+}
+
+// Merge attributes described in `riscv.attributes` section.
+void RISCVMergedAttributeSection::mergeAttributes(
+    DenseMap<unsigned, DenseSet<unsigned>> &attributes,
+    DenseMap<unsigned, DenseSet<StringRef>> &attributesStr) {
+  auto attributesTags = RISCVAttrs::getRISCVAttributeTags();
+  for (const auto &tag : attributesTags) {
+    switch (tag.attr) {
+    case RISCVAttrs::UNALIGNED_ACCESS:
+      if (attributes[RISCVAttrs::UNALIGNED_ACCESS].size() == 1) {
+        mergedNumericalAttributes[RISCVAttrs::UNALIGNED_ACCESS] =
+            *attributes[RISCVAttrs::UNALIGNED_ACCESS].begin();
+      } else if (attributes[RISCVAttrs::UNALIGNED_ACCESS].size() > 1) {
+        mergedNumericalAttributes[RISCVAttrs::UNALIGNED_ACCESS] =
+            RISCVAttrs::ALLOWED;
+      }
+      break;
+    case RISCVAttrs::ARCH:
+      if (attributesStr[RISCVAttrs::ARCH].size() > 1) {
+        std::string mergedAttribute =
+            mergeArchExtensions(attributesStr[RISCVAttrs::ARCH]);
+        mergedStringAttributes[RISCVAttrs::ARCH] = mergedAttribute;
+      } else if (!attributesStr[RISCVAttrs::ARCH].empty()) {
+        mergedStringAttributes[RISCVAttrs::ARCH] =
+            attributesStr[RISCVAttrs::ARCH].begin()->str();
+      }
+      break;
+    default:
+      if (attributes[tag.attr].size() > 1 ||
+          attributesStr[tag.attr].size() > 1) {
+        error("Input files contain different values for " + tag.tagName.str() +
+              " attribute.");
+      }
+      if (attributes[tag.attr].size() == 1) {
+        mergedNumericalAttributes[tag.attr] = *attributes[tag.attr].begin();
+      }
+      if (attributesStr[tag.attr].size() == 1) {
+        mergedStringAttributes[tag.attr] =
+            attributesStr[tag.attr].begin()->str();
+      }
+    }
+  }
+}
+
+// Create `riscv.attributes` section with attributes values merged from all
+// input files.
+InputSectionBase *
+RISCVMergedAttributeSection::createRISCVMergedAttributesSection(
+    SmallVector<InputSectionBase *, 0> &&attributesSections) {
+  assert(!attributesSections.empty() &&
+         "No `riscv.attributes` sections to merge");
+  const InputSectionBase *existingSection = attributesSections[0];
+  in.riscvAttributes = std::unique_ptr<RISCVMergedAttributeSection>(
+      new RISCVMergedAttributeSection(existingSection->flags,
+                                      existingSection->type,
+                                      existingSection->alignment));
+
+  DenseMap<unsigned, DenseSet<unsigned>> attributes;
+  DenseMap<unsigned, DenseSet<StringRef>> attributesStr;
+  parseInputRISCVAttributesSections(attributesSections, attributes,
+                                    attributesStr);
+  in.riscvAttributes->mergeAttributes(attributes, attributesStr);
+  in.riscvAttributes->calculateContentsSize();
+  return in.riscvAttributes.get();
+}
+
+RISCVMergedAttributeSection::RISCVMergedAttributeSection(uint64_t flags,
+                                                         uint32_t type,
+                                                         uint32_t alignment)
+    : SyntheticSection(flags, type, alignment, ".riscv.attributes") {}
+
+// Calculate size of attributes values subsection of `riscv.attributes`.
+void RISCVMergedAttributeSection::calculateContentsSize() {
+  size_t calculatedSize = 0;
+  for (const auto &attribute : mergedNumericalAttributes) {
+    calculatedSize += getULEB128Size(attribute.first);
+    calculatedSize += getULEB128Size(attribute.second);
+  }
+
+  for (const auto &attribute : mergedStringAttributes) {
+    calculatedSize += getULEB128Size(attribute.first);
+    calculatedSize += attribute.second.size() + 1; // string + '\0'
+  }
+
+  contentsSize = calculatedSize;
+}
+
+size_t RISCVMergedAttributeSection::getSize() const {
+  // Vendor size + Vendor name + '\0'
+  const size_t vendorHeaderSize = 4 + currentVendor.size() + 1;
+
+  return vendorHeaderSize + tagHeaderSize + contentsSize + 1;
+}
+
+void RISCVMergedAttributeSection::writeTo(uint8_t *buf) {
+  endian::write(buf, ELFAttrs::Format_Version, config->endianness);
+  write32(buf + 1, getSize() - 1);
+  memcpy(buf + 5, currentVendor.data(), currentVendor.size());
+  endian::write(buf + currentVendor.size() + 5, 0, config->endianness); // '\0'
+  endian::write(buf + currentVendor.size() + 6, ELFAttrs::File,
+                config->endianness);
+  write32(buf + currentVendor.size() + 7, tagHeaderSize + contentsSize);
+
+  auto attributesTags = RISCVAttrs::getRISCVAttributeTags();
+  uint8_t *position = buf + currentVendor.size() + 11;
+
+  // Write attributes values.
+  for (auto tagItem : attributesTags) {
+    if (mergedNumericalAttributes.count(tagItem.attr)) {
+      position += writeULEB128IntValue(position, tagItem.attr);
+      position += writeULEB128IntValue(position,
+                                       mergedNumericalAttributes[tagItem.attr]);
+    }
+    if (mergedStringAttributes.count(tagItem.attr)) {
+      position += writeULEB128IntValue(position, tagItem.attr);
+      std::string stringValue = mergedStringAttributes[tagItem.attr];
+      memcpy(position, stringValue.data(), stringValue.size());
+      position += stringValue.size();
+      endian::write(position, 0, config->endianness); // '\0'
+      position++;
+    }
+  }
+}
+
 static uint8_t getAbiVersion() {
   // MIPS non-PIC executable gets ABI version 1.
   if (config->emachine == EM_MIPS) {
     if (!config->isPic && !config->relocatable &&
         (config->eflags & (EF_MIPS_PIC | EF_MIPS_CPIC)) == EF_MIPS_CPIC)
       return 1;
     return 0;
   }

lld/test/ELF/Inputs/riscv-attributes1.s

 // Input that generates an object with a populated SHT_RISCV_ATTRIBUTES section
-.attribute 4, 4
+.attribute 4, 16
 .attribute 5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0"

lld/test/ELF/Inputs/riscv-attributes2.s

 // Input that generates an object with a populated SHT_RISCV_ATTRIBUTES section
 .attribute 4, 16
 .attribute 5, "rv32i2p0_m2p0_f2p0_d2p0_zbb1p0"
+.attribute 6, 1

lld/test/ELF/riscv-merge-attributes.s

 # REQUIRES: riscv
 
 # RUN: llvm-mc -filetype=obj -triple=riscv64-unknown-elf -mattr=-relax %S/Inputs/riscv-attributes1.s -o %t1.o
 # RUN: llvm-mc -filetype=obj -triple=riscv64-unknown-elf -mattr=-relax %S/Inputs/riscv-attributes2.s -o %t2.o
 # RUN: llvm-mc -filetype=obj -triple=riscv64-unknown-elf -mattr=-relax %s -o %t3.o
 # RUN: ld.lld %t1.o %t2.o %t3.o -o %t
 # RUN: llvm-readelf --arch-specific %t | FileCheck %s
 
 # CHECK:      BuildAttributes {
 # CHECK-NEXT:   FormatVersion: 0x41
 # CHECK-NEXT:   Section 1 {
-# CHECK-NEXT:     SectionLength: 52
+# CHECK-NEXT:     SectionLength: 61
 # CHECK-NEXT:     Vendor: riscv
 # CHECK-NEXT:     Tag: Tag_File (0x1)
-# CHECK-NEXT:     Size: 42
+# CHECK-NEXT:     Size: 51
 # CHECK-NEXT:     FileAttributes {
 # CHECK-NEXT:       Attribute {
 # CHECK-NEXT:         Tag: 4
-# CHECK-NEXT:         Value: 4
+# CHECK-NEXT:         Value: 16
 # CHECK-NEXT:         TagName: stack_align
-# CHECK-NEXT:         Description: Stack alignment is 4-bytes
+# CHECK-NEXT:         Description: Stack alignment is 16-bytes
 # CHECK-NEXT:       }
 # CHECK-NEXT:       Attribute {
 # CHECK-NEXT:         Tag: 5
 # CHECK-NEXT:         TagName: arch
-# CHECK-NEXT:         Value: rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0
+# CHECK-NEXT:         Value: rv32i2p0_m2p0_a2p0_f2p0_d2p0_c2p0_zbb1p0
+# CHECK-NEXT:       }
+# CHECK-NEXT:       Attribute {
+# CHECK-NEXT:         Tag: 6
+# CHECK-NEXT:         Value: 1
+# CHECK-NEXT:         TagName: unaligned_access
+# CHECK-NEXT:         Description: Unaligned access
 # CHECK-NEXT:       }
 # CHECK-NEXT:     }
 # CHECK-NEXT:   }
 # CHECK-NEXT: }
 
 .attribute 4, 16
 .attribute 5, "rv32i2p0_m2p0_a2p0_f2p0_d2p0"
+

llvm/include/llvm/Support/RISCVISAInfo.h

struct ExtensionComparator {
     }
   };
 
   /// OrderedExtensionMap is std::map, it's specialized to keep entries
   /// in canonical order of extension.
   typedef std::map<std::string, RISCVExtensionInfo, ExtensionComparator>
       OrderedExtensionMap;
 
+  RISCVISAInfo(unsigned XLen, OrderedExtensionMap &Exts)
+      : XLen(XLen), FLen(0), MinVLen(0), MaxELen(0), MaxELenFp(0), Exts(Exts) {}
+
   /// Parse RISCV ISA info from arch string.
   static llvm::Expected<std::unique_ptr<RISCVISAInfo>>
   parseArchString(StringRef Arch, bool EnableExperimentalExtension,
                   bool ExperimentalExtensionVersionCheck = true);
 
   /// Parse RISCV ISA info from feature vector.
   static llvm::Expected<std::unique_ptr<RISCVISAInfo>>
   parseFeatures(unsigned XLen, const std::vector<std::string> &Features);
public:
   std::string toString() const;
   std::vector<std::string> toFeatureVector() const;
   StringRef computeDefaultABI() const;
 
   static bool isSupportedExtensionFeature(StringRef Ext);
   static bool isSupportedExtension(StringRef Ext);
   static bool isSupportedExtension(StringRef Ext, unsigned MajorVersion,
                                    unsigned MinorVersion);
+  static llvm::Expected<std::unique_ptr<RISCVISAInfo>>
+  postProcessAndChecking(std::unique_ptr<RISCVISAInfo> &&ISAInfo);
 
 private:
   RISCVISAInfo(unsigned XLen)
       : XLen(XLen), FLen(0), MinVLen(0), MaxELen(0), MaxELenFp(0) {}
 
   unsigned XLen;
   unsigned FLen;
   unsigned MinVLen;
   unsigned MaxELen, MaxELenFp;
 
   OrderedExtensionMap Exts;
 
   void addExtension(StringRef ExtName, unsigned MajorVersion,
                     unsigned MinorVersion);
 
   Error checkDependency();
 
   void updateImplication();
   void updateCombination();
   void updateFLen();
   void updateMinVLen();
   void updateMaxELen();
-
-  static llvm::Expected<std::unique_ptr<RISCVISAInfo>>
-  postProcessAndChecking(std::unique_ptr<RISCVISAInfo> &&ISAInfo);
 };
 
 } // namespace llvm
 
 #endif

llvm/lib/Support/RISCVISAInfo.cpp

return createStringError(
         "zvfh requires zfh, zfhmin, zhinx or zhinxmin extension to also be "
         "specified");
 
   if (HasZvl && !HasVector)
     return createStringError(
         errc::invalid_argument,
         "zvl*b requires v or zve* extension to also be specified");
 
+  if (HasF && HasZfinx)
+    return createStringError(
+        errc::invalid_argument,
+        "zfinx extension can't be specified along with f extension");
+
   // Additional dependency checks.
   // TODO: The 'q' extension requires rv64.
   // TODO: It is illegal to specify 'e' extensions with 'f' and 'd'.
 
   return Error::success();
 }
 
 static const char *ImpliedExtsV[] = {"zvl128b", "zve64d", "f", "d"};