[PATCH] D143693: [llvm-readobj] Add --memtag

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/AArch64/memtag-missing.test:5
+
+## Ensure the header is printed, even if there's not relevant dynamic entries.
+# CHECK: Memtag Dynamic Entries
----------------



================
Comment at: llvm/test/tools/llvm-readobj/ELF/AArch64/memtag-missing.test:7
+# CHECK: Memtag Dynamic Entries
+# CHECK: < none found >
+
----------------
Should this be a `CHECK-NEXT`?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/AArch64/memtag-missing.test:13
+# CHECK-NOT: Memtag Android Note
+# CHECK-NOT: Memtag Android Note
+
----------------
There's no point in the duplicate CHECK-NOT. What are you trying to test with these lines?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:301
+  virtual void printMemtag(
+      const std::vector<std::pair<std::string, std::string>> &DynamicEntries,
+      ArrayRef<uint8_t> AndroidNoteDesc) = 0;
----------------
Any reason this can't be an `ArrayRef<std::pair<std::string, std::string>>` or similar?

It may also be more self-documenting to use a two-field class instead of `std::pair<std::string, std::string>`, as you can then give the fields specific names.



================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2274
+        default:
+          return std::string("Unknown (") + std::to_string(Value) + ")";
+      }
----------------
This should be using `Twine` for the string concatenation.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2284
+        default:
+          return std::string("Unknown (") + std::to_string(Value) + ")";
+      }
----------------
Ditto.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:2287
+    case DT_AARCH64_MEMTAG_GLOBALS:
+      return std::string("0x") + utohexstr(Value, /* LowerCase */ true);
     default:
----------------
Ditto. Also, the style for inline comments that are labelling a parameter is `/*LowerCase=*/true`. I believe the trailing `=` causes clang-format to format it like this.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5241
+    OS << "    < none found >\n";
+  for (const auto &DynamicEntry : DynamicEntries)
+    OS << "    " << DynamicEntry.first << ": " << DynamicEntry.second << "\n";
----------------
Especially if you adopt my suggestion about adding a struct/class for the fields, you shouldn't use `auto` here in the loop (see Coding Standards about use of `auto` - it's not clear what the type is from the context).


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5894
+  std::vector<std::pair<std::string, std::string>> DynamicEntries;
+  for (auto Entry : this->dynamic_table()) {
+    uintX_t Tag = Entry.getTag();
----------------
1) Do you need the `this->`?
2) Don't use `auto` here - it's unclear what the type of `Entry` is.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5902-5903
+    case DT_AARCH64_MEMTAG_GLOBALS:
+      DynamicEntries.emplace_back(this->Obj.getDynamicTagAsString(Tag).c_str(),
+                                  this->getDynamicEntry(Tag, Entry.getVal()));
+    }
----------------
As above, do you need to use `this->`?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:7309
+    W.printString("< none found >");
+  for (const auto &DynamicEntry : DynamicEntries)
+    W.printString(DynamicEntry.first, DynamicEntry.second);
----------------
As above re. `auto`.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5917
+
+  printNotesHelper(
+      *this,
----------------
fmayer wrote:
> AFAICT this doesn't actually print anything, so this is a bit awkward. But maybe that's easier than manually iterating? At least worth a comment IMO.
Seems to me like `printNotesHelper` might need renaming or restructuring.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143693