[PATCH] D69759: [BPF] Add preserve_access_index attribute for record definition

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

Why did this get committed when there were still outstanding review questions that were not answered?

In D69759#1739935 <https://reviews.llvm.org/D69759#1739935>, @yonghong-song wrote:

> Regarding to this one "This is missing a bunch of C++ tests that show what happens in the case of inheritance, template instantiations, etc."
>  Could you give me some example tests which I can take a look in order to add support. FYI, BPF backend publicly only supports C language,
>  (a restrict C for example __thread keyword is not supported). I guess template instantiations does not apply here? Or we can still test
>  template instantiation since we are at clang stage?


If the attribute only makes sense in C mode, it should not be accepted in C++ mode. However, it currently is accepted in C++ mode which is why I was asking for tests demonstrating how it should behave.

In D69759#1742264 <https://reviews.llvm.org/D69759#1742264>, @yonghong-song wrote:

> @aaron.ballman just ping, could you let me know if you have any further comments? Thanks!


I was out all last week at wg21 meetings and a short vacation, so that's why the delay in review. FWIW, we usually give a week before pinging a review.



================
Comment at: clang/include/clang/Basic/Attr.td:1584
+                             TargetSpecificAttr<TargetBPF>  {
+  let Spellings = [Clang<"preserve_access_index">];
+  let Subjects = SubjectList<[Record], ErrorDiag>;
----------------
If this attribute is only supported in C mode, then this attribute should have: `let LangOpts = [COnly];` in it.


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:3411-3422
+  const auto *ImplicitCast = dyn_cast<ImplicitCastExpr>(ArrayBase);
+  if (!ImplicitCast)
+    return false;
+
+  // Only support base as either a MemberExpr or DeclRefExpr.
+  // DeclRefExpr to cover cases like:
+  //    struct s { int a; int b[10]; };
----------------
Is there a reason to do this as opposed to `IgnoreImpCasts()`?


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:3423-3424
+  const Expr *E = ImplicitCast->getSubExpr();
+  const auto *MemberCast = dyn_cast<MemberExpr>(E);
+  if (MemberCast)
+    return MemberCast->getMemberDecl()->hasAttr<BPFPreserveAccessIndexAttr>();
----------------
```
if (const auto *ME = dyn_cast<MemberExpr>(E))
  return ...
```



================
Comment at: clang/lib/CodeGen/CGExpr.cpp:3427-3428
+
+  const auto *DeclRefCast = dyn_cast<DeclRefExpr>(E);
+  if (DeclRefCast) {
+    const VarDecl *VarDef = dyn_cast<VarDecl>(DeclRefCast->getDecl());
----------------
Similar here -- you can sink the variable into the `if`.


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:3433
+
+    const auto *PtrT = dyn_cast<PointerType>(VarDef->getType().getTypePtr());
+    if (!PtrT)
----------------
`const auto *PtrT = VarDef->getType()->getAs<PointerType>();`


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:3440-3443
+    while (TypedefT) {
+      PointeeT = TypedefT->desugar().getTypePtr();
+      TypedefT = dyn_cast<TypedefType>(PointeeT);
+    }
----------------
Is there a reason you only want to strip off typedefs, and not other forms of sugar? e.g., why not call `getUnqualifiedDesugaredType()`?


================
Comment at: clang/lib/CodeGen/CGExpr.cpp:3446
+    // Not a typedef any more, it should be an elaborated type.
+    const auto ElaborateT = dyn_cast<ElaboratedType>(PointeeT);
+    if (!ElaborateT)
----------------
`const auto *`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759