[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-06-01 Thread Djordje Todorovic via Phabricator via cfe-commits 
djtodoro added a comment.

Thanks for the reviews!


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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-06-01 Thread Djordje Todorovic via Phabricator via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rG40a3fcb05c83: [DebugInfo][CallSites] Remove decl subprograms 
from retainedTypes: (authored by djtodoro).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D80369

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGen/debug-info-extern-call.c
  clang/test/Modules/DebugInfoTransitiveImport.m
  clang/test/Modules/ModuleDebugInfo.cpp
  clang/test/Modules/ModuleDebugInfo.m

Index: clang/test/Modules/ModuleDebugInfo.m
===
--- clang/test/Modules/ModuleDebugInfo.m
+++ clang/test/Modules/ModuleDebugInfo.m
@@ -36,19 +36,13 @@
 // CHECK-NOT:  name:
 // CHECK-SAME: elements:
 
-// CHECK: !DISubprogram(name: "+[ObjCClass classMethod]",
-// CHECK-SAME:  scope: ![[MODULE]],
+// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "FwdDecl",
+// CHECK-SAME: scope: ![[MODULE]],
 
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "ObjCClass",
 // CHECK-SAME: scope: ![[MODULE]],
 // CHECK-SAME: elements
 
-// The forward declaration should not be in the module scope.
-// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "OpaqueData", file
-
-// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "FwdDecl",
-// CHECK-SAME: scope: ![[MODULE]],
-
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "ObjCClassWithPrivateIVars",
 // CHECK-SAME: scope: ![[MODULE]],
 // CHECK-SAME: elements
@@ -85,11 +79,8 @@
 // The output order is sublty different for module vs. pch,
 // so these are checked separately:
 //
-// CHECK2: !DISubprogram(name: "+[ObjCClass classMethod]"
-// CHECK2: !DISubprogram(name: "-[ObjCClass instanceMethodWithInt:]"
+// CHECK2: !DICompositeType(tag: DW_TAG_structure_type, name: "FwdDecl",
 // CHECK2: !DICompositeType(tag: DW_TAG_structure_type, name: "ObjCClass",
 // CHECK2: !DIObjCProperty(name: "property",
 // CHECK2: !DIDerivedType(tag: DW_TAG_member, name: "ivar"
-// CHECK2: !DISubprogram(name: "-[Category(Category) categoryMethod]"
-// CHECK2: !DICompositeType(tag: DW_TAG_structure_type, name: "FwdDecl",
 // CHECK2: !DIDerivedType(tag: DW_TAG_typedef, name: "InnerEnum"
Index: clang/test/Modules/ModuleDebugInfo.cpp
===
--- clang/test/Modules/ModuleDebugInfo.cpp
+++ clang/test/Modules/ModuleDebugInfo.cpp
@@ -51,15 +51,6 @@
 // CHECK-SAME: )
 // CHECK: !DIEnumerator(name: "e5", value: 5, isUnsigned: true)
 
-// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
-// no mangled name here yet.
-
-// This type is anchored by a function parameter.
-// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A"
-// CHECK-SAME: elements:
-// CHECK-SAME: templateParams:
-// CHECK-SAME: identifier: "_ZTSN8DebugCXX1AIJvEEE")
-
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Struct"
 // CHECK-SAME: identifier: "_ZTSN8DebugCXX6StructE")
 
@@ -85,6 +76,12 @@
 // CHECK-SAME: templateParams:
 // CHECK-SAME: identifier: "_ZTSN8DebugCXX8TemplateIlNS_6traitsIl")
 
+// This type is anchored by a function parameter.
+// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A"
+// CHECK-SAME: elements:
+// CHECK-SAME: templateParams:
+// CHECK-SAME: identifier: "_ZTSN8DebugCXX1AIJvEEE")
+
 // CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "FloatInstantiation"
 // no mangled name here yet.
 
@@ -93,6 +90,9 @@
 // CHECK-SAME: flags: DIFlagFwdDecl
 // CHECK-SAME: identifier: "_ZTSN8DebugCXX8TemplateIfNS_6traitsIf")
 
+// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
+// no mangled name here yet.
+
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Virtual"
 // CHECK-SAME: elements:
 // CHECK-SAME: identifier: "_ZTS7Virtual")
Index: clang/test/Modules/DebugInfoTransitiveImport.m
===
--- clang/test/Modules/DebugInfoTransitiveImport.m
+++ clang/test/Modules/DebugInfoTransitiveImport.m
@@ -12,10 +12,10 @@
 
 // Definition of left:
 // CHECK: !DICompileUnit({{.*}}dwoId:
-// CHECK: ![[LEFT:[0-9]+]] = !DIFile({{.*}}diamond_left.h
 // CHECK: !DIImportedEntity(tag: DW_TAG_imported_declaration,
-// CHECK-SAME:  entity: ![[MODULE:.*]], file: ![[LEFT]], line: 3)
+// CHECK-SAME:  entity: ![[MODULE:.*]], file: ![[LEFT:.*]], line: 3)
 // CHECK: ![[MODULE]] = !DIModule(scope: null, name: "diamond_top"
+// CHECK: ![[LEFT]] = !DIFile({{.*}}diamond_left.h
 
 // Skeleton for top:
 // CHECK: !DICompileUnit({{.*}}splitDebugFilename: {{.*}}diamond_top{{.*}}dwoId:
Index:

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-29 Thread Adrian Prantl via Phabricator via cfe-commits 
aprantl added a comment.

@vsk: this is going to be relevant if we ever decide to produce DWARF for the 
extra attributes currently read from the .apinotes (swift names, availability, 
nullabilkity, ...)


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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-29 Thread Adrian Prantl via Phabricator via cfe-commits 
aprantl added inline comments.



Comment at: clang/test/Modules/ModuleDebugInfo.m:46-47
 
-// The forward declaration should not be in the module scope.
-// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "OpaqueData", file
-

dblaikie wrote:
> djtodoro wrote:
> > dblaikie wrote:
> > > Did this type go missing with this change? (ie: was this type /only/ 
> > > accessible via a subprogram in the retained types list?)
> > Yes, it was **only** accessible via that subprogram we removed.
> @aprantl - you might want to check whether you really wanted this type & if 
> so, how it should be handled. To the best of my knowledge/understanding, this 
> type never would've made it through to the DWARF output even prior to this 
> change to remove DISubprograms from retainedType - so this doesn't represent 
> (again, to the best of my understanding) a regression, but exposes a 
> potential problem for your use case, I guess?
Correct. I will cycle back to this, when I find a use-case that depends on this 
and potentially add the ability to thread this through to DWARF in that case. 
Let's not hold up this patch because of that though!


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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-29 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added inline comments.



Comment at: clang/test/Modules/ModuleDebugInfo.m:46-47
 
-// The forward declaration should not be in the module scope.
-// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "OpaqueData", file
-

djtodoro wrote:
> dblaikie wrote:
> > Did this type go missing with this change? (ie: was this type /only/ 
> > accessible via a subprogram in the retained types list?)
> Yes, it was **only** accessible via that subprogram we removed.
@aprantl - you might want to check whether you really wanted this type & if so, 
how it should be handled. To the best of my knowledge/understanding, this type 
never would've made it through to the DWARF output even prior to this change to 
remove DISubprograms from retainedType - so this doesn't represent (again, to 
the best of my understanding) a regression, but exposes a potential problem for 
your use case, I guess?


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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-29 Thread Djordje Todorovic via Phabricator via cfe-commits 
djtodoro marked an inline comment as done.
djtodoro added inline comments.



