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

[Mitch Phillips via Phabricator via llvm-commits]

hctim added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/AArch64/memtag-missing.test:13
+# CHECK-NOT: Memtag Android Note
+# CHECK-NOT: Memtag Android Note
+
----------------
jhenderson wrote:
> There's no point in the duplicate CHECK-NOT. What are you trying to test with these lines?
Removed the duplicate. We're trying to assert (unlike in `memtag.test`) that `readelf/readobj --memtag` indiscriminately prints "Memtag Dynamic Entries", but doesn't print a missing Android ELF note.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/AArch64/note-android-memtag.test:1
-# RUN: yaml2obj -D DESC='0d000000' %s -o %t
-# RUN: llvm-readelf --notes %t | FileCheck %s --check-prefixes=GNU,GNU-OK,ASYNC,HEAP,STACK
-# RUN: llvm-readobj --notes %t | FileCheck %s --check-prefixes=LLVM,LLVM-OK,ASYNC,HEAP,STACK
+# RUN: yaml2obj -D DESC='0d000000' -D MODE=1 -D HEAP=1 -D STACK=1 %s -o %t
+# RUN: llvm-readelf --notes --dynamic --memtag %t | FileCheck %s --check-prefixes=GNU,GNU-OK,ASYNC,HEAP,STACK
----------------
MaskRay wrote:
> Add a test when no memtag tag is present in an object file.
done, in a separate file (memtag-missing.test)


================
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;
----------------
jhenderson wrote:
> 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.
> 
I've changed it to `ArrayRef` in the callees, but I'm don't think adding a struct for a pair of strings would drastically improve readability here. 


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5241
+    OS << "    < none found >\n";
+  for (const auto &DynamicEntry : DynamicEntries)
+    OS << "    " << DynamicEntry.first << ": " << DynamicEntry.second << "\n";
----------------
jhenderson wrote:
> 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).
I've updated all the range-based loops to use `const auto& DynamicEntryKV` (note the new `KV` suffix). 

I agree with you (and the style guide) on "avoid `auto` for non-obvious type deductions", but IMHO using auto for a key-value iteration is idiomatic, as it's used for all `std::map<string, string>` iteration.


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


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5917
+
+  printNotesHelper(
+      *this,
----------------
jhenderson wrote:
> 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.
renamed to `processNotesHelper`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143693