This is an archive of the discontinued LLVM Phabricator instance.

BPF: Add name_off and name to emitted BTF comments
Needs ReviewPublic

Authored by davemarchevsky on Apr 13 2023, 8:52 AM.

Details

Reviewers
yonghong-song
Summary

Aside from changes in BTFDebug.{h,cpp}, all the changes in this
patch are mechanical test refactorings.

Before this patch, BTFTypeBase::emitType's emitted comment would look
something like BTF_KIND_VAR(id = 23), with only id being emitted.

This patch adds name_off (or name) to information emitted between
the parens, resulting in comments like:

BTF_KIND_VAR(id = 23, name = _license)
BTF_KIND_ARRAY(id = 22, name_off = 0)

The main goal of this change is to improve LLVM BTF tests - which use
FileCheck to compare llc -filetype asm output with expected lines.
With name and name_off, it's significantly easier to understand why
a particular type is expected in BTF output.

A BTFTypeBase::emitType(MCStreamer &OS, StringRef Name) method is
added which acts as BTFTypeBase::emitType(MCStreamer &OS) did before
the patch, except that id and name are emitted if a non-empty name
is provided, and id and name_off are emitted if an empty name is
provided. BTFTypeBase::emitType(MCStreamer &OS) now just calls the new
method with an empty Name arg.

Derived classes of BTFTypeBase call BTFTypeBase::emitType in their
own emitType implementations. For derived types that have a name, it's
straightforward to migrate to calling the new BTFTypeBase::emitType
with additional Name arg.

It is necessary to fix existing tests after the aforementioned change,
since they expect to see something like BTF_KIND_VAR(id = 23). Luckily
there are some straightforward ways to modify existing tests such that
they pass without having to find name/name_off for them.

It would be sufficient to remove the last paren from CHECK:-type
statements, changing their expected string from BTF_KIND_VAR(id = 23)
to BTF_KIND_VAR(id = 23, which will match both BTF_KIND_VAR(id = 23)
and BTF_KIND_VAR(id = 23, name = whatever. This patch goes one step
further, changing the CHECK: to look for BTF_KIND_VAR( and using
CHECK-SAME: statements to look for id or other fields. For example:

CHECK: BTF_KIND_VAR(id = 23, name = _license)

Becomes

CHECK: BTF_KIND_VAR(
CHECK-SAME: id = 23
CHECK-SAME: name = _license

This CHECK-SAME: refactoring has the benefit of not caring about how
emitted field info is ordered - the previous refactoring would match
both of:

BTF_KIND_VAR(name = _license, id = 23)
BTF_KIND_VAR(id = 23, name_off = 123, name = _license)

So if we later decide to add other fields or always emit name_off even
if a name is found, it won't be necessary to change the tests.

Signed-off-by: Dave Marchevsky <davemarchevsky@fb.com>

Diff Detail

Unit TestsFailed

Event Timeline

davemarchevsky created this revision.Apr 13 2023, 8:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2023, 8:52 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
davemarchevsky requested review of this revision.Apr 13 2023, 8:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 13 2023, 8:52 AM
davemarchevsky edited the summary of this revision. (Show Details)Apr 13 2023, 8:53 AM
davemarchevsky added a reviewer: yonghong-song.
davemarchevsky set the repository for this revision to rG LLVM Github Monorepo.

Do we have cases where name_off is not 0? If that is the case, should we just go ahead print the name? If not, maybe we should not print name_off = 0 at all?