Comment at: clang/test/Modules/ModuleDebugInfo.m:46-47
 
-// The forward declaration should not be in the module scope.
-// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "OpaqueData", file
-

dblaikie wrote:
> Did this type go missing with this change? (ie: was this type /only/ 
> accessible via a subprogram in the retained types list?)
Yes, it was **only** accessible via that subprogram we removed.


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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-28 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added inline comments.



Comment at: clang/test/Modules/ModuleDebugInfo.m:46-47
 
-// The forward declaration should not be in the module scope.
-// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "OpaqueData", file
-

Did this type go missing with this change? (ie: was this type /only/ accessible 
via a subprogram in the retained types list?)


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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-28 Thread Djordje Todorovic via Phabricator via cfe-commits 
djtodoro updated this revision to Diff 266776.
djtodoro added a comment.

-Tests clean up


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

https://reviews.llvm.org/D80369

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGen/debug-info-extern-call.c
  clang/test/Modules/DebugInfoTransitiveImport.m
  clang/test/Modules/ModuleDebugInfo.cpp
  clang/test/Modules/ModuleDebugInfo.m

Index: clang/test/Modules/ModuleDebugInfo.m
===
--- clang/test/Modules/ModuleDebugInfo.m
+++ clang/test/Modules/ModuleDebugInfo.m
@@ -36,19 +36,13 @@
 // CHECK-NOT:  name:
 // CHECK-SAME: elements:
 
