This is an archive of the discontinued LLVM Phabricator instance.

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

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

Details

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.

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

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

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 updated this revision to Diff 506203.Mar 17 2023, 3:00 PM
hctim marked 15 inline comments as done.

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

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 marked 3 inline comments as done.Mar 20 2023, 4:09 PM
hctim added inline comments.
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 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
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 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.

llvm/tools/llvm-readobj/ELFDumper.cpp
5979

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.

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.

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

LGTM

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
llvm/test/tools/llvm-readobj/ELF/AArch64/memtag.test
334

sure, done, in submit.