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

[hctim]

Summary

Global variables are described in a metadata table called
SHT_AARCH64_MEMTAG_GLOBALS_DYNAMIC. It's basically a ULEB-encoded skip
list with some other fancy encoding tricks to make it smaller. You can
see the ABI at
https://github.com/ARM-software/abi-aa/blob/main/memtagabielf64/memtagabielf64.rst#83encoding-of-sht_aarch64_memtag_globals_dynamic

This extends readelf/readobj to understand these sections.

[jhenderson]

llvm/lib/ObjectYAML/ELFYAML.cpp

711–712

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).

llvm/test/tools/llvm-readobj/ELF/AArch64/memtag.test

150

Honestly, I'm finding this comment incomprehensible (possibly because I know nothing about memtags). What is it trying to show?

llvm/tools/llvm-readobj/ELFDumper.cpp

5295

This seems unrelated?

5323

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?

5324–5325

You don't need the llvm:: prefixes here.

5979

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?

5981–5982

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)

5993–5994

These aren't named in compliance with LLVM's naming scheme.

6048–6051

See above re. errors and warnings in llvm-readobj.

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

6058

I not i.

6065–6066

As above.

6077–6078

As above

7537

Drop llvm::. Also, the std::string construction is unnecessary. Also, consider using Twine here and in other string concatenation places.

[MaskRay]

(I'm going to be out for 3 weeks in the next 4 weeks. Happy if others are happy. Don't wait on me :) )

llvm/tools/llvm-readobj/ELFDumper.cpp

6056

Delete blank line.

Also, add a link to the ABI doc describing the encoding scheme?

7537

std::string("0x") => "0x" as llvm::utohexstr returns a std::string.

printHex takes a StringRef, so Twine cannot be used (without toStringRef).

[hctim]

Address jhenderson@'s comments.

llvm/lib/ObjectYAML/ELFYAML.cpp

711–712

sure, added bits to machine-specific-section-types.test

llvm/test/tools/llvm-readobj/ELF/AArch64/memtag.test

150

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.

llvm/tools/llvm-readobj/ELFDumper.cpp

5295

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.

5979

yeah, i reckon the cantFail wrapping here was vestigial. removed.

7537

dropped the namespace prefix and explicit construction of 0x, but yeah, can't use twine here unfortunately

[jhenderson]

This is going to need review by somebody with memtag experience still.

Meanwhile, I think some test cases are needed for a few of the warnings.

llvm/tools/llvm-readobj/ELFDumper.cpp

5979

I don't think this is tested?

6063

Test case?

6075

Test case?

[hctim]

llvm/tools/llvm-readobj/ELFDumper.cpp

5979

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.

[hctim]

Add some extra edge case tests.

[jhenderson]

llvm/test/tools/llvm-readobj/ELF/AArch64/memtag.test

342

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.

llvm/tools/llvm-readobj/ELFDumper.cpp

5979

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.

[hctim]

Add more tests for edge cases in decoding.

llvm/tools/llvm-readobj/ELFDumper.cpp

5979

thanks for the handy link to stackmap.test, done.

[jhenderson]

No more comments from me, as aside from one minor suggestion in the test.

It probably needs a review from someone familiar with the memtag encoding though, as I'm not.

llvm/test/tools/llvm-readobj/ELF/AArch64/memtag.test

334

I don't think we have the same line length expectations in lit tests, so I personally wouldn't split the messages across multiple checks like this, due to the fact they have a slight difference in behaviour. However, I don't have a strong opinion on it, so if you prefer it this way, that's fine. I would make it a CHECK-SAME directive though (and if you do that, I'd add some spaces before "warning" to make the second CHECK's contents line up with the first one i.e.

# BAD-STREAM1:      warning: ...
# BAD-STREAM1-SAME: SHT_...

Same comment applies below.

[pcc]

LGTM

[hctim]

llvm/test/tools/llvm-readobj/ELF/AArch64/memtag.test

334

sure, done, in submit.