-// CHECK: !DISubprogram(name: "+[ObjCClass classMethod]",
-// CHECK-SAME:  scope: ![[MODULE]],
+// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "FwdDecl",
+// CHECK-SAME: scope: ![[MODULE]],
 
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "ObjCClass",
 // CHECK-SAME: scope: ![[MODULE]],
 // CHECK-SAME: elements
 
-// The forward declaration should not be in the module scope.
-// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "OpaqueData", file
-
-// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "FwdDecl",
-// CHECK-SAME: scope: ![[MODULE]],
-
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "ObjCClassWithPrivateIVars",
 // CHECK-SAME: scope: ![[MODULE]],
 // CHECK-SAME: elements
@@ -85,11 +79,8 @@
 // The output order is sublty different for module vs. pch,
 // so these are checked separately:
 //
-// CHECK2: !DISubprogram(name: "+[ObjCClass classMethod]"
-// CHECK2: !DISubprogram(name: "-[ObjCClass instanceMethodWithInt:]"
+// CHECK2: !DICompositeType(tag: DW_TAG_structure_type, name: "FwdDecl",
 // CHECK2: !DICompositeType(tag: DW_TAG_structure_type, name: "ObjCClass",
 // CHECK2: !DIObjCProperty(name: "property",
 // CHECK2: !DIDerivedType(tag: DW_TAG_member, name: "ivar"
-// CHECK2: !DISubprogram(name: "-[Category(Category) categoryMethod]"
-// CHECK2: !DICompositeType(tag: DW_TAG_structure_type, name: "FwdDecl",
 // CHECK2: !DIDerivedType(tag: DW_TAG_typedef, name: "InnerEnum"
Index: clang/test/Modules/ModuleDebugInfo.cpp
===
--- clang/test/Modules/ModuleDebugInfo.cpp
+++ clang/test/Modules/ModuleDebugInfo.cpp
@@ -51,15 +51,6 @@
 // CHECK-SAME: )
 // CHECK: !DIEnumerator(name: "e5", value: 5, isUnsigned: true)
 
-// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
-// no mangled name here yet.
-
-// This type is anchored by a function parameter.
-// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A"
-// CHECK-SAME: elements:
-// CHECK-SAME: templateParams:
-// CHECK-SAME: identifier: "_ZTSN8DebugCXX1AIJvEEE")
-
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Struct"
 // CHECK-SAME: identifier: "_ZTSN8DebugCXX6StructE")
 
@@ -85,6 +76,12 @@
 // CHECK-SAME: templateParams:
 // CHECK-SAME: identifier: "_ZTSN8DebugCXX8TemplateIlNS_6traitsIl")
 
+// This type is anchored by a function parameter.
+// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A"
+// CHECK-SAME: elements:
+// CHECK-SAME: templateParams:
+// CHECK-SAME: identifier: "_ZTSN8DebugCXX1AIJvEEE")
+
 // CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "FloatInstantiation"
 // no mangled name here yet.
 
@@ -93,6 +90,9 @@
 // CHECK-SAME: flags: DIFlagFwdDecl
 // CHECK-SAME: identifier: "_ZTSN8DebugCXX8TemplateIfNS_6traitsIf")
 
+// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
+// no mangled name here yet.
+
 // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Virtual"
 // CHECK-SAME: elements:
 // CHECK-SAME: identifier: "_ZTS7Virtual")
Index: clang/test/Modules/DebugInfoTransitiveImport.m
===
--- clang/test/Modules/DebugInfoTransitiveImport.m
+++ clang/test/Modules/DebugInfoTransitiveImport.m
@@ -12,10 +12,10 @@
 
 // Definition of left:
 // CHECK: !DICompileUnit({{.*}}dwoId:
-// CHECK: ![[LEFT:[0-9]+]] = !DIFile({{.*}}diamond_left.h
 // CHECK: !DIImportedEntity(tag: DW_TAG_imported_declaration,
-// CHECK-SAME:  entity: ![[MODULE:.*]], file: ![[LEFT]], line: 3)
+// CHECK-SAME:  entity: ![[MODULE:.*]], file: ![[LEFT:.*]], line: 3)
 // CHECK: ![[MODULE]] = !DIModule(scope: null, name: "diamond_top"
+// CHECK: ![[LEFT]] = !DIFile({{.*}}diamond_left.h
 
 // Skeleton for top:
 // CHECK: !DICompileUnit({{.*}}splitDebugFilename: {{.*}}diamond_top{{.*}}dwoId:
Index: clang/test/CodeGen/debug-info-extern-call.c
===
--- clang/test/CodeGen/debug-info-extern-call.c
+++

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-28 Thread Djordje Todorovic via Phabricator via cfe-commits 
djtodoro added a comment.

