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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D69759#1746192 , @yonghong-song 
wrote:

> Sorry about the committing. I thought all the review questions are addressed.


No worries, this happens sometimes. :-)

> Definitely will be more patient next time until getting review confirmation 
> and
>  also adhering to llvm review policy.

Thanks!

> Thanks for the detailed review! I have addressed all of your review comments 
> in patch
>  https://reviews.llvm.org/D70257. I have added you as the reviewer.

Fantastic, thank you!




Comment at: clang/lib/CodeGen/CGExpr.cpp:3440-3443
+while (TypedefT) {
+  PointeeT = TypedefT->desugar().getTypePtr();
+  TypedefT = dyn_cast(PointeeT);
+}

yonghong-song wrote:
> aaron.ballman wrote:
> > Is there a reason you only want to strip off typedefs, and not other forms 
> > of sugar? e.g., why not call `getUnqualifiedDesugaredType()`?
> Actually, ElaboratedT is also a sugar type, the above 
> getUnqualifiedDesugaredType() will remove it too.
Yes, but you just strip off that sugar anyway, so that's why I suggested this. 
It seems you got the gist of my suggestion in the other review though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song marked 7 inline comments as done.
yonghong-song added a comment.

Sorry about the committing. I thought all the review questions are addressed.
Definitely will be more patient next time until getting review confirmation and
also adhering to llvm review policy.
Thanks for the detailed review! I have addressed all of your review comments in 
patch
https://reviews.llvm.org/D70257. I have added you as the reviewer.




Comment at: clang/include/clang/Basic/Attr.td:1584
+ TargetSpecificAttr  {
+  let Spellings = [Clang<"preserve_access_index">];
+  let Subjects = SubjectList<[Record], ErrorDiag>;

aaron.ballman wrote:
> If this attribute is only supported in C mode, then this attribute should 
> have: `let LangOpts = [COnly];` in it.
Yes, added.



Comment at: clang/lib/CodeGen/CGExpr.cpp:3440-3443
+while (TypedefT) {
+  PointeeT = TypedefT->desugar().getTypePtr();
+  TypedefT = dyn_cast(PointeeT);
+}

aaron.ballman wrote:
> Is there a reason you only want to strip off typedefs, and not other forms of 
> sugar? e.g., why not call `getUnqualifiedDesugaredType()`?
Actually, ElaboratedT is also a sugar type, the above 
getUnqualifiedDesugaredType() will remove it too.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[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 , @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 , @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  {
+  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(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(E);
+  if (MemberCast)
+return MemberCast->getMemberDecl()->hasAttr();

```
if (const auto *ME = dyn_cast(E))
  return ...
```




Comment at: clang/lib/CodeGen/CGExpr.cpp:3427-3428
+
+  const auto *DeclRefCast = dyn_cast(E);
+  if (DeclRefCast) {
+const VarDecl *VarDef = dyn_cast(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(VarDef->getType().getTypePtr());
+if (!PtrT)

`const auto *PtrT = VarDef->getType()->getAs();`



Comment at: clang/lib/CodeGen/CGExpr.cpp:3440-3443
+while (TypedefT) {
+  PointeeT = TypedefT->desugar().getTypePtr();
+  TypedefT = dyn_cast(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(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



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yonghong Song via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4e2ce228ae79: [BPF] Add preserve_access_index attribute for 
record definition (authored by yonghong-song).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/bpf-attr-preserve-access-index-1.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-2.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-3.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-4.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-5.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-6.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-7.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-8.c
  clang/test/Sema/bpf-attr-preserve-access-index.c

Index: clang/test/Sema/bpf-attr-preserve-access-index.c
===
--- /dev/null
+++ clang/test/Sema/bpf-attr-preserve-access-index.c
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -x c -triple bpf-pc-linux-gnu -dwarf-version=4 -fsyntax-only -verify %s
+
+#define __reloc__ __attribute__((preserve_access_index))
+#define __err_reloc__ __attribute__((preserve_access_index(0)))
+
+struct t1 {
+  int a;
+  int b[4];
+  int c:1;
+} __reloc__;
+
+union t2 {
+  int a;
+  int b[4];
+  int c:1;
+} __reloc__;
+
+struct t3 {
+  int a;
+} __err_reloc__; // expected-error {{'preserve_access_index' attribute takes no arguments}}
+
+struct t4 {
+  union {
+int a;
+char b[5];
+  };
+  struct {
+int c:1;
+  } __reloc__;
+  int d;
+} __reloc__;
+
+struct __reloc__ p;
+struct __reloc__ q;
+struct p {
+  int a;
+};
+
+int a __reloc__; // expected-error {{'preserve_access_index' attribute only applies to structs, unions, and classes}}
+struct s *p __reloc__; // expected-error {{'preserve_access_index' attribute only applies to structs, unions, and classes}}
+
+void invalid1(const int __reloc__ *arg) {} // expected-error {{'preserve_access_index' attribute only applies to structs, unions, and classes}}
+void invalid2() { const int __reloc__ *arg; } // expected-error {{'preserve_access_index' attribute only applies to structs, unions, and classes}}
+int valid3(struct t4 *arg) { return arg->a + arg->b[3] + arg->c + arg->d; }
+int valid4(void *arg) {
+  struct local_t { int a; int b; } __reloc__;
+  return ((struct local_t *)arg)->b;
+}
Index: clang/test/CodeGen/bpf-attr-preserve-access-index-8.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-attr-preserve-access-index-8.c
@@ -0,0 +1,36 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+#define __reloc__ __attribute__((preserve_access_index))
+
+// chain of records, all with attributes
+struct s1;
+struct s2;
+struct s3;
+
+struct s1 {
+  int c;
+} __reloc__;
+typedef struct s1 __s1;
+
+struct s2 {
+  union {
+__s1 b[3];
+  };
+} __reloc__;
+typedef struct s2 __s2;
+
+struct s3 {
+  __s2 a;
+} __reloc__;
+typedef struct s3 __s3;
+
+int test(__s3 *arg) {
+  return arg->a.b[2].c;
+}
+
+// CHECK: call %struct.s2* @llvm.preserve.struct.access.index.p0s_struct.s2s.p0s_struct.s3s(%struct.s3* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: call %union.anon* @llvm.preserve.struct.access.index.p0s_union.anons.p0s_struct.s2s(%struct.s2* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: call %union.anon* @llvm.preserve.union.access.index.p0s_union.anons.p0s_union.anons(%union.anon* %{{[0-9a-z]+}}, i32 0)
+// CHECK: call %struct.s1* @llvm.preserve.array.access.index.p0s_struct.s1s.p0a3s_struct.s1s([3 x %struct.s1]* %{{[0-9a-z]+}}, i32 1, i32 2)
+// CHECK: call i32* @llvm.preserve.struct.access.index.p0i32.p0s_struct.s1s(%struct.s1* %{{[0-9a-z]+}}, i32 0, i32 0)
Index: clang/test/CodeGen/bpf-attr-preserve-access-index-7.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-attr-preserve-access-index-7.c
@@ -0,0 +1,36 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+#define __reloc__ __attribute__((preserve_access_index))
+
+// chain of records, all with attributes
+struct __reloc__ s1;
+struct __reloc__ s2;
+struct __reloc__ s3;
+
+struct s1 {
+  int c;
+};
+typedef struct s1 __s1;
+
+struct s2 {
+  union {
+__s1 b[3];
+  };
+};
+typedef struct s2 __s2;
+
+struct s3 {
+  __s2 a;
+};
+typedef struct s3 __s3;
+
+int test(__s3 *arg) {
+  return arg->a.b[2].c;
+}
+
+// CHECK: call %struct.s2* @llvm.preserve.struct.access.index.p0s_struct.s2s.p0s_struct.s3s(%struct.s3* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: call %union.anon* @llvm.preserve.struct.access.index.p0s_union.anons.p0s_struct.s2s(%struct.s2* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: c

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

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

@aaron.ballman BTW, I indeed tested C-style inheritance. An attribute for the 
forward declaration successfully inherited by a later actual declaration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexei Starovoitov via Phabricator via cfe-commits]

ast added a comment.

I've tested this patch on several kernel selftests/bpf/ and it works like 
magic. Very nice improvement.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song marked 2 inline comments as done.
yonghong-song added a comment.

Hi, @aaron.ballman I think I addressed all your comments, could you take a look 
again? I tested all failed tests as exposed by the buildbot here 
(http://lab.llvm.org:8011/builders/llvm-clang-lld-x86_64-scei-ps4-ubuntu-fast/builds/57856).
 They are all passing now. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song updated this revision to Diff 228588.
yonghong-song added a comment.

use implicit attr to annotate records or fields not explicitly marked by users.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/bpf-attr-preserve-access-index-1.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-2.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-3.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-4.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-5.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-6.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-7.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-8.c
  clang/test/Sema/bpf-attr-preserve-access-index.c

Index: clang/test/Sema/bpf-attr-preserve-access-index.c
===
--- /dev/null
+++ clang/test/Sema/bpf-attr-preserve-access-index.c
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -x c -triple bpf-pc-linux-gnu -dwarf-version=4 -fsyntax-only -verify %s
+
+#define __reloc__ __attribute__((preserve_access_index))
+#define __err_reloc__ __attribute__((preserve_access_index(0)))
+
+struct t1 {
+  int a;
+  int b[4];
+  int c:1;
+} __reloc__;
+
+union t2 {
+  int a;
+  int b[4];
+  int c:1;
+} __reloc__;
+
+struct t3 {
+  int a;
+} __err_reloc__; // expected-error {{'preserve_access_index' attribute takes no arguments}}
+
+struct t4 {
+  union {
+int a;
+char b[5];
+  };
+  struct {
+int c:1;
+  } __reloc__;
+  int d;
+} __reloc__;
+
+struct __reloc__ p;
+struct __reloc__ q;
+struct p {
+  int a;
+};
+
+int a __reloc__; // expected-error {{'preserve_access_index' attribute only applies to structs, unions, and classes}}
+struct s *p __reloc__; // expected-error {{'preserve_access_index' attribute only applies to structs, unions, and classes}}
+
+void invalid1(const int __reloc__ *arg) {} // expected-error {{'preserve_access_index' attribute only applies to structs, unions, and classes}}
+void invalid2() { const int __reloc__ *arg; } // expected-error {{'preserve_access_index' attribute only applies to structs, unions, and classes}}
+int valid3(struct t4 *arg) { return arg->a + arg->b[3] + arg->c + arg->d; }
+int valid4(void *arg) {
+  struct local_t { int a; int b; } __reloc__;
+  return ((struct local_t *)arg)->b;
+}
Index: clang/test/CodeGen/bpf-attr-preserve-access-index-8.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-attr-preserve-access-index-8.c
@@ -0,0 +1,36 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+#define __reloc__ __attribute__((preserve_access_index))
+
+// chain of records, all with attributes
+struct s1;
+struct s2;
+struct s3;
+
+struct s1 {
+  int c;
+} __reloc__;
+typedef struct s1 __s1;
+
+struct s2 {
+  union {
+__s1 b[3];
+  };
+} __reloc__;
+typedef struct s2 __s2;
+
+struct s3 {
+  __s2 a;
+} __reloc__;
+typedef struct s3 __s3;
+
+int test(__s3 *arg) {
+  return arg->a.b[2].c;
+}
+
+// CHECK: call %struct.s2* @llvm.preserve.struct.access.index.p0s_struct.s2s.p0s_struct.s3s(%struct.s3* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: call %union.anon* @llvm.preserve.struct.access.index.p0s_union.anons.p0s_struct.s2s(%struct.s2* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: call %union.anon* @llvm.preserve.union.access.index.p0s_union.anons.p0s_union.anons(%union.anon* %{{[0-9a-z]+}}, i32 0)
+// CHECK: call %struct.s1* @llvm.preserve.array.access.index.p0s_struct.s1s.p0a3s_struct.s1s([3 x %struct.s1]* %{{[0-9a-z]+}}, i32 1, i32 2)
+// CHECK: call i32* @llvm.preserve.struct.access.index.p0i32.p0s_struct.s1s(%struct.s1* %{{[0-9a-z]+}}, i32 0, i32 0)
Index: clang/test/CodeGen/bpf-attr-preserve-access-index-7.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-attr-preserve-access-index-7.c
@@ -0,0 +1,36 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+#define __reloc__ __attribute__((preserve_access_index))
+
+// chain of records, all with attributes
+struct __reloc__ s1;
+struct __reloc__ s2;
+struct __reloc__ s3;
+
+struct s1 {
+  int c;
+};
+typedef struct s1 __s1;
+
+struct s2 {
+  union {
+__s1 b[3];
+  };
+};
+typedef struct s2 __s2;
+
+struct s3 {
+  __s2 a;
+};
+typedef struct s3 __s3;
+
+int test(__s3 *arg) {
+  return arg->a.b[2].c;
+}
+
+// CHECK: call %struct.s2* @llvm.preserve.struct.access.index.p0s_struct.s2s.p0s_struct.s3s(%struct.s3* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: call %union.anon* @llvm.preserve.struct.access.index.p0s_union.anons.p0s_struct.s2s(%struct.s2* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: call %union.anon* @llvm.preserve.unio

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

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song updated this revision to Diff 228583.
yonghong-song added a comment.

The Decl "D" could be a nullptr in ProcessDeclAttributeDelayed, which will 
cause segfault. Handle this properly.
Also addressed most of Aaron's comments.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/bpf-attr-preserve-access-index-1.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-2.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-3.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-4.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-5.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-6.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-7.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-8.c
  clang/test/Sema/bpf-attr-preserve-access-index.c

Index: clang/test/Sema/bpf-attr-preserve-access-index.c
===
--- /dev/null
+++ clang/test/Sema/bpf-attr-preserve-access-index.c
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -x c -triple bpf-pc-linux-gnu -dwarf-version=4 -fsyntax-only -verify %s
+
+#define __reloc__ __attribute__((preserve_access_index))
+#define __err_reloc__ __attribute__((preserve_access_index(0)))
+
+struct t1 {
+  int a;
+  int b[4];
+  int c:1;
+} __reloc__;
+
+union t2 {
+  int a;
+  int b[4];
+  int c:1;
+} __reloc__;
+
+struct t3 {
+  int a;
+} __err_reloc__; // expected-error {{'preserve_access_index' attribute takes no arguments}}
+
+struct t4 {
+  union {
+int a;
+char b[5];
+  };
+  struct {
+int c:1;
+  } __reloc__;
+  int d;
+} __reloc__;
+
+struct __reloc__ p;
+struct __reloc__ q;
+struct p {
+  int a;
+};
+
+int a __reloc__; // expected-error {{'preserve_access_index' attribute only applies to structs, unions, and classes}}
+struct s *p __reloc__; // expected-error {{'preserve_access_index' attribute only applies to structs, unions, and classes}}
+
+void invalid1(const int __reloc__ *arg) {} // expected-error {{'preserve_access_index' attribute only applies to structs, unions, and classes}}
+void invalid2() { const int __reloc__ *arg; } // expected-error {{'preserve_access_index' attribute only applies to structs, unions, and classes}}
+int valid3(struct t4 *arg) { return arg->a + arg->b[3] + arg->c + arg->d; }
+int valid4(void *arg) {
+  struct local_t { int a; int b; } __reloc__;
+  return ((struct local_t *)arg)->b;
+}
Index: clang/test/CodeGen/bpf-attr-preserve-access-index-8.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-attr-preserve-access-index-8.c
@@ -0,0 +1,36 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+#define __reloc__ __attribute__((preserve_access_index))
+
+// chain of records, all with attributes
+struct s1;
+struct s2;
+struct s3;
+
+struct s1 {
+  int c;
+} __reloc__;
+typedef struct s1 __s1;
+
+struct s2 {
+  union {
+__s1 b[3];
+  };
+} __reloc__;
+typedef struct s2 __s2;
+
+struct s3 {
+  __s2 a;
+} __reloc__;
+typedef struct s3 __s3;
+
+int test(__s3 *arg) {
+  return arg->a.b[2].c;
+}
+
+// CHECK: call %struct.s2* @llvm.preserve.struct.access.index.p0s_struct.s2s.p0s_struct.s3s(%struct.s3* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: call %union.anon* @llvm.preserve.struct.access.index.p0s_union.anons.p0s_struct.s2s(%struct.s2* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: call %union.anon* @llvm.preserve.union.access.index.p0s_union.anons.p0s_union.anons(%union.anon* %{{[0-9a-z]+}}, i32 0)
+// CHECK: call %struct.s1* @llvm.preserve.array.access.index.p0s_struct.s1s.p0a3s_struct.s1s([3 x %struct.s1]* %{{[0-9a-z]+}}, i32 1, i32 2)
+// CHECK: call i32* @llvm.preserve.struct.access.index.p0i32.p0s_struct.s1s(%struct.s1* %{{[0-9a-z]+}}, i32 0, i32 0)
Index: clang/test/CodeGen/bpf-attr-preserve-access-index-7.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-attr-preserve-access-index-7.c
@@ -0,0 +1,36 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+#define __reloc__ __attribute__((preserve_access_index))
+
+// chain of records, all with attributes
+struct __reloc__ s1;
+struct __reloc__ s2;
+struct __reloc__ s3;
+
+struct s1 {
+  int c;
+};
+typedef struct s1 __s1;
+
+struct s2 {
+  union {
+__s1 b[3];
+  };
+};
+typedef struct s2 __s2;
+
+struct s3 {
+  __s2 a;
+};
+typedef struct s3 __s3;
+
+int test(__s3 *arg) {
+  return arg->a.b[2].c;
+}
+
+// CHECK: call %struct.s2* @llvm.preserve.struct.access.index.p0s_struct.s2s.p0s_struct.s3s(%struct.s3* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: call %union.anon* @llvm.preserve.struct.access.index.p0s_union.anons.p0s_struct.s2s(%struct.s2* %{{

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

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

Thanks, @aaron.ballman, I will fix the issue and address all you comments. I 
will re-open this commit.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

This is missing a bunch of C++ tests that show what happens in the case of 
inheritance, template instantiations, etc.




Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:10068
   "__builtin_preserve_field_info argument %0 not a constant">;
+def err_preserve_access_index_wrong_type: Error<"%0 attribute only applies to 
%1">;
 

This is unnecessary.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5706
+  // Add preserve_access_index attribute to all fields and inner records.
+  for (DeclContext::decl_iterator D = RD->decls_begin(), DEnd = 
RD->decls_end();
+   D != DEnd; ++D) {

Any reason not to use a range-based for?



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5712
+
+RecordDecl *Rec = dyn_cast(*D);
+if (Rec) {

Can use `auto` here



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5713-5718
+if (Rec) {
+  Rec->addAttr(::new (S.Context) BPFPreserveAccessIndexAttr(S.Context, 
AL));
+  handleBPFPreserveAIRecord(S, Rec, AL);
+} else {
+  D->addAttr(::new (S.Context) BPFPreserveAccessIndexAttr(S.Context, AL));
+}

These should be implicit attributes, not explicit ones, right?



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5725
+  // Add preserve_access_index attribute to all fields and inner records.
+  for (DeclContext::decl_iterator D = RD->decls_begin(), DEnd = 
RD->decls_end();
+   D != DEnd; ++D) {

Similar here.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5727
+   D != DEnd; ++D) {
+RecordDecl *Rec = dyn_cast(*D);
+if (Rec) {

Can use `auto` here as well.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5730-5736
+  if (!Rec->hasAttr()) {
+Rec->addAttr(::new (S.Context) BPFPreserveAccessIndexAttr(S.Context, 
AL));
+handleBPFPreserveAIRecord(S, Rec, AL);
+  }
+} else {
+  D->addAttr(::new (S.Context) BPFPreserveAccessIndexAttr(S.Context, AL));
+}

Implicit attributes?



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5742-5747
+  RecordDecl *Rec = dyn_cast(D);
+  if (!Rec) {
+S.Diag(D->getLocation(), diag::err_preserve_access_index_wrong_type)
+<< "preserve_addess_index" << "struct or union type";
+return;
+  }

This should be handled in Attr.td with an explicit subject.



Comment at: clang/lib/Sema/SemaDeclAttr.cpp:5749
+
+  if (!checkAttributeNumArgs(S, AL, 0))
+return;

This is not needed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yonghong Song via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG4a5aa1a7bf8b: [BPF] Add preserve_access_index attribute for 
record definition (authored by yonghong-song).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/bpf-attr-preserve-access-index-1.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-2.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-3.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-4.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-5.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-6.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-7.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-8.c
  clang/test/Sema/bpf-attr-preserve-access-index.c

Index: clang/test/Sema/bpf-attr-preserve-access-index.c
===
--- /dev/null
+++ clang/test/Sema/bpf-attr-preserve-access-index.c
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -x c -triple bpf-pc-linux-gnu -dwarf-version=4 -fsyntax-only -verify %s
+
+#define __reloc__ __attribute__((preserve_access_index))
+#define __err_reloc__ __attribute__((preserve_access_index(0)))
+
+struct t1 {
+  int a;
+  int b[4];
+  int c:1;
+} __reloc__;
+
+union t2 {
+  int a;
+  int b[4];
+  int c:1;
+} __reloc__;
+
+struct t3 {
+  int a;
+} __err_reloc__; // expected-error {{'preserve_access_index' attribute takes no arguments}}
+
+struct t4 {
+  union {
+int a;
+char b[5];
+  };
+  struct {
+int c:1;
+  } __reloc__;
+  int d;
+} __reloc__;
+
+struct __reloc__ p;
+struct __reloc__ q;
+struct p {
+  int a;
+};
+
+int a __reloc__; // expected-error {{preserve_addess_index attribute only applies to struct or union type}}
+struct s *p __reloc__; // expected-error {{preserve_addess_index attribute only applies to struct or union type}}
+
+void invalid1(const int __reloc__ *arg) {} // expected-error {{preserve_addess_index attribute only applies to struct or union type}}
+void invalid2() { const int __reloc__ *arg; } // expected-error {{preserve_addess_index attribute only applies to struct or union type}}
+int valid3(struct t4 *arg) { return arg->a + arg->b[3] + arg->c + arg->d; }
+int valid4(void *arg) {
+  struct local_t { int a; int b; } __reloc__;
+  return ((struct local_t *)arg)->b;
+}
Index: clang/test/CodeGen/bpf-attr-preserve-access-index-8.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-attr-preserve-access-index-8.c
@@ -0,0 +1,36 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+#define __reloc__ __attribute__((preserve_access_index))
+
+// chain of records, all with attributes
+struct s1;
+struct s2;
+struct s3;
+
+struct s1 {
+  int c;
+} __reloc__;
+typedef struct s1 __s1;
+
+struct s2 {
+  union {
+__s1 b[3];
+  };
+} __reloc__;
+typedef struct s2 __s2;
+
+struct s3 {
+  __s2 a;
+} __reloc__;
+typedef struct s3 __s3;
+
+int test(__s3 *arg) {
+  return arg->a.b[2].c;
+}
+
+// CHECK: call %struct.s2* @llvm.preserve.struct.access.index.p0s_struct.s2s.p0s_struct.s3s(%struct.s3* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: call %union.anon* @llvm.preserve.struct.access.index.p0s_union.anons.p0s_struct.s2s(%struct.s2* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: call %union.anon* @llvm.preserve.union.access.index.p0s_union.anons.p0s_union.anons(%union.anon* %{{[0-9a-z]+}}, i32 0)
+// CHECK: call %struct.s1* @llvm.preserve.array.access.index.p0s_struct.s1s.p0a3s_struct.s1s([3 x %struct.s1]* %{{[0-9a-z]+}}, i32 1, i32 2)
+// CHECK: call i32* @llvm.preserve.struct.access.index.p0i32.p0s_struct.s1s(%struct.s1* %{{[0-9a-z]+}}, i32 0, i32 0)
Index: clang/test/CodeGen/bpf-attr-preserve-access-index-7.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-attr-preserve-access-index-7.c
@@ -0,0 +1,36 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+#define __reloc__ __attribute__((preserve_access_index))
+
+// chain of records, all with attributes
+struct __reloc__ s1;
+struct __reloc__ s2;
+struct __reloc__ s3;
+
+struct s1 {
+  int c;
+};
+typedef struct s1 __s1;
+
+struct s2 {
+  union {
+__s1 b[3];
+  };
+};
+typedef struct s2 __s2;
+
+struct s3 {
+  __s2 a;
+};
+typedef struct s3 __s3;
+
+int test(__s3 *arg) {
+  return arg->a.b[2].c;
+}
+
+// CHECK: call %struct.s2* @llvm.preserve.struct.access.index.p0s_struct.s2s.p0s_struct.s3s(%struct.s3* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: call %union.anon* @llvm.preserve.struct.access.index.p0s_union.anons.p0s_struct.s2s(%struct.s2* %{{[0-9a-z]+}}, i32 0, i32 0)
+

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

[Alexei Starovoitov via Phabricator via cfe-commits]

ast accepted this revision.
ast added a comment.
This revision is now accepted and ready to land.

Great. Looks like inner propagation fix was straighforward. Thanks!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song updated this revision to Diff 228538.
yonghong-song edited the summary of this revision.
yonghong-song added a comment.

handle forward declaration correctly, i.e., not losing its attribute/action.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/bpf-attr-preserve-access-index-1.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-2.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-3.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-4.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-5.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-6.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-7.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-8.c
  clang/test/Sema/bpf-attr-preserve-access-index.c

Index: clang/test/Sema/bpf-attr-preserve-access-index.c
===
--- /dev/null
+++ clang/test/Sema/bpf-attr-preserve-access-index.c
@@ -0,0 +1,48 @@
+// RUN: %clang_cc1 -x c -triple bpf-pc-linux-gnu -dwarf-version=4 -fsyntax-only -verify %s
+
+#define __reloc__ __attribute__((preserve_access_index))
+#define __err_reloc__ __attribute__((preserve_access_index(0)))
+
+struct t1 {
+  int a;
+  int b[4];
+  int c:1;
+} __reloc__;
+
+union t2 {
+  int a;
+  int b[4];
+  int c:1;
+} __reloc__;
+
+struct t3 {
+  int a;
+} __err_reloc__; // expected-error {{'preserve_access_index' attribute takes no arguments}}
+
+struct t4 {
+  union {
+int a;
+char b[5];
+  };
+  struct {
+int c:1;
+  } __reloc__;
+  int d;
+} __reloc__;
+
+struct __reloc__ p;
+struct __reloc__ q;
+struct p {
+  int a;
+};
+
+int a __reloc__; // expected-error {{preserve_addess_index attribute only applies to struct or union type}}
+struct s *p __reloc__; // expected-error {{preserve_addess_index attribute only applies to struct or union type}}
+
+void invalid1(const int __reloc__ *arg) {} // expected-error {{preserve_addess_index attribute only applies to struct or union type}}
+void invalid2() { const int __reloc__ *arg; } // expected-error {{preserve_addess_index attribute only applies to struct or union type}}
+int valid3(struct t4 *arg) { return arg->a + arg->b[3] + arg->c + arg->d; }
+int valid4(void *arg) {
+  struct local_t { int a; int b; } __reloc__;
+  return ((struct local_t *)arg)->b;
+}
Index: clang/test/CodeGen/bpf-attr-preserve-access-index-8.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-attr-preserve-access-index-8.c
@@ -0,0 +1,36 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+#define __reloc__ __attribute__((preserve_access_index))
+
+// chain of records, all with attributes
+struct s1;
+struct s2;
+struct s3;
+
+struct s1 {
+  int c;
+} __reloc__;
+typedef struct s1 __s1;
+
+struct s2 {
+  union {
+__s1 b[3];
+  };
+} __reloc__;
+typedef struct s2 __s2;
+
+struct s3 {
+  __s2 a;
+} __reloc__;
+typedef struct s3 __s3;
+
+int test(__s3 *arg) {
+  return arg->a.b[2].c;
+}
+
+// CHECK: call %struct.s2* @llvm.preserve.struct.access.index.p0s_struct.s2s.p0s_struct.s3s(%struct.s3* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: call %union.anon* @llvm.preserve.struct.access.index.p0s_union.anons.p0s_struct.s2s(%struct.s2* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: call %union.anon* @llvm.preserve.union.access.index.p0s_union.anons.p0s_union.anons(%union.anon* %{{[0-9a-z]+}}, i32 0)
+// CHECK: call %struct.s1* @llvm.preserve.array.access.index.p0s_struct.s1s.p0a3s_struct.s1s([3 x %struct.s1]* %{{[0-9a-z]+}}, i32 1, i32 2)
+// CHECK: call i32* @llvm.preserve.struct.access.index.p0i32.p0s_struct.s1s(%struct.s1* %{{[0-9a-z]+}}, i32 0, i32 0)
Index: clang/test/CodeGen/bpf-attr-preserve-access-index-7.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-attr-preserve-access-index-7.c
@@ -0,0 +1,36 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+#define __reloc__ __attribute__((preserve_access_index))
+
+// chain of records, all with attributes
+struct __reloc__ s1;
+struct __reloc__ s2;
+struct __reloc__ s3;
+
+struct s1 {
+  int c;
+};
+typedef struct s1 __s1;
+
+struct s2 {
+  union {
+__s1 b[3];
+  };
+};
+typedef struct s2 __s2;
+
+struct s3 {
+  __s2 a;
+};
+typedef struct s3 __s3;
+
+int test(__s3 *arg) {
+  return arg->a.b[2].c;
+}
+
+// CHECK: call %struct.s2* @llvm.preserve.struct.access.index.p0s_struct.s2s.p0s_struct.s3s(%struct.s3* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: call %union.anon* @llvm.preserve.struct.access.index.p0s_union.anons.p0s_struct.s2s(%struct.s2* %{{[0-9a-z]+}}, i32

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

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

Let me take a look whether this is a *relatively easy way* to support sticky 
forward declaration (w.r.t. applying to inner structures).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

Yes, in the above, all three of them will be relocatable.

  -bash-4.4$ cat t2.c
  #define __reloc__ __attribute__((preserve_access_index))
   struct S1;
   struct S2 {
struct S1 *f;
   } __reloc__;
   struct S1 {
  int i;
   } __reloc__;
   // access s2->f
   // access s2->f->i
   int test(struct S2 *arg) {
 return arg->f->i;
   }
  -bash-4.4$ clang -target bpf -S -O2 -emit-llvm -g t2.c
  ...
%0 = tail call %struct.S1** 
@llvm.preserve.struct.access.index.p0p0s_struct.S1s.p0s_struct.S2s(%struct.S2* 
%arg, i32 0, i32 0
  ), !dbg !22, !llvm.preserve.access.index !12
%1 = load %struct.S1*, %struct.S1** %0, align 8, !dbg !22, !tbaa !23
%2 = tail call i32* 
@llvm.preserve.struct.access.index.p0i32.p0s_struct.S1s(%struct.S1* %1, i32 0, 
i32 0), !dbg !28, !llvm.pr
  eserve.access.index !16

In clang, BPF routines will see the record definition if it has the attribute. 
So in the above, the actual definition of
struct S1 and S2 will be seen and marked with attributes properly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexei Starovoitov via Phabricator via cfe-commits]

ast added a comment.

So what happens in the following case:

  struct S1;
  struct S2 {
struct S1 *f;
  } relo *s2;
  // access s2->f
  struct S1 {
int i;
  } relo;
  // access s2->f->i

lack of relo on fwd declaration is not going to be sticky 'non-relo' when it's 
seen later, right?
So all 3 deref will be relocatable?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

But it does not work for inner structs any more.
 -bash-4.4$ cat t.c
 #define __reloc__ __attribute__((preserve_access_index))
 struct __reloc__ p;
 struct p {

  int a;
  struct {
   int b;
  };

};

int test(struct p *arg) {

  return arg->b;

}
 -bash-4.4$

In only generates reloc for struct p's immediate members, but not inner 
structures.
The forward declaration gives empty record with attribute set, but this set 
does not
extend to inner record.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

For question

>   Is there a use case to apply that attribute to inner types automatically ?

It is possible, but I feel make attribute per record is better. For example,

  struct s1 {
   int foo;
  }; 
  struct s2 {
   struct s1 *ptr;
  } __reloc__ *s2;

If we implicitly mark struct `s1` as __reloc__, later user may have
another code like

  struct s1 *p;
  p->foo

he may be surprised by relocation.
If this is user intention to have relocation for struct `s1`,
I think explicit attribute is better.

Another question would be during IR generation can we magically generate 
relocation for `ptr->foo` in `s2->ptr->foo`? Without marking the record, this 
will require pass context sensitive information along code generation, which 
could be error prone as when to clear such context sensitive information. I 
would prefer explicit marking.  If user uses vmlinux.h, all struct/unions in 
vmlinux.h could have this attribute. If user defines their own structures for 
later relocation, I expect this should not be a huge amount and some manual 
work by users should be okay.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

> Is the attribute sticky with forward delcarations?

forward declaration is not a record type, so an error will be emited if you 
have attribute on a forward declaration.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexei Starovoitov via Phabricator via cfe-commits]

ast added a comment.

Is the attribute sticky with forward delcarations?

  struct s __reloc;
  
  struct s {
int foo;
  } *s;

what is s->foo ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song added a comment.

For the below:
 struct s1 {

  int foo;

};
 struct s2 {

  struct s1 *ptr;

} __reloc__ *s2;

s2->ptr->foo -- will both deref be relocatable or only first?
Only s2->ptr is relocated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Alexei Starovoitov via Phabricator via cfe-commits]

ast added a comment.

Looks great. This is a big improvement in usability.
Is there a use case to apply that attribute to inner types automatically ?

  struct s1 {
int foo;
  };
  struct s2 {
 struct s1 *ptr;
  } __reloc__ *s2;

s2->ptr->foo -- will both deref be relocatable or only first?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Yonghong Song via Phabricator via cfe-commits]

yonghong-song updated this revision to Diff 228474.
yonghong-song retitled this revision from "[RFC][BPF] Add preserve_access_index 
attribute to record definition" to "[BPF] Add preserve_access_index attribute 
for record definition".
yonghong-song edited the summary of this revision.
yonghong-song added a comment.

handling inner records and array subscript accesses. removing RFC tag.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69759

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGExpr.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/bpf-attr-preserve-access-index-1.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-2.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-3.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-4.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-5.c
  clang/test/CodeGen/bpf-attr-preserve-access-index-6.c
  clang/test/Sema/bpf-attr-preserve-access-index.c

Index: clang/test/Sema/bpf-attr-preserve-access-index.c
===
--- /dev/null
+++ clang/test/Sema/bpf-attr-preserve-access-index.c
@@ -0,0 +1,37 @@
+// RUN: %clang_cc1 -x c -triple bpf-pc-linux-gnu -dwarf-version=4 -fsyntax-only -verify %s
+
+#define __reloc__ __attribute__((preserve_access_index))
+
+struct t1 {
+  int a;
+  int b[4];
+  int c:1;
+} __reloc__;
+
+union t2 {
+  int a;
+  int b[4];
+  int c:1;
+} __reloc__;
+
+struct u3 {
+  union {
+int a;
+char b[5];
+  };
+  struct {
+int c:1;
+  } __reloc__;
+  int d;
+} __reloc__;
+
+int a __reloc__; // expected-error {{preserve_addess_index attribute only applies to struct or union type}}
+struct s *p __reloc__; // expected-error {{preserve_addess_index attribute only applies to struct or union type}}
+
+void invalid1(const int __reloc__ *arg) {} // expected-error {{preserve_addess_index attribute only applies to struct or union type}}
+void invalid2() { const int __reloc__ *arg; } // expected-error {{preserve_addess_index attribute only applies to struct or union type}}
+int valid3(struct u3 *arg) { return arg->a + arg->b[3] + arg->c + arg->d; }
+int valid4(void *arg) {
+  struct local_t { int a; int b; } __reloc__;
+  return ((struct local_t *)arg)->b;
+}
Index: clang/test/CodeGen/bpf-attr-preserve-access-index-6.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-attr-preserve-access-index-6.c
@@ -0,0 +1,32 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+#define __reloc__ __attribute__((preserve_access_index))
+
+// chain of records, both inner and outer record have attributes.
+struct s1 {
+  int c;
+} __reloc__;
+typedef struct s1 __s1;
+
+struct s2 {
+  union {
+__s1 b[3];
+  } __reloc__;
+} __reloc__;
+typedef struct s2 __s2;
+
+struct s3 {
+  __s2 a;
+} __reloc__;
+typedef struct s3 __s3;
+
+int test(__s3 *arg) {
+  return arg->a.b[2].c;
+}
+
+// CHECK: call %struct.s2* @llvm.preserve.struct.access.index.p0s_struct.s2s.p0s_struct.s3s(%struct.s3* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: call %union.anon* @llvm.preserve.struct.access.index.p0s_union.anons.p0s_struct.s2s(%struct.s2* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK: call %union.anon* @llvm.preserve.union.access.index.p0s_union.anons.p0s_union.anons(%union.anon* %{{[0-9a-z]+}}, i32 0)
+// CHECK: call %struct.s1* @llvm.preserve.array.access.index.p0s_struct.s1s.p0a3s_struct.s1s([3 x %struct.s1]* %{{[0-9a-z]+}}, i32 1, i32 2)
+// CHECK: call i32* @llvm.preserve.struct.access.index.p0i32.p0s_struct.s1s(%struct.s1* %{{[0-9a-z]+}}, i32 0, i32 0)
Index: clang/test/CodeGen/bpf-attr-preserve-access-index-5.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-attr-preserve-access-index-5.c
@@ -0,0 +1,32 @@
+// REQUIRES: bpf-registered-target
+// RUN: %clang -target bpf -emit-llvm -S -g %s -o - | FileCheck %s
+
+#define __reloc__ __attribute__((preserve_access_index))
+
+// chain of records, attribute may be in inner record.
+struct s1 {
+  int c;
+} __reloc__;
+typedef struct s1 __s1;
+
+struct s2 {
+  union {
+__s1 b[3];
+  } __reloc__;
+};
+typedef struct s2 __s2;
+
+struct s3 {
+  __s2 a;
+} __reloc__;
+typedef struct s3 __s3;
+
+int test(__s3 *arg) {
+  return arg->a.b[2].c;
+}
+
+// CHECK: call %struct.s2* @llvm.preserve.struct.access.index.p0s_struct.s2s.p0s_struct.s3s(%struct.s3* %{{[0-9a-z]+}}, i32 0, i32 0)
+// CHECK-NOT: call %union.anon* @llvm.preserve.struct.access.index.p0s_union.anons.p0s_struct.s2s
+// CHECK: call %union.anon* @llvm.preserve.union.access.index.p0s_union.anons.p0s_union.anons(%union.anon* %{{[0-9a-z]+}}, i32 0)
+// CHECK: call %struct.s1* @llvm.preserve.array.access.index.p0s_struct.s1s.p0a3s_struct.s1s([3 x %struct.s1]