[PATCH] D119381: [MTE] Add NT_ANDROID_TYPE_MEMTAG

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:1536-1537
+enum {
+  NT_ANDROID_TYPE_IDENT = 1,
+  NT_ANDROID_TYPE_KUSER = 3,
+  NT_ANDROID_TYPE_MEMTAG = 4,
----------------
I guess the `2` value isn't used?


================
Comment at: llvm/include/llvm/BinaryFormat/ELF.h:1546-1547
+  NT_MEMTAG_LEVEL_MASK = 3,
+  NT_MEMTAG_HEAP = 4,
+  NT_MEMTAG_STACK = 8,
+};
----------------
I guess values 5-7 aren't used?


================
Comment at: llvm/test/tools/llvm-readobj/ELF/AArch64/memtag/aarch64-note-memtag-all-async.test:1
+# RUN: yaml2obj %s -o %t
+# RUN: llvm-readelf --notes %t | FileCheck %s --check-prefix=GNU
----------------
Up to you, but I think it would be better to fold these tests into a single test case. You can then reduce duplication of the YAML in one of two ways - either by just having one of each note in the note section, or by using yaml2obj -D options to parameterise the YAML doc. There are plenty of examples of the latter in the llvm-readobj tests.

My comments in this test file also apply to the other files, if there's a strong motivation to keep them separate.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/AArch64/memtag/aarch64-note-memtag-all-async.test:5
+
+# GNU: Displaying notes found in: .note.android.memtag
+# GNU-NEXT:   Owner    Data size	 Description
----------------
Nit: make things line up neatly to slightly improve readability.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/AArch64/memtag/aarch64-note-memtag-all-async.test:34-35
+Sections:
+  - Name:        .note.android.memtag
+    Type:        SHT_NOTE
+    Notes:
----------------
We tend to remove excessive spacing in YAML, keeping values within the same block lined up vertically, at the minimum indentation.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5048-5049
+  switch (NoteType) {
+  default:
+    return false;
+  case ELF::NT_ANDROID_TYPE_MEMTAG:
----------------
It's probably necessary to have a test case that triggers this `return false`.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5051-5052
+  case ELF::NT_ANDROID_TYPE_MEMTAG:
+    if (Desc.empty())
+      return false;
+
----------------
Ditto.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5065-5068
+    default:
+      OS << "Unknown (" << Twine::utohexstr(Desc[0] & NT_MEMTAG_LEVEL_MASK)
+         << ")\n";
+      break;
----------------
Needs test case for this label.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:7076-7077
+template <typename ELFT>
+static bool printAndroidNoteLLVMStyle(uint32_t NoteType, ArrayRef<uint8_t> Desc,
+                                      ScopedPrinter &W) {
+  // Return true if we were able to pretty-print the note, false otherwise.
----------------
This and the GNU function are very similar structurally, which makes me think there's an opportunity for some code reuse here. If I'm not mistaken, the only difference is how these strings are printed? Could you have a helper function which returns a set of strings that are then passed to the corresponding print functions? I'm thinking something like the following sketch:

```
using std::unordered_map<StringRef, std::string> AndroidNoteProperties;
static Optional<AndroidNoteProperties> getAndroidNoteProperties(uint32_t NoteType, ArrayRef<uint8_t> Desc) {
  // Return None if we were unable to get the note properties.
  AndroidNoteProperties Props;
  switch (NoteType) {
  default:
     return None;
  case ELF::NT_ANDROID_TYPE_MEMTAG:
    if (Desc.empty())
      return None;

    switch (Desc[0] & NT_MEMTAG_LEVEL_MASK) {
    case NT_MEMTAG_LEVEL_NONE:
      Props["Tagging Mode"] = "NONE";
      break;
    case NT_MEMTAG_LEVEL_ASYNC:
      Props["Tagging Mode"] = "ASYNC";
      break;
    case NT_MEMTAG_LEVEL_SYNC:
      Props["Tagging Mode"] = "SYNC";
      break;
    default:
      Props["Tagging Mode"] =
          ("Unknown (" + Twine::utohexstr(Desc[0] & NT_MEMTAG_LEVEL_MASK) + ")")
              .str();
      break;
    }
    Props["Heap"] = (Desc[0] & NT_MEMTAG_HEAP) ? "Enabled" : "Disabled");
    Props["Stack"] = (Desc[0] & NT_MEMTAG_STACK) ? "Enabled" : "Disabled");
  }
  return Props;
}

template <typename ELFT>
static bool printAndroidNoteLLVMStyle(uint32_t NoteType, ArrayRef<uint8_t> Desc,
                                      ScopedPrinter &W) {
  // Return true if we were able to pretty-print the note, false otherwise.
  Optional<AndroidNoteProperties> Props = getAndroidNoteProperties(NoteType, Desc);
  if (!Props)
    return false;
  for(const auto &KV : Props)
    W.printString(KV.first, KV.second);
}
```
I don't know if the map will be general enough for future notes, but at least for this memtag note it certainly is. If we need to support other types, there may be a better structure, but I suggest waiting until then to worry about it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119381