In D80369#2057866 , @dblaikie wrote:

> Not sure I follow - why was it a problem that there was no DISubprogram at 
> all?


Actually, since the DISubprograms from the retained types don't affect the 
final DWARF, it's not a problem at all. :)


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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-27 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

In D80369#2057874 , @aprantl wrote:

> > The declaration subprograms are in the retained types list - but when 
> > retained types are emitted, only types in the retained types list are used, 
> > the subprograms (and the types they only indirectly reference) are ignored.
>
> So adding the subprogram nodes to retained types has no effect?


So far as I can tell. The only use (other than serializing and deserializing) 
of retained types seems to be here: 
https://github.com/llvm-mirror/llvm/blob/master/lib/CodeGen/AsmPrinter/DwarfDebug.cpp#L882
 which filter only the types in the retained types list. I'd say, even, that 
the retained types list should probably be only types (verifier check?) 
unless/until we have a use case otherwise. (& if anyone wants to merge the 
enums list with the retained types list, that'd be a nice cleanup too).

> That's good to know. I think we can remove the code then, as long as the 
> existing tests aren't affected.




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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-27 Thread Adrian Prantl via Phabricator via cfe-commits 
aprantl added a comment.

> The declaration subprograms are in the retained types list - but when 
> retained types are emitted, only types in the retained types list are used, 
> the subprograms (and the types they only indirectly reference) are ignored.

So adding the subprogram nodes to retained types has no effect? That's good to 
know. I think we can remove the code then, as long as the existing tests aren't 
affected.


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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-27 Thread Adrian Prantl via Phabricator via cfe-commits 
aprantl added a comment.

One way to make the tests more robust is capture the output in a `%t.ll` file 
and do repeated `RUN: cat %t.ll | FileCheck --check-prefix=TYPE1` lines. 
Anyway, oif the output is just reshuffled, this should be all fine.


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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-27 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

In D80369#2057077 , @djtodoro wrote:

> >> @dblaikie wrote:
> > 
> > ... At least for the C++ test, this change makes it pass:
> > 
> >   diff --git clang/test/Modules/ModuleDebugInfo.cpp 
> > clang/test/Modules/ModuleDebugInfo.cpp
> >   index 26369c89605..b1ffe27ec22 100644
> >   --- clang/test/Modules/ModuleDebugInfo.cpp
> >   +++ clang/test/Modules/ModuleDebugInfo.cpp
> >   @@ -51,15 +51,6 @@
> >// CHECK-SAME: )
> >// CHECK: !DIEnumerator(name: "e5", value: 5, isUnsigned: true)
> >
> >   -// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
> >   -// no mangled name here yet.
> >   -
> >   -// This type is anchored by a function parameter.
> >   -// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A"
> >   -// CHECK-SAME: elements:
> >   -// CHECK-SAME: templateParams:
> >   -// CHECK-SAME: identifier: "_ZTSN8DebugCXX1AIJvEEE")
> >   -
> >// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Struct"
> >// CHECK-SAME: identifier: "_ZTSN8DebugCXX6StructE")
> >
> >   @@ -85,6 +76,12 @@
> >// CHECK-SAME: templateParams:
> >// CHECK-SAME: identifier: 
> > "_ZTSN8DebugCXX8TemplateIlNS_6traitsIl")
> >
> >   +// This type is anchored by a function parameter.
> >   +// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A"
> >   +// CHECK-SAME: elements:
> >   +// CHECK-SAME: templateParams:
> >   +// CHECK-SAME: identifier: "_ZTSN8DebugCXX1AIJvEEE")
> >   +
> >// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "FloatInstantiation"
> >// no mangled name here yet.
> >
> >   @@ -93,6 +90,9 @@
> >// CHECK-SAME: flags: DIFlagFwdDecl
> >// CHECK-SAME: identifier: 
> > "_ZTSN8DebugCXX8TemplateIfNS_6traitsIf")
> >
> >   +// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
> >   +// no mangled name here yet.
> >   +
> >// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Virtual"
> >// CHECK-SAME: elements:
> >// CHECK-SAME: identifier: "_ZTS7Virtual")
>
> I've also faced this scenario, but the problem was with the 
> `Modules/ModuleDebugInfo.m` (Objective-C) test, since after applying previous 
> version of the patch [0] (plus refacotred Modules/DebugInfoTransitiveImport.m 
> & Modules/ModuleDebugInfo.cpp) there wasn't any `DISubprogram` at all, and 
> that was the reason I made this version of the patch.


Not sure I follow - why was it a problem that there was no DISubprogram at all?


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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-27 Thread Djordje Todorovic via Phabricator via cfe-commits 
djtodoro added a comment.

