[AArch64][Build Attributes] Improve Parsing and Formatting

[sivan-shani]

• Removed assertion for duplicate values as adding them is valid.
• Fix parsing: reject strings for unknown tags, allow any value for Tag_PAuth_Platform and Tag_PAuth_Schema.
• Print tags by using numbers with comments to reduce compiler-assembler dependencies.
• Parsing error messages now only point to the symbol (^) instead of printing it.

[llvmbot]

@llvm/pr-subscribers-mc

@llvm/pr-subscribers-backend-aarch64

Author: SivanShani-Arm (sivan-shani)

Changes

When adding build attributes from assembly, an existing value should not trigger an assertion. This case is valid, and behavior should remain consistent between builds with and without assertions.

---

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

2 Files Affected:

• (modified) llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.cpp (+2-6)
• (added) tests.sh (+11)

diff --git a/llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.cpp b/llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.cpp
index 1ed4a81a9767390..d4081e606448b4b 100644
--- a/llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.cpp
+++ b/llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.cpp
@@ -222,13 +222,9 @@ void AArch64TargetStreamer::emitAttribute(StringRef VendorName, unsigned Tag,
                      "Can not add AArch64 build attribute: An attribute with "
                      "the same tag and a different value already exists");
               return;
-            } else {
-              // Case Item.IntValue == Value, no need to emit twice
-              assert(0 &&
-                     "AArch64 build attribute: An attribute with the same tag "
-                     "and a same value already exists");
-              return;
             }
+            // Case Item.IntValue == Value, no need to emit twice
+            return;
           }
         }
       }
diff --git a/tests.sh b/tests.sh
new file mode 100755
index 000000000000000..182932267a6cdbf
--- /dev/null
+++ b/tests.sh
@@ -0,0 +1,11 @@
+${binrl}/llvm-lit -v llvm/test/MC/AArch64/aarch64-build-attributes-asm-*
+${binral}/llvm-lit -v llvm/test/MC/AArch64/aarch64-build-attributes-asm-*
+${bindl}/llvm-lit -v llvm/test/MC/AArch64/aarch64-build-attributes-asm-*
+
+${binrl}/llvm-lit -v  llvm/test/CodeGen/AArch64/aarch64-build-attributes-*
+${binral}/llvm-lit -v  llvm/test/CodeGen/AArch64/aarch64-build-attributes-*
+${bindl}/llvm-lit -v  llvm/test/CodeGen/AArch64/aarch64-build-attributes-*
+
+${binrl}/llvm-lit -v  /home/sivsha01/Repos/llvm-project/llvm/test/tools/llvm-readobj/ELF/AArch64/aarch64-build-attributes-*
+${binral}/llvm-lit -v  /home/sivsha01/Repos/llvm-project/llvm/test/tools/llvm-readobj/ELF/AArch64/aarch64-build-attributes-*
+${bindl}/llvm-lit -v  /home/sivsha01/Repos/llvm-project/llvm/test/tools/llvm-readobj/ELF/AArch64/aarch64-build-attributes-*

[ostannard]

Would you mind splitting this into multiple patches to make it easier to review? Based on the PR description, this looks like it could be three patches:

• Improve testing of the existing behaviour, no functional changes
• Do not allow unknown tags to be strings, and add/change tests for this
• Remove assertion when value exists, and add/change tests for this

I think these are closely related enough to stay in one PR, but splitting them into patches allows me (and other reviewers) to see which test changes correspond to which code changes.

[sivan-shani]

Since it is a relatively small patch, I will try to describe what has changed and what belongs where.
If it is still not clear. please let me know and I will try to split the PR to patches.

Tag_PAuth_Platform and Tag_PAuth_Schema
Allow assembly to pass any unsigned values, not only 0 or 1
code: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
Achieved by changing the condition
if (ActiveSubsectionID == AArch64BuildAttrs::AEABI_FEATURE_AND_BITS) {
test: llvm/test/MC/AArch64/aarch64-build-attributes-asm-aeabi-aeabi-known.s

.aeabi_attribute Tag_PAuth_Platform, 7
.aeabi_attribute Tag_PAuth_Schema, 777

also removing .aeabi_attribute Tag_PAuth_Platform, 4 from
llvm/test/MC/AArch64/aarch64-build-attributes-asm-aeabi-err-attrs.s

Do not allow unknown tags to be strings
code: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
case AArch64BuildAttrs::VENDOR_UNKNOWN: under } else if (Parser.getTok().is(AsmToken::Identifier)) {
now gives an error.
also moved if (Parser.getTok().is(AsmToken::Integer)) { before so it is clear that the integer case is being handled.
test: llvm/test/MC/AArch64/aarch64-build-attributes-asm-non_aeabi-err.s
.aeabi_attribute str_not_int, "1"

Remove assertion when value exists
llvm/lib/Target/AArch64/MCTargetDesc/AArch64TargetStreamer.cpp

// Case Item.IntValue == Value, no need to emit twice
assert(0 &&
"AArch64 build attribute: An attribute with the same tag "
"and a same value already exists");

removed. This is not an error and now release and debug behave the same.
test: llvm/test/MC/AArch64/aarch64-build-attributes-asm-aeabi-aeabi-known.s

.aeabi_attribute Tag_PAuth_Schema, 777
.aeabi_attribute Tag_PAuth_Schema, 777

produce same object as before.

general testing (and other) improvement
-> error msgs at llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp standardize, made 2 specific cases like the others.
-> change files names, 'private' -> 'non-aeabi', more clear.
-> change files structure - put syntax and logical tests separately.
-> add file mixing 'aeabi' and 'non-aeabi' tests llvm/test/MC/AArch64/aarch64-build-attributes-asm-aeabi-mixed.s

Print and parse known string tags as a number + comment
emission-
code: llvm/lib/Target/AArch64/MCTargetDesc/AArch64ELFStreamer.cpp
OS << "\t.aeabi_attribute" << "\t" << Tag << ", " << Value << "\t@ "
test: llvm/test/MC/AArch64/aarch64-build-attributes-asm-aeabi-aeabi-known.s
parsing-
code: llvm/lib/Target/AArch64/AsmParser/AArch64AsmParser.cpp
see under comment: // Parsing finished, hereafter only accept comments; otherwise no trailing tokens.
test: llvm/test/MC/AArch64/aarch64-build-attributes-asm-aeabi-aeabi-known.s

[ostannard]

You now have 5 different changes in one patch, please split it up.

[sivan-shani]

Break down the change to several patches

[ostannard]

When splitting patches up, please make sure that the tests are updated in the same patch which makes the functional change. This makes it much easier for the reviewer to see what has/hasn't been tested, and the tests are also useful to the reviewer as an example of what behaviour is changed/added by the patch. In the current version of this PR, you've deleted most of the tests in the first commit and re-added them in the last one, which makes it harder to see what has been added/changed in them.

[github-actions]

✅ With the latest revision this PR passed the C/C++ code formatter.

[ostannard]

LGTM