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

[Yonghong Song via Phabricator via cfe-commits]

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. __preserve_static_offset is for base pointer while
__preserve_access_index is for offset. So the current implementation seems 
OKAY. But __preserve_static_offset actually means 'static offset' (no offset 
change),
so __preserve_access_index seems not necessary, so if possible maybe clang 
frontend should make a choice to remove __preserve_access_index if
__preserve_static_offset is there. But this can be done later (need frontend 
review again, need to update doc etc.)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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

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

[Yonghong Song via Phabricator via cfe-commits]

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: clang/include/clang/Basic/AttrDocs.td:2227
+  void buz(struct bar *g) {
+g->a.b = 42;
+  }

g->b.a ?



Comment at: llvm/lib/Target/BPF/BPFPreserveStaticOffset.cpp:82
+//   for (int i = 0; i < 4; ++i)
+// foo(ctx->x[i]);
+//

Is it possible that such a pattern already changed by GVN/CSE so later on 
converting to preserve_static_offset becomes impossible? I guess it is unlikely 
but want to double check. Maybe add a comment to clarify.



Comment at: llvm/lib/Target/BPF/BPFPreserveStaticOffset.cpp:102
+// - the (get.and.load %p) would come from the callee function.
+// Thus clobbering CSE / GVN passes done after inlining.
+

So the above implies that this is bad for CSE/GVN due to different 
representation, right? But we want to do (1) since we want to avoid later 
CSE/GVN etc, so argument of clobbering is not that important, right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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