>> @dblaikie wrote:
> 
> ... At least for the C++ test, this change makes it pass:
> 
>   diff --git clang/test/Modules/ModuleDebugInfo.cpp 
> clang/test/Modules/ModuleDebugInfo.cpp
>   index 26369c89605..b1ffe27ec22 100644
>   --- clang/test/Modules/ModuleDebugInfo.cpp
>   +++ clang/test/Modules/ModuleDebugInfo.cpp
>   @@ -51,15 +51,6 @@
>// CHECK-SAME: )
>// CHECK: !DIEnumerator(name: "e5", value: 5, isUnsigned: true)
>
>   -// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
>   -// no mangled name here yet.
>   -
>   -// This type is anchored by a function parameter.
>   -// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A"
>   -// CHECK-SAME: elements:
>   -// CHECK-SAME: templateParams:
>   -// CHECK-SAME: identifier: "_ZTSN8DebugCXX1AIJvEEE")
>   -
>// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Struct"
>// CHECK-SAME: identifier: "_ZTSN8DebugCXX6StructE")
>
>   @@ -85,6 +76,12 @@
>// CHECK-SAME: templateParams:
>// CHECK-SAME: identifier: 
> "_ZTSN8DebugCXX8TemplateIlNS_6traitsIl")
>
>   +// This type is anchored by a function parameter.
>   +// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A"
>   +// CHECK-SAME: elements:
>   +// CHECK-SAME: templateParams:
>   +// CHECK-SAME: identifier: "_ZTSN8DebugCXX1AIJvEEE")
>   +
>// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "FloatInstantiation"
>// no mangled name here yet.
>
>   @@ -93,6 +90,9 @@
>// CHECK-SAME: flags: DIFlagFwdDecl
>// CHECK-SAME: identifier: 
> "_ZTSN8DebugCXX8TemplateIfNS_6traitsIf")
>
>   +// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
>   +// no mangled name here yet.
>   +
>// CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Virtual"
>// CHECK-SAME: elements:
>// CHECK-SAME: identifier: "_ZTS7Virtual")

I've also faced this scenario, but the problem was with the 
`Modules/ModuleDebugInfo.m` (Objective-C) test, since after applying previous 
version of the patch [0] (plus refacotred Modules/DebugInfoTransitiveImport.m & 
Modules/ModuleDebugInfo.cpp) there wasn't any `DISubprogram` at all, and that 
was the reason I made this version of the patch.

[0]: F12010783: debuginfo-remove-decls-from-ret-types.patch 



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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-26 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

In D80369#2056094 , @aprantl wrote:

> > How does this data get used for Swift code and ObjC interoperability? At 
> > the moment I see no use of this IR metadata in LLVM. Does ObjC/Swift 
> > interop use the DI IR over in the Swift compiler? Got a link to the code 
> > there?
>
> Are you saying the retained types are not emitted in DWARF?


Sort of. I'm saying /these/ types (the ones that these tests are testing for) 
aren't retained types. The declaration subprograms are in the retained types 
list - but when retained types are emitted, only types in the retained types 
list are used, the subprograms (and the types they only indirectly reference) 
are ignored.

Ah, nope, I'm wrong - I /thought/ the test was correctly flagging these types 
as missing - that the only reason the types were there was because their 
subprogram was in the retained types list.

That's incorrect - the types are here, they're just reordered because. At least 
for the C++ test, this change makes it pass:

  diff --git clang/test/Modules/ModuleDebugInfo.cpp 
clang/test/Modules/ModuleDebugInfo.cpp
  index 26369c89605..b1ffe27ec22 100644
  --- clang/test/Modules/ModuleDebugInfo.cpp
  +++ clang/test/Modules/ModuleDebugInfo.cpp
  @@ -51,15 +51,6 @@
   // CHECK-SAME: )
   // CHECK: !DIEnumerator(name: "e5", value: 5, isUnsigned: true)
   
  -// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
  -// no mangled name here yet.
  -
  -// This type is anchored by a function parameter.
  -// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A"
  -// CHECK-SAME: elements:
  -// CHECK-SAME: templateParams:
  -// CHECK-SAME: identifier: "_ZTSN8DebugCXX1AIJvEEE")
  -
   // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Struct"
   // CHECK-SAME: identifier: "_ZTSN8DebugCXX6StructE")
   
  @@ -85,6 +76,12 @@
   // CHECK-SAME: templateParams:
   // CHECK-SAME: identifier: 
