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

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
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;
----------------
jhenderson wrote:
> 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).
sure, added bits to `machine-specific-section-types.test`


================
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
----------------
jhenderson wrote:
> Honestly, I'm finding this comment incomprehensible (possibly because I know nothing about memtags). What is it trying to show?
I've tried to flesh out the comments so that it's clear that this is the hand-asembling of the `.memtag.globals.dynamic` section contents. HTH.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5230
     OS << "    " << KV.first << ": " << KV.second << '\n';
-  OS << '\n';
   return true;
----------------
jhenderson wrote:
> This seems unrelated?
It's a slight formatting fix that I found during this patch. There's an extra unnecessary newline between the Android note (if present) and the 

Given that this is all `--memtag` related parsing, seems right to clean up this spurious newline while we're adding more text.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5913
+    Expected<ArrayRef<uint8_t>> Contents =
+        cantFail(File.getSectionContents(Sec));
+    if (auto E = Contents.takeError()) {
----------------
jhenderson wrote:
> 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`?
yeah, i reckon the `cantFail` wrapping here was vestigial. removed.


================
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);
----------------
MaskRay wrote:
> jhenderson wrote:
> > Drop `llvm::`. Also, the `std::string` construction is unnecessary. Also, consider using `Twine` here and in other string concatenation places.
> `std::string("0x")` => `"0x"` as `llvm::utohexstr` returns a `std::string`.
> 
> `printHex` takes a `StringRef`, so `Twine` cannot be used (without `toStringRef`).
dropped the namespace prefix and explicit construction of `0x`, but yeah, can't use twine here unfortunately


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145761