[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:3791
+  if (hasBPFPreserveStaticOffset(Base))
+addr = wrapWithBPFPreserveStaticOffset(CGF, addr);
+

ast wrote:
> eddyz87 wrote:
> > ast wrote:
> > > eddyz87 wrote:
> > > > ast wrote:
> > > > > If I'm reading this correctly wrapping with preserve_static_offset 
> > > > > doesn't prevent further preserver_access_index wrapping which is a 
> > > > > wasted effort for pai at the end ?
> > > > Yes, pai calls are undone in 
> > > > `BPFPreserveStaticOffset.cpp:removePAICalls()`. I can put back the 
> > > > logic that suppresses pai if preserve static offset is present.
> > > I see. I guess I missed a previous discussion. Why this approach was 
> > > chosen?
> > Initial version used `__attribute__((btf_decl_tag("ctx")))` and Yonghong 
> > did not want to have prioritization between `btf_decl_tag` and 
> > `preserve_access_index` basing on decl tag string parameter. Now this 
> > limitation is gone (and I think this was one of your arguments in favor of 
> > separate attribute).
> Ahh. Right. It made sense to avoid special treatment of strings in decl_tag, 
> but now it's gone and PSO takes precedence over PAI. Here we're adding PAI 
> just to remove it later. Looks like a waste of cpu cycles and code. Unless 
> applying PAI to outer struct and PSO in inner makes implementation tricky. I 
> doubt we need to support such combo though. I'm fine cleaning this up in a 
> follow up. If such cleanup makes sense at all.
> If such cleanup makes sense at all.

I think it does, I'll implement this change and push an update.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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

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

[Alexei Starovoitov via Phabricator via cfe-commits]

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 wrote:
> > > > If I'm reading this correctly wrapping with preserve_static_offset 
> > > > doesn't prevent further preserver_access_index wrapping which is a 
> > > > wasted effort for pai at the end ?
> > > Yes, pai calls are undone in 
> > > `BPFPreserveStaticOffset.cpp:removePAICalls()`. I can put back the logic 
> > > that suppresses pai if preserve static offset is present.
> > I see. I guess I missed a previous discussion. Why this approach was chosen?
> Initial version used `__attribute__((btf_decl_tag("ctx")))` and Yonghong did 
> not want to have prioritization between `btf_decl_tag` and 
> `preserve_access_index` basing on decl tag string parameter. Now this 
> limitation is gone (and I think this was one of your arguments in favor of 
> separate attribute).
Ahh. Right. It made sense to avoid special treatment of strings in decl_tag, 
but now it's gone and PSO takes precedence over PAI. Here we're adding PAI just 
to remove it later. Looks like a waste of cpu cycles and code. Unless applying 
PAI to outer struct and PSO in inner makes implementation tricky. I doubt we 
need to support such combo though. I'm fine cleaning this up in a follow up. If 
such cleanup makes sense at all.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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

[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:3791
+  if (hasBPFPreserveStaticOffset(Base))
+addr = wrapWithBPFPreserveStaticOffset(CGF, addr);
+

ast wrote:
> eddyz87 wrote:
> > ast wrote:
> > > If I'm reading this correctly wrapping with preserve_static_offset 
> > > doesn't prevent further preserver_access_index wrapping which is a wasted 
> > > effort for pai at the end ?
> > Yes, pai calls are undone in 
> > `BPFPreserveStaticOffset.cpp:removePAICalls()`. I can put back the logic 
> > that suppresses pai if preserve static offset is present.
> I see. I guess I missed a previous discussion. Why this approach was chosen?
Initial version used `__attribute__((btf_decl_tag("ctx")))` and Yonghong did 
not want to have prioritization between `btf_decl_tag` and 
`preserve_access_index` basing on decl tag string parameter. Now this 
limitation is gone (and I think this was one of your arguments in favor of 
separate attribute).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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

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

[Alexei Starovoitov via Phabricator via cfe-commits]

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 preserve_static_offset doesn't 
> > prevent further preserver_access_index wrapping which is a wasted effort 
> > for pai at the end ?
> Yes, pai calls are undone in `BPFPreserveStaticOffset.cpp:removePAICalls()`. 
> I can put back the logic that suppresses pai if preserve static offset is 
> present.
I see. I guess I missed a previous discussion. Why this approach was chosen?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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

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

[Eduard Zingerman via Phabricator via cfe-commits]

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 wrapping with preserve_static_offset doesn't 
> prevent further preserver_access_index wrapping which is a wasted effort for 
> pai at the end ?
Yes, pai calls are undone in `BPFPreserveStaticOffset.cpp:removePAICalls()`. I 
can put back the logic that suppresses pai if preserve static offset is present.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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

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

[Alexei Starovoitov via Phabricator via cfe-commits]

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 further preserver_access_index wrapping which is a wasted effort for 
pai at the end ?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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

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

[Eduard Zingerman via Phabricator via cfe-commits]

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:
> > erichkeane wrote:
> > > We override `operator bool` to make this work.
> > Sorry, just to clarify, currently such modification fails with the 
> > following error:
> > 
> > ```
> > lang=c++
> > clang/lib/CodeGen/CGExpr.cpp:3710:7: error: invalid argument type 
> > 'QualType' to unary expression
> >   if (!PointeeType)
> >   ^~~~
> > 1 error generated.
> > ```
> > 
> > And you want me to modify `QualType` as follows:
> > 
> > ```
> > --- a/clang/include/clang/AST/Type.h
> > +++ b/clang/include/clang/AST/Type.h
> > @@ -796,6 +796,8 @@ public:
> >  return getTypePtr();
> >}
> >  
> > +  explicit operator bool() const { return isNull(); }
> > +
> >bool isCanonical() const;
> >bool isCanonicalAsParam() const;
> > ```
> > 
> > Right?
> No, don't do that, you can leave it just checking isNull.  I could have sworn 
> we already had that operator, but perhaps it was removed at one point.
Understood.
Thank you for the review.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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

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

[Erich Keane via Phabricator via cfe-commits]

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 this work.
> Sorry, just to clarify, currently such modification fails with the following 
> error:
> 
> ```
> lang=c++
> clang/lib/CodeGen/CGExpr.cpp:3710:7: error: invalid argument type 'QualType' 
> to unary expression
>   if (!PointeeType)
>   ^~~~
> 1 error generated.
> ```
> 
> And you want me to modify `QualType` as follows:
> 
> ```
> --- a/clang/include/clang/AST/Type.h
> +++ b/clang/include/clang/AST/Type.h
> @@ -796,6 +796,8 @@ public:
>  return getTypePtr();
>}
>  
> +  explicit operator bool() const { return isNull(); }
> +
>bool isCanonical() const;
>bool isCanonicalAsParam() const;
> ```
> 
> Right?
No, don't do that, you can leave it just checking isNull.  I could have sworn 
we already had that operator, but perhaps it was removed at one point.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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

[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:3701
+  QualType PointeeType = E->getType()->getPointeeType();
+  if (PointeeType.isNull())
+return false;

erichkeane wrote:
> We override `operator bool` to make this work.
Sorry, just to clarify, currently such modification fails with the following 
error:

```
lang=c++
clang/lib/CodeGen/CGExpr.cpp:3710:7: error: invalid argument type 'QualType' to 
unary expression
  if (!PointeeType)
  ^~~~
1 error generated.
```

And you want me to modify `QualType` as follows:

```
--- a/clang/include/clang/AST/Type.h
+++ b/clang/include/clang/AST/Type.h
@@ -796,6 +796,8 @@ public:
 return getTypePtr();
   }
 
+  explicit operator bool() const { return isNull(); }
+
   bool isCanonical() const;
   bool isCanonicalAsParam() const;
```

Right?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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

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

[Erich Keane via Phabricator via cfe-commits]

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 PointeeType = E->getType()->getPointeeType();
+  if (PointeeType.isNull())
+return false;

We override `operator bool` to make this work.



Comment at: clang/lib/CodeGen/CGExpr.cpp:3703
+return false;
+  if (auto *BaseDecl = PointeeType->getAsRecordDecl())
+return hasBPFPreserveStaticOffset(BaseDecl);

if possible?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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

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

[Eduard Zingerman via Phabricator via cfe-commits]

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}

erichkeane wrote:
> eddyz87 wrote:
> > 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 
> > (g...@github.com:llvm/llvm-project.git 
> > 7f9a008a2db285aca57bfa0c09858c9527a7aa98)"}
> > ```
> It seems at least removing your home-path would be a good idea, but I can't 
> really review these either.
> 
> Note the 'trunk #' is from our SVN days, so that isn't particularly 
> useful at all.  IMO everything in the parens is worthless in the tests, but 
> hopefully someone familiar with the LLVM tests can stop by and correct me.
Most of the tests use generated IR as a starting point and it looks like people 
don't bother to hide these details. But I checked and test passes if I replace 
this string with just "clang", I'll update the tests, not a big deal.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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

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

[Erich Keane via Phabricator via cfe-commits]

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 128}

eddyz87 wrote:
> 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 
> (g...@github.com:llvm/llvm-project.git 
> 7f9a008a2db285aca57bfa0c09858c9527a7aa98)"}
> ```
It seems at least removing your home-path would be a good idea, but I can't 
really review these either.

Note the 'trunk #' is from our SVN days, so that isn't particularly useful 
at all.  IMO everything in the parens is worthless in the tests, but hopefully 
someone familiar with the LLVM tests can stop by and correct me.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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

[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())
> > 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 
(g...@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

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

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

[Erich Keane via Phabricator via cfe-commits]

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:
> > 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())
> 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`.



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}

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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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

[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:
> 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())
return PT->getPointeeType();
  ...
  return {};
}
```

The `getAsRecordDecl()` can return null indeed, but that null is checked.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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

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

[Eduard Zingerman via Phabricator via cfe-commits]

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);
+  Rec->addAttr(::new (S.Context) BPFPreserveStaticOffsetAttr(S.Context, AL));
+}

erichkeane wrote:
> 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.
Replaced by a call to `handleSimpleAttribute` as suggested by Aaron.



Comment at: clang/test/Sema/bpf-attr-preserve-static-offset.c:1
+// RUN: %clang_cc1 -fsyntax-only -ast-dump -triple bpf-pc-linux-gnu %s | 
FileCheck %s
+

aaron.ballman wrote:
> You should also add a `-verify` test that verifies we diagnose applying the 
> attribute to something other than a structure or union, accepts no arguments, 
> is diagnosed on a non-BPF target, etc to ensure we've got correct diagnostic 
> behavior.
`-verify` looks neat, thank you.

I've added two test cases:
- clang/test/Sema/bpf-attr-preserve-static-offset-warns.c
- clang/test/Sema/bpf-attr-preserve-static-offset-warns-nonbpf.c

Those check for things you listed:
- can't be used for non-bpf target
- can't take parameters
- is error to put it on something other than struct / union.

Tbh, I don't know if there is anything else to test in this regard.



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:
> 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
```


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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

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

[Erich Keane via Phabricator via cfe-commits]

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 "clang version"
> metadata comment by @erichkeane, I added inline reply there.
>
>> This will also need reviewers for the LLVM changes -- any ideas on
>> who usually reviews BPF-related changes in LLVM?
>
> I'll communicate with @ast and @yonghong-song.

I don't see the comment response you had to me.




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

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).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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

[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();
+}





Comment at: clang/lib/CodeGen/CGExpr.cpp:3698
+static bool hasBPFPreserveStaticOffset(const Expr *E) {
+  if (auto *PtrType = dyn_cast(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(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

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

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

[Aaron Ballman via Phabricator via cfe-commits]

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 usually reviews 
BPF-related changes in LLVM?




Comment at: clang/lib/Sema/SemaDeclAttr.cpp:7597-7604
 
+static void handleBPFPreserveStaticOffsetAttr(Sema , Decl *D,
+  const ParsedAttr ) {
+  auto *Rec = cast(D);
+  Rec->addAttr(::new (S.Context) BPFPreserveStaticOffsetAttr(S.Context, AL));
+}
+





Comment at: clang/lib/Sema/SemaDeclAttr.cpp:8871
+  case ParsedAttr::AT_BPFPreserveStaticOffset:
+handleBPFPreserveStaticOffsetAttr(S, D, AL);
+break;





Comment at: clang/test/Sema/bpf-attr-preserve-static-offset.c:1
+// RUN: %clang_cc1 -fsyntax-only -ast-dump -triple bpf-pc-linux-gnu %s | 
FileCheck %s
+

You should also add a `-verify` test that verifies we diagnose applying the 
attribute to something other than a structure or union, accepts no arguments, 
is diagnosed on a non-BPF target, etc to ensure we've got correct diagnostic 
behavior.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D133361

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