"_ZTSN8DebugCXX8TemplateIlNS_6traitsIl")
   
  +// This type is anchored by a function parameter.
  +// CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A"
  +// CHECK-SAME: elements:
  +// CHECK-SAME: templateParams:
  +// CHECK-SAME: identifier: "_ZTSN8DebugCXX1AIJvEEE")
  +
   // CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "FloatInstantiation"
   // no mangled name here yet.
   
  @@ -93,6 +90,9 @@
   // CHECK-SAME: flags: DIFlagFwdDecl
   // CHECK-SAME: identifier: 
"_ZTSN8DebugCXX8TemplateIfNS_6traitsIf")
   
  +// CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
  +// no mangled name here yet.
  +
   // CHECK: !DICompositeType(tag: DW_TAG_structure_type, name: "Virtual"
   // CHECK-SAME: elements:
   // CHECK-SAME: identifier: "_ZTS7Virtual")




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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-26 Thread Adrian Prantl via Phabricator via cfe-commits 
aprantl added a comment.

There's a cute diagram of the interaction: 
https://github.com/apple/llvm-project/blob/1fda14a45e23c41ac661c20a248a7fa4b102230d/lldb/source/Symbol/SwiftASTContext.cpp#L3020


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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-26 Thread Adrian Prantl via Phabricator via cfe-commits 
aprantl added a comment.

> How does this data get used for Swift code and ObjC interoperability? At the 
> moment I see no use of this IR metadata in LLVM. Does ObjC/Swift interop use 
> the DI IR over in the Swift compiler? Got a link to the code there?

Are you saying the retained types are not emitted in DWARF?
[Assuming the types do show up in the DWARF sections of the the fat .pcm files 
—] Swift has a component called ClangImporter that reads Clang ASTs and 
generates Swift ASTs to provide Swift <-> (Objective-)C interoperability. In 
the debugger, we don't want to have to depend on the availability of C headers 
(and the associated brittleness of include paths) just to realize mixed 
Swift-Objective-C types. Since LLDB already knows how to create Clang ASTs from 
DWARF, we read the types from DWARF build a Clang AST hand that over to the 
ClangImporter in LLDB's embedded Swift compiler and thus realize Clang-imported 
Swift types.


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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-22 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

In D80369#2051681 , @aprantl wrote:

> In D80369#2051278 , @dblaikie wrote:
>
> > @aprantl can you check here? I've attached two IR files for the 
> > ModuleDebugInfo.m test, before/after ('x' is before), stripped of the 
> > metadata numbers to make diffing easier - but I think you can still follow 
> > what's connected to what with reasonable guesswork. The point is these two 
> > function declarations end up in the retainedTypes list, and since the 
> > function declarations are emitted, so are the types used in their 
> > parameters, etc - but those types aren't reachable from anywhere else in 
> > the debug info metadata, so they won't be emitted into the final object 
> > file so far as I can see (because nothing looks at the function 
> > declarations in retainedTypes - only types).
> >
> > F11980002: x.ll 
> >
> > F11980001: y.ll 
>
>
> These two files are for the C++ test, not the Objective-C one. Is the 
> Objective-C case similar?


Haven't looked.

> If there is no explicit CHECK line for the types anchored by the function 
> calls, removing them is fine. However, for ObjC there is explicit code in 
> `clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp` 
> `DebugTypeVisitor::VisitFunctionDecl()` to emit the Objective-C methods. I do 
> want to keep this functionality; it's needed for debugging Swift code with 
> ObjC interoperability.

How does this data get used for Swift code and ObjC interoperability? At the 
moment I see no use of this IR metadata in LLVM. Does ObjC/Swift interop use 
the DI IR over in the Swift compiler? Got a link to the code there?

> I would be fine with manually adding everything created by this particular 
> visitor to the retained types if that makes the separation easier.




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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-22 Thread Adrian Prantl via Phabricator via cfe-commits 
aprantl added a comment.

In D80369#2051278 , @dblaikie wrote:

> @aprantl can you check here? I've attached two IR files for the 
> ModuleDebugInfo.m test, before/after ('x' is before), stripped of the 
> metadata numbers to make diffing easier - but I think you can still follow 
> what's connected to what with reasonable guesswork. The point is these two 
> function declarations end up in the retainedTypes list, and since the 
> function declarations are emitted, so are the types used in their parameters, 
> etc - but those types aren't reachable from anywhere else in the debug info 
> metadata, so they won't be emitted into the final object file so far as I can 
> see (because nothing looks at the function declarations in retainedTypes - 
> only types).
>
> F11980002: x.ll 
>
> F11980001: y.ll 


