[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

yonghong-song added a comment. I tried a couple of examples with both preserve_access_index and preserve_static_offset (both '__preserve_access_index __preserve_static_offset' and '__preserve_static_offset __preserve_access_index'). Looks they cooperate with each other properly.

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

yonghong-song added a comment. I went through LLVM part of the change, mostly look good to me from high-level and I need to go through another pass with details. It would be great if you can post corresponding kernel patches which utilizes this functionality? Comment at:

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

eddyz87 added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3791 + if (hasBPFPreserveStaticOffset(Base)) +addr = wrapWithBPFPreserveStaticOffset(CGF, addr); + ast wrote: > eddyz87 wrote: > > ast wrote: > > > eddyz87 wrote: > > > > ast wrote: > >

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

ast accepted this revision. ast added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3791 + if (hasBPFPreserveStaticOffset(Base)) +addr = wrapWithBPFPreserveStaticOffset(CGF, addr); + eddyz87 wrote: > ast wrote: > > eddyz87 wrote: > > > ast

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

eddyz87 added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3791 + if (hasBPFPreserveStaticOffset(Base)) +addr = wrapWithBPFPreserveStaticOffset(CGF, addr); + ast wrote: > eddyz87 wrote: > > ast wrote: > > > If I'm reading this correctly

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

ast added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3791 + if (hasBPFPreserveStaticOffset(Base)) +addr = wrapWithBPFPreserveStaticOffset(CGF, addr); + eddyz87 wrote: > ast wrote: > > If I'm reading this correctly wrapping with

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

eddyz87 marked an inline comment as done. eddyz87 added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3791 + if (hasBPFPreserveStaticOffset(Base)) +addr = wrapWithBPFPreserveStaticOffset(CGF, addr); + ast wrote: > If I'm reading this correctly

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

ast added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3791 + if (hasBPFPreserveStaticOffset(Base)) +addr = wrapWithBPFPreserveStaticOffset(CGF, addr); + If I'm reading this correctly wrapping with preserve_static_offset doesn't prevent

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

eddyz87 marked 3 inline comments as done. eddyz87 added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3701 + QualType PointeeType = E->getType()->getPointeeType(); + if (PointeeType.isNull()) +return false; erichkeane wrote: > eddyz87 wrote: >

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

erichkeane added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3701 + QualType PointeeType = E->getType()->getPointeeType(); + if (PointeeType.isNull()) +return false; eddyz87 wrote: > erichkeane wrote: > > We override `operator bool` to make

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

eddyz87 added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3701 + QualType PointeeType = E->getType()->getPointeeType(); + if (PointeeType.isNull()) +return false; erichkeane wrote: > We override `operator bool` to make this work. Sorry, just

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. 2 nits on the CFE, else LGTM! Obviously you still need an LLVM reviewer for the backend changes. Comment at: clang/lib/CodeGen/CGExpr.cpp:3701 + QualType

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

eddyz87 added inline comments. Comment at: llvm/test/CodeGen/BPF/preserve-static-offset/load-align.ll:61 +!0 = !{i32 1, !"wchar_size", i32 4} +!1 = !{!"clang version 18.0.0 (/home/eddy/work/llvm-project/clang c899a1ca75d0f1b559204eff79a2578d2cafc7ab)"} +!2 = !{!3, !4, i64 128}

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

erichkeane added inline comments. Comment at: llvm/test/CodeGen/BPF/preserve-static-offset/load-align.ll:61 +!0 = !{i32 1, !"wchar_size", i32 4} +!1 = !{!"clang version 18.0.0 (/home/eddy/work/llvm-project/clang c899a1ca75d0f1b559204eff79a2578d2cafc7ab)"} +!2 = !{!3, !4, i64

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

eddyz87 added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3700 +return false; + if (auto *BaseDecl = E->getType()->getPointeeType()->getAsRecordDecl()) +return hasBPFPreserveStaticOffset(BaseDecl); erichkeane wrote: > eddyz87 wrote: > >

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

erichkeane added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3700 +return false; + if (auto *BaseDecl = E->getType()->getPointeeType()->getAsRecordDecl()) +return hasBPFPreserveStaticOffset(BaseDecl); eddyz87 wrote: > erichkeane wrote: > >

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

eddyz87 added inline comments. Comment at: clang/lib/CodeGen/CGExpr.cpp:3700 +return false; + if (auto *BaseDecl = E->getType()->getPointeeType()->getAsRecordDecl()) +return hasBPFPreserveStaticOffset(BaseDecl); erichkeane wrote: > getPointeeType can

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

eddyz87 added a comment. In D133361#4652368 , @erichkeane wrote: > I don't see the comment response you had to me. Sorry, forgot to click submit. Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7601 + auto *Rec = cast(D); +

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

erichkeane added a comment. In D133361#4652102 , @eddyz87 wrote: > Rebase, changes as requested by @aaron.ballman and @erichkeane. > > Hi @aaron.ballman, @erichkeane, > > Thank you for taking a look. > I beleive this commit covers all feedback except

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

erichkeane added a comment. The CFE stuff is pretty innocuous and I don't see a reason to stop this on our accord, but would like someone familiar with the LLVM stuff to review this at one point. Comment at: clang/lib/CodeGen/CGExpr.cpp:3694 +static bool

[PATCH] D133361: [BPF] Attribute preserve_static_offset for structs

aaron.ballman added a reviewer: erichkeane. aaron.ballman added a comment. I don't have a whole lot of opinions on the attribute itself (I'm not super familiar with BPF), but did spot some things to do on the Clang side. This will also need reviewers for the LLVM changes -- any ideas on who