This is an archive of the discontinued LLVM Phabricator instance.

[BPF] handle opaque-pointer for __builtin_preserve_enum_value
ClosedPublic

Authored by yonghong-song on Apr 14 2022, 9:08 AM.

Details

Summary

Opaque pointer [1] is enabled as the default with commit [2].
Andrii found that current __builtin_preserve_enum_value() can only handle
non opaque pointer code pattern and will segfault with latest
llvm main branch where opaque-pointer is enabled by default.

This patch added the opaque pointer support.
Besides llvm selftests, also verified with bpf-next bpf selftests.

[1] https://llvm.org/docs/OpaquePointers.html
[2] https://reviews.llvm.org/D123122

Diff Detail

Event Timeline

yonghong-song created this revision.Apr 14 2022, 9:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2022, 9:08 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
yonghong-song requested review of this revision.Apr 14 2022, 9:08 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 14 2022, 9:08 AM
ast accepted this revision.Apr 14 2022, 9:17 AM

The fix to prevent the crash looks good, but what should be the expected codegen for a global variable?
CO-RE needs a constant.
Are we assuming that the global data will be inited with some constant and a later pass will propagate it further into __builtin_preserve,
so at the end it will become a constant?

This revision is now accepted and ready to land.Apr 14 2022, 9:17 AM
nikic added a subscriber: nikic.Apr 14 2022, 9:21 AM
nikic added inline comments.
llvm/lib/Target/BPF/BPFAbstractMemberAccess.cpp
953

Is it possible to make this Call->getArgOperand(1)->stripPointerCasts() instead of a manual check for ConstantExpr?

The fix to prevent the crash looks good, but what should be the expected codegen for a global variable?
CO-RE needs a constant.
Are we assuming that the global data will be inited with some constant and a later pass will propagate it further into __builtin_preserve,
so at the end it will become a constant?

Right. These global variables are encoding enum Name/Value information and later the actual generated code for the builtin will be the enum value
as the intrinsic __builtin_preserve_enum_value() returns the enum value, but a relocation record is also generated so libbpf can change
the enum value in the code in case "enum name -> enum value" changed.

yonghong-song added inline comments.Apr 14 2022, 9:31 AM
llvm/lib/Target/BPF/BPFAbstractMemberAccess.cpp
953

Thanks for the suggestion. It indeed simplified the code a lot!

  • simplify code with stripPointerCasts
This revision was landed with ongoing or failed builds.Apr 14 2022, 11:35 AM
This revision was automatically updated to reflect the committed changes.