This is an archive of the discontinued LLVM Phabricator instance.

[BPF] handle typedef of struct/union for CO-RE relocations
ClosedPublic

Authored by yonghong-song on Feb 3 2020, 8:55 AM.

Details

Summary

Linux commit

https://github.com/torvalds/linux/commit/1cf5b23988ea0086a252a5c8b005b075f1e9b030#diff-289313b9fec99c6f0acfea19d9cfd949

uses "#pragma clang attribute push (attribute((preserve_access_index)),

apply_to = record)"

to apply CO-RE relocations to all records including the following pattern:

#pragma clang attribute push (__attribute__((preserve_access_index)), apply_to = record)
typedef struct {
  int a;
} __t;
#pragma clang attribute pop 
int test(__t *arg) { return arg->a; }

The current approach to use struct/union type in the relocation record will
result in an anonymous struct, which make later type matching difficult
in bpf loader. In fact, current BPF backend will fail the above program
with assertion:

clang: ../lib/Target/BPF/BPFAbstractMemberAccess.cpp:796: ... 
   Assertion `TypeName.size()' failed.

clang will change to use the type of the base of the member access
which will preserve the typedef modifier for the
preserve_{struct,union}_access_index intrinsics in the above example.
Here we adjust BPF backend to accept that the debuginfo
type metadata may be 'typedef' and handle them properly.

Diff Detail

Event Timeline

yonghong-song created this revision.Feb 3 2020, 8:55 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 3 2020, 8:55 AM
anakryiko added inline comments.Feb 3 2020, 9:30 AM
llvm/lib/Target/BPF/BPFAbstractMemberAccess.cpp
715

this comment won't be accurate anymore? should it be updated to include typedef?

732–734

I wonder if capturing typedef always is a better approach, even if underlying struct/union is named? If that causes problems due to typedef name not being found, we can always teach libbpf to treat typedef as an alias and try to find candidate types first using typedef name, if that fails - using underlying struct name? WDYT?

yonghong-song marked 2 inline comments as done.Feb 3 2020, 9:42 AM
yonghong-song added inline comments.
llvm/lib/Target/BPF/BPFAbstractMemberAccess.cpp
715

Yes, this needs to updated.

732–734

Yes, we can do this. This probably will give libbpf more information about what is going on, i.e., user actually use type with typedef to access variable instead of the structure (even with explicit names). Not sure how useful this information will be used. But it should not hurt.

address Andrii's comments. Always use the typedef type name if it is available.

anakryiko accepted this revision.Feb 4 2020, 8:30 AM
This revision is now accepted and ready to land.Feb 4 2020, 8:30 AM
This revision was automatically updated to reflect the committed changes.