These two files are for the C++ test, not the Objective-C one. Is the 
Objective-C case similar?
If there is no explicit CHECK line for the types anchored by the function 
calls, removing them is fine. However, for ObjC there is explicit code in 
`clang/lib/CodeGen/ObjectFilePCHContainerOperations.cpp` 
`DebugTypeVisitor::VisitFunctionDecl()` to emit the Objective-C methods. I do 
want to keep this functionality; it's needed for debugging Swift code with ObjC 
interoperability. I would be fine with manually adding everything created by 
this particular visitor to the retained types if that makes the separation 
easier.


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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-22 Thread David Blaikie via Phabricator via cfe-commits 
dblaikie added a comment.

In D80369#2050962 , @djtodoro wrote:

> In D80369#2050022 , @dblaikie wrote:
>
> > In D80369#2048932 , @djtodoro 
> > wrote:
> >
> > > Still have test failing:
> > >
> > >   Clang :: Modules/DebugInfoTransitiveImport.m
> > >   Clang :: Modules/ModuleDebugInfo.cpp
> > >   Clang :: Modules/ModuleDebugInfo.m
> > >   
> > >
> > > I haven't looked into the failures, but I guess something Objective-C++ 
> > > related should be handled with some additional work here.
> >
> >
> > These look like over-constrained/accidental tests. At least the 
> > ModuleDEbugInfo.m test - the type being tested for won't be emitted, 
> > because the type itself isn't in the retained types list nor is it 
> > reachable from any other metadata that is going to be emitted.
> >
> > I think it'd probably be good to clean these tests up - and leave it to 
> > @aprantl to look into if this has exposed other bugs - but the bugs are 
> > already there, and this IR/debug info metadata isn't working around the 
> > bugs, since the IR is going unused anyway.
>
>
> Looks like ObjC expects the declarations within retained types  (based on 
> some tests failures), so I left it as is (I think @aprantl can help us with 
> that).
>  Therefore, this patch will remove the declarations only in the case of call 
> site debug info for now.


Yeah, that's what I was trying to say - I think the tests expect that, but I 
don't see any code path that uses that debug info metadata - I think the test 
is overconstrained/accidentally testing for this behavior.

I think the right thing to do is to change these tests to test the new behavior 
(the behavior of the first version of this patch) - and if some use 
arises/someone can demonstrate a codepath that's using this data, it can be 
added back in.

@aprantl can you check here? I've attached two IR files for the 
ModuleDebugInfo.m test, before/after ('x' is before), stripped of the metadata 
numbers to make diffing easier - but I think you can still follow what's 
connected to what with reasonable guesswork. The point is these two function 
declarations end up in the retainedTypes list, and since the function 
declarations are emitted, so are the types used in their parameters, etc - but 
those types aren't reachable from anywhere else in the debug info metadata, so 
they won't be emitted into the final object file so far as I can see (because 
nothing looks at the function declarations in retainedTypes - only types).

F11980002: x.ll 

F11980001: y.ll 


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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-22 Thread Djordje Todorovic via Phabricator via cfe-commits 
djtodoro added a comment.

In D80369#2050022 , @dblaikie wrote:

> In D80369#2048932 , @djtodoro wrote:
>
> > Still have test failing:
> >
> >   Clang :: Modules/DebugInfoTransitiveImport.m
> >   Clang :: Modules/ModuleDebugInfo.cpp
> >   Clang :: Modules/ModuleDebugInfo.m
> >   
> >
> > I haven't looked into the failures, but I guess something Objective-C++ 
> > related should be handled with some additional work here.
>
>
> These look like over-constrained/accidental tests. At least the 
> ModuleDEbugInfo.m test - the type being tested for won't be emitted, because 
> the type itself isn't in the retained types list nor is it reachable from any 
> other metadata that is going to be emitted.
>
> I think it'd probably be good to clean these tests up - and leave it to 
> @aprantl to look into if this has exposed other bugs - but the bugs are 
> already there, and this IR/debug info metadata isn't working around the bugs, 
> since the IR is going unused anyway.


Looks like ObjC expects the declarations within retained types  (based on some 
tests failures), so I left it as is (I think @aprantl can help us with that).
Therefore, this patch will remove the declarations only in the case of call 
site debug info for now.


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

https://reviews.llvm.org/D80369



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

[PATCH] D80369: [DebugInfo][CallSites] Remove decl subprograms from 'retainedTypes:'

2020-05-22 Thread Djordje Todorovic via Phabricator via cfe-commits 
djtodoro updated this revision to Diff 265722.
djtodoro retitled this revision from "WIP: [DebugInfo] Remove decl subprograms 
from 'retainedTypes:'" to "[DebugInfo][CallSites] Remove decl subprograms from 
'retainedTypes:'".
djtodoro added a comment.

