[LLVM][AArch64] Build attributes: Support switching to a defined subsection by name only

[sivan-shani]

The AArch64 build attribute specification now allows switching to an already-defined subsection using its name alone, without repeating the optionality and type parameters.

This patch updates the parser to support that behavior.

Spec reference: https://github.com/ARM-software/abi-aa/pull/230/files

[llvmbot]

@llvm/pr-subscribers-backend-aarch64

Author: SivanShani-Arm (sivan-shani)

Changes

The AArch64 build attribute specification now allows switching to an already-defined subsection using its name alone, without repeating the optionality and type parameters.

This patch updates the parser to support that behavior.

Spec reference: https://github.com/ARM-software/abi-aa/pull/230/files

---

Full diff: https://github.com/llvm/llvm-project/pull/154159.diff

4 Files Affected:

• (modified) llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp (+24-4)
• (modified) llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.cpp (+13-6)
• (modified) llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.h (+1)
• (added) llvm/test/MC/AArch64/build-attributes-asm-aeabi-switch_using_name_only.s (+39)

diff --git a/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp b/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
index 1ca61f5c6b349..82d295d8d9c30 100644
--- a/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
+++ b/llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
@@ -7909,9 +7909,11 @@ bool AArch64AsmParser::parseDirectiveSEHSavePReg(SMLoc L) {
 }
 
 bool AArch64AsmParser::parseDirectiveAeabiSubSectionHeader(SMLoc L) {
-  // Expecting 3 AsmToken::Identifier after '.aeabi_subsection', a name and 2
-  // parameters, e.g.: .aeabi_subsection (1)aeabi_feature_and_bits, (2)optional,
-  // (3)uleb128 separated by 2 commas.
+  // Handle parsing of .aeabi_subsection directives
+  // - On first declaration of a subsection, expect exactly three identifiers
+  //   after `.aeabi_subsection`: the subsection name and two parameters.
+  // - When switching to an existing subsection, it is valid to provide only
+  //   the subsection name, or the name together with the two parameters.
   MCAsmParser &Parser = getParser();
 
   // Consume the name (subsection name)
@@ -7925,7 +7927,25 @@ bool AArch64AsmParser::parseDirectiveAeabiSubSectionHeader(SMLoc L) {
     return true;
   }
   Parser.Lex();
-  // consume a comma
+
+  // Check whether only the subsection name was provided.
+  // If so, the user is trying to switch to a subsection that should have been
+  // declared before.
+  if (Parser.getTok().is(llvm::AsmToken::EndOfStatement)) {
+    // If subsection does not exists, report error.
+    if (!getTargetStreamer().hasSubsection(SubsectionName)) {
+      Error(Parser.getTok().getLoc(),
+            "Could not switch to subsection '" + SubsectionName +
+                "' using subsection name, subsection has not been defined");
+      return true;
+    }
+    // Since subsection exists, no need to call
+    // AArch64TargetStreamer::emitAttributesSubsection(...)
+    getTargetStreamer().activateAttributesSubsection(SubsectionName);
+    return false;
+  }
+
+  // Otherwise, expecting 2 more parameters: consume a comma
   // parseComma() return *false* on success, and call Lex(), no need to call
   // Lex() again.
   if (Parser.parseComma()) {
diff --git a/llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.cpp b/llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.cpp
index d742b282b617c..6d504a8bce3ea 100644
--- a/llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.cpp
+++ b/llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.cpp
@@ -153,17 +153,24 @@ MCTargetStreamer *llvm::createAArch64NullTargetStreamer(MCStreamer &S) {
   return new AArch64TargetStreamer(S);
 }
 
+bool AArch64TargetStreamer::hasSubsection(StringRef SubSectionName) {
+  for (MCELFStreamer::AttributeSubSection &SubSection : AttributeSubSections) {
+    if (SubSection.VendorName == SubSectionName)
+      return true;
+  }
+  return false;
+}
+
 void AArch64TargetStreamer::emitAttributesSubsection(
     StringRef VendorName, AArch64BuildAttributes::SubsectionOptional IsOptional,
     AArch64BuildAttributes::SubsectionType ParameterType) {
 
-  // If exists, return.
-  for (MCELFStreamer::AttributeSubSection &SubSection : AttributeSubSections) {
-    if (VendorName == SubSection.VendorName) {
-      activateAttributesSubsection(VendorName);
-      return;
-    }
+  // If exists, activate and return.
+  if (hasSubsection(VendorName)) {
+    activateAttributesSubsection(VendorName);
+    return;
   }
+
   // else, add the subsection
   MCELFStreamer::AttributeSubSection AttSubSection;
   AttSubSection.VendorName = VendorName;
diff --git a/llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.h b/llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.h
index f840058cd759e..4ad80baab9a3a 100644
--- a/llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.h
+++ b/llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.h
@@ -117,6 +117,7 @@ class AArch64TargetStreamer : public MCTargetStreamer {
                          MCELFStreamer::AttributeSubSection &AttSubSection);
 
   SmallVector<MCELFStreamer::AttributeSubSection, 64> AttributeSubSections;
+  bool hasSubsection(StringRef SubSectionName);
 
 private:
   std::unique_ptr<AssemblerConstantPools> ConstantPools;
diff --git a/llvm/test/MC/AArch64/build-attributes-asm-aeabi-switch_using_name_only.s b/llvm/test/MC/AArch64/build-attributes-asm-aeabi-switch_using_name_only.s
new file mode 100644
index 0000000000000..cf89d3e376f3b
--- /dev/null
+++ b/llvm/test/MC/AArch64/build-attributes-asm-aeabi-switch_using_name_only.s
@@ -0,0 +1,39 @@
+// RUN: llvm-mc -triple=aarch64 %s -o - | FileCheck %s --check-prefix=ASM
+// RUN: llvm-mc -triple=aarch64 -filetype=obj %s -o - | llvm-readelf --hex-dump=.ARM.attributes - | FileCheck %s --check-prefix=ELF
+
+// ASM: .aeabi_subsection aeabi_pauthabi, required, uleb128
+// ASM: .aeabi_subsection aeabi_feature_and_bits, optional, uleb128
+// ASM: .aeabi_attribute	0, 1 // Tag_Feature_BTI
+// ASM: .aeabi_attribute	2, 1 // Tag_PAuth_Schema
+// ASM: .aeabi_attribute	1, 1 // Tag_PAuth_Platform
+// ASM: .aeabi_attribute	2, 1 // Tag_Feature_GCS
+// ASM: .aeabi_attribute	1, 0 // Tag_Feature_PAC
+// ASM: .aeabi_attribute	7, 1
+// ASM: .aeabi_attribute	7, 0
+
+// ELF: Hex dump of section '.ARM.attributes':
+// ELF-NEXT: 0x00000000 411b0000 00616561 62695f70 61757468 A....aeabi_pauth
+// ELF-NEXT: 0x00000010 61626900 00000201 01010700 25000000 abi.........%...
+// ELF-NEXT: 0x00000020 61656162 695f6665 61747572 655f616e aeabi_feature_an
+// ELF-NEXT: 0x00000030 645f6269 74730001 00000102 01010007 d_bits..........
+// ELF-NEXT: 0x00000040 01
+
+
+.aeabi_subsection aeabi_pauthabi, required, uleb128
+.aeabi_subsection aeabi_feature_and_bits, optional, uleb128
+.aeabi_attribute Tag_Feature_BTI, 1
+.aeabi_subsection aeabi_feature_and_bits
+.aeabi_subsection aeabi_pauthabi
+.aeabi_attribute Tag_PAuth_Schema, 1
+.aeabi_subsection aeabi_pauthabi
+.aeabi_attribute Tag_PAuth_Platform, 1
+.aeabi_subsection aeabi_pauthabi
+.aeabi_subsection aeabi_feature_and_bits
+.aeabi_attribute Tag_Feature_GCS, 1
+.aeabi_subsection aeabi_pauthabi
+.aeabi_subsection aeabi_feature_and_bits
+.aeabi_attribute Tag_Feature_PAC, 0
+.aeabi_subsection aeabi_feature_and_bits
+.aeabi_attribute 7, 1
+.aeabi_subsection aeabi_pauthabi
+.aeabi_attribute 7, 0

[smithp35]

Change matches the updated assembly specification. This does not change any existing use of the directive.

I have a suggestion to simplify the code. I think we should keep the API as small as we can. For example only one way to find an existing subsection, and one way to change to it.

[smithp35]

I think you can simplify the change by moving the call to SubsectionExists here. This can supply the parameters for emitAttributesSubsection so you wouldn't need to change anything in the AArch64TargetStreamer.

  std::unique_ptr<MCELFStreamer::AttributeSubSection> SubsectionExists =
      getTargetStreamer().getAttributesSubsectionByName(SubsectionName);

  if (Parser.getTok().is(llvm::AsmToken::EndOfStatement)) {
    if (SubsectionExists) {
      getTargetStreamer().emitAttributesSubsection(SubsectionName, SubsectionExists->IsOptional, SubsectionExists->Type);
      return false;
    }
    else {
      Error(Parser.getTok().getLoc(),
            "Could not switch to subsection '" + SubsectionName +
                "' using subsection name, subsection has not been defined");
      return true;
    }
  }
  // Otherwise, expecting 2 more parameters: consume a comma

[sivan-shani]

Thank you, comment addressed.

[smithp35]

With your latest changes I don't think these are necessary anymore. Or at least they are now just refactoring that could be moved into a separate NFC patch.

I suggest removing these to keep the diff small.

Otherwise looks good to me.

[sivan-shani]

Done

[smithp35]

LGTM thanks for the update