[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D143967#4656478 , @dblaikie wrote:

> Fair enough - all seems a bit unfortunate to be pushing these attributes 
> through to places they don't technically apply to (both complicates the 
> implementation, and might be confusing to users).

Thank you for taking a look!
I'll wait till the last part of the commit stack (BPF-specific) is ironed out 
and apply all three patches.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 updated this revision to Diff 558086.
eddyz87 marked 2 inline comments as done.
eddyz87 added a comment.

Rebase, fixes for review feedback.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/test/CodeGen/attr-btf_type_tag-circular.c
  clang/test/CodeGen/attr-btf_type_tag-const.c
  clang/test/CodeGen/attr-btf_type_tag-func-ptr.c
  clang/test/CodeGen/attr-btf_type_tag-func.c
  clang/test/CodeGen/attr-btf_type_tag-restrict.c
  clang/test/CodeGen/attr-btf_type_tag-similar-type.c
  clang/test/CodeGen/attr-btf_type_tag-typedef-field.c
  clang/test/CodeGen/attr-btf_type_tag-var.c
  clang/test/CodeGen/attr-btf_type_tag-void.c
  clang/test/CodeGen/attr-btf_type_tag-volatile.c
  llvm/include/llvm/IR/DIBuilder.h
  llvm/lib/IR/DIBuilder.cpp

Index: llvm/lib/IR/DIBuilder.cpp
===
--- llvm/lib/IR/DIBuilder.cpp
+++ llvm/lib/IR/DIBuilder.cpp
@@ -342,6 +342,17 @@
 Annotations);
 }
 
+DIDerivedType *
+DIBuilder::createAnnotationsPlaceholder(DIType *Ty, DINodeArray Annotations) {
+  auto *RetTy =
+  DIDerivedType::getTemporary(
+  VMContext, dwarf::DW_TAG_LLVM_annotation, "", nullptr, 0, nullptr, Ty,
+  0, 0, 0, std::nullopt, DINode::FlagZero, nullptr, Annotations)
+  .release();
+  trackIfUnresolved(RetTy);
+  return RetTy;
+}
+
 DIDerivedType *DIBuilder::createFriend(DIType *Ty, DIType *FriendTy) {
   assert(Ty && "Invalid type!");
   assert(FriendTy && "Invalid friend type!");
Index: llvm/include/llvm/IR/DIBuilder.h
===
--- llvm/include/llvm/IR/DIBuilder.h
+++ llvm/include/llvm/IR/DIBuilder.h
@@ -294,6 +294,9 @@
  DINode::DIFlags Flags = DINode::FlagZero,
  DINodeArray Annotations = nullptr);
 
+DIDerivedType *createAnnotationsPlaceholder(DIType *Ty,
+DINodeArray Annotations);
+
 /// Create debugging information entry for a 'friend'.
 DIDerivedType *createFriend(DIType *Ty, DIType *FriendTy);
 
Index: clang/test/CodeGen/attr-btf_type_tag-volatile.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-btf_type_tag-volatile.c
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 \
+// RUN:   -triple %itanium_abi_triple -debug-info-kind=limited \
+// RUN:   -S -emit-llvm -o - %s | FileCheck %s
+
+// See attr-btf_type_tag-const.c for reasoning behind this test.
+// Alternatively, see the following method:
+//   CGDebugInfo::CreateType(const BTFTagAttributedType, llvm::DIFile)
+
+#define __tag1 __attribute__((btf_type_tag("tag1")))
+
+volatile int foo;
+typeof(foo) __tag1 bar;
+
+// CHECK: ![[#]] = distinct !DIGlobalVariable(name: "bar", {{.*}}, type: ![[L1:[0-9]+]], {{.*}})
+// CHECK: ![[L1]] = !DIDerivedType(tag: DW_TAG_volatile_type, baseType: ![[L2:[0-9]+]])
+// CHECK: ![[L2]] = !DIBasicType(name: "int", size: [[#]], {{.*}}, annotations: ![[L3:[0-9]+]])
+// CHECK: ![[L3]] = !{![[L4:[0-9]+]]}
+// CHECK: ![[L4]] = !{!"btf:type_tag", !"tag1"}
Index: clang/test/CodeGen/attr-btf_type_tag-void.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-btf_type_tag-void.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -debug-info-kind=limited -S -emit-llvm -o - %s | FileCheck %s
+
+#define __tag1 __attribute__((btf_type_tag("tag1")))
+void __tag1 *g;
+
+// CHECK: distinct !DIGlobalVariable(name: "g", scope: ![[#]], file: ![[#]], line: [[#]], type: ![[L1:[0-9]+]], isLocal: false, isDefinition: true)
+// CHECK: ![[L1]] = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: ![[L2:[0-9]+]], size: [[#]], annotations: ![[L3:[0-9]+]])
+// CHECK: ![[L2]] = !DIBasicType(tag: DW_TAG_unspecified_type, name: "void", annotations: ![[L4:[0-9]+]])
+// CHECK: ![[L4]] = !{![[L5:[0-9]+]]}
+// CHECK: ![[L5]] = !{!"btf:type_tag", !"tag1"}
+// CHECK: ![[L3]] = !{![[L6:[0-9]+]]}
+// CHECK: ![[L6]] = !{!"btf_type_tag", !"tag1"}
Index: clang/test/CodeGen/attr-btf_type_tag-var.c
===
--- clang/test/CodeGen/attr-btf_type_tag-var.c
+++ clang/test/CodeGen/attr-btf_type_tag-var.c
@@ -21,23 +21,30 @@
 const int __tag1 __tag2 volatile * const __tag3  __tag4  volatile * __tag5  __tag6 const volatile * g;
 #endif
 
-// CHECK:  distinct !DIGlobalVariable(name: "g", scope: ![[#]], file: ![[#]], line: [[#]], type: ![[L6:[0-9]+]]
-// CHECK:  ![[L6]] = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: ![[L7:[0-9]+]], size: [[#]], annotations: ![[L22:[0-9]+]]
-// CHECK:  ![[L7]] = !DIDerivedType(tag: DW_TAG_const_type, baseType: ![[L8:[0-9]+]]
-// CHECK:  ![[L8]] = !DIDerivedType(tag: DW_TAG_volatile_type, 

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

Hi @dblaikie,

If you have some time, could you please take a look at my response 
?
It would be great if decision on this revision could be reached before next 
LLVM release.
The overall design of this thing was discussed with GCC BPF team and we really 
would like to have identical implementation for LLVM and GCC.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

___
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