-Remove the decls only in the case of call-site debug info


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

https://reviews.llvm.org/D80369

Files:
  clang/lib/CodeGen/CGDebugInfo.cpp
  clang/test/CodeGen/debug-info-extern-call.c


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
@@ -1,6 +1,10 @@
 // When entry values are emitted, expect a subprogram for extern decls so that
 // the dwarf generator can describe call site parameters at extern call sites.
 //
+// Initial implementation relied on the 'retainedTypes:' from the corresponding
+// DICompileUnit, so we also ensure that we do not store the extern declaration
+// subprogram into the 'retainedTypes:'.
+//
 // RUN: %clang -g -O2 -target x86_64-none-linux-gnu -S -emit-llvm %s -o - \
 // RUN:   | FileCheck %s -check-prefix=DECLS-FOR-EXTERN
 
@@ -17,6 +21,8 @@
 // RUN: %clang -g -O2 -target x86_64-none-linux-gnu -gsce -S -emit-llvm %s -o 
- \
 // RUN:   | FileCheck %s -check-prefix=NO-DECLS-FOR-EXTERN
 
+// DECLS-FOR-EXTERN-NOT: !DICompileUnit({{.*}}retainedTypes: 
![[RETTYPES:[0-9]+]]
+// DECLS-FOR-EXTERN-NOT: ![[RETTYPES]] = !{
 // DECLS-FOR-EXTERN: !DISubprogram(name: "fn1"
 // DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "memcmp"
 // DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "__some_reserved_name"
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -3868,10 +3868,14 @@
   getOrCreateFunctionType(D, FnType, Unit), ScopeLine, Flags, SPFlags,
   TParamsArray.get(), getFunctionDeclaration(D));
 
-  if (IsDeclForCallSite)
-Fn->setSubprogram(SP);
+  // ObjC expects declarations within retained types.
+  if (!IsDeclForCallSite) {
+DBuilder.retainType(SP);
+return;
+  }
 
-  DBuilder.retainType(SP);
+  Fn->setSubprogram(SP);
+  DBuilder.finalizeSubprogram(SP);
 }
 
 void CGDebugInfo::EmitFuncDeclForCallSite(llvm::CallBase *CallOrInvoke,


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
@@ -1,6 +1,10 @@
 // When entry values are emitted, expect a subprogram for extern decls so that
 // the dwarf generator can describe call site parameters at extern call sites.
 //
+// Initial implementation relied on the 'retainedTypes:' from the corresponding
+// DICompileUnit, so we also ensure that we do not store the extern declaration
+// subprogram into the 'retainedTypes:'.
+//
 // RUN: %clang -g -O2 -target x86_64-none-linux-gnu -S -emit-llvm %s -o - \
 // RUN:   | FileCheck %s -check-prefix=DECLS-FOR-EXTERN
 
@@ -17,6 +21,8 @@
 // RUN: %clang -g -O2 -target x86_64-none-linux-gnu -gsce -S -emit-llvm %s -o - \
 // RUN:   | FileCheck %s -check-prefix=NO-DECLS-FOR-EXTERN
 
+// DECLS-FOR-EXTERN-NOT: !DICompileUnit({{.*}}retainedTypes: ![[RETTYPES:[0-9]+]]
+// DECLS-FOR-EXTERN-NOT: ![[RETTYPES]] = !{
 // DECLS-FOR-EXTERN: !DISubprogram(name: "fn1"
 // DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "memcmp"
 // DECLS-FOR-EXTERN-NOT: !DISubprogram(name: "__some_reserved_name"
Index: clang/lib/CodeGen/CGDebugInfo.cpp
===
--- clang/lib/CodeGen/CGDebugInfo.cpp
+++ clang/lib/CodeGen/CGDebugInfo.cpp
@@ -3868,10 +3868,14 @@
   getOrCreateFunctionType(D, FnType, Unit), ScopeLine, Flags, SPFlags,
   TParamsArray.get(), getFunctionDeclaration(D));
 
-  if (IsDeclForCallSite)
-Fn->setSubprogram(SP);
+  // ObjC expects declarations within retained types.
+  if (!IsDeclForCallSite) {
+DBuilder.retainType(SP);
+return;
+  }
 
-  DBuilder.retainType(SP);
+  Fn->setSubprogram(SP);
+  DBuilder.finalizeSubprogram(SP);
 }
 
 void CGDebugInfo::EmitFuncDeclForCallSite(llvm::CallBase *CallOrInvoke,
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits