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

[James Henderson via Phabricator via llvm-commits]

jhenderson added inline comments.


================
Comment at: llvm/test/tools/llvm-readobj/ELF/AArch64/memtag.test:342
+
+# BAD-STREAM: warning: {{.*}} error decoding SHT_AARCH64_MEMTAG_GLOBALS_DYNAMIC
+
----------------
I think you need to check the context of the message to show that the right warning is being emitted (since there are two possible locations where this could come from). This will make the test less at risk of rotting in the future too.


================
Comment at: llvm/tools/llvm-readobj/ELFDumper.cpp:5908
+    if (auto E = Contents.takeError()) {
+      reportUniqueWarning("couldn't get SHT_AARCH64_MEMTAG_GLOBALS_DYNAMIC section contents: " + toString(std::move(E)));
+      return ArrayRef<uint8_t>();
----------------
hctim wrote:
> jhenderson wrote:
> > I don't think this is tested?
> this isn't really testable here; `getSectionContents()` has a few failure conditions but none of them are really memtag-relevant.
> 
> i have however added a check that the address of the SHT_AARCH64_MEMTAG_GLOBALS_DYNAMIC section is the same as the DT_AARCH64_MEMTAG_GLOBALS dynamic table entry, and tested it.
We should still have a test that shows that if `getSectionContents` fails, we handle the failure appropriately. The easiest way of doing this is probably to simply override the sh_offset value of the section, using the `ShOffset` YAML key. There are a number of good examples of things like this in llvm-readobj tests. A simple one is in `stackmap.test`, where the same YAML is used, but with the yaml2obj -D option used to provide a specific sh_offset value for one test case that is for the equivalent of what you're checking here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D145761