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

[Erich Keane via Phabricator via cfe-commits]

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 hasBPFPreserveStaticOffset(const RecordDecl *D) {
+  return D->getAttr<BPFPreserveStaticOffsetAttr>();
+}
----------------



================
Comment at: clang/lib/CodeGen/CGExpr.cpp:3698
+static bool hasBPFPreserveStaticOffset(const Expr *E) {
+  if (auto *PtrType = dyn_cast<PointerType>(E->getType()))
+    if (auto *BaseDecl = PtrType->getPointeeType()->getAsRecordDecl())
----------------
You can use `E->getType()->getPointeeType()` here instead, unless you REALLY care that this is only a normal-pointer deref (and not a PMF or reference type).


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:3778
 
+  if (Base && hasBPFPreserveStaticOffset(Base))
+    addr = wrapWithBPFPreserveStaticOffset(CGF, addr);
----------------
I'd suggest just making `hasBPFPreserveStaticOffset` be `nullptr` tolerant and remove the 1st half of this.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7601
+  auto *Rec = cast<RecordDecl>(D);
+  Rec->addAttr(::new (S.Context) BPFPreserveStaticOffsetAttr(S.Context, AL));
+}
----------------
This should use `BPFPreserveStaticOffsetAttr::Create` instead of using placement `new`.  We've been meaning to translate these all at one point, but never got around to it.


================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8871
+  case ParsedAttr::AT_BPFPreserveStaticOffset:
+    handleBPFPreserveStaticOffsetAttr(S, D, AL);
+    break;
----------------
aaron.ballman wrote:
> 
Or this, this is probably better.


================
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}
----------------
Are we sure we want to do something like this?  It seems this both depends on YOUR computer AND us never releasing Clang 18.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D133361/new/

https://reviews.llvm.org/D133361