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

2022-10-28 Thread Yonghong Song via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG524c640090a8: [clang][DebugInfo] Emit DISubprogram for 
extern functions with reserved names (authored by eddyz87, committed by 
yonghong-song).

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
@@ -3468,9 +3468,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

2022-10-19 Thread 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

2022-10-19 Thread 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

2022-10-18 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay added inline comments.



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

Removing this check does not break any `check-llvm check-clang` test.


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

2022-10-18 Thread Fangrui Song via Phabricator via cfe-commits 
MaskRay added a comment.

Nice. This also fixes DW_AT_call_all_calls for functions which call 
builtins/reserved name functions.




Comment at: llvm/test/Verifier/callsite-dbgloc.ll:49
+; CHECK-NOT: inlinable function call in a function with debug info must have a 
!dbg location
+; CHECK-NOT: @i
+  call void (...) @i()

to not match other functions named `@i*`


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

2022-10-18 Thread 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

2022-10-18 Thread Paul Robinson via Phabricator via cfe-commits 
probinson added inline comments.



Comment at: llvm/lib/IR/Verifier.cpp:3470
+  // (Interposable functions are not inlinable as are functions w/o
+  //  declarations).
   if (Call.getFunction()->getSubprogram() && Call.getCalledFunction() &&

(Interposable functions are not inlinable, neither are functions without
definitions.)


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

2022-10-18 Thread 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

2022-10-18 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie accepted this revision.
dblaikie added a comment.

In D136041#3864171 , @yonghong-song 
wrote:

> 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?
>
> @dblaikie do you have a concrete example to illustrate the problem?

One translation unit with a call to a function declaration, that call has no 
!dbg attachment.
Another translation unit with the definition of that declared function.

As-is, this would not cause any verifier error or backend debug info assertion.
Link the two together and now it's a verifier error - and, depending on the 
contents of the function definition, maybe a backend debug info assertion.

In D136041#3864782 , @eddyz87 wrote:

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

Fair point

> My change further narrows conditions when verifier would report an error, 
> thus it should not add any new failures to the existing code.

No, the opposite.

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

Right - now things that were invalid categorically, are now valid, unless you 
link them. Which is a bit tricky.

Ah well, as you say - it's best effort.


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

2022-10-18 Thread 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

2022-10-18 Thread 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

2022-10-18 Thread Yonghong Song via Phabricator via cfe-commits 
yonghong-song 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?

@dblaikie do you have a concrete example to illustrate the problem?


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

2022-10-17 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

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?


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

2022-10-17 Thread 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

2022-10-17 Thread Adrian Prantl via Phabricator via cfe-commits 
aprantl accepted this revision as: aprantl.
aprantl added a comment.
This revision is now accepted and ready to land.

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?


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

2022-10-17 Thread Yonghong Song via Phabricator via cfe-commits 
yonghong-song added a comment.

@aprantl The following example shows the difference between gcc and clang 
w.r.t. reserved names.

  [$ ~/tmp3] cat t.c
  extern void *__bpf_kptr_new(int, int, void *)
  __attribute__((section(".ksyms")));
  #define bpf_kptr_new(x) __bpf_kptr_new(x, 0, 0)
  
  struct foo {
  int data;
  };
  
  int main(void)
  {
  struct foo *f;
  
  f = bpf_kptr_new(0);
  return f->data;
  }
  [$ ~/tmp3] clang -target bpf -O2 -g -c t.c
  [$ ~/tmp3] llvm-dwarfdump t.o | grep bpf_kptr_new
  [$ ~/tmp3] gcc -O2 -g -c t.c
  [$ ~/tmp3] llvm-dwarfdump t.o | grep bpf_kptr_new
DW_AT_abstract_origin (0x00a3 "__bpf_kptr_new")
  DW_AT_linkage_name  ("__bpf_kptr_new")
  DW_AT_name  ("__bpf_kptr_new")
  [$ ~/tmp3]

If not using reserved name, the function will appear in debuginfo/dwarf:

  [$ ~/tmp3] cat t1.c
  extern void *bpf_kptr_new_(int, int, void *)
  __attribute__((section(".ksyms")));
  #define bpf_kptr_new(x) bpf_kptr_new_(x, 0, 0)
  
  struct foo {
  int data;
  };
  
  int main(void)
  {
  struct foo *f;
  
  f = bpf_kptr_new(0);
  return f->data;
  }
  [$ ~/tmp3] clang -target bpf -O2 -g -c t1.c
  [$ ~/tmp3] llvm-dwarfdump t1.o | grep bpf_kptr_new
  DW_AT_name  ("bpf_kptr_new_")
  [$~/tmp3] gcc -O2 -g -c t1.c
  [$ ~/tmp3] llvm-dwarfdump t1.o | grep bpf_kptr_new
DW_AT_abstract_origin (0x00a3 "bpf_kptr_new_")
  DW_AT_linkage_name  ("bpf_kptr_new_")
  DW_AT_name  ("bpf_kptr_new_")
  [$ ~/tmp3]

In BPF, we need the above extern function in debuginfo. This happens when a  
bpf program is calling a function defined in kernel. For example,
in kernel we have function

  int tcp_drop(struct sk_buff *skb, ...) { ...
  }
  /* allow tcp_drop() function accessible to bpf programs */

In bpf program, we could have

  extern int tcp_drop(struct sk_buff *sbk, ...);
  BPF_PROG(...) {
  tcp_drop(...)

BPF verifier will need to check whether the signature of tcp_drop() in bpf 
program matched the one in kernel or not. In kernel, we DO have some global 
functions with '__' prefix in the function name. Although no reserved function 
name works, we want all legal names working so user won't be surprised why some 
kernel functions are not available to them.

  }


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

2022-10-16 Thread 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

2022-10-16 Thread 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