[PATCH] D75015: [ARM] Rewrite ARMAttributeParser

[Saleem Abdulrasool via Phabricator via llvm-commits]

compnerd accepted this revision.
compnerd added inline comments.
This revision is now accepted and ready to land.


================
Comment at: lld/test/ELF/arm-tag-vfp-args-illegal.s:5
 
-// CHECK: arm-tag-vfp-args-illegal.s.tmp.o: unknown Tag_ABI_VFP_args value: 5
+// CHECK: {{.*}}.o:(.ARM.attributes): invalid ABI_VFP_args value, expected to be inside [0,4)
         .arch armv7-a
----------------
I think that the original error message was better here


================
Comment at: llvm/include/llvm/Support/ARMAttributeParser.h:42
+  Error stringAttribute(ARMBuildAttrs::AttrType tag);
+  Error CPU_arch(ARMBuildAttrs::AttrType tag);
+  Error CPU_arch_profile(ARMBuildAttrs::AttrType tag);
----------------
A newline before this would be nice to delimit the general handler vs the known attribute list.


================
Comment at: llvm/include/llvm/Support/ARMAttributeParser.h:78
+  Error Virtualization_use(ARMBuildAttrs::AttrType tag);
+  Error nodefaults(ARMBuildAttrs::AttrType tag);
 
----------------
It might be nice to use a `XMACRO` here.  But thats not critical/important.


================
Comment at: llvm/include/llvm/Support/ARMAttributeParser.h:82
+  void parseIndexList(SmallVectorImpl<uint8_t> &indexList);
+  Error parseSubsection(uint32_t length);
 
----------------
Would be nice to group this with the other general handlers.


================
Comment at: llvm/include/llvm/Support/ARMAttributeParser.h:87
+  ARMAttributeParser() : sw(nullptr) {}
+  ~ARMAttributeParser() { (void)!cursor.takeError(); }
 
----------------
`static_cast<void>` please.


================
Comment at: llvm/lib/Support/ARMAttributeParser.cpp:67
+     ATTRIBUTE_HANDLER(Virtualization_use),
+     ATTRIBUTE_HANDLER(nodefaults)};
 
----------------
Please put a trailing comma after the `nodefaults` handler and re-clang-format.


================
Comment at: llvm/lib/Support/ARMAttributeParser.cpp:402
+  uint64_t pos;
+  uint64_t end = cursor.tell() - 5 + length;
+  while ((pos = cursor.tell()) < end) {
----------------
I think that the original code was clearer here.  I think at the very least, we should have a comment explaining this offset.


================
Comment at: llvm/lib/Support/ARMAttributeParser.cpp:417
+    if (!handled) {
+      if (tag < 32)
+        return createStringError(errc::invalid_argument,
----------------
Bleh, could you please check if we have a constant for this yet?  IIRC, we didn't have one when I wrote this, but really this is the limit of the namespace and should really have a constant.


================
Comment at: llvm/lib/Support/ARMAttributeParser.cpp:447
+Error ARMAttributeParser::parseSubsection(uint32_t length) {
+  uint64_t end = cursor.tell() - 4 + length;
+  StringRef vendorName = de.getCStrRef(cursor);
----------------
Again, I think that the original code was more readable here.  Please use `sizeof(length)` or add a comment explaining the 4.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75015/new/

https://reviews.llvm.org/D75015