Page MenuHomePhabricator

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

Authored by hctim on Mar 9 2023, 8:21 PM.



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

This extends readelf/readobj to understand these sections.

Diff Detail

Event Timeline

hctim created this revision.Mar 9 2023, 8:21 PM
Herald added a project: Restricted Project. · View Herald Transcript
hctim requested review of this revision.Mar 9 2023, 8:21 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 9 2023, 8:21 PM
jhenderson requested changes to this revision.Mar 10 2023, 12:04 AM
jhenderson added inline comments.

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


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


This seems unrelated?


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?


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


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?


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)


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


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

Also please refer to


I not i.


As above.


As above


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

This revision now requires changes to proceed.Mar 10 2023, 12:04 AM

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


Delete blank line.

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


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

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

hctim updated this revision to Diff 506203.Mar 17 2023, 3:00 PM
hctim marked 15 inline comments as done.

Address jhenderson@'s comments.


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


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.


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.


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


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

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.


I don't think this is tested?


Test case?


Test case?

hctim marked 3 inline comments as done.Mar 20 2023, 4:09 PM
hctim added inline comments.

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 updated this revision to Diff 506761.Mar 20 2023, 4:09 PM
hctim marked an inline comment as done.

Add some extra edge case tests.

jhenderson added inline comments.Mar 21 2023, 2:05 AM

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.


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 updated this revision to Diff 512261.Apr 10 2023, 2:15 PM
hctim marked 2 inline comments as done.

Add more tests for edge cases in decoding.


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

jhenderson accepted this revision.Apr 11 2023, 12:53 AM

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.


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

Same comment applies below.

This revision is now accepted and ready to land.Apr 11 2023, 12:53 AM
pcc accepted this revision.Apr 11 2023, 6:23 PM


MaskRay accepted this revision.Apr 11 2023, 6:35 PM
This revision was landed with ongoing or failed builds.Apr 12 2023, 10:24 AM
This revision was automatically updated to reflect the committed changes.
hctim marked an inline comment as done.
hctim added inline comments.Apr 12 2023, 10:24 AM

sure, done, in submit.