[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

[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

[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

[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

[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

[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

[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 btf_decl_tag("ctx") for structs

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D133361#4629292 , @ast wrote:

> ...
> Right. Such recursive propagation of PAI is necessary. For btf_tag we cannot 
> do it. Always propagating it won't be correct.
> New preserve_const_field_offset would need to be propagated too and we 
> actually have nested unions.
> __bpf_md_ptr is such example. btf_tag wouldn't propagate into that union, but 
> attr(preserve_const_field_offset) should.

Hi Alexei,

It just occurred to me that such an attribute would also require DWARF and BTF 
encoding in order to get reflected in vmlinux.h (which we already have for 
btf_decl_tag). Given this I think we can rename decl tag "ctx" to 
`btf_decl_tag("preserve_const_field_offset")` but we should still keep it a 
`btf_decl_tag`. I'll try to replace usage of `bpf_context_marker` intrinsic by 
metadata, if that fails will just rename the intrinsic to 
`preserve_const_field_offset`.
What do you think? (Sorry, I should have thought this through last week).


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 btf_decl_tag("ctx") for structs

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

After retesting kernel build with LLVM=1 and libbpf patch 
 to reconstruct 
btf_decl_tag [1], statistics for BPF selftests looks as follows:

- out of 653 object files 13 have some differences with and w/o this change;
- for 2 programs there is small instruction count increase (+2 insn total);
- for 5 programs there is small instruction decrease (-6 insn total);
- 6 programs differ slightly but number of instructions is the same.

(The differences are insignificant, the rest of the comment could be skipped as 
it is probably not interesting for anyone but me).

---

Differences for first 5 programs were already described in this 
 comment. The rest of the differences 
in described below.

netns_cookie_prog.bpf.o
---

Without ctx: 46 instructions
With ctx: 46 instructions

Instruction reordering:

   :
r6 = r1
  - r2 = *(u64 *)(r6 + 0x48)
r1 = *(u32 *)(r6 + 0x10)
  - if w1 != 0xa goto +0xb 
  + if w1 != 0xa goto +0xc 
  + r2 = *(u64 *)(r6 + 0x48)
if r2 == 0x0 goto +0xa 
r1 = 0x0 ll
r3 = 0x0

The difference is introduced by "Machine code sinking" transformation. Before 
the transformation both 0x48 and 0x10 loads reside in the same basic block:

  ;; Old:
  bb.0.entry:
...
%0:gpr = CORE_LD64 345, %2:gpr, @"llvm.sk_msg_md:0:72$0:10:0"
%9:gpr32 = CORE_LD32 350, %2:gpr, @"llvm.sk_msg_md:0:16$0:2"
JNE_ri_32 killed %9:gpr32, 10, %bb.3
  
  ;; New:
  bb.0.entry:
...
%0:gpr = LDD %2:gpr, 72
%3:gpr32 = LDW32 %2:gpr, 16
JNE_ri_32 killed %3:gpr32, 10, %bb.3

Note: CORE pseudo-instructions are replaced by regular loads because 
btf_decl_tag("ctx") has priority over preserve_access_index attribute. The 
"Machine code sinking" transformation (MachineSink.cpp) can move `LDD`, `LDW` 
instructions, but can't move `CORE_LD*` because `CORE_LD*` instructions are 
marked as `MCID::UnmodeledSideEffects` in `BPFGenInstrInfo.inc` (maybe 
something to adjust):

  // called from MachineSinking::SinkInstruction
  bool MachineInstr::isSafeToMove(AAResults *AA, bool ) const {
if (... hasUnmodeledSideEffects())
  return false;
...
  }



sock_destroy_prog.bpf.o
---

Without ctx: 102 instructions
With ctx: 101 instructions

In the following code fragment:

if (ctx->protocol == IPPROTO_TCP)
bpf_map_update_elem(_conn_sockets, , _cookie, 0);
else if (ctx->protocol == IPPROTO_UDP)
bpf_map_update_elem(_conn_sockets, , _cookie, 0);
else
return 1;

Version w/o btf_decl_tag("ctx") keeps two loads for `ctx->protocol` because of 
the llvm.bpf.passthrough call. Version with btf_decl_tag("ctx") eliminates 
second load via a combination of EarlyCSEPass/InstCombinePass/SimplifyCFGPass 
passes.

socket_cookie_prog.bpf.o


Without ctx: 66 instructions
With ctx: 66 instructions

For the following C code fragment:

  SEC("sockops")
  int update_cookie_sockops(struct bpf_sock_ops *ctx)
  {
struct bpf_sock *sk = ctx->sk;
struct socket_cookie *p;
  
if (ctx->family != AF_INET6)
return 1;
  
if (ctx->op != BPF_SOCK_OPS_TCP_CONNECT_CB)
return 1;
  
if (!sk)
return 1;
  ...
  }

Code with decl_tag("ctx") does reordering for ctx->sk load relative to 
ctx->family and ctx->op loads:

  //  old   new
   :  :
  r6 = r1   r6 = r1
  -   r2 = *(u64 *)(r6 + 0xb8)
  r1 = *(u32 *)(r6 + 0x14)  r1 = *(u32 *)(r6 + 0x14)
  if w1 != 0xa goto +0x13   if w1 != 0xa goto +0x14 
  r1 = *(u32 *)(r6 + 0x0)   r1 = *(u32 *)(r6 + 0x0)
  if w1 != 0x3 goto +0x11   if w1 != 0x3 goto +0x12 
+   r2 = *(u64 *)(r6 + 0xb8)
  if r2 == 0x0 goto +0x10   if r2 == 0x0 goto +0x10 
  r1 = 0x0 ll   r1 = 0x0 ll
  r3 = 0x0  r3 = 0x0

Code w/o decl_tag("ctx") uses `CORE_LD*` instructions for these loads and does 
not reorder loads due to reasons as in netns_cookie_prog.bpf.o.

test_lwt_reroute.bpf.o
--

Without ctx: 18 instructions
With ctx: 17 instructions

The difference boils down EarlyCSEPass being able to remove last load in the 
store/load pair:

  llvm
  ; Before EarlyCSEPass
store i32 %and, ptr %mark24, align 8
%mark25 = getelementptr inbounds %struct.__sk_buff, ptr %skb, i32 0, i32 2
%19 = load i32, ptr %mark25, align 8
%cmp26 = icmp eq i32 %19, 0
  ; After EarlyCSEPass
%and = and i32 %cond, 255
store i32 %and, ptr %mark, align 8
%cmp26 = icmp eq i32 %and, 0

And unable to do so when get.element.and.{store,load} intrinsics are used. 

[PATCH] D133361: [BPF] Attribute btf_decl_tag("ctx") for structs

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D133361#4628951 , @ast wrote:

> I agree that it's not useful outside of BPF, but it's useful outside of 
> 'ctx'. I think 'preserve_constant_field_offset' would be more accurate 
> description of the restriction.
> We can expand in the doc that it's a constant offset when field of the struct 
> is accessed.
>
> Also instead of btf_tag it would be better to add another builtin similar to 
> preserve_access_index.
> Currently we add __attribute__((preserve_access_index)) to trigger CO-RE.
> This one will be a new __attribute__((preserve_constant_field_offset)) that 
> will be specified manually either in uapi/bpf.h or in vmlinux.h on some 
> structs

This makes sense, I'll adjust the implementation to use new attribute (also 
will try to use metadata instead of intrinsic call, replace by intrinsic on the 
back-end side).

> and it will have precedence over preserve_access_index, so
> (__attribute__((preserve_access_index)), apply_to = record) in vmlinux.h will 
> be ignored on such structs.
> Otherwise it's a bit odd that special names inside btf_tag have stronger 
> rules than other __attribute__((preserve_access_index)).

Note on current propagation logic: whenever there is an expression E of type T, 
where T is a struct annotated with btf_decl_tag("ctx"), all usages of  E are 
traversed recursively visiting `getelementptr` and calls to 
`preserve_{struct,union,array}_index`, the latter are replaced by 
`getelementptr`. E.g.:

  #define __pai __attribute__((preserve_access_context));
  #define __ctx __attribute__((btf_decl_tag("ctx")))
  struct bar { int a; int b; } __pai;
  struct foo {
struct bar b;
  } __ctx __pai;
  ... struct foo f; ...
  ... f.b.bb ...

The call to `preserve_struct_index` generated for `bb` in `f.b.bb` would be 
replaced by `getelementptr` and later by `getelementptr.and.load`.
However, context structures don't have nested structures at the moment. The 
list of context structures is:

- __sk_buff
- bpf_cgroup_dev_ctx
- bpf_sk_lookup
- bpf_sock
- bpf_sock_addr
- bpf_sock_ops
- bpf_sockopt
- bpf_sysctl
- sk_msg_md
- sk_reuseport_md
- xdp_md


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 btf_decl_tag("ctx") for structs

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added inline comments.



Comment at: llvm/include/llvm/IR/Intrinsics.td:2432
   ImmArg>]>;
+def int_context_marker_bpf : DefaultAttrsIntrinsic<[llvm_ptr_ty],
+   [llvm_ptr_ty],

yonghong-song wrote:
> eddyz87 wrote:
> > yonghong-song wrote:
> > > Is it possible to make this builtin as BPF specific one?
> > Currently `llvm::Intrinsic::context_marker_bpf` gets defined in 
> > `llvm/IR/Intrinsics.h` (via include of generated file `IntrinsicEnums.inc`, 
> > same as `preserve_{struct,union,array}_access_index`).
> > 
> > BPF specific intrinsics are defined in `llvm/IR/IntrinsicsBPF.h` (generated 
> > directly w/o .inc intermediary).
> > 
> > Thus, if I move `context_marker_bpf` to `IntrinsicsBPF.td` I would have to 
> > include `IntrinsicsBPF.h` in `CGExpr.cpp`. However, I don't see any target 
> > specific includes in that file.
> > 
> I went through the related clang code and indeed found it is hard to get a 
> BPF target defined function in CGF or CGM. On the other hand, we can consider 
> this new builtin under the umbrella "Intrinsics that are used to preserve 
> debug information".  Maybe we can rename the intrinsic name
> to 'int_preserve_context_marker'? The goal of this builtin to preserve 
> certain load/store which should be immune from optimizations. I try to 
> generalize this so your function name 'wrapWithBPFContextMarker' can be 
> renamed to
> 'wrapWithContextMarker'. There is no need to mention BPF any more.
> 
> In the commit message, you can mention that
> similar to int_preserve_array_access_index, int_preserve_context_marker is 
> only implemented in BPF backend. But other architectures can implement 
> processing these intrinsics if they want to achieve some results similar to 
> bpf backend.
> 
> WDYT? 
I can rename these things, but tbh I don't think this functionality would be 
useful anywhere outside BPF, thus such renaming would be kind-of deceptive (and 
in case it would be useful, the renaming could be done at the time of second 
use).

---

Something more generic might look like `!btf_decl_tag ` metadata 
node attached to something. However, in the current form this would require 
transfer of such decl tag from type to function parameters and variables, e.g.:

```
lang=c
struct foo {  ... } __attribute__((btf_decl_tag("ctx")));
void bar(struct foo *p) { ... }
```

During code-gen for `bar` use rule like "if function parameter has type 
annotated with btf_decl_tag, attach metadata to such parameter":

```
lang=llvm
define void @bar(ptr %p !btf_decl_tag !1) { ... }
!1 = { "ctx" }
```

Such rule looks a bit weird:
- tag is transferred from type to it's usage
- what usages should be annotated? we care about function parameters but from 
generic point of view `alloca`s or field accesses should be annotated as well.

---

The same metadata approach but with "ctx" attributes annotating function 
parameters (as you suggested originally, if I recall correctly) seems to be 
most generic and least controversial of all, e.g.:

```
lang=c
void bar(struct foo *p __attribute__((btf_decl_tag("ctx" { ... }
```

Converted to:

```
lang=llvm
define void @bar(ptr %p !btf_decl_tag !1) { ... }
!1 = { "ctx" }
```

However, this is less ergonomic for BPF, because user will have to annotate 
function parameters. (On the other hand, no changes in the kernel headers would 
be necessary).


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 btf_decl_tag("ctx") for structs

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added inline comments.



Comment at: llvm/include/llvm/IR/Intrinsics.td:2432
   ImmArg>]>;
+def int_context_marker_bpf : DefaultAttrsIntrinsic<[llvm_ptr_ty],
+   [llvm_ptr_ty],

yonghong-song wrote:
> Is it possible to make this builtin as BPF specific one?
Currently `llvm::Intrinsic::context_marker_bpf` gets defined in 
`llvm/IR/Intrinsics.h` (via include of generated file `IntrinsicEnums.inc`, 
same as `preserve_{struct,union,array}_access_index`).

BPF specific intrinsics are defined in `llvm/IR/IntrinsicsBPF.h` (generated 
directly w/o .inc intermediary).

Thus, if I move `context_marker_bpf` to `IntrinsicsBPF.td` I would have to 
include `IntrinsicsBPF.h` in `CGExpr.cpp`. However, I don't see any target 
specific includes in that file.



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] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

Hi @dblaikie,

If you have some time, could you please take a look at my response 
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

As an additional data point, the same example but w/o section specification 
compiles fine:

  const int with_init = 1;
  const int no_init;

And puts both globals to the same section:

  $ clang -c t.c -o - | llvm-readelf --section-headers -s -
  Section Headers:
[Nr] Name  TypeAddress  OffSize   ES 
Flg Lk Inf Al
...
[ 3] .rodata   PROGBITS 40 08 00   
A  0   0  4
...
  
  Symbol table '.symtab' contains 4 entries:
 Num:Value  Size TypeBind   Vis   Ndx Name
   ...
   2:  4 OBJECT  GLOBAL DEFAULT 3 with_init
   3: 0004 4 OBJECT  GLOBAL DEFAULT 3 no_init

(`Ndx` stands for section `Nr`).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156726

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

[PATCH] D156726: Make globals with mutable members non-constant, even in custom sections

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

Hi @dblaikie,

After this revision landed yesterday one BPF kernel selftest (written in C) 
stopped building. I narrowed the issue to the following example:

  #define SEC(n) __attribute__((section(n)))
  
  const int with_init SEC("foo") = 1;
  const int no_init SEC("foo");

It fails with the following error:

  $ clang -c t.c -o -
  t.c:4:11: error: 'no_init' causes a section type conflict with 'with_init'
  4 | const int no_init SEC("foo");
|   ^
  t.c:3:11: note: declared here
  3 | const int with_init SEC("foo") = 1;
|   ^
  1 error generated.

The error occurs because clang infers `PSF_Read` attribute for section "foo" 
when `with_init` is processed and `PSF_Read | PSF_Write` attributes for section 
"foo" when `no_init` is processed, thus reporting `diag::err_section_conflict`. 
The behavior changed because of the following lines introduced in this revision:

  void Sema::CheckCompleteVariableDeclaration(VarDecl *var) {
...
if (GlobalStorage && var->isThisDeclarationADefinition() &&
!inTemplateInstantiation()) {
  ...
  if (var->hasInit() && HasConstInit && !(Reason =
  var->getType().isNonConstantStorage(Context, true, false))) {
Stack = 
  } else {
SectionFlags |= ASTContext::PSF_Write;
Stack = var->hasInit() && HasConstInit ?  : 
  }
  ...
}
...
  }

Because of the `var->hasInit()` check any global w/o initializer gets a 
`PSF_Write` flag, which was not the case before this revision. So, now if one 
wants to have some `const` globals in the same section, these globals need to 
have an initializer.

I checked C language standard (N3088, section "6.7.3 Type qualifiers") and it 
does not offer much to tell if my example is correct or not, so I guess it is 
implementation dependent. GCC accepts my example w/o errors or warnings.

So, a question: might it be the case that `var->hasInit()` check is too 
restrictive?
(I do understand that w/o some custom linker behavior `const` globals w/o 
initializer don't make much sense).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D156726

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

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

Hi @dblaikie,

Could you please take a look at my response 
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

Hi @dblaikie

Thank you for the detailed response!

> So you get some bunch of annotations from the BTFTagAttributedType, then you 
> build the underlying type (which might already be built/needed/fine because 
> it's used without attributes in other places/needs to exist independently) - 
> and then at the end you copy any of these types that are needed and put the 
> annotations on them?

Yes.

> Does the BTFTagAttributedType always have to apply to the immediate type 
> that's going to be modified/mutated to have the attributes applied to it 
> directly? Is the set of types that may have these attributes/annotations 
> added small/closed? (struct types, void, anything else? could you add tags to 
> int __tag *, for instance and get a DW_TAG_base_type for "int" with 
> annotations on it?

We decided against applying it to const/volatile/restrict, but it can be 
applied to struct types, void, "base" types ("int"), pointers and typedefs.

> If it's really generic/can apply to any type - but always the /immediate/ 
> type (I guess with the special handling you've got to skip applying it to the 
> DW_TAG_const_type, etc)...
>
> What if you skipped getOrCreateType - and called into the CreateType dispatch 
> below that? Since you never want a cached instance of a type, right? You want 
> to create a new copy of the type you could then apply annotations to.
>
> Except, I guess, in the instance you showed, where the type is being 
> completed - can't go and create another one, because we're part-way through 
> building this one... hmm, maybe you can, though? What happens if we start 
> making a totally distinct copy of that same type? I guess there's some map 
> that contains such partially completed types, and that map would get 
> confused/break if we tried to build two types for the same type without 
> adding in some extra key goo that contained the annotations... - maybe that 
> wouldn't be too invasive, though?

I made a prototype of such change, available here 
.
 The interesting function is `CGDebugInfo::CreateType(const 
BTFTagAttributedType *Ty, ...)`.
Pseudo code for old version (this revision):

  def CGDebugInfo::CreateType(const BTFTagAttributedType *Ty, ...):
WrappedTy, Annotations = collect annotations are base type from Ty
WrappedDI = getOrCreateType(WrappedTy, ...)
Placeholder = create temporary placeholder node with
  references to WrappedDI and Annotations
AnnotationPlaceholders.push_back(Placeholder)
return Placeholder

  def CGDebugInfo::finalize():
...
for Placeholder in AnnotationPlaceholders:
  T = Placeholder.WrappedDI.Clone()
  T.replaceAnnotations(Placeholder.Annotations)
  replace Placeholder with T

Pseudo code for new version (link to my github from above):

  def CGDebugInfo::CreateType(const BTFTagAttributedType *Ty, ...):
WrappedTy, Annotations = collect annotations are base type from Ty
Placeholder = empty temporary node
TypeCache[Ty] = Placeholder
WrappedDI = CreateType(UnwrapTypeForDebugInfo(WrappedTy), ...)
T = WrappedDI.Clone()
T.replaceAnnotations(Annotations)
replace Placeholder with T
return T

Comparing the "old" and "new" versions "old" appears to be less hacky to me: it 
does not break `getOrCreateType()` abstraction and it does not process members 
two times for circular types.

Note that this 

 diff is important for the new version, without it `CreateType` won't actually 
create a new object in the circular type case. I checked call-graph for 
`CreateType(RecordType *)` and it looks like it is only invoked from 
`getOrCreateType()`, so `getTypeOrNull()` within it is not actually needed. It 
was added in commit 3b1cc9b85846 back in 2013. Test cases for that commit are 
passing.

> given this code:
>
>   #define __tag1 __attribute__((btf_type_tag("tag1")))
>   
>   struct st {
> struct st *self;
>   };
>   struct st __tag1 x;
>
> What's the expected DWARF here? x's type is a the attributed/annotated st 
> type, but then does the self pointer inside there point to the attributed 
> type or the unattributed type?

The `self` should have type w/o tag, the `x` should have type with tag.

> Maybe what you've got is the best of some bad options, but I'm not 
> sure/trying to think it through. (@aprantl wouldn't mind your perspective - 
> if it's too much to consume easily, let me know and I can make a best-effort 
> at summarizing, maybe even better over a video chat if you've got a few 
> minutes)

I appreciate the effort, if you and @aprantl decide that the call is necessary 
I can attend if you need me.


Repository:
  rG LLVM 

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

Hi @dblaikie,

Could you please take a look at my reply 
?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D144829: [BPF] Add a few new insns under cpu=v4

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 accepted this revision.
eddyz87 added a comment.

Hi Yonghong,

Looks good to me, thanks!
Before landing this, could you please adjust tests a little bit more?

- Extend `assembler-disassembler-v4.s` with signed `div` and `mod`, e.g.:

  // CHECK: 3f 31 01 00 00 00 00 00 r1 s/= r3
  // CHECK: 9f 42 01 00 00 00 00 00 r2 s%= r4
  r1 s/= r3
  r2 s%= r4
  
  // CHECK: 3c 31 01 00 00 00 00 00 w1 s/= w3
  // CHECK: 9c 42 01 00 00 00 00 00 w2 s%= w4
  w1 s/= w3
  w2 s%= w4

- For `gotol` add a test case which tries each possibility in 
`BPFMIPreEmitPeephole::adjustBranch()`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144829

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

[PATCH] D144829: [BPF] Add a few new insns under cpu=v4

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

Hi Yonghong,

Thank you for the comments.
Could you please also add a few tests for `gotol`?
Sorry, I should have asked for those last week.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144829

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

[PATCH] D144829: [BPF] Add a few new insns under cpu=v4

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

I tried adding a test similar to `assemble-disassemble.ll`:

  // RUN: llvm-mc -triple bpfel --mcpu=v4 --assemble --filetype=obj %s \
  // RUN:   | llvm-objdump -d --mattr=+alu32 - \
  // RUN:   | FileCheck %s
  
  // CHECK: d7 01 00 00 10 00 00 00 r1 = bswap16 r1
  // CHECK: d7 02 00 00 20 00 00 00 r2 = bswap32 r2
  // CHECK: d7 03 00 00 40 00 00 00 r3 = bswap64 r3
  r1 = bswap16 r1
  r2 = bswap32 r2
  r3 = bswap64 r3
  
  // CHECK: 91 41 00 00 00 00 00 00 r1 = *(s8 *)(r4 + 0x0)
  // CHECK: 89 52 04 00 00 00 00 00 r2 = *(s16 *)(r5 + 0x4)
  // CHECK: 81 63 08 00 00 00 00 00 r3 = *(s32 *)(r6 + 0x8)
  r1 = *(s8 *)(r4 + 0)
  r2 = *(s16 *)(r5 + 4)
  r3 = *(s32 *)(r6 + 8)
  
  // CHECK: 91 41 00 00 00 00 00 00 w1 = *(s8 *)(r4 + 0x0)
  // CHECK: 89 52 04 00 00 00 00 00 w2 = *(s16 *)(r5 + 0x4)
  w1 = *(s8 *)(r4 + 0)
  w2 = *(s16 *)(r5 + 4)
  
  // CHECK: bf 41 08 00 00 00 00 00 r1 = (s8)r4
  // CHECK: bf 52 10 00 00 00 00 00 r2 = (s16)r5
  // CHECK: bf 63 20 00 00 00 00 00 r3 = (s32)w6
  r1 = (s8)r4
  r2 = (s16)r5
  r3 = (s32)w6
  // Should this work as well: r3 = (s32)r6 ?
  
  // CHECK: bc 31 08 00 00 00 00 00 w1 = (s8)w3
  // CHECK: bc 42 10 00 00 00 00 00 w2 = (s16)w4
  w1 = (s8)w3
  w2 = (s16)w4

And it looks like some instructions are not printed correctly:

  $ llvm-mc -triple bpfel --mcpu=v4 --assemble --filetype=obj 
/home/eddy/work/llvm-project/llvm/test/CodeGen/BPF/assembler-disassembler-v4.s 
| llvm-objdump -d --mattr=+alu32 -
  
  :  file format elf64-bpf
  
  Disassembly of section .text:
  
   <.text>:
 0: d7 01 00 00 10 00 00 00 r1 = bswap16 r1
 1: d7 02 00 00 20 00 00 00 r2 = bswap32 r2
 2: d7 03 00 00 40 00 00 00 r3 = bswap64 r3
 3: 91 41 00 00 00 00 00 00 w1 = *(s8 *)(r4 + 0x0)
 4: 89 52 04 00 00 00 00 00 w2 = *(s16 *)(r5 + 0x4)
 5: 81 63 08 00 00 00 00 00 
 6: 91 41 00 00 00 00 00 00 w1 = *(s8 *)(r4 + 0x0)
 7: 89 52 04 00 00 00 00 00 w2 = *(s16 *)(r5 + 0x4)
 8: bf 41 08 00 00 00 00 00 r1 = (s8)r4
 9: bf 52 10 00 00 00 00 00 r2 = (s16)r5
10: bf 63 20 00 00 00 00 00 r3 = (s32)w6
11: bc 31 08 00 00 00 00 00 w1 = (s8)w3
12: bc 42 10 00 00 00 00 00 w2 = (s16)w4

I'm not sure if this is an issue with disassembler or some additional `--mattr` 
options are needed.




Comment at: llvm/lib/Target/BPF/BPFInstrInfo.td:379
+  "$dst = (s8)$src",
+  [(set GPR:$dst, (sra (shl GPR:$src, (i64 56)), (i64 
56)))]>;
+  def MOVSX_rr_16 : ALU_RR;
+  [(set GPR:$dst, (sext_inreg GPR:$src, i8))]>;
   def MOVSX_rr_16 : ALU_RR;
+  [(set GPR:$dst, (sext_inreg GPR:$src, i16))]>;
   def MOVSX_rr_32 : ALU_RR;
+  [(set GPR32:$dst, (sext_inreg GPR32:$src, i8))]>;
   def MOVSX_rr_32_16 : ALU_RR;
+  [(set GPR32:$dst, (sext_inreg GPR32:$src, i16))]>;
 }
 }
 ```



Comment at: llvm/lib/Target/BPF/BPFMIPeephole.cpp:321
+
+  std::map ReverseCondOpMap;
 

Is this map unused?



Comment at: llvm/lib/Target/BPF/BPFMIPeephole.cpp:412
+  int CurrNumInsns = 0;
+  std::map SoFarNumInsns;
+  std::map FollowThroughBB;

Nitpick: Fangrui suggested in my llvm-objdump revisions to use `DenseMap` in 
most cases (as `std::map` allocates for each pair).



Comment at: llvm/test/CodeGen/BPF/movsx.ll:30
+}
+; CHECK: w0 = w1 # encoding: 
[0xbc,0x10,0x00,0x00,0x00,0x00,0x00,0x00]
+

This does not seem right, as it does not sign extend 8-bit argument to 16-bit 
value.



Comment at: llvm/test/CodeGen/BPF/movsx.ll:38
+}
+; CHECK: w0 = w1 # encoding: 
[0xbc,0x10,0x00,0x00,0x00,0x00,0x00,0x00]
+

Shouldn't this be `w0 = (s8)w1`?
A few checks below also look strange.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144829

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

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

Hi @dblaikie ,

Could you please take a look at my reply here 
?
It would be really great if this could be sorted out before LLVM 17 release...


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D144829: [WIP][BPF] Add a few new insns under cpu=v4

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

Hi Yonghong,

What is the current plan for these changes?
I'd like to refresh D140804  to make BPF_ST 
instruction available for cpu v4.
I see that the latest CI run failed because of libcxx issue, I think it is 
completely unrelated to this revision, see here 
.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144829

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

[PATCH] D151963: [clang][NFC] Remove trailing whitespaces and enforce it in lib, include and docs

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added inline comments.



Comment at: libcxx/utils/ci/buildkite-pipeline-clang.yml:20-31
+  - label: "Format"
+commands:
+  - "! grep -rnI '[[:blank:]]$' clang/lib clang/include clang/docs || 
false"
+
+agents:
+  queue: "libcxx-builders"
+  os: "linux"

ldionne wrote:
> Something in here broke the Clang CI. See for example 
> https://buildkite.com/llvm-project/libcxx-ci/builds/27050#0188fe23-c359-4a1d-8d10-f0fb966f142e.
> 
> It's actually interesting, now the libc++ CI is lightning fast since we're 
> not sharing our resources anymore. I'll follow-up on that, I never expected 
> it to make such a huge difference.
I believe it's the following line:

```
commands:
  - "! grep -rnI '[[:blank:]]$' clang/lib clang/include clang/docs || false"
```

buildkite-agent interpolates variables in commands and reacts to `$`, so it 
should be:

```
commands:
  - "! grep -rnI '[[:blank:]]$$' clang/lib clang/include clang/docs || 
false"
```

Instead (note `$$`).

It is possible to test this locally using the following commands:

```
$ ./libcxx/utils/ci/generate-buildkite-pipeline > 1.ym
$ docker run -v `realpath 1.yml`:`realpath 1.yml` \
  --rm buildkite/agent:3 pipeline upload --dry-run \
  --agent-access-token xx `realpath 1.yml`
2023-06-30 14:36:35 INFO   Reading pipeline config from 
"/home/eddy/work/llvm-project/1.yml"
2023-06-30 14:36:35 FATAL  Pipeline parsing of "1.yml" failed (Expected 
identifier to start with a letter, got ')
```

The error is gone if '$$' change is applied. (But I have not tested if 
semantics of the command is preserved).
As far as I understand [[ 
https://github.com/buildkite/agent/issues/584#issuecomment-343809421 | this 
link ]] applies here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D151963

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

[PATCH] D152986: [clang] Allow 'nomerge' attribute for function pointers

[Eduard Zingerman via Phabricator via cfe-commits]

This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.
Closed by commit rG06eee734c1ea: [clang] Allow nomerge attribute 
for function pointers (authored by eddyz87).

Changed prior to commit:
  https://reviews.llvm.org/D152986?vs=534585=534762#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152986

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-nomerge.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Parser/stmt-attributes.c
  clang/test/Parser/stmt-attributes.cpp
  clang/test/Parser/stmt-attributes.m
  clang/test/Sema/attr-nomerge-ast.cpp
  clang/test/Sema/attr-nomerge.cpp

Index: clang/test/Sema/attr-nomerge.cpp
===
--- clang/test/Sema/attr-nomerge.cpp
+++ clang/test/Sema/attr-nomerge.cpp
@@ -8,10 +8,14 @@
   int x;
   [[clang::nomerge]] x = 10; // expected-warning {{'nomerge' attribute is ignored because there exists no call expression inside the statement}}
 
-  [[clang::nomerge]] label: bar(); // expected-error {{'nomerge' attribute only applies to functions and statements}}
+  [[clang::nomerge]] label: bar(); // expected-error {{'nomerge' attribute only applies to functions, statements and variables}}
 
 }
 
 [[clang::nomerge]] int f();
 
-[[clang::nomerge]] static int i = f(); // expected-error {{'nomerge' attribute only applies to functions and statements}}
+[[clang::nomerge]] static int i = f(); // expected-warning {{'nomerge' attribute is ignored because 'i' is not a function pointer}}
+
+[[clang::nomerge]] void (*j)(void);
+
+struct [[clang::nomerge]] buz {}; // expected-error {{'nomerge' attribute only applies to functions, statements and variables}}
Index: clang/test/Sema/attr-nomerge-ast.cpp
===
--- clang/test/Sema/attr-nomerge-ast.cpp
+++ clang/test/Sema/attr-nomerge-ast.cpp
@@ -4,6 +4,7 @@
 [[clang::nomerge]] void func();
 void func();
 [[clang::nomerge]] void func() {}
+[[clang::nomerge]] void (*var)(void);
 
 // CHECK: FunctionDecl {{.*}} func 'void ()'
 // CHECK-NEXT: NoMergeAttr
@@ -14,3 +15,6 @@
 // CHECK-NEXT: FunctionDecl {{.*}} func 'void ()'
 // CHECK-NEXT: CompoundStmt
 // CHECK-NEXT: NoMergeAttr
+
+// CHECK-NEXT: VarDecl {{.*}} var 'void (*)()'
+// CHECK-NEXT: NoMergeAttr
Index: clang/test/Parser/stmt-attributes.m
===
--- clang/test/Parser/stmt-attributes.m
+++ clang/test/Parser/stmt-attributes.m
@@ -19,7 +19,7 @@
 
 @implementation Test
 - (void)foo __attribute__((nomerge)) {
-  // expected-error@-1 {{'nomerge' attribute only applies to functions and statements}}
+  // expected-error@-1 {{'nomerge' attribute only applies to functions, statements and variables}}
 }
 
 - (void)bar {
Index: clang/test/Parser/stmt-attributes.cpp
===
--- clang/test/Parser/stmt-attributes.cpp
+++ clang/test/Parser/stmt-attributes.cpp
@@ -6,7 +6,7 @@
 
 template 
 class __attribute__((nomerge)) A {
-  // expected-error@-1 {{'nomerge' attribute only applies to functions and statements}}
+  // expected-error@-1 {{'nomerge' attribute only applies to functions, statements and variables}}
 };
 
 class B : public A<> {
Index: clang/test/Parser/stmt-attributes.c
===
--- clang/test/Parser/stmt-attributes.c
+++ clang/test/Parser/stmt-attributes.c
@@ -82,9 +82,11 @@
   int x;
   __attribute__((nomerge)) x = 10; // expected-warning {{'nomerge' attribute is ignored because there exists no call expression inside the statement}}
 
-  __attribute__((nomerge)) label : bar(); // expected-error {{'nomerge' attribute only applies to functions and statements}}
+  __attribute__((nomerge)) label : bar(); // expected-error {{'nomerge' attribute only applies to functions, statements and variables}}
 }
 
 int f(void);
 
-__attribute__((nomerge)) static int i; // expected-error {{'nomerge' attribute only applies to functions and statements}}
+__attribute__((nomerge)) static int (*fptr)(void);
+__attribute__((nomerge)) static int i; // expected-warning {{'nomerge' attribute is ignored because 'i' is not a function pointer}}
+struct buz {} __attribute__((nomerge)); // expected-error {{'nomerge' attribute only applies to functions, statements and variables}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -105,7 +105,7 @@

[PATCH] D152986: [clang] Allow 'nomerge' attribute for function pointers

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 updated this revision to Diff 534585.
eddyz87 added a comment.

Rebase


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152986

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-nomerge.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Parser/stmt-attributes.c
  clang/test/Parser/stmt-attributes.cpp
  clang/test/Parser/stmt-attributes.m
  clang/test/Sema/attr-nomerge-ast.cpp
  clang/test/Sema/attr-nomerge.cpp

Index: clang/test/Sema/attr-nomerge.cpp
===
--- clang/test/Sema/attr-nomerge.cpp
+++ clang/test/Sema/attr-nomerge.cpp
@@ -8,10 +8,14 @@
   int x;
   [[clang::nomerge]] x = 10; // expected-warning {{'nomerge' attribute is ignored because there exists no call expression inside the statement}}
 
-  [[clang::nomerge]] label: bar(); // expected-error {{'nomerge' attribute only applies to functions and statements}}
+  [[clang::nomerge]] label: bar(); // expected-error {{'nomerge' attribute only applies to functions, statements and variables}}
 
 }
 
 [[clang::nomerge]] int f();
 
-[[clang::nomerge]] static int i = f(); // expected-error {{'nomerge' attribute only applies to functions and statements}}
+[[clang::nomerge]] static int i = f(); // expected-warning {{'nomerge' attribute is ignored because 'i' is not a function pointer}}
+
+[[clang::nomerge]] void (*j)(void);
+
+struct [[clang::nomerge]] buz {}; // expected-error {{'nomerge' attribute only applies to functions, statements and variables}}
Index: clang/test/Sema/attr-nomerge-ast.cpp
===
--- clang/test/Sema/attr-nomerge-ast.cpp
+++ clang/test/Sema/attr-nomerge-ast.cpp
@@ -4,6 +4,7 @@
 [[clang::nomerge]] void func();
 void func();
 [[clang::nomerge]] void func() {}
+[[clang::nomerge]] void (*var)(void);
 
 // CHECK: FunctionDecl {{.*}} func 'void ()'
 // CHECK-NEXT: NoMergeAttr
@@ -14,3 +15,6 @@
 // CHECK-NEXT: FunctionDecl {{.*}} func 'void ()'
 // CHECK-NEXT: CompoundStmt
 // CHECK-NEXT: NoMergeAttr
+
+// CHECK-NEXT: VarDecl {{.*}} var 'void (*)()'
+// CHECK-NEXT: NoMergeAttr
Index: clang/test/Parser/stmt-attributes.m
===
--- clang/test/Parser/stmt-attributes.m
+++ clang/test/Parser/stmt-attributes.m
@@ -19,7 +19,7 @@
 
 @implementation Test
 - (void)foo __attribute__((nomerge)) {
-  // expected-error@-1 {{'nomerge' attribute only applies to functions and statements}}
+  // expected-error@-1 {{'nomerge' attribute only applies to functions, statements and variables}}
 }
 
 - (void)bar {
Index: clang/test/Parser/stmt-attributes.cpp
===
--- clang/test/Parser/stmt-attributes.cpp
+++ clang/test/Parser/stmt-attributes.cpp
@@ -6,7 +6,7 @@
 
 template 
 class __attribute__((nomerge)) A {
-  // expected-error@-1 {{'nomerge' attribute only applies to functions and statements}}
+  // expected-error@-1 {{'nomerge' attribute only applies to functions, statements and variables}}
 };
 
 class B : public A<> {
Index: clang/test/Parser/stmt-attributes.c
===
--- clang/test/Parser/stmt-attributes.c
+++ clang/test/Parser/stmt-attributes.c
@@ -82,9 +82,11 @@
   int x;
   __attribute__((nomerge)) x = 10; // expected-warning {{'nomerge' attribute is ignored because there exists no call expression inside the statement}}
 
-  __attribute__((nomerge)) label : bar(); // expected-error {{'nomerge' attribute only applies to functions and statements}}
+  __attribute__((nomerge)) label : bar(); // expected-error {{'nomerge' attribute only applies to functions, statements and variables}}
 }
 
 int f(void);
 
-__attribute__((nomerge)) static int i; // expected-error {{'nomerge' attribute only applies to functions and statements}}
+__attribute__((nomerge)) static int (*fptr)(void);
+__attribute__((nomerge)) static int i; // expected-warning {{'nomerge' attribute is ignored because 'i' is not a function pointer}}
+struct buz {} __attribute__((nomerge)); // expected-error {{'nomerge' attribute only applies to functions, statements and variables}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -105,7 +105,7 @@
 // CHECK-NEXT: NoEscape (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NoInline (SubjectMatchRule_function)
 // CHECK-NEXT: NoInstrumentFunction (SubjectMatchRule_function, SubjectMatchRule_objc_method)
-// CHECK-NEXT: NoMerge 

[PATCH] D152986: [clang] Allow 'nomerge' attribute for function pointers

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 updated this revision to Diff 533824.
eddyz87 added a comment.

Rebase, `nomerge` documentation updated.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152986

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-nomerge.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Parser/stmt-attributes.c
  clang/test/Parser/stmt-attributes.cpp
  clang/test/Parser/stmt-attributes.m
  clang/test/Sema/attr-nomerge-ast.cpp
  clang/test/Sema/attr-nomerge.cpp

Index: clang/test/Sema/attr-nomerge.cpp
===
--- clang/test/Sema/attr-nomerge.cpp
+++ clang/test/Sema/attr-nomerge.cpp
@@ -8,10 +8,14 @@
   int x;
   [[clang::nomerge]] x = 10; // expected-warning {{'nomerge' attribute is ignored because there exists no call expression inside the statement}}
 
-  [[clang::nomerge]] label: bar(); // expected-error {{'nomerge' attribute only applies to functions and statements}}
+  [[clang::nomerge]] label: bar(); // expected-error {{'nomerge' attribute only applies to functions, statements and variables}}
 
 }
 
 [[clang::nomerge]] int f();
 
-[[clang::nomerge]] static int i = f(); // expected-error {{'nomerge' attribute only applies to functions and statements}}
+[[clang::nomerge]] static int i = f(); // expected-warning {{'nomerge' attribute is ignored because 'i' is not a function pointer}}
+
+[[clang::nomerge]] void (*j)(void);
+
+struct [[clang::nomerge]] buz {}; // expected-error {{'nomerge' attribute only applies to functions, statements and variables}}
Index: clang/test/Sema/attr-nomerge-ast.cpp
===
--- clang/test/Sema/attr-nomerge-ast.cpp
+++ clang/test/Sema/attr-nomerge-ast.cpp
@@ -4,6 +4,7 @@
 [[clang::nomerge]] void func();
 void func();
 [[clang::nomerge]] void func() {}
+[[clang::nomerge]] void (*var)(void);
 
 // CHECK: FunctionDecl {{.*}} func 'void ()'
 // CHECK-NEXT: NoMergeAttr
@@ -14,3 +15,6 @@
 // CHECK-NEXT: FunctionDecl {{.*}} func 'void ()'
 // CHECK-NEXT: CompoundStmt
 // CHECK-NEXT: NoMergeAttr
+
+// CHECK-NEXT: VarDecl {{.*}} var 'void (*)()'
+// CHECK-NEXT: NoMergeAttr
Index: clang/test/Parser/stmt-attributes.m
===
--- clang/test/Parser/stmt-attributes.m
+++ clang/test/Parser/stmt-attributes.m
@@ -19,7 +19,7 @@
 
 @implementation Test
 - (void)foo __attribute__((nomerge)) {
-  // expected-error@-1 {{'nomerge' attribute only applies to functions and statements}}
+  // expected-error@-1 {{'nomerge' attribute only applies to functions, statements and variables}}
 }
 
 - (void)bar {
Index: clang/test/Parser/stmt-attributes.cpp
===
--- clang/test/Parser/stmt-attributes.cpp
+++ clang/test/Parser/stmt-attributes.cpp
@@ -6,7 +6,7 @@
 
 template 
 class __attribute__((nomerge)) A {
-  // expected-error@-1 {{'nomerge' attribute only applies to functions and statements}}
+  // expected-error@-1 {{'nomerge' attribute only applies to functions, statements and variables}}
 };
 
 class B : public A<> {
Index: clang/test/Parser/stmt-attributes.c
===
--- clang/test/Parser/stmt-attributes.c
+++ clang/test/Parser/stmt-attributes.c
@@ -82,9 +82,11 @@
   int x;
   __attribute__((nomerge)) x = 10; // expected-warning {{'nomerge' attribute is ignored because there exists no call expression inside the statement}}
 
-  __attribute__((nomerge)) label : bar(); // expected-error {{'nomerge' attribute only applies to functions and statements}}
+  __attribute__((nomerge)) label : bar(); // expected-error {{'nomerge' attribute only applies to functions, statements and variables}}
 }
 
 int f(void);
 
-__attribute__((nomerge)) static int i; // expected-error {{'nomerge' attribute only applies to functions and statements}}
+__attribute__((nomerge)) static int (*fptr)(void);
+__attribute__((nomerge)) static int i; // expected-warning {{'nomerge' attribute is ignored because 'i' is not a function pointer}}
+struct buz {} __attribute__((nomerge)); // expected-error {{'nomerge' attribute only applies to functions, statements and variables}}
Index: clang/test/Misc/pragma-attribute-supported-attributes-list.test
===
--- clang/test/Misc/pragma-attribute-supported-attributes-list.test
+++ clang/test/Misc/pragma-attribute-supported-attributes-list.test
@@ -104,7 +104,7 @@
 // CHECK-NEXT: NoEscape (SubjectMatchRule_variable_is_parameter)
 // CHECK-NEXT: NoInline (SubjectMatchRule_function)
 // CHECK-NEXT: NoInstrumentFunction (SubjectMatchRule_function, 

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 updated this revision to Diff 533746.
eddyz87 edited the summary of this revision.
eddyz87 added a comment.

Rebase: replace usage of removeLocalCVRQualifiers by removeLocalFastQualifiers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/test/CodeGen/attr-btf_type_tag-circular.c
  clang/test/CodeGen/attr-btf_type_tag-const.c
  clang/test/CodeGen/attr-btf_type_tag-func-ptr.c
  clang/test/CodeGen/attr-btf_type_tag-func.c
  clang/test/CodeGen/attr-btf_type_tag-restrict.c
  clang/test/CodeGen/attr-btf_type_tag-similar-type.c
  clang/test/CodeGen/attr-btf_type_tag-typedef-field.c
  clang/test/CodeGen/attr-btf_type_tag-var.c
  clang/test/CodeGen/attr-btf_type_tag-void.c
  clang/test/CodeGen/attr-btf_type_tag-volatile.c
  llvm/include/llvm/IR/DIBuilder.h
  llvm/lib/IR/DIBuilder.cpp

Index: llvm/lib/IR/DIBuilder.cpp
===
--- llvm/lib/IR/DIBuilder.cpp
+++ llvm/lib/IR/DIBuilder.cpp
@@ -340,6 +340,17 @@
 Annotations);
 }
 
+DIDerivedType *
+DIBuilder::createAnnotationsPlaceholder(DIType *Ty, DINodeArray Annotations) {
+  auto *RetTy =
+  DIDerivedType::getTemporary(
+  VMContext, dwarf::DW_TAG_LLVM_annotation, "", nullptr, 0, nullptr, Ty,
+  0, 0, 0, std::nullopt, DINode::FlagZero, nullptr, Annotations)
+  .release();
+  trackIfUnresolved(RetTy);
+  return RetTy;
+}
+
 DIDerivedType *DIBuilder::createFriend(DIType *Ty, DIType *FriendTy) {
   assert(Ty && "Invalid type!");
   assert(FriendTy && "Invalid friend type!");
Index: llvm/include/llvm/IR/DIBuilder.h
===
--- llvm/include/llvm/IR/DIBuilder.h
+++ llvm/include/llvm/IR/DIBuilder.h
@@ -287,6 +287,9 @@
  DINode::DIFlags Flags = DINode::FlagZero,
  DINodeArray Annotations = nullptr);
 
+DIDerivedType *createAnnotationsPlaceholder(DIType *Ty,
+DINodeArray Annotations);
+
 /// Create debugging information entry for a 'friend'.
 DIDerivedType *createFriend(DIType *Ty, DIType *FriendTy);
 
Index: clang/test/CodeGen/attr-btf_type_tag-volatile.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-btf_type_tag-volatile.c
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 \
+// RUN:   -triple %itanium_abi_triple -debug-info-kind=limited \
+// RUN:   -S -emit-llvm -o - %s | FileCheck %s
+
+// See attr-btf_type_tag-const.c for reasoning behind this test.
+// Alternatively, see the following method:
+//   CGDebugInfo::CreateType(const BTFTagAttributedType, llvm::DIFile)
+
+#define __tag1 __attribute__((btf_type_tag("tag1")))
+
+volatile int foo;
+typeof(foo) __tag1 bar;
+
+// CHECK: ![[#]] = distinct !DIGlobalVariable(name: "bar", {{.*}}, type: ![[L1:[0-9]+]], {{.*}})
+// CHECK: ![[L1]] = !DIDerivedType(tag: DW_TAG_volatile_type, baseType: ![[L2:[0-9]+]])
+// CHECK: ![[L2]] = !DIBasicType(name: "int", size: [[#]], {{.*}}, annotations: ![[L3:[0-9]+]])
+// CHECK: ![[L3]] = !{![[L4:[0-9]+]]}
+// CHECK: ![[L4]] = !{!"btf:type_tag", !"tag1"}
Index: clang/test/CodeGen/attr-btf_type_tag-void.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-btf_type_tag-void.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -debug-info-kind=limited -S -emit-llvm -o - %s | FileCheck %s
+
+#define __tag1 __attribute__((btf_type_tag("tag1")))
+void __tag1 *g;
+
+// CHECK: distinct !DIGlobalVariable(name: "g", scope: ![[#]], file: ![[#]], line: [[#]], type: ![[L1:[0-9]+]], isLocal: false, isDefinition: true)
+// CHECK: ![[L1]] = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: ![[L2:[0-9]+]], size: [[#]], annotations: ![[L3:[0-9]+]])
+// CHECK: ![[L2]] = !DIBasicType(tag: DW_TAG_unspecified_type, name: "void", annotations: ![[L4:[0-9]+]])
+// CHECK: ![[L4]] = !{![[L5:[0-9]+]]}
+// CHECK: ![[L5]] = !{!"btf:type_tag", !"tag1"}
+// CHECK: ![[L3]] = !{![[L6:[0-9]+]]}
+// CHECK: ![[L6]] = !{!"btf_type_tag", !"tag1"}
Index: clang/test/CodeGen/attr-btf_type_tag-var.c
===
--- clang/test/CodeGen/attr-btf_type_tag-var.c
+++ clang/test/CodeGen/attr-btf_type_tag-var.c
@@ -21,23 +21,30 @@
 const int __tag1 __tag2 volatile * const __tag3  __tag4  volatile * __tag5  __tag6 const volatile * g;
 #endif
 
-// CHECK:  distinct !DIGlobalVariable(name: "g", scope: ![[#]], file: ![[#]], line: [[#]], type: ![[L6:[0-9]+]]
-// CHECK:  ![[L6]] = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: ![[L7:[0-9]+]], size: [[#]], annotations: ![[L22:[0-9]+]]
-// CHECK:  ![[L7]] = !DIDerivedType(tag: DW_TAG_const_type, baseType: ![[L8:[0-9]+]]
-// CHECK:  ![[L8]] = 

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D143967#4438815 , @dblaikie wrote:

> I haven't looked closely at this, but from a vague/quick reading, it sounds 
> like this is creating annotations on certain type DINodes, but then moving 
> them around to different types? It'd be nice if we could avoid that/create 
> them in the right place in the first place.

Hi @dblaikie, thank you for taking a look!

I assume you refer to the "placeholder" nodes created for annotated types.
I added these to handle the following example:

  #define __tag1 __attribute__((btf_type_tag("tag1")))
  
  struct st {
struct st __tag1 *self;
  } g;

Here I need to create two `DICompositeType` instances for `struct st`:

- one with `btf_type_tag` annotations;
- one without `btf_type_tag` annotations.

The AST for `struct st __tag1 *` looks as follows:

  PointerType 0x55c364ead270 'struct st  __attribute__((btf_type_tag("tag1")))*'
  `-BTFTagAttributedType 0x55c364ead210
'struct st __attribute__((btf_type_tag("tag1")))' sugar
`-ElaboratedType 0x55c364ead1a0 'struct st' sugar
  `-RecordType 0x55c364ead120 'struct st'
`-Record 0x55c364ead0a0 'st'

(Note the `BTFTagAttributedType` node).

W/o type tags this examples is processed as follows:

- `getOrCreateType()` is called for `struct st`
  - `CreateType(const RecordType *)` is called for `struct st`
- `CreateTypeDefinition(const RecordType *)` is called for `struct st`
  - `getOrCreateLimitedType(const RecordType *)` is called for `struct st`
- `CreateLimitedType(const RecordType *)` is called for `struct st`, 
which returns a `TempDICompositeType` (let's call it `T`)
- `TypeCache` for `struct st` is set to `T`
  - members of `struct st` are processed:
- `getOrCreateType()` is called for `struct st *`
  - `CreateType(const PointerType *)` is called for `struct st *`
- `getOrCreateType()` is called for `struct st`, which returns `T`, 
because of `TypeCache` state.
  - all uses of `T` are replaced by complete type.

With type tags processing is similar, but pointee type for `struct st *` is 
actually `(BTFTagAttributedType (RecordType 'struct st'))`.
The `CreateType(const BTFTagAttributedType *)` creates an instance of 
`TempDIDerivedType` with correct annotations (the placeholder) and base type 
corresponding to `struct st`.
Then, at `CGDebugInfo::finalize()` the placeholder is removed:

- the copy of the base type with correct annotations is created;
- all used of placeholder are replaced by this copy.

So, because at the point when `CreateType(BTFTagAttributedType *)` is called, 
the base type is not guaranteed to be complete yet there is a need to create 
some kind of temporary node.

An alternative to my current implementation would be creation of 
`TempDICompositeType` nodes, but this would require passing annotations down 
from `CreateType(BTFTagAttributedType *)` to `CreateLimitedType(const 
RecordType *)` via `getOrCreateType()` / `CreateTypeNode()`. But this seems 
very intrusive for the sake of a narrow feature used by a single backend.

Thus, I think that current implementation is better, because all `btf_decl_tag` 
annotations processing is grouped in two points:

- `CreateType(const BTFTagAttributedType *)`;
- `copyAnnotations()` called from `finalize()`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D152986: [clang] Allow 'nomerge' attribute for function pointers

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D152986#4425736 , @rnk wrote:

> 

Thank you for the review!

> The purpose of the attribute is really limited to preserving source location 
> information on instructions, and this isn't really a supported usage.

But it would prevent merging, right? I understand that using this attribute 
might block some legitimate optimizations.
Or do you mean that it is a "best effort" thing and not all transformations 
might honor it?

> The BPF backend and verifier needs to learn to tolerate valid LLVM transforms 
> if it wants to be a real LLVM backend. Of course, you can do what you like.

Well, I agree with that. The issue here is with the map API design: it is 
polymorphic, but it is expected that at load time verifier can replace all 
polymorphic calls with static calls.
People know it and write code in a way that allows verifier to infer which 
static functions to call. So I have limited options here:

- either adjust IR at early stages of pipeline so that specific calls are not 
altered;
- or do nothing and recommend users to use `[[clang::nomerge]]` on statement 
level or insert something like `asm volatile ("" :::"memory")` here and there.

But, yeah, I understand that is is not a C language semantics and headache is 
mine.

> Considered in the context of the original use case, I think it's reasonable 
> to allow the attribute on function pointers for the same reasons we allow it 
> on function declarations. It makes it easy to work the attribute onto the 
> direct call sites of the function without modifying tons of source code. 
> However, I'd like to see clearer documentation on the limitations.

Please see my "inline" response.




Comment at: clang/include/clang/Basic/AttrDocs.td:551-555
 calls to the specified function from merging. It has no effect on indirect 
 calls.
+
+``nomerge`` attribute can be specified for pointers to functions, all
+calls done through such pointer would be protected from merging.

rnk wrote:
> This statement of the attribute having "no effect on indirect calls" is 
> slightly confusing now that we talk about function pointers immediately 
> afterward. Can you please rework this a bit, and clarify that when applied to 
> function pointers, the attribute only takes effect when the call target is 
> directly the variable which carries the attribute? For example, this has no 
> effect:
> ```
> void (*fp)() __attribute__((nomerge));
> void callit() {
>   auto tmp = fp;
>   tmp();
>   (*fp)(); // I think TargetDecl will be null in the code, tell me if I'm 
> wrong
> }
> ```
I can make it more elaborate, would the text as below be fine?
Regarding TargetDecl value it is not null both times:
- `(VarDecl 'tmp' (ImplicitCastExpr (DeclRefExpr (Var fp`
- `(VarDecl 'fp')`

---

``nomerge`` attribute can also be used as function attribute to prevent all 
calls to the specified function from merging. It has no effect on indirect 
calls to such functions. For example:

.. code-block:: c++

  [[clang::nomerge]] void foo(int) {}
  
  void bar(int x) {
auto *ptr = foo;
if (x) foo(1); else foo(2); // will not be merged
if (x) ptr(1); else ptr(2); // indirect call, can be merged
  }

``nomerge`` attribute can also be used for pointers to functions to
prevent calls through such pointer from merging. In such case the
effect applies only to a specific function pointer. For example:

.. code-block:: c++

  [[clang::nomerge]] void (*foo)(int);
  
  void bar(int x) {
auto *ptr = foo;
if (x) foo(1); else foo(2); // will not be merged
if (x) ptr(1); else ptr(2); // 'ptr' has no 'nomerge' attribute,
// can be merged
  }



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152986

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

[PATCH] D152986: [clang] Allow 'nomerge' attribute for function pointers

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added subscribers: dfaust, jemarch.
eddyz87 added a comment.

Hi @jemarch, @dfaust,

You might be interested in this discussion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152986

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

[PATCH] D152986: [clang] Allow 'nomerge' attribute for function pointers

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added reviewers: rnk, yonghong-song.
eddyz87 added a subscriber: rnk.
eddyz87 added a comment.

Hi @aaron.ballman, @rnk,

I see that you were involved in the discussion when `nomerge` attribute was 
added in D79121 .
Could you please take a look at this proposed change?
It would be useful for BPF back-end and I tried to explain the use-case in the 
revision description.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152986

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

[PATCH] D152986: [clang] Allow 'nomerge' attribute for function pointers

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 created this revision.
Herald added a reviewer: aaron.ballman.
Herald added subscribers: jeroen.dobbelaere, jdoerfert.
Herald added a project: All.
eddyz87 published this revision for review.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Allow specifying 'nomerge' attribute for function pointers,
e.g. like in the following C code:

  extern void (*foo)(void) __attribute__((nomerge));
  void bar(long i) {
if (i)
  foo();
else
  foo();
  }

With the goal to attach 'nomerge' to both calls done through 'foo':

  @foo = external local_unnamed_addr global ptr, align 8
  define dso_local void @bar(i64 noundef %i) local_unnamed_addr #0 {
; ...
%0 = load ptr, ptr @foo, align 8, !tbaa !5
; ...
  if.then:
tail call void %0() #1
br label %if.end
  if.else:
tail call void %0() #1
br label %if.end
  if.end:
ret void
  }
  ; ...
  attributes #1 = { nomerge ... }

Report a warning in case if 'nomerge' is specified for a variable that
is not a function pointer, e.g.:

  t.c:2:22: warning: 'nomerge' attribute is ignored because 'j' is not a 
function pointer [-Wignored-attributes]
  2 | int j __attribute__((nomerge));
|  ^

The intended use-case is for BPF backend.

BPF provides a sort of "standard library" functions that are called
helpers. BPF also verifies usage of these helpers before program
execution. Because of limitations of verification / runtime model it
is important to keep calls to some of such helpers from merging.

An example could be found by the link [1], there input C code:

  if (data_end - data > 1024) {
  bpf_for_each_map_elem(, cb, _data, 0);
  } else {
  bpf_for_each_map_elem(, cb, _data, 0);
  }

Is converted to bytecode equivalent to:

  if (data_end - data > 1024)
tmp = 
  else
tmp = 
  bpf_for_each_map_elem(tmp, cb, _data, 0);

However, BPF verification/runtime requires to use the same map address
for each particular `bpf_for_each_map_elem()` call.

The 'nomerge' attribute is a perfect match for this situation, but
unfortunately BPF helpers are declared as pointers to functions:

  static long (*bpf_for_each_map_elem)(void *map, ...) = (void *) 164;

Hence, this commit, allowing to use 'nomerge' for function pointers.

[1] https://lore.kernel.org/bpf/03bdf90f-f374-1e67-69d6-76dd9c831...@meta.com/


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D152986

Files:
  clang/include/clang/Basic/Attr.td
  clang/include/clang/Basic/AttrDocs.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/CodeGen/CGCall.cpp
  clang/lib/Sema/SemaDeclAttr.cpp
  clang/test/CodeGen/attr-nomerge.cpp
  clang/test/Misc/pragma-attribute-supported-attributes-list.test
  clang/test/Parser/stmt-attributes.c
  clang/test/Parser/stmt-attributes.cpp
  clang/test/Parser/stmt-attributes.m
  clang/test/Sema/attr-nomerge-ast.cpp
  clang/test/Sema/attr-nomerge.cpp

Index: clang/test/Sema/attr-nomerge.cpp
===
--- clang/test/Sema/attr-nomerge.cpp
+++ clang/test/Sema/attr-nomerge.cpp
@@ -8,10 +8,14 @@
   int x;
   [[clang::nomerge]] x = 10; // expected-warning {{'nomerge' attribute is ignored because there exists no call expression inside the statement}}
 
-  [[clang::nomerge]] label: bar(); // expected-error {{'nomerge' attribute only applies to functions and statements}}
+  [[clang::nomerge]] label: bar(); // expected-error {{'nomerge' attribute only applies to functions, statements and variables}}
 
 }
 
 [[clang::nomerge]] int f();
 
-[[clang::nomerge]] static int i = f(); // expected-error {{'nomerge' attribute only applies to functions and statements}}
+[[clang::nomerge]] static int i = f(); // expected-warning {{'nomerge' attribute is ignored because 'i' is not a function pointer}}
+
+[[clang::nomerge]] void (*j)(void);
+
+struct [[clang::nomerge]] buz {}; // expected-error {{'nomerge' attribute only applies to functions, statements and variables}}
Index: clang/test/Sema/attr-nomerge-ast.cpp
===
--- clang/test/Sema/attr-nomerge-ast.cpp
+++ clang/test/Sema/attr-nomerge-ast.cpp
@@ -4,6 +4,7 @@
 [[clang::nomerge]] void func();
 void func();
 [[clang::nomerge]] void func() {}
+[[clang::nomerge]] void (*var)(void);
 
 // CHECK: FunctionDecl {{.*}} func 'void ()'
 // CHECK-NEXT: NoMergeAttr
@@ -14,3 +15,6 @@
 // CHECK-NEXT: FunctionDecl {{.*}} func 'void ()'
 // CHECK-NEXT: CompoundStmt
 // CHECK-NEXT: NoMergeAttr
+
+// CHECK-NEXT: VarDecl {{.*}} var 'void (*)()'
+// CHECK-NEXT: NoMergeAttr
Index: clang/test/Parser/stmt-attributes.m
===
--- clang/test/Parser/stmt-attributes.m
+++ clang/test/Parser/stmt-attributes.m
@@ -19,7 +19,7 @@
 
 @implementation Test
 - (void)foo __attribute__((nomerge)) {
-  // expected-error@-1 {{'nomerge' attribute only applies to functions and statements}}
+  // 

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

Hi @dblaikie ,

Could you please take a look at this revision (and the previous one in the 
stack, D143966 ) or suggest some other 
reviewer familiar with this part of the code base?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 updated this revision to Diff 523233.
eddyz87 added a comment.

Fix for case when CVR qualifier is behind type ignored by 
UnwrapTypeForDebugInfo().
E.g. for the following example:

  const int *foo;
  typeof(*foo) __attribute__((btf_type_tag("tag"))) buz;


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/test/CodeGen/attr-btf_type_tag-circular.c
  clang/test/CodeGen/attr-btf_type_tag-const.c
  clang/test/CodeGen/attr-btf_type_tag-func-ptr.c
  clang/test/CodeGen/attr-btf_type_tag-func.c
  clang/test/CodeGen/attr-btf_type_tag-restrict.c
  clang/test/CodeGen/attr-btf_type_tag-similar-type.c
  clang/test/CodeGen/attr-btf_type_tag-typedef-field.c
  clang/test/CodeGen/attr-btf_type_tag-var.c
  clang/test/CodeGen/attr-btf_type_tag-void.c
  clang/test/CodeGen/attr-btf_type_tag-volatile.c
  llvm/include/llvm/IR/DIBuilder.h
  llvm/lib/IR/DIBuilder.cpp

Index: llvm/lib/IR/DIBuilder.cpp
===
--- llvm/lib/IR/DIBuilder.cpp
+++ llvm/lib/IR/DIBuilder.cpp
@@ -352,6 +352,17 @@
 Annotations);
 }
 
+DIDerivedType *
+DIBuilder::createAnnotationsPlaceholder(DIType *Ty, DINodeArray Annotations) {
+  auto *RetTy =
+  DIDerivedType::getTemporary(
+  VMContext, dwarf::DW_TAG_LLVM_annotation, "", nullptr, 0, nullptr, Ty,
+  0, 0, 0, std::nullopt, DINode::FlagZero, nullptr, Annotations)
+  .release();
+  trackIfUnresolved(RetTy);
+  return RetTy;
+}
+
 DIDerivedType *DIBuilder::createFriend(DIType *Ty, DIType *FriendTy) {
   assert(Ty && "Invalid type!");
   assert(FriendTy && "Invalid friend type!");
Index: llvm/include/llvm/IR/DIBuilder.h
===
--- llvm/include/llvm/IR/DIBuilder.h
+++ llvm/include/llvm/IR/DIBuilder.h
@@ -284,6 +284,9 @@
  DINode::DIFlags Flags = DINode::FlagZero,
  DINodeArray Annotations = nullptr);
 
+DIDerivedType *createAnnotationsPlaceholder(DIType *Ty,
+DINodeArray Annotations);
+
 /// Create debugging information entry for a 'friend'.
 DIDerivedType *createFriend(DIType *Ty, DIType *FriendTy);
 
Index: clang/test/CodeGen/attr-btf_type_tag-volatile.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-btf_type_tag-volatile.c
@@ -0,0 +1,18 @@
+// RUN: %clang_cc1 \
+// RUN:   -triple %itanium_abi_triple -debug-info-kind=limited \
+// RUN:   -S -emit-llvm -o - %s | FileCheck %s
+
+// See attr-btf_type_tag-const.c for reasoning behind this test.
+// Alternatively, see the following method:
+//   CGDebugInfo::CreateType(const BTFTagAttributedType, llvm::DIFile)
+
+#define __tag1 __attribute__((btf_type_tag("tag1")))
+
+volatile int foo;
+typeof(foo) __tag1 bar;
+
+// CHECK: ![[#]] = distinct !DIGlobalVariable(name: "bar", {{.*}}, type: ![[L1:[0-9]+]], {{.*}})
+// CHECK: ![[L1]] = !DIDerivedType(tag: DW_TAG_volatile_type, baseType: ![[L2:[0-9]+]])
+// CHECK: ![[L2]] = !DIBasicType(name: "int", size: [[#]], {{.*}}, annotations: ![[L3:[0-9]+]])
+// CHECK: ![[L3]] = !{![[L4:[0-9]+]]}
+// CHECK: ![[L4]] = !{!"btf:type_tag", !"tag1"}
Index: clang/test/CodeGen/attr-btf_type_tag-void.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-btf_type_tag-void.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -debug-info-kind=limited -S -emit-llvm -o - %s | FileCheck %s
+
+#define __tag1 __attribute__((btf_type_tag("tag1")))
+void __tag1 *g;
+
+// CHECK: distinct !DIGlobalVariable(name: "g", scope: ![[#]], file: ![[#]], line: [[#]], type: ![[L1:[0-9]+]], isLocal: false, isDefinition: true)
+// CHECK: ![[L1]] = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: ![[L2:[0-9]+]], size: [[#]], annotations: ![[L3:[0-9]+]])
+// CHECK: ![[L2]] = !DIBasicType(tag: DW_TAG_unspecified_type, name: "void", annotations: ![[L4:[0-9]+]])
+// CHECK: ![[L4]] = !{![[L5:[0-9]+]]}
+// CHECK: ![[L5]] = !{!"btf:type_tag", !"tag1"}
+// CHECK: ![[L3]] = !{![[L6:[0-9]+]]}
+// CHECK: ![[L6]] = !{!"btf_type_tag", !"tag1"}
Index: clang/test/CodeGen/attr-btf_type_tag-var.c
===
--- clang/test/CodeGen/attr-btf_type_tag-var.c
+++ clang/test/CodeGen/attr-btf_type_tag-var.c
@@ -21,23 +21,30 @@
 const int __tag1 __tag2 volatile * const __tag3  __tag4  volatile * __tag5  __tag6 const volatile * g;
 #endif
 
-// CHECK:  distinct !DIGlobalVariable(name: "g", scope: ![[#]], file: ![[#]], line: [[#]], type: ![[L6:[0-9]+]]
-// CHECK:  ![[L6]] = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: ![[L7:[0-9]+]], size: [[#]], annotations: ![[L22:[0-9]+]]
-// CHECK:  ![[L7]] = !DIDerivedType(tag: 

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D143967#4346934 , @dfaust wrote:

> In D143967#4343841 , @eddyz87 wrote:
>
>> Changes to avoid attaching type tags to DWARF derived types for 
>> const/volatile/restrict qualifiers.
>
> I just tested this locally and can confirm it LGTM in terms of implementing 
> the DWARF format we've been discussing and agreed upon.
> Also compared against my WIP patches implementing the same in GCC, and looks 
> like it is all consistent (apart from one known bug in my patches)

Heads-up, it currently fails on the following example:

  #define __rcu __attribute__((btf_type_tag("rcu")))
  
  struct cred;
  const struct cred *cred;
  void commit_creds()
  {
cred = (typeof(*cred) __rcu *)(0);
  }

I'm working on the fix.
Also I missed the fact that D145891  (actual 
BTF generation) needs adjustment as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 updated this revision to Diff 522338.
eddyz87 edited the summary of this revision.
eddyz87 added a comment.

Changes to avoid attaching type tags to DWARF derived types for 
const/volatile/restrict qualifiers.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

Files:
  clang/lib/AST/ASTContext.cpp
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/lib/CodeGen/CGDebugInfo.h
  clang/test/CodeGen/attr-btf_type_tag-circular.c
  clang/test/CodeGen/attr-btf_type_tag-func-ptr.c
  clang/test/CodeGen/attr-btf_type_tag-func.c
  clang/test/CodeGen/attr-btf_type_tag-similar-type.c
  clang/test/CodeGen/attr-btf_type_tag-typedef-field.c
  clang/test/CodeGen/attr-btf_type_tag-var.c
  clang/test/CodeGen/attr-btf_type_tag-void.c
  llvm/include/llvm/IR/DIBuilder.h
  llvm/lib/IR/DIBuilder.cpp

Index: llvm/lib/IR/DIBuilder.cpp
===
--- llvm/lib/IR/DIBuilder.cpp
+++ llvm/lib/IR/DIBuilder.cpp
@@ -352,6 +352,17 @@
 Annotations);
 }
 
+DIDerivedType *
+DIBuilder::createAnnotationsPlaceholder(DIType *Ty, DINodeArray Annotations) {
+  auto *RetTy =
+  DIDerivedType::getTemporary(
+  VMContext, dwarf::DW_TAG_LLVM_annotation, "", nullptr, 0, nullptr, Ty,
+  0, 0, 0, std::nullopt, DINode::FlagZero, nullptr, Annotations)
+  .release();
+  trackIfUnresolved(RetTy);
+  return RetTy;
+}
+
 DIDerivedType *DIBuilder::createFriend(DIType *Ty, DIType *FriendTy) {
   assert(Ty && "Invalid type!");
   assert(FriendTy && "Invalid friend type!");
Index: llvm/include/llvm/IR/DIBuilder.h
===
--- llvm/include/llvm/IR/DIBuilder.h
+++ llvm/include/llvm/IR/DIBuilder.h
@@ -284,6 +284,9 @@
  DINode::DIFlags Flags = DINode::FlagZero,
  DINodeArray Annotations = nullptr);
 
+DIDerivedType *createAnnotationsPlaceholder(DIType *Ty,
+DINodeArray Annotations);
+
 /// Create debugging information entry for a 'friend'.
 DIDerivedType *createFriend(DIType *Ty, DIType *FriendTy);
 
Index: clang/test/CodeGen/attr-btf_type_tag-void.c
===
--- /dev/null
+++ clang/test/CodeGen/attr-btf_type_tag-void.c
@@ -0,0 +1,12 @@
+// RUN: %clang_cc1 -triple %itanium_abi_triple -debug-info-kind=limited -S -emit-llvm -o - %s | FileCheck %s
+
+#define __tag1 __attribute__((btf_type_tag("tag1")))
+void __tag1 *g;
+
+// CHECK: distinct !DIGlobalVariable(name: "g", scope: ![[#]], file: ![[#]], line: [[#]], type: ![[L1:[0-9]+]], isLocal: false, isDefinition: true)
+// CHECK: ![[L1]] = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: ![[L2:[0-9]+]], size: [[#]], annotations: ![[L3:[0-9]+]])
+// CHECK: ![[L2]] = !DIBasicType(tag: DW_TAG_unspecified_type, name: "void", annotations: ![[L4:[0-9]+]])
+// CHECK: ![[L4]] = !{![[L5:[0-9]+]]}
+// CHECK: ![[L5]] = !{!"btf:type_tag", !"tag1"}
+// CHECK: ![[L3]] = !{![[L6:[0-9]+]]}
+// CHECK: ![[L6]] = !{!"btf_type_tag", !"tag1"}
Index: clang/test/CodeGen/attr-btf_type_tag-var.c
===
--- clang/test/CodeGen/attr-btf_type_tag-var.c
+++ clang/test/CodeGen/attr-btf_type_tag-var.c
@@ -21,23 +21,30 @@
 const int __tag1 __tag2 volatile * const __tag3  __tag4  volatile * __tag5  __tag6 const volatile * g;
 #endif
 
-// CHECK:  distinct !DIGlobalVariable(name: "g", scope: ![[#]], file: ![[#]], line: [[#]], type: ![[L6:[0-9]+]]
-// CHECK:  ![[L6]] = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: ![[L7:[0-9]+]], size: [[#]], annotations: ![[L22:[0-9]+]]
-// CHECK:  ![[L7]] = !DIDerivedType(tag: DW_TAG_const_type, baseType: ![[L8:[0-9]+]]
-// CHECK:  ![[L8]] = !DIDerivedType(tag: DW_TAG_volatile_type, baseType: ![[L9:[0-9]+]]
-// CHECK:  ![[L9]] = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: ![[L10:[0-9]+]], size: [[#]], annotations: ![[L19:[0-9]+]]
-// CHECK:  ![[L10]] = !DIDerivedType(tag: DW_TAG_const_type, baseType: ![[L11:[0-9]+]]
-// CHECK:  ![[L11]] = !DIDerivedType(tag: DW_TAG_volatile_type, baseType: ![[L12:[0-9]+]]
-// CHECK:  ![[L12]] = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: ![[L13:[0-9]+]], size: [[#]], annotations: ![[L16:[0-9]+]]
-// CHECK:  ![[L13]] = !DIDerivedType(tag: DW_TAG_const_type, baseType: ![[L14:[0-9]+]]
-// CHECK:  ![[L14]] = !DIDerivedType(tag: DW_TAG_volatile_type, baseType: ![[L15:[0-9]+]]
-// CHECK:  ![[L15]] = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed
-// CHECK:  ![[L16]] = !{![[L17:[0-9]+]], ![[L18:[0-9]+]]}
-// CHECK:  ![[L17]] = !{!"btf_type_tag", !"tag1"}
-// CHECK:  ![[L18]] = !{!"btf_type_tag", !"tag2"}
-// CHECK:  ![[L19]] = !{![[L20:[0-9]+]], ![[L21:[0-9]+]]}
-// CHECK:  ![[L20]] = !{!"btf_type_tag", !"tag3"}
-// CHECK:  ![[L21]] = !{!"btf_type_tag", !"tag4"}
-// 

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 planned changes to this revision.
eddyz87 added a comment.

In D143967#4312746 , @yonghong-song 
wrote:

> I see the following in the Summary:
>
>   Type tag for CVR modifier type
>   
>   C:
>   
>   volatile int __attribute__((btf_type_tag("__b"))) b;
>   
>   DWARF:
>   
>   0x31:   DW_TAG_variable
> DW_AT_name  ("b")
> DW_AT_type  (0x3c "volatile int")
>   
>   0x3c:   DW_TAG_volatile_type
> DW_AT_type  (0x45 "int")
>   
>   0x41: DW_TAG_LLVM_annotation
>   DW_AT_name("btf:type_tag")
>   DW_AT_const_value ("__b")
>
> Basically, the btf_type_tag is put under 'volatile' type in dwarf. Is this 
> what gcc also agrees with?

We decided against it but changes to force all type tags after CVR modifiers 
are not implemented, should probably add a wip tag.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

Moving type tags past typedefs would also make C code reconstruction from BTF 
incomplete. Such reconstruction is used now by e.g. bpftool to create a 
vmlinux.h header with all kernel type definitions. So, I think type tags should 
not be moved past typedefs.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D143967#4233414 , @jemarch wrote:

>> If some tooling applies such tags movement it should also apply
>> appropriate copying of tags, e.g. it should transform DWARF like this:
>>
>>   var1 -> const -> typedef (bar) -> int
>>  |
>>   __tag1
>>   
>>   var2 --> typedef (bar) -> int
>>
>> (and it is what needs to be implemented in pahole to get BTF
>> qualifiers ordering expected by kernel, but the move is in the
>> opposite direction).
>
> So the kernel expects tags to precede typedefs as well as qualifiers?
> i.e. given this DWARF:
>
>   var1 -> const -> typedef (bar) -> int
>^ |
>|   __tag1
>|
>   var2 +
>
> We have to transform to these two BTF type chains:
>
>   var1 -> __tag1 -> const -> typedef (bar) -> int
>   ^
>   |
>   var2 -> __tag1 -+
>
> Correct?

This is controlled by the following code (btf.c:btf_check_type_tags()):

https://elixir.bootlin.com/linux/latest/source/kernel/bpf/btf.c#L5349

It uses `btf_type_is_modifier()` utility function, which treats typedef as a 
modifier.

So, in theory the transformation moving tags past typedef is necessary. On the 
other hand, such transformation is not applied now, and this does not cause 
issues. Which suggests that there are no cases in practice where type tag 
follows typedef (and thus, this is not required for backwards compatibility).

Moving type tags cross typedef feels wrong.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D143967#4232089 , @jemarch wrote:

> Thinking about typedef, some C cases may be problematic if we adopt the
> flexible rule we are discussing:
>
>   typedef int bar;
>   const bar __tag1 var1;
>   bar var2;
>   
>   DWARF:
>   
>   var1 -> const -> typedef (bar) -> int
>|   ^
>   __tag1   |
>|
>   var2 +
>
> If we would allow __tag1 to be "moved" to either the typedef DWARF node
> or the node for int like this:
>
>   DWARF:
>   
>   var1 -> const -> typedef (bar) -> int
>^ |
>|  __tag1
>   var2 +
>
> Then the __tag1 would also apply to var2's type.  But I would say in the
> C snippet above __tag1 should apply to the type of var1, but not
> to the type of var2.

I'm not sure I understand, the DWARF #2 is not a valid representation of the 
program for all the reasons you write.
Current LLVM implementation should generate DWARF #1 in this case.
If some tooling applies such tags movement it should also apply appropriate 
copying of tags, e.g. it should transform DWARF like this:

  var1 -> const -> typedef (bar) -> int
 |
  __tag1
  
  var2 --> typedef (bar) -> int

(and it is what needs to be implemented in pahole to get BTF qualifiers 
ordering expected by kernel, but the move is in the opposite direction).

> PS: I am a bit concerned with the fact that the kernel's interpretation
>
>   of BTF is so rigit in this case as to assume C's type system
>   semantics when it comes to type qualifiers.  Other languages may be
>   coming to the BPF world (Rust, for example) and in these languages
>   the ordering of type qualifiers may actually matter.  There is a
>   reason why DWARF encodes qualifiers as explicit nodes in the type
>   chain rather than as attributes of the type nodes.

Need to read a bit about Rust, can't comment right now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a subscriber: anakryiko.
eddyz87 added a comment.

In D143967#4231746 , @jemarch wrote:

>> eddyz87 added a comment.
>> ...
>> If we consider type tag a qualifier, conceptually it would be simpler
>> if ordering would not matter for it as well, so that the following
>> have identical meaning:
>>
>> - `__tag volatile int`
>> - `volatile __tag int`
>
> Makes sense.
>
> But then I think we should allow the tags to be anywhere in the chain in
> the DWARF, and not necessarily in the qualified type.  For example
> given:
>
>   typedef const int bar;
>   volatile bar __tag1 foo;
>
> The resulting DWARF type chain is:
>
>   volatile -> typedef (bar) -> const -> int
>
> IMO we should allow __tag1 to be a child of any of the elements of the
> chain, like:
>
>   volatile -> typedef (bar) -> const -> int
>|
>  __tag1
>
> or
>
>   volatile -> typedef (bar) -> const -> int
>  |
>   __tag1
>
> or
>
>   volatile -> typedef (bar) -> const -> int
>  |
>   __tag1
>
> or
>
>   volatile -> typedef (bar) -> const -> int
>  |
>__tag1
>
> would be all accepted and equivalent.  Also Faust noted that GCC
> sometimes optimizes out the typedef nodes, so we need to be able to
> place the tags (in the internal representatinos) in the typedef'ed type
> instead.

Well, it's a logical consequence, I guess.
In practice I'm going to emit it like below:

  volatile -> const -> int
|
 __tag1

But please remember that in BTF it would have to be encoded like this:

  __tag1 -> volatile -> const -> int

(that's how kernel expects it, we can change the kernel but will loose 
backwards compatibility).
For DWARF -> BTF transformation in `pahole` I will make the necessary 
modifications, but BTF might also be emitted C->BPF backend.

@yonghong-song, @anakryiko, what do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D143967#4231517 , @dfaust wrote:

> In D143967#4220233 , @jemarch wrote:
>
>> 
>
> Ok, I understand your point. 
> For example if we have:
>
>   volatile int __tag1 b;
>
> The tag applies to the type (volatile int) and we have 3 types total:
>
> 1. __tag1 volatile int
> 2. volatile int
> 3. int
>
> And in DWARF the tag must be child of the qualifier:
>
>   var(b) -> volatile -> int
>|
>   __tag1
>   
>
>
> Doing it the other way would be incorrect - if we instead did this:
>
>   var(b) -> volatile -> int
>   |
>__tag1
>
> We encode a different chain of types:
>
>   var(b) -> volatile -> __tag1 -> int
>
> Which reflects a different set of types than the ones we actually have:
>
> 1. volatile __tag1 int
> 2. __tag1 int
> 3. int
>
> So I guess it is clear, conceptually the tags do belong on the qualifiers.

But that's just an encoding quirk. For the C language "const volatile int *" 
and "volatile const int *" have the same meaning. In clang AST such qualifiers 
are represented as bitfields.

If we consider type tag a qualifier, conceptually it would be simpler if 
ordering would not matter for it as well, so that the following have identical 
meaning:

- `__tag volatile int`
- `volatile __tag int`

>> And now that I think about this.  In this conceptual situation:
>>
>>   const -> volatile -> annotated (foo) -> annotated (bar) -> pointer-to -> 
>> int
>>
>> I guess we are encoding it with several DW_TAG_LLVM_annotation children:
>>
>>   const -> volatile -> pointer-to -> int
>>|
>>  +-+-+
>>  |   |
>> anotated (foo)annotated (bar)
>>
>> Basically accumulating/reducing what are conceptually two different
>> types into one.  Since typedefs are explicitly encoded in the chain as
>> their own node, this situation will only happen when there are several
>> tags specified consecutively in the source code (this is another reason
>> why putting the annotation DIEs as children of the qualifiers is
>> necessary, because otherwise we would be accumulating/reducing
>> annotation across these nodes and we could end with situations where
>> annotations get lost.)
>>
>> When accumulating/reducign tags like above:
>>
>> Does the ordering matter here?  Does it matter?  Should we require
>> keeping the order among the annotation siblings as part of the
>> DW_TAG_LLVM_annotation specification?  Even if we don't, we probably
>> want to behave the same in both compilers.
>
> A quick check suggests order does not matter for DWARF for normal qualifiers:
>
> const volatile int x;
> volatile const int y;
>
> Both compilers emit something like this ('x' and 'y' share type):
>
>   0x001e:   DW_TAG_variable
>   DW_AT_name  ("x")
>   DW_AT_type  (0x0029 "const volatile int")
>   
>   0x0029:   DW_TAG_const_type
>   DW_AT_type  (0x002e "volatile int")
>   
>   0x002e:   DW_TAG_volatile_type
>   DW_AT_type  (0x0033 "int")
>   
>   0x0033:   DW_TAG_base_type
>   DW_AT_name  ("int")
>   
>   0x0037:   DW_TAG_variable
>   DW_AT_name  ("y")
>   DW_AT_type  (0x0029 "const volatile int")
>
> Interestingly, GCC and LLVM do not behave exactly the same in that in 
> GCC the DWARF types for both variables are
>
>   x, y   -> volatile -> const -> int
>
> while in LLVM (shown above) it is
>
>   x, y -> const -> volatile -> int
>
> So I'm not sure we necessarily need both compilers to be exactly the same.

Unfortunately, for ordering we are also limited by a consumer. In the kernel 
case the ordering of const/volatile/restrict does not matter, but it does want 
type tags to come first.

>> In case two identical annotations are applied to the same type, are we
>> deduplicating them?  Should we actually require that?  Even if we don't,
>> we probably want to behave the same in both compilers.
>
> FWIW, if you write something like:
>
>   int __attribute__((btf_type_tag("__c"))) 
> __attribute__((btf_type_tag("__c"))) c;
>
> GCC de-duplicates attributes with the same value, so the tag "__c" will only 
> appear
> once. I don't know if this would be easy to change if we wanted to.

LLVM de-duplicates as well.

AFAIK current type tag use cases do not require to distinguish "const __tag" 
from "__tag const" and do not need to handle "__tag __tag" separately from 
"__tag". So, I think we should stick with simpler semantics.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

___
cfe-commits mailing list
cfe-commits@lists.llvm.org

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D143967#4200331 , @dfaust wrote:

> In D143967#4197288 , @eddyz87 wrote:
>
>> ...
>> I think there are two sides to this:
>>
>> - conceptual: is it ok to allow annotations for CVR modifiers?
>
> Maybe someone more an expert in DWARF could chime in whether this is 
> problematic?
> As far as I know it would be "ok" in the sense that it should not break DWARF 
> or cause issues for DWARF consumers which do not know about the tags.
>
> My initial reaction was that placing the annotations on CVR modifiers makes 
> less sense conceptually than placing them on the types proper.

I agree conceptually.

> But, I suppose it is a question of what precisely the annotated type is. e.g. 
> in `volatile int __attribute__((btf_type_tag("__b"))) b;` is it the case that:
>
> - type tag "__b" applies to the type `volatile int` (i.e. cvr quals  "bind 
> more closely" than the tag), or
> - type tag "__b" and `volatile` both apply to `int` (i.e. cvr quals and type 
> tags are "equal")
>
> For all other cases the new "btf:type_tag" format places the annotation as a 
> child of exactly the type that is annotated, so I think the answer determines 
> where the annotation logically belongs in DWARF:
>
> - If the tag applies to `volatile int` then it should be a child of the 
> volatile DIE.
> - If the tag applies to `int` it should be a child of the integer type DIE.

I'm aware of three use-cases for "btf_type_tag": "percpu", "user", "rcu" marks 
in kernel code. First is used for per-cpu variables, second is used to mark 
pointers to userspace memory, third to mark the data that should be read with 
RCU lock. For these use-cases there is no difference between e.g. `const __user 
*ptr` and `__user const *ptr`. I don't think the difference was ever thought 
through.

> At the moment I can't say that one is more correct than the other, so I guess 
> I have no real objection placing the annotation on the CVR modifier.
>
>> - technical: what is the point where such reordering is easiest to 
>> implement? For LLVM / pahole stack the path of least resistance is to modify 
>> DWARF generation (but again @dblaikie might disagree). How hard is it to 
>> adjust DI generation in GCC in this way?
>
> It is not a difficult change in GCC. Some added complexity but not too much. 
> I have a prototype that seems to work from initial testing.

Same here, it is a small change in clang code, but I don't like that it has to 
be handled as a special case.

> It is probably simpler than instead modifying the internal transformation to 
> BTF to reorder tags/CVR qualifiers as kernel currently requires.
>
> But I don't think ease of implementation should be a factor unless we are 
> sure the format makes sense.
> We are changing the format because the old one was flawed, let's try to make 
> sure we aren't just replacing it with something flawed in a different way 
> because it is easy.

The main issue is that kernel currently expects type tags before modifiers. 
Even if the kernel code would be modified there is still an issue of backwards 
compatibility. During kernel build BTF is generated from DWARF and is embedded 
in a kernel image, it is done by tool named pahole 
. If we forgo modifiers reordering the 
embedded BTF would not match format supported by kernel.

So, I assume that reordering _has_ to happen somewhere. Either at DWARF 
generation stage, or in two places:

- compiler BPF backend, when BPF programs are compiled;
- `pahole`, when BTF is generated from DWARF.

Let me prototype pahole change to see how much of hassle that is, it will take 
a day or two.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

> ! In D143967#4197142 , @jemarch 
> wrote:
>  ...
>  In GCC we also translate from the internal DWARF structures into BTF.
>  So, for us, it would also imply to reoder (before generating the BTF
>  from the interal DWARF) in case we wanted to use a different approach in
>  DWARF than in BTF.

It's not only the BTF generated for BPF programs. There are two ways
to obtain BTF that we should worry about:

1. BTF generated for BPF programs by compiler;
2. BTF generated during kernel build for the whole kernel, this one is 
generated by `pahole` from DWARF.

> I think the question is: what are the consequences of having the
> annotation DIE as a child of the qualifiers instead of the qualified
> type?

Are there any? I just tried the following code with GDB:

  volatile int __attribute__((btf_type_tag("__a"))) a = 1;
  
  void foo(void) {
a = 2;
  }
  
  int main(int argc, char **argv) {
foo();
return 0;
  }

No warnings, `a` can be inspected.

I think there are two sides to this:

- conceptual: is it ok to allow annotations for CVR modifiers?
- technical: what is the point where such reordering is easiest to implement? 
For LLVM / pahole stack the path of least resistance is to modify DWARF 
generation (but again @dblaikie might disagree). How hard is it to adjust DI 
generation in GCC in this way?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D143967#4197041 , @dfaust wrote:

> The way I see it both 'volatile' and the type tag are modifying 'int' type 
> here, so the annotation DIE more properly fits as a child of 'int' rather 
> than the 'volatile'.
>
> I don't think we discussed this particular case, and I'm not sure whether 
> there is any precedent here.
>
> WDYT @eddyz87, @jemarch ?

This is related to my previous comment. Kernel BTF parser wants all type tags 
to precede modifiers (see this 
 code). 
So such reordering will have to happen either during DWARF generation or by 
`pahole`. Currently `pahole` is "dumb" in a sense that it reflects DWARF 
structure in BTF effectively one-to-one. Adding such rewrite at a `pahole` 
level is possible but one-to-one mapping to DWARF would be lost, so the tool 
would require significant changes.

So, at least for LLVM its a tradeoff, either do it in the compiler (less code), 
or in the pahole (more code). I opted for less code :) If you think that this 
is conceptually unacceptable, I'll do the change on the pahole side (DI 
maintainers on LLVM side might agree, the patch has not been reviewed yet).

Another option would be to modify the kernel, but this might impede backwards 
compatibility.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

Hi @dfaust ,

Two head-ups, the following items turned out to be important when I tested 
using kernel BPF testsuite:

- in the final BTF, type tags have to precede CVR modifiers, e.g. TYPE_TAG 
'foo' -> CONST -> INT. Right now `pahole` does not do any reordering, so I 
ended up putting the type tag annotations on the DIE with outermost modifier. 
Will see if DI maintainers would be ok with this.
- CO-RE relocation entries have to point to the actual type, not type tag. For 
both field relocations and variable relocations (this is related to the next 
 patch in a stack).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a subscriber: jemarch.
eddyz87 added a comment.

Hi @jemarch,

Could you please take a look to verify that this implementation is on the same 
page with what is planned for GCC?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143967

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

[PATCH] D143967: [DebugInfo][BPF] Add 'btf:type_tag' annotation in DWARF

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 created this revision.
Herald added a subscriber: hiraditya.
Herald added a project: All.
eddyz87 updated this revision to Diff 497835.
eddyz87 added a comment.
eddyz87 retitled this revision from "[DebugInfo][BPF] 'annotations' on a target 
type to represent btf_type_tag" to "[DebugInfo][BPF] Add 'btf_type_tag:v2' 
annotation in DWARF".
eddyz87 edited the summary of this revision.
eddyz87 updated this revision to Diff 499281.
eddyz87 edited the summary of this revision.
eddyz87 updated this revision to Diff 502033.
eddyz87 edited the summary of this revision.
eddyz87 retitled this revision from "[DebugInfo][BPF] Add 'btf_type_tag:v2' 
annotation in DWARF" to "[DebugInfo][BPF] Add 'btf:type_tag' annotation in 
DWARF".
eddyz87 updated this revision to Diff 502321.
eddyz87 updated this revision to Diff 503198.
eddyz87 updated this revision to Diff 504493.
eddyz87 published this revision for review.
eddyz87 added reviewers: yonghong-song, echristo, dblaikie.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

- Removed DWARF tag 0x6001, use 'btf_type_tag:v2' name instead.
- Removed CopyBTFTypeTags pass, generate 'btf_type_tag' and 'btf_type_tag:v2' 
in `CGDebugInfo` instead.
- Added tests for BTF deduplication logic for debug info types that differ only 
in annotations.


yonghong-song added a comment.

When I tried to build llvm with the patch, I hit the following errors:

  [2198/3751] Building CXX object 
lib/Target/BPF/CMakeFiles/LLVMBPFCodeGen.dir/BTFDebug.cpp.o
  FAILED: lib/Target/BPF/CMakeFiles/LLVMBPFCodeGen.dir/BTFDebug.cpp.o 
  /bin/c++ -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_GNU_SOURCE 
-D_LIBCPP_ENABLE_ASSERTIONS -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS 
-D__STDC_LIMIT_MACROS -Ilib/Target/BPF -I../lib/Target/BPF -Iinclude 
-I../include -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden 
-Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings 
-Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long 
-Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess 
-Wno-noexcept-type -Wdelete-non-virtual-dtor -Wno-comment 
-Wno-misleading-indentation -fdiagnostics-color -ffunction-sections 
-fdata-sections -O3 -DNDEBUG -fvisibility=hidden  -fno-exceptions -fno-rtti 
-UNDEBUG -std=c++17 -MD -MT 
lib/Target/BPF/CMakeFiles/LLVMBPFCodeGen.dir/BTFDebug.cpp.o -MF 
lib/Target/BPF/CMakeFiles/LLVMBPFCodeGen.dir/BTFDebug.cpp.o.d -o 
lib/Target/BPF/CMakeFiles/LLVMBPFCodeGen.dir/BTFDebug.cpp.o -c 
../lib/Target/BPF/BTFDebug.cpp
  ../lib/Target/BPF/BTFDebug.cpp: In function ‘llvm::DINodeArray 
lookupAnnotations(const llvm::DIType*)’:
  ../lib/Target/BPF/BTFDebug.cpp:516:21: error: ‘const class llvm::DIBasicType’ 
has no member named ‘getAnnotations’; did you mean ‘getEncoding’?
   Annots = SubTy->getAnnotations();
   ^~
   getEncoding
  ../lib/Target/BPF/BTFDebug.cpp:522:21: error: ‘const class 
llvm::DISubroutineType’ has no member named ‘getAnnotations’
   Annots = SubTy->getAnnotations();
   ^~
  [2272/3751] Building CXX object 
lib/Passes/CMakeFiles/LLVMPasses.dir/PassBuilderPipelines.cpp.o
  In file included from ../lib/Passes/PassBuilderPipelines.cpp:113:
  ../include/llvm/Transforms/Scalar/SROA.h:95:7: warning: ‘llvm::SROAPass’ 
declared with greater visibility than the type of its field 
‘llvm::SROAPass::SelectsToRewrite’ [-Wattributes]
   class SROAPass : public PassInfoMixin {
 ^~~~
  [2279/3751] Building CXX object 
lib/Passes/CMakeFiles/LLVMPasses.dir/PassBuilder.cpp.o
  In file included from ../lib/Passes/PassBuilder.cpp:214:
  ../include/llvm/Transforms/Scalar/SROA.h:95:7: warning: ‘llvm::SROAPass’ 
declared with greater visibility than the type of its field 
‘llvm::SROAPass::SelectsToRewrite’ [-Wattributes]
   class SROAPass : public PassInfoMixin {
 ^~~~
  ninja: build stopped: subcommand failed.
  [yhs@devbig309.ftw3 ~/work/llvm-project/llvm/build (main)]$

The top commit is

  959216f9b1f1  [AMDGPU] MIR-Tests for Multiplication using KBA

Could you take a look?

My cmake command line:

  cmake .. -DCMAKE_BUILD_TYPE=Release -G Ninja \
  -DLLVM_ENABLE_PROJECTS="clang;lld" \
  -DLLVM_TARGETS_TO_BUILD="BPF;X86" \
  -DLLVM_ENABLE_ASSERTIONS=ON \
  -DLLVM_ENABLE_ZLIB=ON \
  -DCMAKE_INSTALL_PREFIX=$PWD/install


eddyz87 added a comment.

> ../lib/Target/BPF/BTFDebug.cpp: In function ‘llvm::DINodeArray 
> lookupAnnotations(const llvm::DIType*)’:
> ../lib/Target/BPF/BTFDebug.cpp:516:21: error: ‘const class llvm::DIBasicType’ 
> has no member named ‘getAnnotations’; did you mean ‘getEncoding’?
>
>   Annots = SubTy->getAnnotations();
>   ^~
>   getEncoding
>
> ../lib/Target/BPF/BTFDebug.cpp:522:21: error: ‘const class 
> llvm::DISubroutineType’ has no member named ‘getAnnotations’
>
>   Annots = SubTy->getAnnotations();

[PATCH] D144829: [WIP][BPF] Add a few new insns under cpu=v4

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

Hi Yonghong,

Left a few nitpicks and one comment in `BPFMIPreEmitPeephole::adjustBranch()` 
that I think points to a bug.
Overall `adjustBranch()` algorithm looks good. It would be great to have some 
test cases for it, e.g. preprocess test .ll by replacing some template with a 
bunch of noops or something like this.
The instruction encoding seem to match mailing list description.
Also one `check-all` test is failing for me:

  home/eddy/work/llvm-project/clang/test/Misc/target-invalid-cpu-note.c:76:14: 
error: BPF-NEXT: expected string not found in input
  // BPF-NEXT: note: valid target CPU values are: generic, v1, v2, v3, 
probe{{$}}




Comment at: llvm/lib/Target/BPF/BPFInstrFormats.td:93
 def BPF_MEM  : BPFModeModifer<0x3>;
+def BPF_MEMS : BPFModeModifer<0x4>;
 def BPF_ATOMIC : BPFModeModifer<0x6>;

Nitpick: the mailing list doc refers to this as `BPF_SMEM`.



Comment at: llvm/lib/Target/BPF/BPFMIPeephole.cpp:331
+if (IsCPUv4)
+  Changed = adjustBranch() || Changed;
+return Changed;

Nitpick: this would not be executed for `-O0`, but is required for correct 
execution.

```
void BPFPassConfig::addPreEmitPass() {
  addPass(createBPFMIPreEmitCheckingPass());
  if (getOptLevel() != CodeGenOpt::None)
if (!DisableMIPeephole)
  addPass(createBPFMIPreEmitPeepholePass());
}
```



Comment at: llvm/lib/Target/BPF/BPFMIPeephole.cpp:452
+  JmpBB = UncondJmp->getOperand(0).getMBB();
+  Dist = SoFarNumInsns[JmpBB] - CurrNumInsns;
+  if (Dist <= INT16_MAX && Dist >= INT16_MIN)

As far as I understand:
- `SoFarNumInsns[JmpBB]` is a number of instructions from function start till 
the end of `JmpBB`;
- `CurrNumInsns` is a number of instructions from function start till the end 
of `MBB`.

So, `SoFarNumInsns[JmpBB] - CurrNumInsns` gives the distance between basic 
block ends. However, the jump would happen to the basic block start, so the 
actual distance should be computed as `SoFarNumInsns[JmpBB] - JmpBB.size() - 
CurrNumInsns`.

Am I confused?



Comment at: llvm/lib/Target/BPF/BPFMIPeephole.cpp:479
+  //
+  // We do not have 32bit cond jmp insn. So we try to do
+  // the following.

Is it possible to rewrite as below instead?

```
B2: ...
if (!cond) goto B3
gotol B5
B3: ...
```

Seems to be equivalent but with less instructions.



Comment at: llvm/lib/Target/BPF/BPFMIPeephole.cpp:549
+Dist = SoFarNumInsns[CondTargetBB] - CurrNumInsns;
+if (Dist > INT16_MAX || Dist < INT16_MIN) {
+  MachineBasicBlock *New_B = MF->CreateMachineBasicBlock(TermBB);

Nitpick: `(Dist <= INT16_MAX && Dist >= INT16_MIN)` is used in the previous two 
cases.



Comment at: llvm/lib/Target/BPF/MCTargetDesc/BPFAsmBackend.cpp:108
+  } else if (Fixup.getTargetKind() == BPF::FK_BPF_PCRel_4) {
+Value = (uint32_t)((Value - 8) / 8);
+support::endian::write([Fixup.getOffset() + 4], Value,

This is because `Value` is in bytes, right?
Could you please drop a comment here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144829

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

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

Hi Yonghong,

Thank you for taking care of this issue.
This was sloppy on my side as the parameter name was completely
irrelevant for the test.

Best regards,
Eduard


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142046

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

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

I can relax the warning to note to be on the same page with GCC, the reason I 
didn't is that similar things in DiagnosticDriverKinds.td are warnings.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142046

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

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

Slightly above my edit there is a similar logic for NVPTX target:

  static void RenderSSPOptions(const Driver , const ToolChain ,
   const ArgList , ArgStringList ,
   bool KernelOrKext) {
const llvm::Triple  = TC.getEffectiveTriple();
  
// NVPTX doesn't support stack protectors; from the compiler's perspective, 
it
// doesn't even have a stack!
if (EffectiveTriple.isNVPTX())
  return;
...

I don't like it because it produces rather vague error message:

  warning: argument unused during compilation: '-fstack-protector' 
[-Wunused-command-line-argument]

However I'm hesitant to merge the NVPTX check with BPF check to avoid any 
potential changes in the NVPTX target behavior (e.g. different warning).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D142046

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

[PATCH] D142046: [BPF][clang] Ignore stack protector options for BPF target

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 created this revision.
Herald added a project: All.
eddyz87 requested review of this revision.
Herald added subscribers: cfe-commits, MaskRay.
Herald added a project: clang.

Stack protector builtin functions are not implemented for BPF target,
thus compiling programs with one of the following options would result
in an error:

  -fstack-protector
  -fstack-protector-all
  -fstack-protector-strong

This commit adds logic to ignore these options for BPF target.
Searching through DiagnosticDriverKinds.td shows that all messages for
such kind of behavior are implemented as warnings, this commit follows
the suit.

Here is an example of the diagnostic message:

clang-16: warning: ignoring '-fstack-protector' option \

  as it is not currently supported for target 'bpf' [-Woption-ignored]


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D142046

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/CodeGen/bpf-stack-protector.c


Index: clang/test/CodeGen/bpf-stack-protector.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-stack-protector.c
@@ -0,0 +1,34 @@
+// REQUIRES: bpf-registered-target
+
+// RUN %clang -target bpf -S -emit-llvm -o - %s -fno-stack-protector 2>&1 \
+// RUN| FileCheck -check-prefix=OFF -check-prefix=COMMON %s
+
+// RUN: %clang -target bpf -S -emit-llvm -o - %s -fstack-protector 2>&1 \
+// RUN:| FileCheck -check-prefix=ON -check-prefix=COMMON %s
+
+// RUN: %clang -target bpf -S -emit-llvm -o - %s -fstack-protector-all 2>&1 \
+// RUN:| FileCheck -check-prefix=ALL -check-prefix=COMMON %s
+
+// RUN: %clang -target bpf -S -emit-llvm -o - %s -fstack-protector-strong 2>&1 
\
+// RUN:| FileCheck -check-prefix=STRONG -check-prefix=COMMON %s
+
+typedef __SIZE_TYPE__ size_t;
+
+int printf(const char * _Format, ...);
+size_t strlen(const char *s);
+char *strcpy(char *s1, const char *s2);
+
+// OFF-NOT: warning
+//  ON: warning: ignoring '-fstack-protector'
+// ALL: warning: ignoring '-fstack-protector-all'
+//  STRONG: warning: ignoring '-fstack-protector-strong'
+// COMMON-SAME: option as it is not currently supported for target 'bpf'
+
+// COMMON: define {{.*}}void @test1(ptr noundef %msg) #[[A:.*]] {
+void test1(const char *msg) {
+  char a[strlen(msg) + 1];
+  strcpy(a, msg);
+  printf("%s\n", a);
+}
+
+// COMMON-NOT: attributes #[[A]] = {{.*}} ssp
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3239,6 +3239,12 @@
   StackProtectorLevel = LangOptions::SSPStrong;
 else if (A->getOption().matches(options::OPT_fstack_protector_all))
   StackProtectorLevel = LangOptions::SSPReq;
+
+if (EffectiveTriple.isBPF() && StackProtectorLevel != LangOptions::SSPOff) 
{
+  D.Diag(diag::warn_drv_unsupported_option_for_target)
+  << A->getSpelling() << EffectiveTriple.getTriple();
+  StackProtectorLevel = DefaultStackProtectorLevel;
+}
   } else {
 StackProtectorLevel = DefaultStackProtectorLevel;
   }


Index: clang/test/CodeGen/bpf-stack-protector.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-stack-protector.c
@@ -0,0 +1,34 @@
+// REQUIRES: bpf-registered-target
+
+// RUN %clang -target bpf -S -emit-llvm -o - %s -fno-stack-protector 2>&1 \
+// RUN| FileCheck -check-prefix=OFF -check-prefix=COMMON %s
+
+// RUN: %clang -target bpf -S -emit-llvm -o - %s -fstack-protector 2>&1 \
+// RUN:| FileCheck -check-prefix=ON -check-prefix=COMMON %s
+
+// RUN: %clang -target bpf -S -emit-llvm -o - %s -fstack-protector-all 2>&1 \
+// RUN:| FileCheck -check-prefix=ALL -check-prefix=COMMON %s
+
+// RUN: %clang -target bpf -S -emit-llvm -o - %s -fstack-protector-strong 2>&1 \
+// RUN:| FileCheck -check-prefix=STRONG -check-prefix=COMMON %s
+
+typedef __SIZE_TYPE__ size_t;
+
+int printf(const char * _Format, ...);
+size_t strlen(const char *s);
+char *strcpy(char *s1, const char *s2);
+
+// OFF-NOT: warning
+//  ON: warning: ignoring '-fstack-protector'
+// ALL: warning: ignoring '-fstack-protector-all'
+//  STRONG: warning: ignoring '-fstack-protector-strong'
+// COMMON-SAME: option as it is not currently supported for target 'bpf'
+
+// COMMON: define {{.*}}void @test1(ptr noundef %msg) #[[A:.*]] {
+void test1(const char *msg) {
+  char a[strlen(msg) + 1];
+  strcpy(a, msg);
+  printf("%s\n", a);
+}
+
+// COMMON-NOT: attributes #[[A]] = {{.*}} ssp
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -3239,6 +3239,12 @@
   StackProtectorLevel = LangOptions::SSPStrong;
 else if 

[PATCH] D140970: [BPF] preserve btf_decl_tag for parameters of extern functions

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 updated this revision to Diff 486295.
eddyz87 added a comment.

Fix for formatting issue reported by CI.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140970

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGen/bpf-decl-tag-extern-func-args.c


Index: clang/test/CodeGen/bpf-decl-tag-extern-func-args.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-decl-tag-extern-func-args.c
@@ -0,0 +1,37 @@
+// RUN: %clang -target bpf -S -emit-llvm -g -O2 -o - %s  | FileCheck %s
+
+#define __decl_tag(x) __attribute__((btf_decl_tag(x)))
+
+extern void foo(int aa __decl_tag("aa_tag"), long bb __decl_tag("bb_tag"));
+extern void bar(char cc __decl_tag("cc_tag"));
+extern void buz(int __decl_tag("buz_arg_tag"));
+
+void root(void) {
+  foo(0, 1);
+  bar(0);
+  buz(0);
+}
+// CHECK: [[foo:![0-9]+]] = !DISubprogram(name: "foo", {{.*}}, retainedNodes: 
[[foo_nodes:![0-9]+]])
+// CHECK: [[foo_nodes]] = !{[[aa:![0-9]+]], [[bb:![0-9]+]]}
+
+// CHECK: [[aa]] = !DILocalVariable(name: "aa", arg: 1, scope: [[foo]], 
{{.*}}, annotations: [[aa_annot:![0-9]+]])
+// CHECK: [[aa_annot]] = !{[[aa_tag:![0-9]+]]}
+// CHECK: [[aa_tag]] = !{!"btf_decl_tag", !"aa_tag"}
+
+// CHECK: [[bb]] = !DILocalVariable(name: "bb", arg: 2, scope: [[foo]], 
{{.*}}, annotations: [[bb_annot:![0-9]+]])
+// CHECK: [[bb_annot]] = !{[[bb_tag:![0-9]+]]}
+// CHECK: [[bb_tag]] = !{!"btf_decl_tag", !"bb_tag"}
+
+// CHECK: [[bar:![0-9]+]] = !DISubprogram(name: "bar", {{.*}}, retainedNodes: 
[[bar_nodes:![0-9]+]])
+// CHECK: [[bar_nodes]] = !{[[cc:![0-9]+]]}
+
+// CHECK: [[cc]] = !DILocalVariable(name: "cc", arg: 1, scope: [[bar]], 
{{.*}}, annotations: [[cc_annot:![0-9]+]])
+// CHECK: [[cc_annot]] = !{[[cc_tag:![0-9]+]]}
+// CHECK: [[cc_tag]] = !{!"btf_decl_tag", !"cc_tag"}
+
+// CHECK: [[buz:![0-9]+]] = !DISubprogram(name: "buz", {{.*}}, retainedNodes: 
[[buz_nodes:![0-9]+]])
+// CHECK: [[buz_nodes]] = !{[[buz_arg:![0-9]+]]}
+
+// CHECK: [[buz_arg]] = !DILocalVariable(arg: 1, scope: [[buz]], {{.*}}, 
annotations: [[buz_arg_annot:![0-9]+]])
+// CHECK: [[buz_arg_annot]] = !{[[buz_arg_tag:![0-9]+]]}
+// CHECK: [[buz_arg_tag]] = !{!"btf_decl_tag", !"buz_arg_tag"}
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -4206,10 +4206,28 @@
 SPFlags |= llvm::DISubprogram::SPFlagOptimized;
 
   llvm::DINodeArray Annotations = CollectBTFDeclTagAnnotations(D);
-  llvm::DISubprogram *SP = DBuilder.createFunction(
-  FDContext, Name, LinkageName, Unit, LineNo,
-  getOrCreateFunctionType(D, FnType, Unit), ScopeLine, Flags, SPFlags,
-  TParamsArray.get(), getFunctionDeclaration(D), nullptr, Annotations);
+  llvm::DISubroutineType *STy = getOrCreateFunctionType(D, FnType, Unit);
+  llvm::DISubprogram *SP =
+  DBuilder.createFunction(FDContext, Name, LinkageName, Unit, LineNo, STy,
+  ScopeLine, Flags, SPFlags, TParamsArray.get(),
+  getFunctionDeclaration(D), nullptr, Annotations);
+
+  // Preserve btf_decl_tag attributes for parameters of extern functions
+  // for BPF target. The parameters created in this loop are attached as
+  // DISubprogram's retainedNodes in the subsequent finalizeSubprogram call.
+  if (IsDeclForCallSite && CGM.getTarget().getTriple().isBPF()) {
+if (auto *FD = dyn_cast(D)) {
+  llvm::DITypeRefArray ParamTypes = STy->getTypeArray();
+  unsigned ArgNo = 1;
+  for (ParmVarDecl *PD : FD->parameters()) {
+llvm::DINodeArray ParamAnnotations = CollectBTFDeclTagAnnotations(PD);
+DBuilder.createParameterVariable(
+SP, PD->getName(), ArgNo, Unit, LineNo, ParamTypes[ArgNo], true,
+llvm::DINode::FlagZero, ParamAnnotations);
+++ArgNo;
+  }
+}
+  }
 
   if (IsDeclForCallSite)
 Fn->setSubprogram(SP);


Index: clang/test/CodeGen/bpf-decl-tag-extern-func-args.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-decl-tag-extern-func-args.c
@@ -0,0 +1,37 @@
+// RUN: %clang -target bpf -S -emit-llvm -g -O2 -o - %s  | FileCheck %s
+
+#define __decl_tag(x) __attribute__((btf_decl_tag(x)))
+
+extern void foo(int aa __decl_tag("aa_tag"), long bb __decl_tag("bb_tag"));
+extern void bar(char cc __decl_tag("cc_tag"));
+extern void buz(int __decl_tag("buz_arg_tag"));
+
+void root(void) {
+  foo(0, 1);
+  bar(0);
+  buz(0);
+}
+// CHECK: [[foo:![0-9]+]] = !DISubprogram(name: "foo", {{.*}}, retainedNodes: [[foo_nodes:![0-9]+]])
+// CHECK: [[foo_nodes]] = !{[[aa:![0-9]+]], [[bb:![0-9]+]]}
+
+// CHECK: [[aa]] = !DILocalVariable(name: "aa", arg: 1, scope: [[foo]], {{.*}}, annotations: [[aa_annot:![0-9]+]])
+// CHECK: [[aa_annot]] = !{[[aa_tag:![0-9]+]]}
+// CHECK: [[aa_tag]] = 

[PATCH] D140970: [BPF] preserve btf_decl_tag for parameters of extern functions

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 created this revision.
Herald added a project: All.
eddyz87 published this revision for review.
eddyz87 added a reviewer: yonghong-song.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.

Generate DILocalVariable entries for parameters of extern functions,
the "annotations" field of DILocalVariable is used to link
"btf_decl_tag" annotation with the parameter.

Do this only for BPF backend as there are no other users for this
information. Final DWARF is valid as "Appendix A" is very much lax in
what is allowed as attributes for "DW_TAG_formal_parameter":

> DWARF does not in general require that a given debugging information
> entry contain a particular attribute or set of attributes. Instead,
> a DWARF producer is free to generate any, all, or none of the
> attributes ... other attributes ... may also appear in a given
> debugging information entry.

DWARF Debugging Information Format Version 5,
Appendix A: Attributes by Tag Value (Informative)
Page 251, Line 3.

Depends on D140969 


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140970

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGen/bpf-decl-tag-extern-func-args.c


Index: clang/test/CodeGen/bpf-decl-tag-extern-func-args.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-decl-tag-extern-func-args.c
@@ -0,0 +1,37 @@
+// RUN: %clang -target bpf -S -emit-llvm -g -O2 -o - %s  | FileCheck %s
+
+#define __decl_tag(x) __attribute__((btf_decl_tag(x)))
+
+extern void foo(int aa __decl_tag("aa_tag"), long bb __decl_tag("bb_tag"));
+extern void bar(char cc __decl_tag("cc_tag"));
+extern void buz(int __decl_tag("buz_arg_tag"));
+
+void root(void) {
+  foo(0, 1);
+  bar(0);
+  buz(0);
+}
+// CHECK: [[foo:![0-9]+]] = !DISubprogram(name: "foo", {{.*}}, retainedNodes: 
[[foo_nodes:![0-9]+]])
+// CHECK: [[foo_nodes]] = !{[[aa:![0-9]+]], [[bb:![0-9]+]]}
+
+// CHECK: [[aa]] = !DILocalVariable(name: "aa", arg: 1, scope: [[foo]], 
{{.*}}, annotations: [[aa_annot:![0-9]+]])
+// CHECK: [[aa_annot]] = !{[[aa_tag:![0-9]+]]}
+// CHECK: [[aa_tag]] = !{!"btf_decl_tag", !"aa_tag"}
+
+// CHECK: [[bb]] = !DILocalVariable(name: "bb", arg: 2, scope: [[foo]], 
{{.*}}, annotations: [[bb_annot:![0-9]+]])
+// CHECK: [[bb_annot]] = !{[[bb_tag:![0-9]+]]}
+// CHECK: [[bb_tag]] = !{!"btf_decl_tag", !"bb_tag"}
+
+// CHECK: [[bar:![0-9]+]] = !DISubprogram(name: "bar", {{.*}}, retainedNodes: 
[[bar_nodes:![0-9]+]])
+// CHECK: [[bar_nodes]] = !{[[cc:![0-9]+]]}
+
+// CHECK: [[cc]] = !DILocalVariable(name: "cc", arg: 1, scope: [[bar]], 
{{.*}}, annotations: [[cc_annot:![0-9]+]])
+// CHECK: [[cc_annot]] = !{[[cc_tag:![0-9]+]]}
+// CHECK: [[cc_tag]] = !{!"btf_decl_tag", !"cc_tag"}
+
+// CHECK: [[buz:![0-9]+]] = !DISubprogram(name: "buz", {{.*}}, retainedNodes: 
[[buz_nodes:![0-9]+]])
+// CHECK: [[buz_nodes]] = !{[[buz_arg:![0-9]+]]}
+
+// CHECK: [[buz_arg]] = !DILocalVariable(arg: 1, scope: [[buz]], {{.*}}, 
annotations: [[buz_arg_annot:![0-9]+]])
+// CHECK: [[buz_arg_annot]] = !{[[buz_arg_tag:![0-9]+]]}
+// CHECK: [[buz_arg_tag]] = !{!"btf_decl_tag", !"buz_arg_tag"}
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -4206,11 +4206,29 @@
 SPFlags |= llvm::DISubprogram::SPFlagOptimized;
 
   llvm::DINodeArray Annotations = CollectBTFDeclTagAnnotations(D);
+  llvm::DISubroutineType *STy = getOrCreateFunctionType(D, FnType, Unit);
   llvm::DISubprogram *SP = DBuilder.createFunction(
   FDContext, Name, LinkageName, Unit, LineNo,
-  getOrCreateFunctionType(D, FnType, Unit), ScopeLine, Flags, SPFlags,
+  STy, ScopeLine, Flags, SPFlags,
   TParamsArray.get(), getFunctionDeclaration(D), nullptr, Annotations);
 
+  // Preserve btf_decl_tag attributes for parameters of extern functions
+  // for BPF target. The parameters created in this loop are attached as
+  // DISubprogram's retainedNodes in the subsequent finalizeSubprogram call.
+  if (IsDeclForCallSite && CGM.getTarget().getTriple().isBPF()) {
+if (auto *FD = dyn_cast(D)) {
+  llvm::DITypeRefArray ParamTypes = STy->getTypeArray();
+  unsigned ArgNo = 1;
+  for (ParmVarDecl *PD : FD->parameters()) {
+llvm::DINodeArray ParamAnnotations = CollectBTFDeclTagAnnotations(PD);
+DBuilder.createParameterVariable
+  (SP, PD->getName(), ArgNo, Unit, LineNo, ParamTypes[ArgNo], true,
+   llvm::DINode::FlagZero, ParamAnnotations);
+++ArgNo;
+  }
+}
+  }
+
   if (IsDeclForCallSite)
 Fn->setSubprogram(SP);
 


Index: clang/test/CodeGen/bpf-decl-tag-extern-func-args.c
===
--- /dev/null
+++ clang/test/CodeGen/bpf-decl-tag-extern-func-args.c
@@ -0,0 +1,37 @@
+// RUN: %clang -target bpf -S -emit-llvm -g -O2 -o - %s  | FileCheck %s
+

[PATCH] D140929: [BPF] Support for "btf_decl_tag" annotations for arguments of extern functions

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 abandoned this revision.
eddyz87 added a comment.

I think I messed up while creating this revision. I've created a proper stack 
of revisions with a tip at D140971 . Sorry 
for all the spam in emails.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140929

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

[PATCH] D140929: [BPF] Support for "btf_decl_tag" annotations for arguments of extern functions

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 updated this revision to Diff 486245.
eddyz87 added a comment.

Reorganize commits as a stack.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140929

Files:
  llvm/lib/Target/BPF/BTFDebug.cpp
  llvm/lib/Target/BPF/BTFDebug.h
  llvm/test/CodeGen/BPF/BTF/tag-extern-func.ll

Index: llvm/test/CodeGen/BPF/BTF/tag-extern-func.ll
===
--- /dev/null
+++ llvm/test/CodeGen/BPF/BTF/tag-extern-func.ll
@@ -0,0 +1,96 @@
+; RUN: llc -march=bpfel -filetype=asm -o - %s | FileCheck -check-prefixes=CHECK %s
+; RUN: llc -march=bpfeb -filetype=asm -o - %s | FileCheck -check-prefixes=CHECK %s
+
+; Source code:
+;   #define __tag(x) __attribute__((btf_decl_tag(x)))
+;
+;   extern void foo(int x __tag("x_tag"), int y __tag("y_tag")) __tag("foo_tag");
+;
+;   void root(void) {
+; foo(0, 0);
+;   }
+; Compilation flag:
+;   clang -target bpf -O2 -g -S -emit-llvm test.c
+
+
+; Function Attrs: nounwind
+define dso_local void @root() local_unnamed_addr #0 !dbg !7 {
+entry:
+  tail call void @foo(i32 noundef 0, i32 noundef 0) #2, !dbg !12
+  ret void, !dbg !13
+}
+
+declare !dbg !14 dso_local void @foo(i32 noundef, i32 noundef) local_unnamed_addr #1
+
+attributes #0 = { nounwind "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
+attributes #1 = { "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
+attributes #2 = { nounwind }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 16.0.0 (https://github.com/llvm/llvm-project.git 603e8490729e477680f0bc8284e136ceeb66e7f4)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "/home/eddy/work/tasks/llvm-decl-tags/test.c", directory: "/home/eddy/work/tasks/llvm-decl-tags", checksumkind: CSK_MD5, checksum: "0a768161d04f21ca6734ef31f6258656")
+!2 = !{i32 7, !"Dwarf Version", i32 5}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 7, !"frame-pointer", i32 2}
+!6 = !{!"clang version 16.0.0 (https://github.com/llvm/llvm-project.git 603e8490729e477680f0bc8284e136ceeb66e7f4)"}
+!7 = distinct !DISubprogram(name: "root", scope: !8, file: !8, line: 5, type: !9, scopeLine: 5, flags: DIFlagPrototyped | DIFlagAllCallsDescribed, spFlags: DISPFlagDefinition | DISPFlagOptimized, unit: !0, retainedNodes: !11)
+!8 = !DIFile(filename: "test.c", directory: "/home/eddy/work/tasks/llvm-decl-tags", checksumkind: CSK_MD5, checksum: "0a768161d04f21ca6734ef31f6258656")
+!9 = !DISubroutineType(types: !10)
+!10 = !{null}
+!11 = !{}
+!12 = !DILocation(line: 6, column: 3, scope: !7)
+!13 = !DILocation(line: 7, column: 1, scope: !7)
+!14 = !DISubprogram(name: "foo", scope: !8, file: !8, line: 3, type: !15, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !18, annotations: !25)
+!15 = !DISubroutineType(types: !16)
+!16 = !{null, !17, !17}
+!17 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!18 = !{!19, !22}
+!19 = !DILocalVariable(name: "x", arg: 1, scope: !14, file: !8, line: 3, type: !17, annotations: !20)
+!20 = !{!21}
+!21 = !{!"btf_decl_tag", !"x_tag"}
+!22 = !DILocalVariable(name: "y", arg: 2, scope: !14, file: !8, line: 3, type: !17, annotations: !23)
+!23 = !{!24}
+!24 = !{!"btf_decl_tag", !"y_tag"}
+!25 = !{!26}
+!26 = !{!"btf_decl_tag", !"foo_tag"}
+
+; CHECK: 	.long	0   # BTF_KIND_FUNC_PROTO(id = 1)
+; CHECK-NEXT: 	.long	218103808   # 0xd00
+; CHECK-NEXT: 	.long	0
+; CHECK-NEXT: 	.long	1   # BTF_KIND_FUNC(id = 2)
+; CHECK-NEXT: 	.long	201326593   # 0xc01
+; CHECK-NEXT: 	.long	1
+; CHECK-NEXT: 	.long	0   # BTF_KIND_FUNC_PROTO(id = 3)
+; CHECK-NEXT: 	.long	218103810   # 0xd02
+; CHECK-NEXT: 	.long	0
+; CHECK-NEXT: 	.long	0
+; CHECK-NEXT: 	.long	4
+; CHECK-NEXT: 	.long	0
+; CHECK-NEXT: 	.long	4
+; CHECK-NEXT: 	.long	71  # BTF_KIND_INT(id = 4)
+; CHECK-NEXT: 	.long	16777216# 0x100
+; CHECK-NEXT: 	.long	4
+; CHECK-NEXT: 	.long	16777248# 0x120
+; CHECK-NEXT: 	.long	75  # BTF_KIND_FUNC(id = 5)
+; CHECK-NEXT: 	.long	201326594   # 0xc02
+; CHECK-NEXT: 	.long	3
+; CHECK-NEXT: 	.long	79  # BTF_KIND_DECL_TAG(id = 6)
+; CHECK-NEXT: 	.long	285212672   # 0x1100
+; CHECK-NEXT: 	.long	5
+; CHECK-NEXT: 	.long	0
+; CHECK-NEXT: 	.long	85  # BTF_KIND_DECL_TAG(id = 7)
+; CHECK-NEXT: 	.long	285212672   # 0x1100
+; CHECK-NEXT: 	.long	5
+; 

[PATCH] D140929: [BPF] Support for "btf_decl_tag" annotations for arguments of extern functions

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D140929#4025562 , @eddyz87 wrote:

> In D140929#4025118 , @yonghong-song 
> wrote:
>
>> Please separate the patch to clang and llvm part.
>
> It's already split in three commits: two for clang, one for llvm.
> Not sure if this is possible to see in the Phabricator UI, though.

However `arc patch D140929` applies this revision as a single commit. Maybe I 
messed something up while creating revision.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140929

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

[PATCH] D140929: [BPF] Support for "btf_decl_tag" annotations for arguments of extern functions

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D140929#4025118 , @yonghong-song 
wrote:

> Please separate the patch to clang and llvm part.

It's already split in three commits: two for clang, one for llvm.
Not sure if this is possible to see in the Phabricator UI, though.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140929

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

[PATCH] D140929: [BPF] Support for "btf_decl_tag" annotations for arguments of extern functions

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 created this revision.
Herald added a subscriber: hiraditya.
Herald added a project: All.
eddyz87 retitled this revision from "This series of patches adds support for 
"btf_decl_tag" annotations
for arguments of extern functions. Patch descriptions follow.

[BPF] Triple::isBPF() utility method" to "[BPF] Support for "btf_decl_tag" 
annotations for arguments of extern functions".
eddyz87 edited the summary of this revision.
eddyz87 published this revision for review.
eddyz87 added a reviewer: yonghong-song.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

This adds support for "btf_decl_tag" annotations for arguments of extern 
functions. The changes are split into several commits:

- `Triple::isBPF() utility method` -- adds a simple utility method for 
llvm::Triple class;
- `preserve btf_decl_tag for parameters of extern functions` -- modifies CLang 
frontend to generate DILocalVariable entries with attached "btf_dec_tag" 
annotations for parameters of extern functions;
- `generate btf_decl_tag records for params of extern functions` -- modifies 
BPF backend to process "btf_decl_tag" annotations for parameters of extern 
functions.

Below are descriptions for the last two commits:

Preserve btf_decl_tag for parameters of extern functions


Generate DILocalVariable entries for parameters of extern functions, the 
"annotations" field of DILocalVariable is used to link "btf_decl_tag" 
annotation with the parameter.

Do this only for BPF backend as there are no other users for this information. 
Final DWARF is valid as "Appendix A" is very much lax in
what is allowed as attributes for "DW_TAG_formal_parameter":

> DWARF does not in general require that a given debugging information entry 
> contain a particular attribute or set of attributes. Instead, a DWARF 
> producer is free to generate any, all, or none of the attributes ... other 
> attributes ... may also appear in a given  debugging information entry.

DWARF Debugging Information Format Version 5,
Appendix A: Attributes by Tag Value (Informative)
Page 251, Line 3.

Generate btf_decl_tag records for params of extern functions


After frontend changes in the following commit: "BPF: preserve btf_decl_tag for 
parameters of extern functions" same mechanics could be used to get the list of 
function parameters and associated btf_decl_tag entries for both extern and 
non-extern functions.

This commit extracts this mechanics as a separate auxiliary function 
BTFDebug::processDISubprogram(). The function is called for both
extern and non-extern functions in order to generated corresponding 
BTF_DECL_TAG records.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D140929

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGen/bpf-decl-tag-extern-func-args.c
  llvm/include/llvm/TargetParser/Triple.h
  llvm/lib/Target/BPF/BTFDebug.cpp
  llvm/lib/Target/BPF/BTFDebug.h
  llvm/test/CodeGen/BPF/BTF/tag-extern-func.ll

Index: llvm/test/CodeGen/BPF/BTF/tag-extern-func.ll
===
--- /dev/null
+++ llvm/test/CodeGen/BPF/BTF/tag-extern-func.ll
@@ -0,0 +1,96 @@
+; RUN: llc -march=bpfel -filetype=asm -o - %s | FileCheck -check-prefixes=CHECK %s
+; RUN: llc -march=bpfeb -filetype=asm -o - %s | FileCheck -check-prefixes=CHECK %s
+
+; Source code:
+;   #define __tag(x) __attribute__((btf_decl_tag(x)))
+;
+;   extern void foo(int x __tag("x_tag"), int y __tag("y_tag")) __tag("foo_tag");
+;
+;   void root(void) {
+; foo(0, 0);
+;   }
+; Compilation flag:
+;   clang -target bpf -O2 -g -S -emit-llvm test.c
+
+
+; Function Attrs: nounwind
+define dso_local void @root() local_unnamed_addr #0 !dbg !7 {
+entry:
+  tail call void @foo(i32 noundef 0, i32 noundef 0) #2, !dbg !12
+  ret void, !dbg !13
+}
+
+declare !dbg !14 dso_local void @foo(i32 noundef, i32 noundef) local_unnamed_addr #1
+
+attributes #0 = { nounwind "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
+attributes #1 = { "frame-pointer"="all" "no-trapping-math"="true" "stack-protector-buffer-size"="8" }
+attributes #2 = { nounwind }
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!2, !3, !4, !5}
+!llvm.ident = !{!6}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C11, file: !1, producer: "clang version 16.0.0 (https://github.com/llvm/llvm-project.git 603e8490729e477680f0bc8284e136ceeb66e7f4)", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, splitDebugInlining: false, nameTableKind: None)
+!1 = !DIFile(filename: "/home/eddy/work/tasks/llvm-decl-tags/test.c", directory: "/home/eddy/work/tasks/llvm-decl-tags", checksumkind: CSK_MD5, checksum: "0a768161d04f21ca6734ef31f6258656")
+!2 = !{i32 7, !"Dwarf Version", i32 5}
+!3 = !{i32 2, !"Debug Info Version", i32 3}
+!4 = !{i32 1, !"wchar_size", i32 4}
+!5 = !{i32 7, 

[PATCH] D136041: [clang][DebugInfo] Emit DISubprogram for extern functions with reserved names

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 marked 2 inline comments as done.
eddyz87 added inline comments.



Comment at: llvm/lib/IR/Verifier.cpp:3473
   !Call.getCalledFunction()->isInterposable() &&
+  !Call.getCalledFunction()->isDeclaration() &&
   Call.getCalledFunction()->getSubprogram())

MaskRay wrote:
> Removing this check does not break any `check-llvm check-clang` test.
Well, that's embarrassing, thank you for catching it!
I've updated `llvm/test/Verifier/callsite-dbgloc.ll` by adding `!dbg` metadata 
for `@i`. Know the test fails if the check above is commented out.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136041

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

[PATCH] D136041: [clang][DebugInfo] Emit DISubprogram for extern functions with reserved names

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 updated this revision to Diff 469081.
eddyz87 marked an inline comment as done.
eddyz87 added a comment.

Fixed test case to fail if `!Call.getCalledFunction()->isDeclaration()` 
condition is removed in `Verifier::visitCallBase`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136041

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGen/debug-info-extern-call.c
  llvm/lib/IR/Verifier.cpp
  llvm/test/Verifier/callsite-dbgloc.ll


Index: llvm/test/Verifier/callsite-dbgloc.ll
===
--- llvm/test/Verifier/callsite-dbgloc.ll
+++ llvm/test/Verifier/callsite-dbgloc.ll
@@ -33,18 +33,24 @@
   ret void, !dbg !22
 }
 
-declare void @i(...) #1
+declare !dbg !23 void @i(...) #1
 
 ; Function Attrs: nounwind ssp uwtable
 define void @g() #0 !dbg !11 {
 entry:
 ; Manually removed !dbg.
 ; CHECK: inlinable function call in a function with debug info must have a 
!dbg location
+; CHECK: @h()
   call void @h()
 ; CHECK-NOT: inlinable function call in a function with debug info must have a 
!dbg location
+; CHECK-NOT: @j()
   call void @j()
 ; CHECK: inlinable function call in a function with debug info must have a 
!dbg location
+; CHECK: @k()
   call void @k()
+; CHECK-NOT: inlinable function call in a function with debug info must have a 
!dbg location
+; CHECK-NOT: @i()
+  call void (...) @i()
   ret void, !dbg !13
 }
 
@@ -86,3 +92,6 @@
 !20 = distinct !DISubprogram(name: "k", scope: !1, file: !1, line: 6, type: 
!8, isLocal: false, isDefinition: true, scopeLine: 6, isOptimized: false, unit: 
!0, retainedNodes: !2)
 !21 = !DILocation(line: 4, column: 12, scope: !20)
 !22 = !DILocation(line: 4, column: 17, scope: !20)
+!23 = !DISubprogram(name: "i", scope: !1, file: !1, line: 1, type: !24, flags: 
DIFlagPrototyped, spFlags: DISPFlagOptimized, retainedNodes: !2)
+!24 = !DISubroutineType(types: !25)
+!25 = !{null}
Index: llvm/lib/IR/Verifier.cpp
===
--- llvm/lib/IR/Verifier.cpp
+++ llvm/lib/IR/Verifier.cpp
@@ -3466,9 +3466,11 @@
   // Verify that each inlinable callsite of a debug-info-bearing function in a
   // debug-info-bearing function has a debug location attached to it. Failure 
to
   // do so causes assertion failures when the inliner sets up inline scope info
-  // (Interposable functions are not inlinable).
+  // (Interposable functions are not inlinable, neither are functions without
+  //  definitions.)
   if (Call.getFunction()->getSubprogram() && Call.getCalledFunction() &&
   !Call.getCalledFunction()->isInterposable() &&
+  !Call.getCalledFunction()->isDeclaration() &&
   Call.getCalledFunction()->getSubprogram())
 CheckDI(Call.getDebugLoc(),
 "inlinable function call in a function with "
Index: clang/test/CodeGen/debug-info-extern-call.c
===
--- clang/test/CodeGen/debug-info-extern-call.c
+++ clang/test/CodeGen/debug-info-extern-call.c
@@ -29,8 +29,8 @@
 // DECLS-FOR-EXTERN: [[FN1_TYPES]] = !{[[X_TYPE:![0-9]+]],
 // DECLS-FOR-EXTERN: [[X_TYPE]] = !DIDerivedType(tag: DW_TAG_typedef, name: 
"x",
 // DECLS-FOR-EXTERN-SAME: baseType: [[INT_TYPE]])
-// DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "memcmp"
-// DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "__some_reserved_name"
+// DECLS-FOR-EXTERN: !DISubprogram(name: "memcmp"
+// DECLS-FOR-EXTERN: !DISubprogram(name: "__some_reserved_name"
 
 // NO-DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "fn1"
 // NO-DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "memcmp"
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -4228,17 +4228,11 @@
   if (Func->getSubprogram())
 return;
 
-  // Do not emit a declaration subprogram for a builtin, a function with 
nodebug
-  // attribute, or if call site info isn't required. Also, elide declarations
-  // for functions with reserved names, as call site-related features aren't
-  // interesting in this case (& also, the compiler may emit calls to these
-  // functions without debug locations, which makes the verifier complain).
-  if (CalleeDecl->getBuiltinID() != 0 || CalleeDecl->hasAttr() ||
+  // Do not emit a declaration subprogram for a function with nodebug
+  // attribute, or if call site info isn't required.
+  if (CalleeDecl->hasAttr() ||
   getCallSiteRelatedAttrs() == llvm::DINode::FlagZero)
 return;
-  if (CalleeDecl->isReserved(CGM.getLangOpts()) !=
-  ReservedIdentifierStatus::NotReserved)
-return;
 
   // If there is no DISubprogram attached to the function being called,
   // create the one describing the function in order to have complete


Index: llvm/test/Verifier/callsite-dbgloc.ll
===
--- 

[PATCH] D136041: [clang][DebugInfo] Emit DISubprogram for extern functions with reserved names

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 updated this revision to Diff 468611.
eddyz87 added a comment.

Comment fixed.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136041

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGen/debug-info-extern-call.c
  llvm/lib/IR/Verifier.cpp
  llvm/test/Verifier/callsite-dbgloc.ll


Index: llvm/test/Verifier/callsite-dbgloc.ll
===
--- llvm/test/Verifier/callsite-dbgloc.ll
+++ llvm/test/Verifier/callsite-dbgloc.ll
@@ -45,6 +45,9 @@
   call void @j()
 ; CHECK: inlinable function call in a function with debug info must have a 
!dbg location
   call void @k()
+; CHECK-NOT: inlinable function call in a function with debug info must have a 
!dbg location
+; CHECK-NOT: @i
+  call void (...) @i()
   ret void, !dbg !13
 }
 
Index: llvm/lib/IR/Verifier.cpp
===
--- llvm/lib/IR/Verifier.cpp
+++ llvm/lib/IR/Verifier.cpp
@@ -3466,9 +3466,11 @@
   // Verify that each inlinable callsite of a debug-info-bearing function in a
   // debug-info-bearing function has a debug location attached to it. Failure 
to
   // do so causes assertion failures when the inliner sets up inline scope info
-  // (Interposable functions are not inlinable).
+  // (Interposable functions are not inlinable, neither are functions without
+  //  definitions.)
   if (Call.getFunction()->getSubprogram() && Call.getCalledFunction() &&
   !Call.getCalledFunction()->isInterposable() &&
+  !Call.getCalledFunction()->isDeclaration() &&
   Call.getCalledFunction()->getSubprogram())
 CheckDI(Call.getDebugLoc(),
 "inlinable function call in a function with "
Index: clang/test/CodeGen/debug-info-extern-call.c
===
--- clang/test/CodeGen/debug-info-extern-call.c
+++ clang/test/CodeGen/debug-info-extern-call.c
@@ -29,8 +29,8 @@
 // DECLS-FOR-EXTERN: [[FN1_TYPES]] = !{[[X_TYPE:![0-9]+]],
 // DECLS-FOR-EXTERN: [[X_TYPE]] = !DIDerivedType(tag: DW_TAG_typedef, name: 
"x",
 // DECLS-FOR-EXTERN-SAME: baseType: [[INT_TYPE]])
-// DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "memcmp"
-// DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "__some_reserved_name"
+// DECLS-FOR-EXTERN: !DISubprogram(name: "memcmp"
+// DECLS-FOR-EXTERN: !DISubprogram(name: "__some_reserved_name"
 
 // NO-DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "fn1"
 // NO-DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "memcmp"
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -4228,17 +4228,11 @@
   if (Func->getSubprogram())
 return;
 
-  // Do not emit a declaration subprogram for a builtin, a function with 
nodebug
-  // attribute, or if call site info isn't required. Also, elide declarations
-  // for functions with reserved names, as call site-related features aren't
-  // interesting in this case (& also, the compiler may emit calls to these
-  // functions without debug locations, which makes the verifier complain).
-  if (CalleeDecl->getBuiltinID() != 0 || CalleeDecl->hasAttr() ||
+  // Do not emit a declaration subprogram for a function with nodebug
+  // attribute, or if call site info isn't required.
+  if (CalleeDecl->hasAttr() ||
   getCallSiteRelatedAttrs() == llvm::DINode::FlagZero)
 return;
-  if (CalleeDecl->isReserved(CGM.getLangOpts()) !=
-  ReservedIdentifierStatus::NotReserved)
-return;
 
   // If there is no DISubprogram attached to the function being called,
   // create the one describing the function in order to have complete


Index: llvm/test/Verifier/callsite-dbgloc.ll
===
--- llvm/test/Verifier/callsite-dbgloc.ll
+++ llvm/test/Verifier/callsite-dbgloc.ll
@@ -45,6 +45,9 @@
   call void @j()
 ; CHECK: inlinable function call in a function with debug info must have a !dbg location
   call void @k()
+; CHECK-NOT: inlinable function call in a function with debug info must have a !dbg location
+; CHECK-NOT: @i
+  call void (...) @i()
   ret void, !dbg !13
 }
 
Index: llvm/lib/IR/Verifier.cpp
===
--- llvm/lib/IR/Verifier.cpp
+++ llvm/lib/IR/Verifier.cpp
@@ -3466,9 +3466,11 @@
   // Verify that each inlinable callsite of a debug-info-bearing function in a
   // debug-info-bearing function has a debug location attached to it. Failure to
   // do so causes assertion failures when the inliner sets up inline scope info
-  // (Interposable functions are not inlinable).
+  // (Interposable functions are not inlinable, neither are functions without
+  //  definitions.)
   if (Call.getFunction()->getSubprogram() && Call.getCalledFunction() &&
   !Call.getCalledFunction()->isInterposable() &&
+  

[PATCH] D136041: [clang][DebugInfo] Emit DISubprogram for extern functions with reserved names

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D136041#3862741 , @aprantl wrote:

> ... Do you have any idea how much this will grow the debug info for something 
> like, e.g., clang?

I made the measurements but I'm not sure what to make of these numbers (e.g. is 
it good or bad).
There were no changes in executable size for Clang (in Debug mode, I'll try in 
Release mode a bit later), so I tried the Linux kernel with debug symbols 
enabled instead.
Here are executable sizes in three modes:

- (main) w/o my changes;
- (a) with these two commits;
- (b) with these two commits but with `CalleeDecl->getBuiltinID() != 0` in 
`CGDebugInfo::EmitFuncDeclForCallSite` put back.

|| a   | b   | main| a - 
main   | b - main   |
| vmlinux size (bytes)   | 667,827,456 | 667,365,792 | 663,996,200 | 
+3,831,256 | +3,369,592 |
| Debug section size (bytes) | 565,548,585 | 565,086,931 | 561,717,339 | 
+3,831,246 | +3,369,592 |
| #call site DIEs| 311,217 | 303,909 | 242,933 | 
+68,284| +60,976|
| #call site parameter DIEs  | 369,405 | 361,080 | 301,738 | 
+67,667| +59,342|
|

The BTF use-case does not care about the builtin functions, so I can change 
this revision to variant (b) to save some space.
What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136041

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

[PATCH] D136041: [clang][DebugInfo] Emit DISubprogram for extern functions with reserved names

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 updated this revision to Diff 468514.
eddyz87 added a comment.

Split the original commit in two to separate clang and llvm parts.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136041

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGen/debug-info-extern-call.c
  llvm/lib/IR/Verifier.cpp
  llvm/test/Verifier/callsite-dbgloc.ll


Index: llvm/test/Verifier/callsite-dbgloc.ll
===
--- llvm/test/Verifier/callsite-dbgloc.ll
+++ llvm/test/Verifier/callsite-dbgloc.ll
@@ -45,6 +45,9 @@
   call void @j()
 ; CHECK: inlinable function call in a function with debug info must have a 
!dbg location
   call void @k()
+; CHECK-NOT: inlinable function call in a function with debug info must have a 
!dbg location
+; CHECK-NOT: @i
+  call void (...) @i()
   ret void, !dbg !13
 }
 
Index: llvm/lib/IR/Verifier.cpp
===
--- llvm/lib/IR/Verifier.cpp
+++ llvm/lib/IR/Verifier.cpp
@@ -3466,9 +3466,11 @@
   // Verify that each inlinable callsite of a debug-info-bearing function in a
   // debug-info-bearing function has a debug location attached to it. Failure 
to
   // do so causes assertion failures when the inliner sets up inline scope info
-  // (Interposable functions are not inlinable).
+  // (Interposable functions are not inlinable as are functions w/o
+  //  declarations).
   if (Call.getFunction()->getSubprogram() && Call.getCalledFunction() &&
   !Call.getCalledFunction()->isInterposable() &&
+  !Call.getCalledFunction()->isDeclaration() &&
   Call.getCalledFunction()->getSubprogram())
 CheckDI(Call.getDebugLoc(),
 "inlinable function call in a function with "
Index: clang/test/CodeGen/debug-info-extern-call.c
===
--- clang/test/CodeGen/debug-info-extern-call.c
+++ clang/test/CodeGen/debug-info-extern-call.c
@@ -29,8 +29,8 @@
 // DECLS-FOR-EXTERN: [[FN1_TYPES]] = !{[[X_TYPE:![0-9]+]],
 // DECLS-FOR-EXTERN: [[X_TYPE]] = !DIDerivedType(tag: DW_TAG_typedef, name: 
"x",
 // DECLS-FOR-EXTERN-SAME: baseType: [[INT_TYPE]])
-// DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "memcmp"
-// DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "__some_reserved_name"
+// DECLS-FOR-EXTERN: !DISubprogram(name: "memcmp"
+// DECLS-FOR-EXTERN: !DISubprogram(name: "__some_reserved_name"
 
 // NO-DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "fn1"
 // NO-DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "memcmp"
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -4228,17 +4228,11 @@
   if (Func->getSubprogram())
 return;
 
-  // Do not emit a declaration subprogram for a builtin, a function with 
nodebug
-  // attribute, or if call site info isn't required. Also, elide declarations
-  // for functions with reserved names, as call site-related features aren't
-  // interesting in this case (& also, the compiler may emit calls to these
-  // functions without debug locations, which makes the verifier complain).
-  if (CalleeDecl->getBuiltinID() != 0 || CalleeDecl->hasAttr() ||
+  // Do not emit a declaration subprogram for a function with nodebug
+  // attribute, or if call site info isn't required.
+  if (CalleeDecl->hasAttr() ||
   getCallSiteRelatedAttrs() == llvm::DINode::FlagZero)
 return;
-  if (CalleeDecl->isReserved(CGM.getLangOpts()) !=
-  ReservedIdentifierStatus::NotReserved)
-return;
 
   // If there is no DISubprogram attached to the function being called,
   // create the one describing the function in order to have complete


Index: llvm/test/Verifier/callsite-dbgloc.ll
===
--- llvm/test/Verifier/callsite-dbgloc.ll
+++ llvm/test/Verifier/callsite-dbgloc.ll
@@ -45,6 +45,9 @@
   call void @j()
 ; CHECK: inlinable function call in a function with debug info must have a !dbg location
   call void @k()
+; CHECK-NOT: inlinable function call in a function with debug info must have a !dbg location
+; CHECK-NOT: @i
+  call void (...) @i()
   ret void, !dbg !13
 }
 
Index: llvm/lib/IR/Verifier.cpp
===
--- llvm/lib/IR/Verifier.cpp
+++ llvm/lib/IR/Verifier.cpp
@@ -3466,9 +3466,11 @@
   // Verify that each inlinable callsite of a debug-info-bearing function in a
   // debug-info-bearing function has a debug location attached to it. Failure to
   // do so causes assertion failures when the inliner sets up inline scope info
-  // (Interposable functions are not inlinable).
+  // (Interposable functions are not inlinable as are functions w/o
+  //  declarations).
   if (Call.getFunction()->getSubprogram() && Call.getCalledFunction() &&
   

[PATCH] D136041: [clang][DebugInfo] Emit DISubprogram for extern functions with reserved names

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D136041#3863748 , @dblaikie wrote:

> Hmm - this does mean linking IR can produce invalid code, though, right (you 
> link in a definition of the function, so what was valid is now invalid - 
> because it now has a definition, can be inlined, etc)? Is that new? 
> concerning?

As far as I understand this 

 discussion the check in question is "best effort".
My change further narrows conditions when verifier would report an error, thus 
it should not add any new failures to the existing code.
But yes, hypothetically there might be a situation when old version of the 
check would have caught a miss-behaving transformation at an earlier stage 
(before linking IR rather then after).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136041

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

[PATCH] D136041: [clang][DebugInfo] Emit DISubprogram for extern functions with reserved names

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a comment.

In D136041#3862741 , @aprantl wrote:

> I think this is reasonable. Ideally the LLVM and Clang patches should be two 
> independent commits.
> Please wait a few days for others to chime in though. Do you have any idea 
> how much this will grow the debug info for something like, e.g., clang?

Thanks for the review,
I will split the patch and provide the measurements, most likely tomorrow.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136041

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

[PATCH] D136041: [clang][DebugInfo] Emit DISubprogram for extern functions with reserved names

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 added a reviewer: aprantl.
eddyz87 added a comment.

Hello Adrian, I've noticed that you was tagged as a reviewer in a somewhat 
related revision: https://reviews.llvm.org/D133060, so decided to tag you here.
Could you please take a look?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D136041

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

[PATCH] D136041: [clang][DebugInfo] Emit DISubprogram for extern functions with reserved names

[Eduard Zingerman via Phabricator via cfe-commits]

eddyz87 created this revision.
Herald added subscribers: hiraditya, kristof.beyls.
Herald added a project: All.
eddyz87 updated this revision to Diff 468102.
eddyz87 added a comment.
eddyz87 published this revision for review.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Rebase


Callsite `DISubprogram` entries are not generated for:

- builtin functions;
- external functions with reserved names (e.g. names starting from "__").

This limitation was added by the commit [1] as a workaround for the
situation described in [2] that triggered the IR verifier error.
The goal of the present commit is to lift this limitation by adjusting
the IR verifier logic.

The logic behind [1] is to avoid the following situation:

- a `DISubprogram` is added for some builtin function;
- there is some location where this builtin is also emitted by a transformation 
(w/o debug location);
- the `Verifier::visitCallBase` sees a call to a function with `DISubprogram` 
but w/o debug location and emits an error.

Here is an updated example of such situation taken from [2]:

  extern "C" int memcmp(void *, void *, long);
  
  struct a { int b; int c; int d; };
  
  struct e { int f[1000]; };
  
  bool foo(e g, e ) {
// DISubprogram for memcmp is created here when [1] is commented out
return memcmp(, , sizeof(e));
  }
  
  bool bar(a , a ) {
// memcmp might be generated here by MergeICmps
return g.b == h.b && g.c == h.c && g.d == h.d;
  }

This triggers the verifier error when:

- compiled for AArch64: `clang++ -c -g -Oz -target 
aarch64-unknown-linux-android21 test.cpp`;
- [1] check is commented out.

Instead of forbidding generation of `DISubprogram` entries as in [1]
one can instead adjust the verifier to additionally check if callee
has a body. Functions w/o bodies cannot be inlined and thus verifier
warning is not necessary.

E.g. `llvm::InlineFunction` requires functions for which
`GlobalValue::isDeclaration() == false`.

[1] 568db780bb7267651a902da8e85bc59fc89aea70 

[2] https://bugs.chromium.org/p/chromium/issues/detail?id=1022296


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D136041

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGen/debug-info-extern-call.c
  llvm/lib/IR/Verifier.cpp
  llvm/test/Verifier/callsite-dbgloc.ll


Index: llvm/test/Verifier/callsite-dbgloc.ll
===
--- llvm/test/Verifier/callsite-dbgloc.ll
+++ llvm/test/Verifier/callsite-dbgloc.ll
@@ -45,6 +45,9 @@
   call void @j()
 ; CHECK: inlinable function call in a function with debug info must have a 
!dbg location
   call void @k()
+; CHECK-NOT: inlinable function call in a function with debug info must have a 
!dbg location
+; CHECK-NOT: @i
+  call void (...) @i()
   ret void, !dbg !13
 }
 
Index: llvm/lib/IR/Verifier.cpp
===
--- llvm/lib/IR/Verifier.cpp
+++ llvm/lib/IR/Verifier.cpp
@@ -3466,9 +3466,11 @@
   // Verify that each inlinable callsite of a debug-info-bearing function in a
   // debug-info-bearing function has a debug location attached to it. Failure 
to
   // do so causes assertion failures when the inliner sets up inline scope info
-  // (Interposable functions are not inlinable).
+  // (Interposable functions are not inlinable as are functions w/o
+  //  declarations).
   if (Call.getFunction()->getSubprogram() && Call.getCalledFunction() &&
   !Call.getCalledFunction()->isInterposable() &&
+  !Call.getCalledFunction()->isDeclaration() &&
   Call.getCalledFunction()->getSubprogram())
 CheckDI(Call.getDebugLoc(),
 "inlinable function call in a function with "
Index: clang/test/CodeGen/debug-info-extern-call.c
===
--- clang/test/CodeGen/debug-info-extern-call.c
+++ clang/test/CodeGen/debug-info-extern-call.c
@@ -29,8 +29,8 @@
 // DECLS-FOR-EXTERN: [[FN1_TYPES]] = !{[[X_TYPE:![0-9]+]],
 // DECLS-FOR-EXTERN: [[X_TYPE]] = !DIDerivedType(tag: DW_TAG_typedef, name: 
"x",
 // DECLS-FOR-EXTERN-SAME: baseType: [[INT_TYPE]])
-// DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "memcmp"
-// DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "__some_reserved_name"
+// DECLS-FOR-EXTERN: !DISubprogram(name: "memcmp"
+// DECLS-FOR-EXTERN: !DISubprogram(name: "__some_reserved_name"
 
 // NO-DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "fn1"
 // NO-DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "memcmp"
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -4228,17 +4228,11 @@
   if (Func->getSubprogram())
 return;
 
-  // Do not emit a declaration subprogram for a builtin, a function with 
nodebug
-  // attribute, or if call site info isn't required. Also, elide