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

[Eduard Zingerman via Phabricator via cfe-commits]

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:
> > erichkeane wrote:
> > > getPointeeType can also return nullptr, so unless you have a test elsewhere to ensure it isn't, you likely have to do a little more work here (and if so, I'd need an assert).
> > Is it? I actually double-checked this before pushing an update, clangd jumps to the following definition:
> > 
> > ```
> > lang=cpp
> > QualType Type::getPointeeType() const {
> >   if (const auto *PT = getAs<PointerType>())
> >     return PT->getPointeeType();
> >   ...
> >   return {};
> > }
> > ```
> > 
> > The `getAsRecordDecl()` can return null indeed, but that null is checked.
> More correctly, it returns an empty `QualType`.  The `operator->` on that will cause a `nullptr`, causing the call to `Type::getAsRecordDecl` to have a `nullptr` `this`.
Oh, right, I'll add an additional check, thank you.


================
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}
----------------
erichkeane wrote:
> eddyz87 wrote:
> > erichkeane wrote:
> > > Are we sure we want to do something like this?  It seems this both depends on YOUR computer AND us never releasing Clang 18.
> > Are you sure this would be an issue?
> > The specific line is not a part of a CHECK and I tried the following command using my system's llvm 16 opt:
> > 
> > ```
> > opt -O2 -mtriple=bpf-pc-linux -S -o - load-align.ll
> > ```
> > 
> > And module was loaded / processed w/o any issues.
> > In general grepping shows that people don't usually mask these in tests:
> > 
> > ```
> > $ cd llvm/test/CodeGen/
> > $ ag '{!"clang version' | wc -l
> > 452
> > ```
> I don't write LLVM tests ever, so I'm not sure.  It just seems odd to provide that much irrelevant info, perhaps one of hte LLVM reviewers can comment.  Also, look at those ~450 and see what they contain?
> Also, look at those ~450 and see what they contain?
Same random clang versions:

```
$ ag '{!"clang version' | head
X86/debug-loclists.ll:129:!6 = !{!"clang version 10.0.0 (trunk 374581) (llvm/trunk 374579)"}
X86/dbg-combine.ll:84:!11 = !{!"clang version 3.7.0 (trunk 227074)"}
X86/debuginfo-locations-dce.ll:46:!6 = !{!"clang version 8.0.0 (trunk 339665)"}
X86/pr31242.ll:48:!5 = !{!"clang version 4.0.0 (trunk 288844)"}
X86/catchpad-regmask.ll:140:!1 = !{!"clang version 3.8.0 "}
X86/debug-nodebug-crash.ll:48:!5 = !{!"clang version 4.0.0"}
X86/limit-split-cost.mir:64:  !2 = !{!"clang version 7.0.0 (trunk 335057)"}
X86/swap.ll:169:!1 = !{!"clang version 9.0.0 (trunk 352631) (llvm/trunk 352632)"}
X86/dwarf-aranges-available-externally.ll:65:!16 = !{!"clang version 15.0.0 (https://github.com/llvm/llvm-project.git 2f52a868225755ebfa5242992d3a650ac6aadce7)"}
X86/label-annotation.ll:96:!7 = !{!"clang version 9.0.0 (git at github.com:llvm/llvm-project.git 7f9a008a2db285aca57bfa0c09858c9527a7aa98)"}
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361