[PATCH] D145761: [MTE] [llvm-readobj] Add globals section parsing to --memtag

[James Henderson via Phabricator via llvm-commits]

jhenderson requested changes to this revision.
jhenderson added inline comments.
This revision now requires changes to proceed.


================
Comment at: llvm/lib/ObjectYAML/ELFYAML.cpp:711-712
+  case ELF::EM_AARCH64:
+    ECase(SHT_AARCH64_MEMTAG_GLOBALS_STATIC);
+    ECase(SHT_AARCH64_MEMTAG_GLOBALS_DYNAMIC);
+    break;
----------------
Could you please check to see if the other machine-specific section types have any specific YAML testing, please? (Usually in the form of a yaml2obj lit test, but could be in obj2yaml lit tests or ObjectYAML unit tests).


================
Comment at: llvm/test/tools/llvm-readobj/ELF/AArch64/memtag.test:150
+## https://github.com/ARM-software/abi-aa/blob/main/memtagabielf64/memtagabielf64.rst#83encoding-of-sht_aarch64_memtag_globals_dynamic
+## 16 bytes at 0xdead0000 (0xdead0000 bytes, skip == 0xdead000 granules)
+##  -> /* skip */ (0xdead000 << 3) + /* size in granules */ 1
----------------
Honestly, I'm finding this comment incomprehensible (possibly because I know nothing about memtags). What is it trying to show?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5230
     OS << "    " << KV.first << ": " << KV.second << '\n';
-  OS << '\n';
   return true;
----------------
This seems unrelated?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5257
+  OS << "Memtag Global Descriptors:\n";
+  for (const auto &Descriptor : Descriptors) {
+    OS << "    0x" << llvm::utohexstr(Descriptor.first, true) << ": 0x"
----------------
Can you use the new C++17 structured bindings to name both first and second values here, rather than just doing `Descriptors.first` and `Descriptors.second`, please?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5258-5259
+  for (const auto &Descriptor : Descriptors) {
+    OS << "    0x" << llvm::utohexstr(Descriptor.first, true) << ": 0x"
+       << llvm::utohexstr(Descriptor.second, true) << "\n";
+  }
----------------
You don't need the `llvm::` prefixes here.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5913
+    Expected<ArrayRef<uint8_t>> Contents =
+        cantFail(File.getSectionContents(Sec));
+    if (auto E = Contents.takeError()) {
----------------
Are you sure `cantFail` is appropriate here? What happens if the sh_offset field of the section is invalid? In fact, if you're using `cantFail`, why are you storing the result in an `Expected`?


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5915-5916
+    if (auto E = Contents.takeError()) {
+      llvm::errs() << "Couldn't get SHT_AARCH64_MEMTAG_GLOBALS_DYNAMIC section "
+                      "contents\n";
+      return ArrayRef<uint8_t>();
----------------
This isn't how errors or warnings are reported in llvm-readobj. Please take a look at other examples and fix appropriately. (You probably want `reportUniqueWarning`)


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5927-5928
+// when compiling Android T with full MTE globals enabled.
+constexpr uint64_t kMemtagStepVarintReservedBits = 3;
+constexpr uint64_t kMemtagGranuleSize = 16;
+
----------------
These aren't named in compliance with LLVM's naming scheme.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5966-5969
+    llvm::errs() << "Mismatch between DT_AARCH64_MEMTAG_GLOBALSSZ ("
+                 << format_hex(AndroidMemtagGlobalsSz, 0)
+                 << ") and SHT_AARCH64_MEMTAG_GLOBALS_DYNAMIC section size ("
+                 << format_hex(Contents.size(), 0) << ")\n";
----------------
See above re. errors and warnings in llvm-readobj.

Also please refer to https://llvm.org/docs/CodingStandards.html#error-and-warning-messages


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5976
+  uint64_t Address = 0;
+  for (size_t i = 0; i < Contents.size();) {
+    const char *Error = nullptr;
----------------
`I` not `i`.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5983-5984
+    if (Error) {
+        llvm::errs() << "Error decoding SHT_AARCH64_MEMTAG_GLOBALS_DYNAMIC: \""
+                     << Twine(Error) << "\"\n";
+        break;
----------------
As above.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5995-5996
+      if (Error) {
+        llvm::errs() << "Error decoding SHT_AARCH64_MEMTAG_GLOBALS_DYNAMIC: \""
+                     << Twine(Error) << "\"\n";
+        break;
----------------
As above


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:7409
+    for (const auto &Descriptor : Descriptors) {
+      W.printHex(std::string("0x") + llvm::utohexstr(Descriptor.first),
+                Descriptor.second);
----------------
Drop `llvm::`. Also, the `std::string` construction is unnecessary. Also, consider using `Twine` here and in other string concatenation places.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145761