[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Wenlei He via Phabricator via cfe-commits]

wenlei added a subscriber: lxfind.
wenlei added a comment.

In D93747#2475852 , @dblaikie wrote:

> In D93747#2470504 , @hoy wrote:
>
>> In D93747#2470387 , @dblaikie wrote:
>>
>>> Please remove the clang test change - if this is an LLVM change with LLVM 
>>> test coverage, that's enough. (we don't generally test every LLVM change 
>>> from both LLVM and Clang)
>>
>> Sounds good.
>>
>>> I'd still be curious if you could look around to see whether there are 
>>> other cases of function splitting/cloning/etc to see how they deal with 
>>> updating the DISubprograms, to see if there's some prior art that should be 
>>> used here.
>>
>> To the best of my knowledge, existing code does not change the linkage name 
>> field of a DISubprogram once created. You can create a new DISubprogram 
>> record with any linkage name you want but bear in mind how the debugger will 
>> consume the record. Looks like we are the first one to change existing 
>> record which will confuse the debugger.
>
> Sure enough - do any other transforms have similar requirements (of creating 
> a record for something that looks like the same function but isn't quite) but 
> take a different approach? Be good to know if other approaches were 
> chosen/how/why they are applicable there but not here, etc. (conversely 
> perhaps other passes worked around the issue in less than ideal ways and 
> could benefit from using this new approach).
>
> Looks like some places that could use this functionality aren't quite there 
> yet - 
> The Attributor has an internalizing feature (added in 
> 87a85f3d57f55f5652ec44f77816c7c9457545fa 
>  ) that 
> produces invalid IR (ends up with two !dbg attachments of the same 
> DISubprogram) if the function it's internalizing has a DISubprogram - but if 
> it succeeded it'd have the same problem that the linkage name on the 
> DISubprogram wouldn't match the actual symbol/function name. (@jdoerfert 
> @bbn).
> The IR Outliner (@AndrewLitteken ) seems to be only a few months old and 
> appears to have no debug info handling - probably results in the same problem.
> The Partial Inliner does clone a function into a new name & so would have an 
> invalid DISubprogram, though it only uses the clone for inlining (this 
> probably then goes on to produce the desired debug info, where the inlining 
> appears to come from the original function)
> ThinLTO does some function cloning within a module for aliases, but it then 
> renames the clone to the aliasees name so I think the name works out to match 
> again.

Another place where mismatch between linkage name and DISubprogram happens is 
the cloning for coroutines (.resume, .destroy and cleanup funclets). We found 
that out again through different kind of AutoFDO profile fidelity issue (cc: 
@lxfind).

> If this is an invariant, that the linkage name on the DISubprogram should 
> match the actual llvm::Function name (@aprantl @JDevlieghere - verifier 
> check, perhaps?) - it'd be nice to make that more reliable, either by 
> removing the name and relying on the llvm::Function name (perhaps with a 
> boolean on the DISubprogram as to whether the linkage name should be emitted 
> or not - I think currently Clang's IRGen makes choices about which 
> DISubprograms will get linkage names) or a verifier and utilities to keep 
> them in sync.

Agreed, clarity on the rules and sanity check built into verifier would be 
beneficial.

> But I'll leave that alone for now/for this review, unless someone else wants 
> to chime in on it.
>
> In D93747#2470178 , @tmsriram wrote:
>
>> In D93747#2469556 , @hoy wrote:
>>
 In D93656 , @dblaikie wrote:
 Though the C case is interesting - it means you'll end up with C functions 
 with the same DWARF 'name' but different linkage name. I don't know what 
 that'll do to DWARF consumers - I guess they'll probably be OK-ish with 
 it, as much as they are with C++ overloading. I think there are some cases 
 of C name mangling so it's probably supported/OK-ish with DWARF Consumers. 
 Wouldn't hurt for you to take a look/see what happens in that case with a 
 debugger like gdb/check other cases of C name mangling to see what DWARF 
 they end up creating (with both GCC and Clang).
>>>
>>> I did a quick experiment with C name managing with GCC and -flto. It turns 
>>> out the `DW_AT_linkage_name` field of `DW_TAG_subprogram` is never set for 
>>> C programs. If set, the gdb debugger will use that field to match the user 
>>> input and set breakpoints. Therefore, giving `DW_AT_linkage_name` a 
>>> uniquefied name prevents the debugger from setting a breakpoint based on 
>>> source names unless the user specifies a 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Hongtao Yu via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG32bcfcda4e28: Rename debug linkage name with 
-funique-internal-linkage-names (authored by hoy).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

Files:
  llvm/include/llvm/IR/DebugInfoMetadata.h
  llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
  
llvm/test/Transforms/UniqueInternalLinkageNames/unique-internal-linkage-names.ll
  llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll

Index: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
===
--- llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
+++ /dev/null
@@ -1,24 +0,0 @@
-; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O0 --check-prefix=UNIQUE
-; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
-; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
-; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
-; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
-
-define internal i32 @foo() {
-entry:
-  ret i32 0
-}
-
-define dso_local i32 (...)* @bar() {
-entry:
-  ret i32 (...)* bitcast (i32 ()* @foo to i32 (...)*)
-}
-
-; O0: Running pass: UniqueInternalLinkageNamesPass
-
-;; Check UniqueInternalLinkageNamesPass is scheduled before SampleProfileProbePass.
-; O2: Running pass: UniqueInternalLinkageNamesPass
-; O2: Running pass: SampleProfileProbePass
-
-; UNIQUE: define internal i32 @foo.__uniq.{{[0-9a-f]+}}()
-; UNIQUE: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
Index: llvm/test/Transforms/UniqueInternalLinkageNames/unique-internal-linkage-names.ll
===
--- /dev/null
+++ llvm/test/Transforms/UniqueInternalLinkageNames/unique-internal-linkage-names.ll
@@ -0,0 +1,50 @@
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O0 --check-prefix=UNIQUE
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes=unique-internal-linkage-names < %s -o - | FileCheck %s --check-prefix=DBG
+
+define internal i32 @foo() !dbg !15 {
+entry:
+  ret i32 0
+}
+
+define dso_local i32 (...)* @bar() {
+entry:
+  ret i32 (...)* bitcast (i32 ()* @foo to i32 (...)*)
+}
+
+define internal i32 @go() !dbg !19 {
+entry:
+  ret i32 0
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, enums: !2)
+!1 = !DIFile(filename: "test.c", directory: "")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!15 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: !1, file: !1, line: 5, type: !16, scopeLine: 5, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !0, declaration: !18, retainedNodes: !2)
+!16 = !DISubroutineType(types: !17)
+!17 = !{!13}
+!18 = !DISubprogram(name: "foo", linkageName: "foo", scope: !1, isDefinition: false, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)
+!19 = distinct !DISubprogram(name: "go", scope: !1, file: !1, line: 5, type: !16, scopeLine: 5, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !0, declaration: !18, retainedNodes: !2)
+
+; O0: Running pass: UniqueInternalLinkageNamesPass
+
+;; Check 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Hongtao Yu via Phabricator via cfe-commits]

hoy updated this revision to Diff 315908.
hoy added a comment.

Rebasing.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

Files:
  llvm/include/llvm/IR/DebugInfoMetadata.h
  llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
  
llvm/test/Transforms/UniqueInternalLinkageNames/unique-internal-linkage-names.ll
  llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll

Index: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
===
--- llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
+++ /dev/null
@@ -1,24 +0,0 @@
-; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O0 --check-prefix=UNIQUE
-; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
-; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
-; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
-; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
-
-define internal i32 @foo() {
-entry:
-  ret i32 0
-}
-
-define dso_local i32 (...)* @bar() {
-entry:
-  ret i32 (...)* bitcast (i32 ()* @foo to i32 (...)*)
-}
-
-; O0: Running pass: UniqueInternalLinkageNamesPass
-
-;; Check UniqueInternalLinkageNamesPass is scheduled before SampleProfileProbePass.
-; O2: Running pass: UniqueInternalLinkageNamesPass
-; O2: Running pass: SampleProfileProbePass
-
-; UNIQUE: define internal i32 @foo.__uniq.{{[0-9a-f]+}}()
-; UNIQUE: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
Index: llvm/test/Transforms/UniqueInternalLinkageNames/unique-internal-linkage-names.ll
===
--- /dev/null
+++ llvm/test/Transforms/UniqueInternalLinkageNames/unique-internal-linkage-names.ll
@@ -0,0 +1,50 @@
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O0 --check-prefix=UNIQUE
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes=unique-internal-linkage-names < %s -o - | FileCheck %s --check-prefix=DBG
+
+define internal i32 @foo() !dbg !15 {
+entry:
+  ret i32 0
+}
+
+define dso_local i32 (...)* @bar() {
+entry:
+  ret i32 (...)* bitcast (i32 ()* @foo to i32 (...)*)
+}
+
+define internal i32 @go() !dbg !19 {
+entry:
+  ret i32 0
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, enums: !2)
+!1 = !DIFile(filename: "test.c", directory: "")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!15 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: !1, file: !1, line: 5, type: !16, scopeLine: 5, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !0, declaration: !18, retainedNodes: !2)
+!16 = !DISubroutineType(types: !17)
+!17 = !{!13}
+!18 = !DISubprogram(name: "foo", linkageName: "foo", scope: !1, isDefinition: false, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)
+!19 = distinct !DISubprogram(name: "go", scope: !1, file: !1, line: 5, type: !16, scopeLine: 5, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !0, declaration: !18, retainedNodes: !2)
+
+; O0: Running pass: UniqueInternalLinkageNamesPass
+
+;; Check UniqueInternalLinkageNamesPass is scheduled before SampleProfileProbePass.
+; O2: Running pass: UniqueInternalLinkageNamesPass
+; O2: Running 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Hongtao Yu via Phabricator via cfe-commits]

hoy updated this revision to Diff 315572.
hoy added a comment.

Removing the UniqueDebugLinakgeNames switch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

Files:
  llvm/include/llvm/IR/DebugInfoMetadata.h
  llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
  
llvm/test/Transforms/UniqueInternalLinkageNames/unique-internal-linkage-names.ll
  llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll

Index: llvm/test/Transforms/UniqueInternalLinkageNames/unique-internal-linkage-names.ll
===
--- llvm/test/Transforms/UniqueInternalLinkageNames/unique-internal-linkage-names.ll
+++ llvm/test/Transforms/UniqueInternalLinkageNames/unique-internal-linkage-names.ll
@@ -3,8 +3,9 @@
 ; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
 ; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
 ; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes=unique-internal-linkage-names < %s -o - | FileCheck %s --check-prefix=DBG
 
-define internal i32 @foo() {
+define internal i32 @foo() !dbg !15 {
 entry:
   ret i32 0
 }
@@ -14,6 +15,27 @@
   ret i32 (...)* bitcast (i32 ()* @foo to i32 (...)*)
 }
 
+define internal i32 @go() !dbg !19 {
+entry:
+  ret i32 0
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, enums: !2)
+!1 = !DIFile(filename: "test.c", directory: "")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!15 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: !1, file: !1, line: 5, type: !16, scopeLine: 5, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !0, declaration: !18, retainedNodes: !2)
+!16 = !DISubroutineType(types: !17)
+!17 = !{!13}
+!18 = !DISubprogram(name: "foo", linkageName: "foo", scope: !1, isDefinition: false, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)
+!19 = distinct !DISubprogram(name: "go", scope: !1, file: !1, line: 5, type: !16, scopeLine: 5, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !0, declaration: !18, retainedNodes: !2)
+
 ; O0: Running pass: UniqueInternalLinkageNamesPass
 
 ;; Check UniqueInternalLinkageNamesPass is scheduled before SampleProfileProbePass.
@@ -22,3 +44,7 @@
 
 ; UNIQUE: define internal i32 @foo.__uniq.{{[0-9a-f]+}}()
 ; UNIQUE: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
+
+; DBG: distinct !DISubprogram(name: "foo", linkageName: "foo.__uniq.{{[0-9a-f]+}}", scope: ![[#]]
+; DBG: !DISubprogram(name: "foo", linkageName: "foo.__uniq.{{[0-9a-f]+}}", scope: ![[#]]
+; DBG: distinct !DISubprogram(name: "go", scope: ![[#]]
Index: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
===
--- llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
+++ llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
@@ -13,8 +13,11 @@
 
 #include "llvm/Transforms/Utils/UniqueInternalLinkageNames.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/IR/DebugInfoMetadata.h"
+#include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Module.h"
 #include "llvm/InitializePasses.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 
@@ -31,11 +34,23 @@
   // this symbol is of internal linkage type.
   std::string ModuleNameHash = (Twine(".__uniq.") + Twine(Str)).str();
   bool Changed = false;
+  MDBuilder MDB(M.getContext());
 
   // Append the module hash to all internal linkage functions.
   for (auto  : M) {
 if (F.hasInternalLinkage()) {
   F.setName(F.getName() + ModuleNameHash);
+  // Replace linkage names in the debug metadata.
+  if (DISubprogram *SP = F.getSubprogram()) {
+if (SP->getRawLinkageName()) {
+  auto *Name = MDB.createString(F.getName());
+  SP->replaceRawLinkageName(Name);
+  if (DISubprogram *SPDecl = SP->getDeclaration()) {
+if (SPDecl->getRawLinkageName())
+  SPDecl->replaceRawLinkageName(Name);
+  }
+}
+  }
   Changed = true;
 }
   }
Index: llvm/include/llvm/IR/DebugInfoMetadata.h
===
--- llvm/include/llvm/IR/DebugInfoMetadata.h
+++ llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -2052,6 +2052,10 @@
 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D93747#2488399 , @dblaikie wrote:

> In D93747#2488390 , @tmsriram wrote:
>
>> In D93747#2488387 , @dblaikie wrote:
>>
>>> Seems alright to me - I think we've hashed out the deeper issues (missing 
>>> opportunity for C functions which could/should be addressed by moving the 
>>> implementation to the frontend, where those C functions can be mangled and 
>>> then use linkageName to give them the same AutoFDO opportunities as C++ 
>>> functions) here and elsewhere - but for what it is, the patch makes sense. 
>>> I'd probably say drop the flag - " check if rawLinkageName is set and only 
>>> set it when it is not null. " was implemented and seems that addressed the 
>>> debug info issue without an awkward tradeoff between AutoFDO fidelity and 
>>> debugging fidelity, so there doesn't seem to be a need to be able to 
>>> configure this.
>>
>> Here is a suggestion for a plan forward. Let's get these patches along with 
>> D94154  in.  No correctness issues but a 
>> missed opportunity.  I will work with @rnk and @dblaikie and send out a 
>> patch where I move the uniqueification to clang?  That patch will also do 
>> linkage name for C functions with mangled name when uniqueification is 
>> needed. Does that sound reasonable?  As for timeline, I can do this in two 
>> weeks.
>
> Sure sure - not urgent, just so long as it doesn't get lost.

Sounds good to me, thanks for all the in-depth discussions!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D93747#2488390 , @tmsriram wrote:

> In D93747#2488387 , @dblaikie wrote:
>
>> Seems alright to me - I think we've hashed out the deeper issues (missing 
>> opportunity for C functions which could/should be addressed by moving the 
>> implementation to the frontend, where those C functions can be mangled and 
>> then use linkageName to give them the same AutoFDO opportunities as C++ 
>> functions) here and elsewhere - but for what it is, the patch makes sense. 
>> I'd probably say drop the flag - " check if rawLinkageName is set and only 
>> set it when it is not null. " was implemented and seems that addressed the 
>> debug info issue without an awkward tradeoff between AutoFDO fidelity and 
>> debugging fidelity, so there doesn't seem to be a need to be able to 
>> configure this.
>
> Here is a suggestion for a plan forward. Let's get these patches along with 
> D94154  in.  No correctness issues but a 
> missed opportunity.  I will work with @rnk and @dblaikie and send out a patch 
> where I move the uniqueification to clang?  That patch will also do linkage 
> name for C functions with mangled name when uniqueification is needed. Does 
> that sound reasonable?  As for timeline, I can do this in two weeks.

Sure sure - not urgent, just so long as it doesn't get lost.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D93747#2488387 , @dblaikie wrote:

> Seems alright to me - I think we've hashed out the deeper issues (missing 
> opportunity for C functions which could/should be addressed by moving the 
> implementation to the frontend, where those C functions can be mangled and 
> then use linkageName to give them the same AutoFDO opportunities as C++ 
> functions) here and elsewhere - but for what it is, the patch makes sense. 
> I'd probably say drop the flag - " check if rawLinkageName is set and only 
> set it when it is not null. " was implemented and seems that addressed the 
> debug info issue without an awkward tradeoff between AutoFDO fidelity and 
> debugging fidelity, so there doesn't seem to be a need to be able to 
> configure this.

Here is a suggestion for a plan forward. Let's get these patches along with 
D94154  in.  No correctness issues but a 
missed opportunity.  I will work with @rnk and @dblaikie and send out a patch 
where I move the uniqueification to clang?  That patch will also do linkage 
name for C functions with mangled name when uniqueification is needed. Does 
that sound reasonable?  As for timeline, I can do this in two weeks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[David Blaikie via Phabricator via cfe-commits]

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

Seems alright to me - I think we've hashed out the deeper issues (missing 
opportunity for C functions which could/should be addressed by moving the 
implementation to the frontend, where those C functions can be mangled and then 
use linkageName to give them the same AutoFDO opportunities as C++ functions) 
here and elsewhere - but for what it is, the patch makes sense. I'd probably 
say drop the flag - " check if rawLinkageName is set and only set it when it is 
not null. " was implemented and seems that addressed the debug info issue 
without an awkward tradeoff between AutoFDO fidelity and debugging fidelity, so 
there doesn't seem to be a need to be able to configure this.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D93747#2482730 , @tmsriram wrote:

> In D93747#2482726 , @hoy wrote:
>
>> In D93747#2482519 , @tmsriram wrote:
>>
>>> In D93747#2481494 , @dblaikie 
>>> wrote:
>>>
 In D93747#2481383 , @hoy wrote:

> In D93747#2481304 , @tmsriram 
> wrote:
>
>> In D93747#2481223 , @tmsriram 
>> wrote:
>>
>>> In D93747#2481163 , @dblaikie 
>>> wrote:
>>>
 In D93747#2481095 , @hoy 
 wrote:

> In D93747#2481073 , 
> @dblaikie wrote:
>
>> Suggesting that gdb isn't using the DW_AT_name at all for "break 
>> " but instead demangling and stripping off the extras 
>> from the linkage name, and since it can't demangle this uniquified 
>> name (unlike the mangled name used when using the overloadable 
>> attribute) that degrades the debugger user experience? I'd have to 
>> test that in more detail/with some hand-hacked DWARF.
>
> Yes, I think in your case the linage name can be demangled by the 
> debugger. In my previous experiment, the uniquefied names could not 
> be demangled therefore I was not able to breakpoint.

 Ah, did some more testing. Seems it's because it isn't a classically 
 mangled name.
>>
>> The simplest fix I can think of is to emit the base 10 number of the 
>> md5hash.  That would preserve all the existing uniqueness and be 
>> demangler friendly.  @hoy @dblaikie  WDYT?
>
> I think using the base 10 form of md5hash is a smart workaround. Thanks 
> for the suggestion.

 Sure - wouldn't mind having some wording from the Itanium ABI/mangling 
 rules that explain/corroborate what we've seen from testing to ensure we 
 are using it correctly and there aren't any other ways that might be more 
 suitable, or lurking gotchas we haven't tested.
>>>
>>> Yes, makes sense. Also, like @dblaikie  pointed out in D94154 
>>>  it is now important that we don't 
>>> generate the DW_AT_linkage_name for C style names using this suffix as it 
>>> will not demangle.  I think this makes it important that we only update 
>>> this field if it already exists.
>>
>> Yes, it makes sense to do the renaming only if the linkage name preexists to 
>> not break the debuggability. Unfortunately we won't be able to support C 
>> with this patch.
>
> There is nothing needed to support C right?  overloadable attribute will 
> mangle with _Z so it will work as expected?  What did I miss?

I mean from the AutoFDO point of view, C programs with static functions are not 
getting the unique debug name support.

 It might be possible for this uniquifying step to check if the name is 
 mangled (does it start with "_Z") and if it isn't mangled, it could 
 mangle it (check the length of the name, then "_Zv") and 
 add the unique suffix. That looks to me like it preserves the original 
 debugging behavior?

 (has the problem that we don't actually know the mangling scheme at 
 this point - we do know it up in clang (where it might be Itanium 
 mangling or Microsoft mangling), might point again to the possibility 
 this feature is more suitable to implement in the frontend rather than 
 a middle end pass. And also the 'v' in the mangling is 'void return 
 type', which is a lie - not sure if we could do something better there)
>
> Doing name unification in the frontend sounds like the ultimate solution 
> and since the frontend has all the knowledge about the name mangler. I 
> think for now we can go with the solution @tmsriram suggested, i.e., 
> using the base 10 form of md5 hash.

 Any general idea of how long "for now" would be? It doesn't seem like 
 putting this off is going to make things especially better & seems like 
 more bug fixes/changes/etc are being built around the solution as it is at 
 the moment. So I'd be hesitant to put off this kind of restructuring too 
 long.
>>
>> I'm not sure. It looks like implementation in the front end has more 
>> complexity as discussed in the other thread D94154 
>> .
>>
 I think it's pretty important that we don't break tradeoff 
 debuggability for profile accuracy. It'll make for a difficult 
 tradeoff for our/any users.
>>>
>>> Agreed, I didn't expect this.
>>>
 (side note: using the original 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D93747#2482726 , @hoy wrote:

> In D93747#2482519 , @tmsriram wrote:
>
>> In D93747#2481494 , @dblaikie wrote:
>>
>>> In D93747#2481383 , @hoy wrote:
>>>
 In D93747#2481304 , @tmsriram 
 wrote:

> In D93747#2481223 , @tmsriram 
> wrote:
>
>> In D93747#2481163 , @dblaikie 
>> wrote:
>>
>>> In D93747#2481095 , @hoy wrote:
>>>
 In D93747#2481073 , @dblaikie 
 wrote:

> Suggesting that gdb isn't using the DW_AT_name at all for "break 
> " but instead demangling and stripping off the extras 
> from the linkage name, and since it can't demangle this uniquified 
> name (unlike the mangled name used when using the overloadable 
> attribute) that degrades the debugger user experience? I'd have to 
> test that in more detail/with some hand-hacked DWARF.

 Yes, I think in your case the linage name can be demangled by the 
 debugger. In my previous experiment, the uniquefied names could not be 
 demangled therefore I was not able to breakpoint.
>>>
>>> Ah, did some more testing. Seems it's because it isn't a classically 
>>> mangled name.
>
> The simplest fix I can think of is to emit the base 10 number of the 
> md5hash.  That would preserve all the existing uniqueness and be 
> demangler friendly.  @hoy @dblaikie  WDYT?

 I think using the base 10 form of md5hash is a smart workaround. Thanks 
 for the suggestion.
>>>
>>> Sure - wouldn't mind having some wording from the Itanium ABI/mangling 
>>> rules that explain/corroborate what we've seen from testing to ensure we 
>>> are using it correctly and there aren't any other ways that might be more 
>>> suitable, or lurking gotchas we haven't tested.
>>
>> Yes, makes sense. Also, like @dblaikie  pointed out in D94154 
>>  it is now important that we don't generate 
>> the DW_AT_linkage_name for C style names using this suffix as it will not 
>> demangle.  I think this makes it important that we only update this field if 
>> it already exists.
>
> Yes, it makes sense to do the renaming only if the linkage name preexists to 
> not break the debuggability. Unfortunately we won't be able to support C with 
> this patch.

There is nothing needed to support C right?  overloadable attribute will mangle 
with _Z so it will work as expected?  What did I miss?

>>> It might be possible for this uniquifying step to check if the name is 
>>> mangled (does it start with "_Z") and if it isn't mangled, it could 
>>> mangle it (check the length of the name, then "_Zv") and 
>>> add the unique suffix. That looks to me like it preserves the original 
>>> debugging behavior?
>>>
>>> (has the problem that we don't actually know the mangling scheme at 
>>> this point - we do know it up in clang (where it might be Itanium 
>>> mangling or Microsoft mangling), might point again to the possibility 
>>> this feature is more suitable to implement in the frontend rather than 
>>> a middle end pass. And also the 'v' in the mangling is 'void return 
>>> type', which is a lie - not sure if we could do something better there)

 Doing name unification in the frontend sounds like the ultimate solution 
 and since the frontend has all the knowledge about the name mangler. I 
 think for now we can go with the solution @tmsriram suggested, i.e., using 
 the base 10 form of md5 hash.
>>>
>>> Any general idea of how long "for now" would be? It doesn't seem like 
>>> putting this off is going to make things especially better & seems like 
>>> more bug fixes/changes/etc are being built around the solution as it is at 
>>> the moment. So I'd be hesitant to put off this kind of restructuring too 
>>> long.
>
> I'm not sure. It looks like implementation in the front end has more 
> complexity as discussed in the other thread D94154 
> .
>
>>> I think it's pretty important that we don't break tradeoff 
>>> debuggability for profile accuracy. It'll make for a difficult tradeoff 
>>> for our/any users.
>>
>> Agreed, I didn't expect this.
>>
>>> (side note: using the original source file name seems problematic - I 
>>> know in google at least, the same source file is sometimes built into 
>>> more than one library/form with different preprocessor defines, so this 
>>> may not produce as unique a name as you are expecting?)
>>
>> It was a best effort and I 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Hongtao Yu via Phabricator via cfe-commits]

hoy updated this revision to Diff 314955.
hoy added a comment.

Renaming debug linkage name if it preexisits.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

Files:
  llvm/include/llvm/IR/DebugInfoMetadata.h
  llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
  llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll

Index: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
===
--- llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
+++ llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
@@ -3,8 +3,10 @@
 ; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
 ; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
 ; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes=unique-internal-linkage-names -unqiue-debug-linkage-names=0 < %s -o - | FileCheck %s --check-prefix=NODBG
+; RUN: opt -S -passes=unique-internal-linkage-names < %s -o - | FileCheck %s --check-prefix=DBG
 
-define internal i32 @foo() {
+define internal i32 @foo() !dbg !15 {
 entry:
   ret i32 0
 }
@@ -14,6 +16,27 @@
   ret i32 (...)* bitcast (i32 ()* @foo to i32 (...)*)
 }
 
+define internal i32 @go() !dbg !19 {
+entry:
+  ret i32 0
+}
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, enums: !2)
+!1 = !DIFile(filename: "test.c", directory: "")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!15 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: !1, file: !1, line: 5, type: !16, scopeLine: 5, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !0, declaration: !18, retainedNodes: !2)
+!16 = !DISubroutineType(types: !17)
+!17 = !{!13}
+!18 = !DISubprogram(name: "foo", linkageName: "foo", scope: !1, isDefinition: false, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)
+!19 = distinct !DISubprogram(name: "go", scope: !1, file: !1, line: 5, type: !16, scopeLine: 5, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !0, declaration: !18, retainedNodes: !2)
+
 ; O0: Running pass: UniqueInternalLinkageNamesPass
 
 ;; Check UniqueInternalLinkageNamesPass is scheduled before SampleProfileProbePass.
@@ -22,3 +45,11 @@
 
 ; UNIQUE: define internal i32 @foo.__uniq.{{[0-9a-f]+}}()
 ; UNIQUE: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
+
+; NODBG: distinct !DISubprogram(name: "foo", linkageName: "foo", scope: ![[#]]
+; NODBG: !DISubprogram(name: "foo", linkageName: "foo", scope: ![[#]]
+; NODBG: distinct !DISubprogram(name: "go", scope: ![[#]]
+
+; DBG: distinct !DISubprogram(name: "foo", linkageName: "foo.__uniq.{{[0-9a-f]+}}", scope: ![[#]]
+; DBG: !DISubprogram(name: "foo", linkageName: "foo.__uniq.{{[0-9a-f]+}}", scope: ![[#]]
+; DBG: distinct !DISubprogram(name: "go", scope: ![[#]]
Index: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
===
--- llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
+++ llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
@@ -13,13 +13,21 @@
 
 #include "llvm/Transforms/Utils/UniqueInternalLinkageNames.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/IR/DebugInfoMetadata.h"
+#include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Module.h"
 #include "llvm/InitializePasses.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 
 using namespace llvm;
 
+static cl::opt
+UniqueDebugLinakgeNames("unqiue-debug-linkage-names", cl::init(true),
+cl::Hidden,
+cl::desc("Uniqueify debug linkage Names"));
+
 static bool uniqueifyInternalLinkageNames(Module ) {
   llvm::MD5 Md5;
   Md5.update(M.getSourceFileName());
@@ -31,11 +39,25 @@
   // this symbol is of internal linkage type.
   std::string ModuleNameHash = (Twine(".__uniq.") + Twine(Str)).str();
   bool Changed = false;
+  MDBuilder MDB(M.getContext());
 
   // Append the module hash to all internal linkage functions.
   for (auto  : M) {
 if (F.hasInternalLinkage()) {
   F.setName(F.getName() + ModuleNameHash);
+  // Replace linkage names in the debug metadata.
+  if (UniqueDebugLinakgeNames) {
+if (DISubprogram *SP = 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D93747#2482519 , @tmsriram wrote:

> In D93747#2481494 , @dblaikie wrote:
>
>> In D93747#2481383 , @hoy wrote:
>>
>>> In D93747#2481304 , @tmsriram 
>>> wrote:
>>>
 In D93747#2481223 , @tmsriram 
 wrote:

> In D93747#2481163 , @dblaikie 
> wrote:
>
>> In D93747#2481095 , @hoy wrote:
>>
>>> In D93747#2481073 , @dblaikie 
>>> wrote:
>>>
 Suggesting that gdb isn't using the DW_AT_name at all for "break 
 " but instead demangling and stripping off the extras 
 from the linkage name, and since it can't demangle this uniquified 
 name (unlike the mangled name used when using the overloadable 
 attribute) that degrades the debugger user experience? I'd have to 
 test that in more detail/with some hand-hacked DWARF.
>>>
>>> Yes, I think in your case the linage name can be demangled by the 
>>> debugger. In my previous experiment, the uniquefied names could not be 
>>> demangled therefore I was not able to breakpoint.
>>
>> Ah, did some more testing. Seems it's because it isn't a classically 
>> mangled name.

 The simplest fix I can think of is to emit the base 10 number of the 
 md5hash.  That would preserve all the existing uniqueness and be demangler 
 friendly.  @hoy @dblaikie  WDYT?
>>>
>>> I think using the base 10 form of md5hash is a smart workaround. Thanks for 
>>> the suggestion.
>>
>> Sure - wouldn't mind having some wording from the Itanium ABI/mangling rules 
>> that explain/corroborate what we've seen from testing to ensure we are using 
>> it correctly and there aren't any other ways that might be more suitable, or 
>> lurking gotchas we haven't tested.
>
> Yes, makes sense. Also, like @dblaikie  pointed out in D94154 
>  it is now important that we don't generate 
> the DW_AT_linkage_name for C style names using this suffix as it will not 
> demangle.  I think this makes it important that we only update this field if 
> it already exists.

Yes, it makes sense to do the renaming only if the linkage name preexists to 
not break the debuggability. Unfortunately we won't be able to support C with 
this patch.

>> It might be possible for this uniquifying step to check if the name is 
>> mangled (does it start with "_Z") and if it isn't mangled, it could 
>> mangle it (check the length of the name, then "_Zv") and 
>> add the unique suffix. That looks to me like it preserves the original 
>> debugging behavior?
>>
>> (has the problem that we don't actually know the mangling scheme at this 
>> point - we do know it up in clang (where it might be Itanium mangling or 
>> Microsoft mangling), might point again to the possibility this feature 
>> is more suitable to implement in the frontend rather than a middle end 
>> pass. And also the 'v' in the mangling is 'void return type', which is a 
>> lie - not sure if we could do something better there)
>>>
>>> Doing name unification in the frontend sounds like the ultimate solution 
>>> and since the frontend has all the knowledge about the name mangler. I 
>>> think for now we can go with the solution @tmsriram suggested, i.e., using 
>>> the base 10 form of md5 hash.
>>
>> Any general idea of how long "for now" would be? It doesn't seem like 
>> putting this off is going to make things especially better & seems like more 
>> bug fixes/changes/etc are being built around the solution as it is at the 
>> moment. So I'd be hesitant to put off this kind of restructuring too long.

I'm not sure. It looks like implementation in the front end has more complexity 
as discussed in the other thread D94154 .

>> I think it's pretty important that we don't break tradeoff debuggability 
>> for profile accuracy. It'll make for a difficult tradeoff for our/any 
>> users.
>
> Agreed, I didn't expect this.
>
>> (side note: using the original source file name seems problematic - I 
>> know in google at least, the same source file is sometimes built into 
>> more than one library/form with different preprocessor defines, so this 
>> may not produce as unique a name as you are expecting?)
>
> It was a best effort and I think the hashing can be improved by using 
> more signals other than just the module name if needed.  For hard data 
> though, this does significantly improve performance which clearly comes 
> from better profile attribution so it does something.
>>
>> I'm OK with the idea that this helped the situation - but it 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D93747#2481494 , @dblaikie wrote:

> In D93747#2481383 , @hoy wrote:
>
>> In D93747#2481304 , @tmsriram wrote:
>>
>>> In D93747#2481223 , @tmsriram 
>>> wrote:
>>>
 In D93747#2481163 , @dblaikie 
 wrote:

> In D93747#2481095 , @hoy wrote:
>
>> In D93747#2481073 , @dblaikie 
>> wrote:
>>
>>> Suggesting that gdb isn't using the DW_AT_name at all for "break 
>>> " but instead demangling and stripping off the extras 
>>> from the linkage name, and since it can't demangle this uniquified name 
>>> (unlike the mangled name used when using the overloadable attribute) 
>>> that degrades the debugger user experience? I'd have to test that in 
>>> more detail/with some hand-hacked DWARF.
>>
>> Yes, I think in your case the linage name can be demangled by the 
>> debugger. In my previous experiment, the uniquefied names could not be 
>> demangled therefore I was not able to breakpoint.
>
> Ah, did some more testing. Seems it's because it isn't a classically 
> mangled name.
>>>
>>> The simplest fix I can think of is to emit the base 10 number of the 
>>> md5hash.  That would preserve all the existing uniqueness and be demangler 
>>> friendly.  @hoy @dblaikie  WDYT?
>>
>> I think using the base 10 form of md5hash is a smart workaround. Thanks for 
>> the suggestion.
>
> Sure - wouldn't mind having some wording from the Itanium ABI/mangling rules 
> that explain/corroborate what we've seen from testing to ensure we are using 
> it correctly and there aren't any other ways that might be more suitable, or 
> lurking gotchas we haven't tested.

Yes, makes sense. Also, like @dblaikie  pointed out in D94154 
 it is now important that we don't generate 
the DW_AT_linkage_name for C style names using this suffix as it will not 
demangle.  I think this makes it important that we only update this field if it 
already exists.

> It might be possible for this uniquifying step to check if the name is 
> mangled (does it start with "_Z") and if it isn't mangled, it could 
> mangle it (check the length of the name, then "_Zv") and 
> add the unique suffix. That looks to me like it preserves the original 
> debugging behavior?
>
> (has the problem that we don't actually know the mangling scheme at this 
> point - we do know it up in clang (where it might be Itanium mangling or 
> Microsoft mangling), might point again to the possibility this feature is 
> more suitable to implement in the frontend rather than a middle end pass. 
> And also the 'v' in the mangling is 'void return type', which is a lie - 
> not sure if we could do something better there)
>>
>> Doing name unification in the frontend sounds like the ultimate solution and 
>> since the frontend has all the knowledge about the name mangler. I think for 
>> now we can go with the solution @tmsriram suggested, i.e., using the base 10 
>> form of md5 hash.
>
> Any general idea of how long "for now" would be? It doesn't seem like putting 
> this off is going to make things especially better & seems like more bug 
> fixes/changes/etc are being built around the solution as it is at the moment. 
> So I'd be hesitant to put off this kind of restructuring too long.
>
> I think it's pretty important that we don't break tradeoff debuggability 
> for profile accuracy. It'll make for a difficult tradeoff for our/any 
> users.

 Agreed, I didn't expect this.

> (side note: using the original source file name seems problematic - I 
> know in google at least, the same source file is sometimes built into 
> more than one library/form with different preprocessor defines, so this 
> may not produce as unique a name as you are expecting?)

 It was a best effort and I think the hashing can be improved by using more 
 signals other than just the module name if needed.  For hard data though, 
 this does significantly improve performance which clearly comes from 
 better profile attribution so it does something.
>
> I'm OK with the idea that this helped the situation - but it doesn't seem 
> very principled/rigorous. Rather than a sliding scale of "better" I think 
> it'd be worth considering in more detail what would be required to make this 
> correct.
>
> Using the object file name may be more robust - also not perfect for all 
> build systems (one could imagine a distributed build system that /might/ be 
> able to get away with having the distributed builders always build a file 
> named x.o - only to have the distribution system rename the file to its 
> 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D93747#2481383 , @hoy wrote:

> In D93747#2481304 , @tmsriram wrote:
>
>> In D93747#2481223 , @tmsriram wrote:
>>
>>> In D93747#2481163 , @dblaikie 
>>> wrote:
>>>
 In D93747#2481095 , @hoy wrote:

> In D93747#2481073 , @dblaikie 
> wrote:
>
>> Suggesting that gdb isn't using the DW_AT_name at all for "break 
>> " but instead demangling and stripping off the extras 
>> from the linkage name, and since it can't demangle this uniquified name 
>> (unlike the mangled name used when using the overloadable attribute) 
>> that degrades the debugger user experience? I'd have to test that in 
>> more detail/with some hand-hacked DWARF.
>
> Yes, I think in your case the linage name can be demangled by the 
> debugger. In my previous experiment, the uniquefied names could not be 
> demangled therefore I was not able to breakpoint.

 Ah, did some more testing. Seems it's because it isn't a classically 
 mangled name.
>>
>> The simplest fix I can think of is to emit the base 10 number of the 
>> md5hash.  That would preserve all the existing uniqueness and be demangler 
>> friendly.  @hoy @dblaikie  WDYT?
>
> I think using the base 10 form of md5hash is a smart workaround. Thanks for 
> the suggestion.

Sure - wouldn't mind having some wording from the Itanium ABI/mangling rules 
that explain/corroborate what we've seen from testing to ensure we are using it 
correctly and there aren't any other ways that might be more suitable, or 
lurking gotchas we haven't tested.

 It might be possible for this uniquifying step to check if the name is 
 mangled (does it start with "_Z") and if it isn't mangled, it could mangle 
 it (check the length of the name, then "_Zv") and add the 
 unique suffix. That looks to me like it preserves the original debugging 
 behavior?

 (has the problem that we don't actually know the mangling scheme at this 
 point - we do know it up in clang (where it might be Itanium mangling or 
 Microsoft mangling), might point again to the possibility this feature is 
 more suitable to implement in the frontend rather than a middle end pass. 
 And also the 'v' in the mangling is 'void return type', which is a lie - 
 not sure if we could do something better there)
>
> Doing name unification in the frontend sounds like the ultimate solution and 
> since the frontend has all the knowledge about the name mangler. I think for 
> now we can go with the solution @tmsriram suggested, i.e., using the base 10 
> form of md5 hash.

Any general idea of how long "for now" would be? It doesn't seem like putting 
this off is going to make things especially better & seems like more bug 
fixes/changes/etc are being built around the solution as it is at the moment. 
So I'd be hesitant to put off this kind of restructuring too long.

 I think it's pretty important that we don't break tradeoff debuggability 
 for profile accuracy. It'll make for a difficult tradeoff for our/any 
 users.
>>>
>>> Agreed, I didn't expect this.
>>>
 (side note: using the original source file name seems problematic - I know 
 in google at least, the same source file is sometimes built into more than 
 one library/form with different preprocessor defines, so this may not 
 produce as unique a name as you are expecting?)
>>>
>>> It was a best effort and I think the hashing can be improved by using more 
>>> signals other than just the module name if needed.  For hard data though, 
>>> this does significantly improve performance which clearly comes from better 
>>> profile attribution so it does something.

I'm OK with the idea that this helped the situation - but it doesn't seem very 
principled/rigorous. Rather than a sliding scale of "better" I think it'd be 
worth considering in more detail what would be required to make this correct.

Using the object file name may be more robust - also not perfect for all build 
systems (one could imagine a distributed build system that /might/ be able to 
get away with having the distributed builders always build a file named x.o - 
only to have the distribution system rename the file to its canonical name on 
the main machine before linking occurs - but I think that's not how most 
distributed build systems work, and most build systems provide a unique object 
file name across a build?) but maybe moreso than using the input file name? (at 
least I think for google/blaze the object filename is genuinely unique)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D93747#2481304 , @tmsriram wrote:

> In D93747#2481223 , @tmsriram wrote:
>
>> In D93747#2481163 , @dblaikie wrote:
>>
>>> In D93747#2481095 , @hoy wrote:
>>>
 In D93747#2481073 , @dblaikie 
 wrote:

> In D93747#2481053 , @tmsriram 
> wrote:
>
>> In D93747#2480442 , @dblaikie 
>> wrote:
>>
>>> @tmsriram - any ideas what your case/example was like that might've 
>>> caused degraded debugging experience? Would be good to understand if 
>>> we're producing some bad DWARF with this patch/if there might be some 
>>> way to avoid it (as it seems like gdb can handle mangled 
>>> names/overloading in C in this example I've tried)
>>
>> I haven't seen anything that caused degraded debugging experience.  I am 
>> interested in this as we do look at this field for the purposes of 
>> profile attribtution for calls that are inlined.  My comment was that we 
>> don't need to create this if it didn't already exist.  I am not fully 
>> aware of what would happen if we did it all the time.
>
> Ah, sorry, I got confused as to who's comment I was reading. I see it was 
> @hoy who said:
>
>> If set, the gdb debugger will use that field to match the user input and 
>> set breakpoints. Therefore, giving DW_AT_linkage_name a uniquefied name 
>> prevents the debugger from setting a breakpoint based on source names 
>> unless the user specifies a decorated name.
>>
>> Hence, this approach sounds like a workaround for us when the profile 
>> quality matters more than debugging experience.
>
> So I'm still a bit confused. My test doesn't seem to demonstrate the 
> issue with setting a linkage name preventing the debugger from setting a 
> breakpoint based on the source name?
>
> Suggesting that gdb isn't using the DW_AT_name at all for "break 
> " but instead demangling and stripping off the extras from 
> the linkage name, and since it can't demangle this uniquified name 
> (unlike the mangled name used when using the overloadable attribute) that 
> degrades the debugger user experience? I'd have to test that in more 
> detail/with some hand-hacked DWARF.

 Yes, I think in your case the linage name can be demangled by the 
 debugger. In my previous experiment, the uniquefied names could not be 
 demangled therefore I was not able to breakpoint.
>>>
>>> Ah, did some more testing. Seems it's because it isn't a classically 
>>> mangled name.
>
> The simplest fix I can think of is to emit the base 10 number of the md5hash. 
>  That would preserve all the existing uniqueness and be demangler friendly.  
> @hoy @dblaikie  WDYT?

I think using the base 10 form of md5hash is a smart workaround. Thanks for the 
suggestion.

>> Yep, that's the issue
>>
>>   $ c++filt _Z3foov.__uniq
>>   foo() [clone .__uniq]
>>   
>>   $ c++filt _Z3foov.__uniq.123
>>   foo() [clone .__uniq.123]
>>   
>>   $ c++filt._Z3foov.__uniq.123abc
>>   _Z3foov.__uniq.123abc
>>
>> The demangler does not understand a mix of numeric and alpha-numeric. It can 
>> be either but not both and md5hashes contain both.  So we might have to 
>> process the hash string or do something different to keep it demangler 
>> friendly.
>>
>>> It might be possible for this uniquifying step to check if the name is 
>>> mangled (does it start with "_Z") and if it isn't mangled, it could mangle 
>>> it (check the length of the name, then "_Zv") and add the 
>>> unique suffix. That looks to me like it preserves the original debugging 
>>> behavior?
>>>
>>> (has the problem that we don't actually know the mangling scheme at this 
>>> point - we do know it up in clang (where it might be Itanium mangling or 
>>> Microsoft mangling), might point again to the possibility this feature is 
>>> more suitable to implement in the frontend rather than a middle end pass. 
>>> And also the 'v' in the mangling is 'void return type', which is a lie - 
>>> not sure if we could do something better there)

Doing name unification in the frontend sounds like the ultimate solution and 
since the frontend has all the knowledge about the name mangler. I think for 
now we can go with the solution @tmsriram suggested, i.e., using the base 10 
form of md5 hash.

>>> I think it's pretty important that we don't break tradeoff debuggability 
>>> for profile accuracy. It'll make for a difficult tradeoff for our/any users.
>>
>> Agreed, I didn't expect this.
>>
>>> (side note: using the original source file name seems problematic - I know 
>>> in google at least, the same source file is sometimes built into more than 
>>> one 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D93747#2481223 , @tmsriram wrote:

> In D93747#2481163 , @dblaikie wrote:
>
>> In D93747#2481095 , @hoy wrote:
>>
>>> In D93747#2481073 , @dblaikie 
>>> wrote:
>>>
 In D93747#2481053 , @tmsriram 
 wrote:

> In D93747#2480442 , @dblaikie 
> wrote:
>
>> @tmsriram - any ideas what your case/example was like that might've 
>> caused degraded debugging experience? Would be good to understand if 
>> we're producing some bad DWARF with this patch/if there might be some 
>> way to avoid it (as it seems like gdb can handle mangled 
>> names/overloading in C in this example I've tried)
>
> I haven't seen anything that caused degraded debugging experience.  I am 
> interested in this as we do look at this field for the purposes of 
> profile attribtution for calls that are inlined.  My comment was that we 
> don't need to create this if it didn't already exist.  I am not fully 
> aware of what would happen if we did it all the time.

 Ah, sorry, I got confused as to who's comment I was reading. I see it was 
 @hoy who said:

> If set, the gdb debugger will use that field to match the user input and 
> set breakpoints. Therefore, giving DW_AT_linkage_name a uniquefied name 
> prevents the debugger from setting a breakpoint based on source names 
> unless the user specifies a decorated name.
>
> Hence, this approach sounds like a workaround for us when the profile 
> quality matters more than debugging experience.

 So I'm still a bit confused. My test doesn't seem to demonstrate the issue 
 with setting a linkage name preventing the debugger from setting a 
 breakpoint based on the source name?

 Suggesting that gdb isn't using the DW_AT_name at all for "break >>> name>" but instead demangling and stripping off the extras from the 
 linkage name, and since it can't demangle this uniquified name (unlike the 
 mangled name used when using the overloadable attribute) that degrades the 
 debugger user experience? I'd have to test that in more detail/with some 
 hand-hacked DWARF.
>>>
>>> Yes, I think in your case the linage name can be demangled by the debugger. 
>>> In my previous experiment, the uniquefied names could not be demangled 
>>> therefore I was not able to breakpoint.
>>
>> Ah, did some more testing. Seems it's because it isn't a classically mangled 
>> name.

The simplest fix I can think of is to emit the base 10 number of the md5hash.  
That would preserve all the existing uniqueness and be demangler friendly.  
@hoy @dblaikie  WDYT?

> Yep, that's the issue
>
>   $ c++filt _Z3foov.__uniq
>   foo() [clone .__uniq]
>   
>   $ c++filt _Z3foov.__uniq.123
>   foo() [clone .__uniq.123]
>   
>   $ c++filt._Z3foov.__uniq.123abc
>   _Z3foov.__uniq.123abc
>
> The demangler does not understand a mix of numeric and alpha-numeric. It can 
> be either but not both and md5hashes contain both.  So we might have to 
> process the hash string or do something different to keep it demangler 
> friendly.
>
>> It might be possible for this uniquifying step to check if the name is 
>> mangled (does it start with "_Z") and if it isn't mangled, it could mangle 
>> it (check the length of the name, then "_Zv") and add the 
>> unique suffix. That looks to me like it preserves the original debugging 
>> behavior?
>>
>> (has the problem that we don't actually know the mangling scheme at this 
>> point - we do know it up in clang (where it might be Itanium mangling or 
>> Microsoft mangling), might point again to the possibility this feature is 
>> more suitable to implement in the frontend rather than a middle end pass. 
>> And also the 'v' in the mangling is 'void return type', which is a lie - not 
>> sure if we could do something better there)
>>
>> I think it's pretty important that we don't break tradeoff debuggability for 
>> profile accuracy. It'll make for a difficult tradeoff for our/any users.
>
> Agreed, I didn't expect this.
>
>> (side note: using the original source file name seems problematic - I know 
>> in google at least, the same source file is sometimes built into more than 
>> one library/form with different preprocessor defines, so this may not 
>> produce as unique a name as you are expecting?)
>
> It was a best effort and I think the hashing can be improved by using more 
> signals other than just the module name if needed.  For hard data though, 
> this does significantly improve performance which clearly comes from better 
> profile attribution so it does something.




Repository:
  rG LLVM Github Monorepo

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D93747#2481163 , @dblaikie wrote:

> In D93747#2481095 , @hoy wrote:
>
>> In D93747#2481073 , @dblaikie wrote:
>>
>>> In D93747#2481053 , @tmsriram 
>>> wrote:
>>>
 In D93747#2480442 , @dblaikie 
 wrote:

> @tmsriram - any ideas what your case/example was like that might've 
> caused degraded debugging experience? Would be good to understand if 
> we're producing some bad DWARF with this patch/if there might be some way 
> to avoid it (as it seems like gdb can handle mangled names/overloading in 
> C in this example I've tried)

 I haven't seen anything that caused degraded debugging experience.  I am 
 interested in this as we do look at this field for the purposes of profile 
 attribtution for calls that are inlined.  My comment was that we don't 
 need to create this if it didn't already exist.  I am not fully aware of 
 what would happen if we did it all the time.
>>>
>>> Ah, sorry, I got confused as to who's comment I was reading. I see it was 
>>> @hoy who said:
>>>
 If set, the gdb debugger will use that field to match the user input and 
 set breakpoints. Therefore, giving DW_AT_linkage_name a uniquefied name 
 prevents the debugger from setting a breakpoint based on source names 
 unless the user specifies a decorated name.

 Hence, this approach sounds like a workaround for us when the profile 
 quality matters more than debugging experience.
>>>
>>> So I'm still a bit confused. My test doesn't seem to demonstrate the issue 
>>> with setting a linkage name preventing the debugger from setting a 
>>> breakpoint based on the source name?
>>>
>>> Suggesting that gdb isn't using the DW_AT_name at all for "break >> name>" but instead demangling and stripping off the extras from the linkage 
>>> name, and since it can't demangle this uniquified name (unlike the mangled 
>>> name used when using the overloadable attribute) that degrades the debugger 
>>> user experience? I'd have to test that in more detail/with some hand-hacked 
>>> DWARF.
>>
>> Yes, I think in your case the linage name can be demangled by the debugger. 
>> In my previous experiment, the uniquefied names could not be demangled 
>> therefore I was not able to breakpoint.
>
> Ah, did some more testing. Seems it's because it isn't a classically mangled 
> name.

Yep, that's the issue

  $ c++filt _Z3foov.__uniq
  foo() [clone .__uniq]
  
  $ c++filt _Z3foov.__uniq.123
  foo() [clone .__uniq.123]
  
  $ c++filt._Z3foov.__uniq.123abc
  _Z3foov.__uniq.123abc

The demangler does not understand a mix of numeric and alpha-numeric. It can be 
either but not both and md5hashes contain both.  So we might have to process 
the hash string or do something different to keep it demangler friendly.

> It might be possible for this uniquifying step to check if the name is 
> mangled (does it start with "_Z") and if it isn't mangled, it could mangle it 
> (check the length of the name, then "_Zv") and add the unique 
> suffix. That looks to me like it preserves the original debugging behavior?
>
> (has the problem that we don't actually know the mangling scheme at this 
> point - we do know it up in clang (where it might be Itanium mangling or 
> Microsoft mangling), might point again to the possibility this feature is 
> more suitable to implement in the frontend rather than a middle end pass. And 
> also the 'v' in the mangling is 'void return type', which is a lie - not sure 
> if we could do something better there)
>
> I think it's pretty important that we don't break tradeoff debuggability for 
> profile accuracy. It'll make for a difficult tradeoff for our/any users.

Agreed, I didn't expect this.

> (side note: using the original source file name seems problematic - I know in 
> google at least, the same source file is sometimes built into more than one 
> library/form with different preprocessor defines, so this may not produce as 
> unique a name as you are expecting?)

It was a best effort and I think the hashing can be improved by using more 
signals other than just the module name if needed.  For hard data though, this 
does significantly improve performance which clearly comes from better profile 
attribution so it does something.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D93747#2481095 , @hoy wrote:

> In D93747#2481073 , @dblaikie wrote:
>
>> In D93747#2481053 , @tmsriram wrote:
>>
>>> In D93747#2480442 , @dblaikie 
>>> wrote:
>>>
 @tmsriram - any ideas what your case/example was like that might've caused 
 degraded debugging experience? Would be good to understand if we're 
 producing some bad DWARF with this patch/if there might be some way to 
 avoid it (as it seems like gdb can handle mangled names/overloading in C 
 in this example I've tried)
>>>
>>> I haven't seen anything that caused degraded debugging experience.  I am 
>>> interested in this as we do look at this field for the purposes of profile 
>>> attribtution for calls that are inlined.  My comment was that we don't need 
>>> to create this if it didn't already exist.  I am not fully aware of what 
>>> would happen if we did it all the time.
>>
>> Ah, sorry, I got confused as to who's comment I was reading. I see it was 
>> @hoy who said:
>>
>>> If set, the gdb debugger will use that field to match the user input and 
>>> set breakpoints. Therefore, giving DW_AT_linkage_name a uniquefied name 
>>> prevents the debugger from setting a breakpoint based on source names 
>>> unless the user specifies a decorated name.
>>>
>>> Hence, this approach sounds like a workaround for us when the profile 
>>> quality matters more than debugging experience.
>>
>> So I'm still a bit confused. My test doesn't seem to demonstrate the issue 
>> with setting a linkage name preventing the debugger from setting a 
>> breakpoint based on the source name?
>>
>> Suggesting that gdb isn't using the DW_AT_name at all for "break > name>" but instead demangling and stripping off the extras from the linkage 
>> name, and since it can't demangle this uniquified name (unlike the mangled 
>> name used when using the overloadable attribute) that degrades the debugger 
>> user experience? I'd have to test that in more detail/with some hand-hacked 
>> DWARF.
>
> Yes, I think in your case the linage name can be demangled by the debugger. 
> In my previous experiment, the uniquefied names could not be demangled 
> therefore I was not able to breakpoint.

Ah, did some more testing. Seems it's because it isn't a classically mangled 
name.

It might be possible for this uniquifying step to check if the name is mangled 
(does it start with "_Z") and if it isn't mangled, it could mangle it (check 
the length of the name, then "_Zv") and add the unique suffix. 
That looks to me like it preserves the original debugging behavior?

(has the problem that we don't actually know the mangling scheme at this point 
- we do know it up in clang (where it might be Itanium mangling or Microsoft 
mangling), might point again to the possibility this feature is more suitable 
to implement in the frontend rather than a middle end pass. And also the 'v' in 
the mangling is 'void return type', which is a lie - not sure if we could do 
something better there)

I think it's pretty important that we don't break tradeoff debuggability for 
profile accuracy. It'll make for a difficult tradeoff for our/any users.

(side note: using the original source file name seems problematic - I know in 
google at least, the same source file is sometimes built into more than one 
library/form with different preprocessor defines, so this may not produce as 
unique a name as you are expecting?)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D93747#2481073 , @dblaikie wrote:

> In D93747#2481053 , @tmsriram wrote:
>
>> In D93747#2480442 , @dblaikie wrote:
>>
>>> @tmsriram - any ideas what your case/example was like that might've caused 
>>> degraded debugging experience? Would be good to understand if we're 
>>> producing some bad DWARF with this patch/if there might be some way to 
>>> avoid it (as it seems like gdb can handle mangled names/overloading in C in 
>>> this example I've tried)
>>
>> I haven't seen anything that caused degraded debugging experience.  I am 
>> interested in this as we do look at this field for the purposes of profile 
>> attribtution for calls that are inlined.  My comment was that we don't need 
>> to create this if it didn't already exist.  I am not fully aware of what 
>> would happen if we did it all the time.
>
> Ah, sorry, I got confused as to who's comment I was reading. I see it was 
> @hoy who said:
>
>> If set, the gdb debugger will use that field to match the user input and set 
>> breakpoints. Therefore, giving DW_AT_linkage_name a uniquefied name prevents 
>> the debugger from setting a breakpoint based on source names unless the user 
>> specifies a decorated name.
>>
>> Hence, this approach sounds like a workaround for us when the profile 
>> quality matters more than debugging experience.
>
> So I'm still a bit confused. My test doesn't seem to demonstrate the issue 
> with setting a linkage name preventing the debugger from setting a breakpoint 
> based on the source name?
>
> Suggesting that gdb isn't using the DW_AT_name at all for "break  name>" but instead demangling and stripping off the extras from the linkage 
> name, and since it can't demangle this uniquified name (unlike the mangled 
> name used when using the overloadable attribute) that degrades the debugger 
> user experience? I'd have to test that in more detail/with some hand-hacked 
> DWARF.

Yes, I think in your case the linage name can be demangled by the debugger. In 
my previous experiment, the uniquefied names could not be demangled therefore I 
was not able to breakpoint.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

In D93747#2481053 , @tmsriram wrote:

> In D93747#2480442 , @dblaikie wrote:
>
>> @tmsriram - any ideas what your case/example was like that might've caused 
>> degraded debugging experience? Would be good to understand if we're 
>> producing some bad DWARF with this patch/if there might be some way to avoid 
>> it (as it seems like gdb can handle mangled names/overloading in C in this 
>> example I've tried)
>
> I haven't seen anything that caused degraded debugging experience.  I am 
> interested in this as we do look at this field for the purposes of profile 
> attribtution for calls that are inlined.  My comment was that we don't need 
> to create this if it didn't already exist.  I am not fully aware of what 
> would happen if we did it all the time.

Ah, sorry, I got confused as to who's comment I was reading. I see it was @hoy 
who said:

> If set, the gdb debugger will use that field to match the user input and set 
> breakpoints. Therefore, giving DW_AT_linkage_name a uniquefied name prevents 
> the debugger from setting a breakpoint based on source names unless the user 
> specifies a decorated name.
>
> Hence, this approach sounds like a workaround for us when the profile quality 
> matters more than debugging experience.

So I'm still a bit confused. My test doesn't seem to demonstrate the issue with 
setting a linkage name preventing the debugger from setting a breakpoint based 
on the source name?

Suggesting that gdb isn't using the DW_AT_name at all for "break " but instead demangling and stripping off the extras from the linkage 
name, and since it can't demangle this uniquified name (unlike the mangled name 
used when using the overloadable attribute) that degrades the debugger user 
experience? I'd have to test that in more detail/with some hand-hacked DWARF.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D93747#2481053 , @tmsriram wrote:

> In D93747#2480442 , @dblaikie wrote:
>
 In D93747#2470178 , @tmsriram 
 wrote:

> In D93747#2469556 , @hoy wrote:
>
>>> In D93656 , @dblaikie wrote:
>>> Though the C case is interesting - it means you'll end up with C 
>>> functions with the same DWARF 'name' but different linkage name. I 
>>> don't know what that'll do to DWARF consumers - I guess they'll 
>>> probably be OK-ish with it, as much as they are with C++ overloading. I 
>>> think there are some cases of C name mangling so it's probably 
>>> supported/OK-ish with DWARF Consumers. Wouldn't hurt for you to take a 
>>> look/see what happens in that case with a debugger like gdb/check other 
>>> cases of C name mangling to see what DWARF they end up creating (with 
>>> both GCC and Clang).
>>
>> I did a quick experiment with C name managing with GCC and -flto. It 
>> turns out the `DW_AT_linkage_name` field of `DW_TAG_subprogram` is never 
>> set for C programs. If set, the gdb debugger will use that field to 
>> match the user input and set breakpoints. Therefore, giving 
>> `DW_AT_linkage_name` a uniquefied name prevents the debugger from 
>> setting a breakpoint based on source names unless the user specifies a 
>> decorated name.
>>
>> Hence, this approach sounds like a workaround for us when the profile 
>> quality matters more than debugging experience. I'm inclined to have it 
>> under a switch. What do you think?
>
> Just a thought, we could always check if rawLinkageName is set and only 
> set it when it is not null.  That seems safe without needing the option. 
> Not a strong opinion.

 Was this thread concluded? Doesn't look like any check was added?
>>>
>>> Thanks for the detailed analysis! Moving forward I would like to see a more 
>>> clear contract about debug linkage names between the compiler and debugger 
>>> as a guideline to code cloning work. For this patch, I'm adding a switch 
>>> for it with a default value "on" since AutoFDO and propeller are the 
>>> primary consumers of the work here and they mostly care about profile 
>>> quality more than debugging experience. But correct me if I'm wrong and 
>>> I'll flip the default value.
>>
>> Presumably people are going to want to debug these binaries - I'm not sure 
>> it's a "one or the other" situation as it sounds like @tmsriram was 
>> suggesting by only modifying the linkage name when it's already set this 
>> might produce a better debugging experience, if I'm following the discussion 
>> correctly?
>>
>> But I'm pretty sure there are places where C supports name mangling, so I 
>> wouldn't mind understanding the gdb behavior in more detail - perhaps 
>> there's a way to make it work better.
>>
>> Using Clang's __attribute__((overloadable)) support, I was able to produce a 
>> C program with mangled names, with DW_AT_linkage_names on those functions, 
>> like this:
>>
>>   $ cat test.c
>>   void __attribute__((overloadable)) f(int i) {
>>   }
>>   void __attribute__((overloadable)) f(long l) {
>>   }
>>   int main() {
>> f(3);
>> f(5l);
>>   }
>>   blaikie@blaikie-linux2:~/dev/scratch$ clang-tot test.c -g
>>   blaikie@blaikie-linux2:~/dev/scratch$ llvm-dwarfdump-tot a.out
>>   a.out:  file format elf64-x86-64
>>   
>>   .debug_info contents:
>>   0x: Compile Unit: length = 0x009e, format = DWARF32, version = 
>> 0x0004, abbr_offset = 0x, addr_size = 0x08 (next unit at 0x00a2)
>>   
>>   0x000b: DW_TAG_compile_unit
>> DW_AT_producer("clang version 12.0.0 
>> (g...@github.com:llvm/llvm-project.git 
>> 20013e0940dea465a173c3c7ce60f0e419242ef8)")
>> DW_AT_language(DW_LANG_C99)
>> DW_AT_name("test.c")
>> DW_AT_stmt_list   (0x)
>> DW_AT_comp_dir
>> ("/usr/local/google/home/blaikie/dev/scratch")
>> DW_AT_low_pc  (0x00401110)
>> DW_AT_high_pc (0x0040114c)
>>   
>>   0x002a:   DW_TAG_subprogram
>>   DW_AT_low_pc(0x00401110)
>>   DW_AT_high_pc   (0x00401119)
>>   DW_AT_frame_base(DW_OP_reg6 RBP)
>>   DW_AT_linkage_name  ("_Z1fi")
>>   DW_AT_name  ("f")
>>   DW_AT_decl_file 
>> ("/usr/local/google/home/blaikie/dev/scratch/test.c")
>>   DW_AT_decl_line (1)
>>   DW_AT_prototyped(true)
>>   DW_AT_external  (true)
>>   
>>   0x0043: DW_TAG_formal_parameter
>> DW_AT_location(DW_OP_fbreg -4)

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D93747#2480442 , @dblaikie wrote:

>>> In D93747#2470178 , @tmsriram 
>>> wrote:
>>>
 In D93747#2469556 , @hoy wrote:

>> In D93656 , @dblaikie wrote:
>> Though the C case is interesting - it means you'll end up with C 
>> functions with the same DWARF 'name' but different linkage name. I don't 
>> know what that'll do to DWARF consumers - I guess they'll probably be 
>> OK-ish with it, as much as they are with C++ overloading. I think there 
>> are some cases of C name mangling so it's probably supported/OK-ish with 
>> DWARF Consumers. Wouldn't hurt for you to take a look/see what happens 
>> in that case with a debugger like gdb/check other cases of C name 
>> mangling to see what DWARF they end up creating (with both GCC and 
>> Clang).
>
> I did a quick experiment with C name managing with GCC and -flto. It 
> turns out the `DW_AT_linkage_name` field of `DW_TAG_subprogram` is never 
> set for C programs. If set, the gdb debugger will use that field to match 
> the user input and set breakpoints. Therefore, giving 
> `DW_AT_linkage_name` a uniquefied name prevents the debugger from setting 
> a breakpoint based on source names unless the user specifies a decorated 
> name.
>
> Hence, this approach sounds like a workaround for us when the profile 
> quality matters more than debugging experience. I'm inclined to have it 
> under a switch. What do you think?

 Just a thought, we could always check if rawLinkageName is set and only 
 set it when it is not null.  That seems safe without needing the option. 
 Not a strong opinion.
>>>
>>> Was this thread concluded? Doesn't look like any check was added?
>>
>> Thanks for the detailed analysis! Moving forward I would like to see a more 
>> clear contract about debug linkage names between the compiler and debugger 
>> as a guideline to code cloning work. For this patch, I'm adding a switch for 
>> it with a default value "on" since AutoFDO and propeller are the primary 
>> consumers of the work here and they mostly care about profile quality more 
>> than debugging experience. But correct me if I'm wrong and I'll flip the 
>> default value.
>
> Presumably people are going to want to debug these binaries - I'm not sure 
> it's a "one or the other" situation as it sounds like @tmsriram was 
> suggesting by only modifying the linkage name when it's already set this 
> might produce a better debugging experience, if I'm following the discussion 
> correctly?
>
> But I'm pretty sure there are places where C supports name mangling, so I 
> wouldn't mind understanding the gdb behavior in more detail - perhaps there's 
> a way to make it work better.
>
> Using Clang's __attribute__((overloadable)) support, I was able to produce a 
> C program with mangled names, with DW_AT_linkage_names on those functions, 
> like this:
>
>   $ cat test.c
>   void __attribute__((overloadable)) f(int i) {
>   }
>   void __attribute__((overloadable)) f(long l) {
>   }
>   int main() {
> f(3);
> f(5l);
>   }
>   blaikie@blaikie-linux2:~/dev/scratch$ clang-tot test.c -g
>   blaikie@blaikie-linux2:~/dev/scratch$ llvm-dwarfdump-tot a.out
>   a.out:  file format elf64-x86-64
>   
>   .debug_info contents:
>   0x: Compile Unit: length = 0x009e, format = DWARF32, version = 
> 0x0004, abbr_offset = 0x, addr_size = 0x08 (next unit at 0x00a2)
>   
>   0x000b: DW_TAG_compile_unit
> DW_AT_producer("clang version 12.0.0 
> (g...@github.com:llvm/llvm-project.git 
> 20013e0940dea465a173c3c7ce60f0e419242ef8)")
> DW_AT_language(DW_LANG_C99)
> DW_AT_name("test.c")
> DW_AT_stmt_list   (0x)
> DW_AT_comp_dir
> ("/usr/local/google/home/blaikie/dev/scratch")
> DW_AT_low_pc  (0x00401110)
> DW_AT_high_pc (0x0040114c)
>   
>   0x002a:   DW_TAG_subprogram
>   DW_AT_low_pc(0x00401110)
>   DW_AT_high_pc   (0x00401119)
>   DW_AT_frame_base(DW_OP_reg6 RBP)
>   DW_AT_linkage_name  ("_Z1fi")
>   DW_AT_name  ("f")
>   DW_AT_decl_file 
> ("/usr/local/google/home/blaikie/dev/scratch/test.c")
>   DW_AT_decl_line (1)
>   DW_AT_prototyped(true)
>   DW_AT_external  (true)
>   
>   0x0043: DW_TAG_formal_parameter
> DW_AT_location(DW_OP_fbreg -4)
> DW_AT_name("i")
> DW_AT_decl_file   
> ("/usr/local/google/home/blaikie/dev/scratch/test.c")
> 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

>> In D93747#2470178 , @tmsriram wrote:
>>
>>> In D93747#2469556 , @hoy wrote:
>>>
> In D93656 , @dblaikie wrote:
> Though the C case is interesting - it means you'll end up with C 
> functions with the same DWARF 'name' but different linkage name. I don't 
> know what that'll do to DWARF consumers - I guess they'll probably be 
> OK-ish with it, as much as they are with C++ overloading. I think there 
> are some cases of C name mangling so it's probably supported/OK-ish with 
> DWARF Consumers. Wouldn't hurt for you to take a look/see what happens in 
> that case with a debugger like gdb/check other cases of C name mangling 
> to see what DWARF they end up creating (with both GCC and Clang).

 I did a quick experiment with C name managing with GCC and -flto. It turns 
 out the `DW_AT_linkage_name` field of `DW_TAG_subprogram` is never set for 
 C programs. If set, the gdb debugger will use that field to match the user 
 input and set breakpoints. Therefore, giving `DW_AT_linkage_name` a 
 uniquefied name prevents the debugger from setting a breakpoint based on 
 source names unless the user specifies a decorated name.

 Hence, this approach sounds like a workaround for us when the profile 
 quality matters more than debugging experience. I'm inclined to have it 
 under a switch. What do you think?
>>>
>>> Just a thought, we could always check if rawLinkageName is set and only set 
>>> it when it is not null.  That seems safe without needing the option. Not a 
>>> strong opinion.
>>
>> Was this thread concluded? Doesn't look like any check was added?
>
> Thanks for the detailed analysis! Moving forward I would like to see a more 
> clear contract about debug linkage names between the compiler and debugger as 
> a guideline to code cloning work. For this patch, I'm adding a switch for it 
> with a default value "on" since AutoFDO and propeller are the primary 
> consumers of the work here and they mostly care about profile quality more 
> than debugging experience. But correct me if I'm wrong and I'll flip the 
> default value.

Presumably people are going to want to debug these binaries - I'm not sure it's 
a "one or the other" situation as it sounds like @tmsriram was suggesting by 
only modifying the linkage name when it's already set this might produce a 
better debugging experience, if I'm following the discussion correctly?

But I'm pretty sure there are places where C supports name mangling, so I 
wouldn't mind understanding the gdb behavior in more detail - perhaps there's a 
way to make it work better.

Using Clang's __attribute__((overloadable)) support, I was able to produce a C 
program with mangled names, with DW_AT_linkage_names on those functions, like 
this:

  $ cat test.c
  void __attribute__((overloadable)) f(int i) {
  }
  void __attribute__((overloadable)) f(long l) {
  }
  int main() {
f(3);
f(5l);
  }
  blaikie@blaikie-linux2:~/dev/scratch$ clang-tot test.c -g
  blaikie@blaikie-linux2:~/dev/scratch$ llvm-dwarfdump-tot a.out
  a.out:  file format elf64-x86-64
  
  .debug_info contents:
  0x: Compile Unit: length = 0x009e, format = DWARF32, version = 
0x0004, abbr_offset = 0x, addr_size = 0x08 (next unit at 0x00a2)
  
  0x000b: DW_TAG_compile_unit
DW_AT_producer("clang version 12.0.0 
(g...@github.com:llvm/llvm-project.git 
20013e0940dea465a173c3c7ce60f0e419242ef8)")
DW_AT_language(DW_LANG_C99)
DW_AT_name("test.c")
DW_AT_stmt_list   (0x)
DW_AT_comp_dir("/usr/local/google/home/blaikie/dev/scratch")
DW_AT_low_pc  (0x00401110)
DW_AT_high_pc (0x0040114c)
  
  0x002a:   DW_TAG_subprogram
  DW_AT_low_pc(0x00401110)
  DW_AT_high_pc   (0x00401119)
  DW_AT_frame_base(DW_OP_reg6 RBP)
  DW_AT_linkage_name  ("_Z1fi")
  DW_AT_name  ("f")
  DW_AT_decl_file 
("/usr/local/google/home/blaikie/dev/scratch/test.c")
  DW_AT_decl_line (1)
  DW_AT_prototyped(true)
  DW_AT_external  (true)
  
  0x0043: DW_TAG_formal_parameter
DW_AT_location(DW_OP_fbreg -4)
DW_AT_name("i")
DW_AT_decl_file   
("/usr/local/google/home/blaikie/dev/scratch/test.c")
DW_AT_decl_line   (1)
DW_AT_type(0x0093 "int")
  
  0x0051: NULL
  
  0x0052:   DW_TAG_subprogram
  DW_AT_low_pc(0x00401120)
  DW_AT_high_pc   (0x0040112a)
  

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Hongtao Yu via Phabricator via cfe-commits]

hoy updated this revision to Diff 314393.
hoy added a comment.

Replacing linkage names for debug declaration as well.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

Files:
  llvm/include/llvm/IR/DebugInfoMetadata.h
  llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
  llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll

Index: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
===
--- llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
+++ llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
@@ -3,8 +3,10 @@
 ; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
 ; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
 ; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes=unique-internal-linkage-names -unqiue-debug-linkage-names=0 < %s -o - | FileCheck %s --check-prefix=NODBG
+; RUN: opt -S -passes=unique-internal-linkage-names < %s -o - | FileCheck %s --check-prefix=DBG
 
-define internal i32 @foo() {
+define internal i32 @foo() !dbg !15 {
 entry:
   ret i32 0
 }
@@ -14,6 +16,21 @@
   ret i32 (...)* bitcast (i32 ()* @foo to i32 (...)*)
 }
 
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, enums: !2)
+!1 = !DIFile(filename: "test.c", directory: "")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!15 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 5, type: !16, scopeLine: 5, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !0, declaration: !18, retainedNodes: !2)
+!16 = !DISubroutineType(types: !17)
+!17 = !{!13}
+!18 = !DISubprogram(name: "foo", scope: !1, isDefinition: false, flags: DIFlagPrototyped, spFlags: DISPFlagOptimized)
+
 ; O0: Running pass: UniqueInternalLinkageNamesPass
 
 ;; Check UniqueInternalLinkageNamesPass is scheduled before SampleProfileProbePass.
@@ -22,3 +39,8 @@
 
 ; UNIQUE: define internal i32 @foo.__uniq.{{[0-9a-f]+}}()
 ; UNIQUE: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
+
+; NODBG: distinct !DISubprogram(name: "foo", scope: ![[#]]
+; NODBG: !DISubprogram(name: "foo", scope: ![[#]]
+; DBG: distinct !DISubprogram(name: "foo", linkageName: "foo.__uniq.{{[0-9a-f]+}}", scope: ![[#]]
+; DBG: !DISubprogram(name: "foo", linkageName: "foo.__uniq.{{[0-9a-f]+}}", scope: ![[#]]
Index: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
===
--- llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
+++ llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
@@ -13,13 +13,21 @@
 
 #include "llvm/Transforms/Utils/UniqueInternalLinkageNames.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/IR/DebugInfoMetadata.h"
+#include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Module.h"
 #include "llvm/InitializePasses.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 
 using namespace llvm;
 
+static cl::opt
+UniqueDebugLinakgeNames("unqiue-debug-linkage-names", cl::init(true),
+cl::Hidden,
+cl::desc("Uniqueify debug linkage Names"));
+
 static bool uniqueifyInternalLinkageNames(Module ) {
   llvm::MD5 Md5;
   Md5.update(M.getSourceFileName());
@@ -31,11 +39,21 @@
   // this symbol is of internal linkage type.
   std::string ModuleNameHash = (Twine(".__uniq.") + Twine(Str)).str();
   bool Changed = false;
+  MDBuilder MDB(M.getContext());
 
   // Append the module hash to all internal linkage functions.
   for (auto  : M) {
 if (F.hasInternalLinkage()) {
   F.setName(F.getName() + ModuleNameHash);
+  // Replace linkage names in the debug metadata.
+  if (UniqueDebugLinakgeNames) {
+if (DISubprogram *SP = F.getSubprogram()) {
+  auto *Name = MDB.createString(F.getName());
+  SP->replaceRawLinkageName(Name);
+  if (DISubprogram *SPDecl = SP->getDeclaration())
+SPDecl->replaceRawLinkageName(Name);
+}
+  }
   Changed = true;
 }
   }
Index: llvm/include/llvm/IR/DebugInfoMetadata.h
===
--- llvm/include/llvm/IR/DebugInfoMetadata.h
+++ 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D93747#2475852 , @dblaikie wrote:

> In D93747#2470504 , @hoy wrote:
>
>> In D93747#2470387 , @dblaikie wrote:
>>
>>> Please remove the clang test change - if this is an LLVM change with LLVM 
>>> test coverage, that's enough. (we don't generally test every LLVM change 
>>> from both LLVM and Clang)
>>
>> Sounds good.
>>
>>> I'd still be curious if you could look around to see whether there are 
>>> other cases of function splitting/cloning/etc to see how they deal with 
>>> updating the DISubprograms, to see if there's some prior art that should be 
>>> used here.
>>
>> To the best of my knowledge, existing code does not change the linkage name 
>> field of a DISubprogram once created. You can create a new DISubprogram 
>> record with any linkage name you want but bear in mind how the debugger will 
>> consume the record. Looks like we are the first one to change existing 
>> record which will confuse the debugger.
>
> Sure enough - do any other transforms have similar requirements (of creating 
> a record for something that looks like the same function but isn't quite) but 
> take a different approach? Be good to know if other approaches were 
> chosen/how/why they are applicable there but not here, etc. (conversely 
> perhaps other passes worked around the issue in less than ideal ways and 
> could benefit from using this new approach).
>
> Looks like some places that could use this functionality aren't quite there 
> yet - 
> The Attributor has an internalizing feature (added in 
> 87a85f3d57f55f5652ec44f77816c7c9457545fa 
>  ) that 
> produces invalid IR (ends up with two !dbg attachments of the same 
> DISubprogram) if the function it's internalizing has a DISubprogram - but if 
> it succeeded it'd have the same problem that the linkage name on the 
> DISubprogram wouldn't match the actual symbol/function name. (@jdoerfert 
> @bbn).
> The IR Outliner (@AndrewLitteken ) seems to be only a few months old and 
> appears to have no debug info handling - probably results in the same problem.
> The Partial Inliner does clone a function into a new name & so would have an 
> invalid DISubprogram, though it only uses the clone for inlining (this 
> probably then goes on to produce the desired debug info, where the inlining 
> appears to come from the original function)
> ThinLTO does some function cloning within a module for aliases, but it then 
> renames the clone to the aliasees name so I think the name works out to match 
> again.
>
> If this is an invariant, that the linkage name on the DISubprogram should 
> match the actual llvm::Function name (@aprantl @JDevlieghere - verifier 
> check, perhaps?) - it'd be nice to make that more reliable, either by 
> removing the name and relying on the llvm::Function name (perhaps with a 
> boolean on the DISubprogram as to whether the linkage name should be emitted 
> or not - I think currently Clang's IRGen makes choices about which 
> DISubprograms will get linkage names) or a verifier and utilities to keep 
> them in sync.
>
> But I'll leave that alone for now/for this review, unless someone else wants 
> to chime in on it.
>
> In D93747#2470178 , @tmsriram wrote:
>
>> In D93747#2469556 , @hoy wrote:
>>
 In D93656 , @dblaikie wrote:
 Though the C case is interesting - it means you'll end up with C functions 
 with the same DWARF 'name' but different linkage name. I don't know what 
 that'll do to DWARF consumers - I guess they'll probably be OK-ish with 
 it, as much as they are with C++ overloading. I think there are some cases 
 of C name mangling so it's probably supported/OK-ish with DWARF Consumers. 
 Wouldn't hurt for you to take a look/see what happens in that case with a 
 debugger like gdb/check other cases of C name mangling to see what DWARF 
 they end up creating (with both GCC and Clang).
>>>
>>> I did a quick experiment with C name managing with GCC and -flto. It turns 
>>> out the `DW_AT_linkage_name` field of `DW_TAG_subprogram` is never set for 
>>> C programs. If set, the gdb debugger will use that field to match the user 
>>> input and set breakpoints. Therefore, giving `DW_AT_linkage_name` a 
>>> uniquefied name prevents the debugger from setting a breakpoint based on 
>>> source names unless the user specifies a decorated name.
>>>
>>> Hence, this approach sounds like a workaround for us when the profile 
>>> quality matters more than debugging experience. I'm inclined to have it 
>>> under a switch. What do you think?
>>
>> Just a thought, we could always check if rawLinkageName is set and only set 
>> it when it is not null.  That seems safe without needing the option. Not a 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Hongtao Yu via Phabricator via cfe-commits]

hoy updated this revision to Diff 314259.
hoy added a comment.

Adding a switch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

Files:
  llvm/include/llvm/IR/DebugInfoMetadata.h
  llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
  llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll

Index: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
===
--- llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
+++ llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
@@ -3,8 +3,10 @@
 ; RUN: opt -S -passes='default' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
 ; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
 ; RUN: opt -S -passes='thinlto-pre-link' -new-pm-pseudo-probe-for-profiling -new-pm-unique-internal-linkage-names -debug-pass-manager < %s 2>&1 | FileCheck %s --check-prefix=O2 --check-prefix=UNIQUE
+; RUN: opt -S -passes=unique-internal-linkage-names -unqiue-debug-linkage-names=0 < %s -o - | FileCheck %s --check-prefix=NODBG
+; RUN: opt -S -passes=unique-internal-linkage-names < %s -o - | FileCheck %s --check-prefix=DBG
 
-define internal i32 @foo() {
+define internal i32 @foo() !dbg !15 {
 entry:
   ret i32 0
 }
@@ -14,6 +16,25 @@
   ret i32 (...)* bitcast (i32 ()* @foo to i32 (...)*)
 }
 
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!3, !4, !5}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C99, file: !1, enums: !2)
+!1 = !DIFile(filename: "test.c", directory: "")
+!2 = !{}
+!3 = !{i32 7, !"Dwarf Version", i32 4}
+!4 = !{i32 2, !"Debug Info Version", i32 3}
+!5 = !{i32 1, !"wchar_size", i32 4}
+!8 = !DISubroutineType(types: !9)
+!9 = !{!10}
+!10 = !DIDerivedType(tag: DW_TAG_pointer_type, baseType: !11, size: 64)
+!11 = !DISubroutineType(types: !12)
+!12 = !{!13, null}
+!13 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!15 = distinct !DISubprogram(name: "foo", scope: !1, file: !1, line: 5, type: !16, scopeLine: 5, spFlags: DISPFlagLocalToUnit | DISPFlagDefinition, unit: !0, retainedNodes: !2)
+!16 = !DISubroutineType(types: !17)
+!17 = !{!13}
+
 ; O0: Running pass: UniqueInternalLinkageNamesPass
 
 ;; Check UniqueInternalLinkageNamesPass is scheduled before SampleProfileProbePass.
@@ -22,3 +43,6 @@
 
 ; UNIQUE: define internal i32 @foo.__uniq.{{[0-9a-f]+}}()
 ; UNIQUE: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
+
+; NODBG: distinct !DISubprogram(name: "foo", scope: ![[#]]
+; DBG: distinct !DISubprogram(name: "foo", linkageName: "foo.__uniq.{{[0-9a-f]+}}", scope: ![[#]]
Index: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
===
--- llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
+++ llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
@@ -13,13 +13,21 @@
 
 #include "llvm/Transforms/Utils/UniqueInternalLinkageNames.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/IR/DebugInfoMetadata.h"
+#include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Module.h"
 #include "llvm/InitializePasses.h"
+#include "llvm/Support/CommandLine.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Transforms/Utils/ModuleUtils.h"
 
 using namespace llvm;
 
+static cl::opt
+UniqueDebugLinakgeNames("unqiue-debug-linkage-names", cl::init(true),
+cl::Hidden,
+cl::desc("Uniqueify debug linkage Names"));
+
 static bool uniqueifyInternalLinkageNames(Module ) {
   llvm::MD5 Md5;
   Md5.update(M.getSourceFileName());
@@ -31,11 +39,18 @@
   // this symbol is of internal linkage type.
   std::string ModuleNameHash = (Twine(".__uniq.") + Twine(Str)).str();
   bool Changed = false;
+  MDBuilder MDB(M.getContext());
 
   // Append the module hash to all internal linkage functions.
   for (auto  : M) {
 if (F.hasInternalLinkage()) {
   F.setName(F.getName() + ModuleNameHash);
+  if (UniqueDebugLinakgeNames) {
+if (DISubprogram *SP = F.getSubprogram()) {
+  auto *Name = MDB.createString(F.getName());
+  SP->replaceRawLinkageName(Name);
+}
+  }
   Changed = true;
 }
   }
Index: llvm/include/llvm/IR/DebugInfoMetadata.h
===
--- llvm/include/llvm/IR/DebugInfoMetadata.h
+++ llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -2053,6 +2053,10 @@
 return getNumOperands() > 10 ? getOperandAs(10) : nullptr;
   }
 
+  void replaceRawLinkageName(MDString *LinkageName) {
+replaceOperandWith(3, LinkageName);
+  }
+
   /// Check if this subprogram describes the given function.
   

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[David Blaikie via Phabricator via cfe-commits]

dblaikie added subscribers: AndrewLitteken, bbn, JDevlieghere, jdoerfert, 
aprantl.
dblaikie added a comment.

In D93747#2470504 , @hoy wrote:

> In D93747#2470387 , @dblaikie wrote:
>
>> Please remove the clang test change - if this is an LLVM change with LLVM 
>> test coverage, that's enough. (we don't generally test every LLVM change 
>> from both LLVM and Clang)
>
> Sounds good.
>
>> I'd still be curious if you could look around to see whether there are other 
>> cases of function splitting/cloning/etc to see how they deal with updating 
>> the DISubprograms, to see if there's some prior art that should be used here.
>
> To the best of my knowledge, existing code does not change the linkage name 
> field of a DISubprogram once created. You can create a new DISubprogram 
> record with any linkage name you want but bear in mind how the debugger will 
> consume the record. Looks like we are the first one to change existing record 
> which will confuse the debugger.

Sure enough - do any other transforms have similar requirements (of creating a 
record for something that looks like the same function but isn't quite) but 
take a different approach? Be good to know if other approaches were 
chosen/how/why they are applicable there but not here, etc. (conversely perhaps 
other passes worked around the issue in less than ideal ways and could benefit 
from using this new approach).

Looks like some places that could use this functionality aren't quite there yet 
- 
The Attributor has an internalizing feature (added in 
87a85f3d57f55f5652ec44f77816c7c9457545fa 
 ) that 
produces invalid IR (ends up with two !dbg attachments of the same 
DISubprogram) if the function it's internalizing has a DISubprogram - but if it 
succeeded it'd have the same problem that the linkage name on the DISubprogram 
wouldn't match the actual symbol/function name. (@jdoerfert @bbn).
The IR Outliner (@AndrewLitteken ) seems to be only a few months old and 
appears to have no debug info handling - probably results in the same problem.
The Partial Inliner does clone a function into a new name & so would have an 
invalid DISubprogram, though it only uses the clone for inlining (this probably 
then goes on to produce the desired debug info, where the inlining appears to 
come from the original function)
ThinLTO does some function cloning within a module for aliases, but it then 
renames the clone to the aliasees name so I think the name works out to match 
again.

If this is an invariant, that the linkage name on the DISubprogram should match 
the actual llvm::Function name (@aprantl @JDevlieghere - verifier check, 
perhaps?) - it'd be nice to make that more reliable, either by removing the 
name and relying on the llvm::Function name (perhaps with a boolean on the 
DISubprogram as to whether the linkage name should be emitted or not - I think 
currently Clang's IRGen makes choices about which DISubprograms will get 
linkage names) or a verifier and utilities to keep them in sync.

But I'll leave that alone for now/for this review, unless someone else wants to 
chime in on it.

In D93747#2470178 , @tmsriram wrote:

> In D93747#2469556 , @hoy wrote:
>
>>> In D93656 , @dblaikie wrote:
>>> Though the C case is interesting - it means you'll end up with C functions 
>>> with the same DWARF 'name' but different linkage name. I don't know what 
>>> that'll do to DWARF consumers - I guess they'll probably be OK-ish with it, 
>>> as much as they are with C++ overloading. I think there are some cases of C 
>>> name mangling so it's probably supported/OK-ish with DWARF Consumers. 
>>> Wouldn't hurt for you to take a look/see what happens in that case with a 
>>> debugger like gdb/check other cases of C name mangling to see what DWARF 
>>> they end up creating (with both GCC and Clang).
>>
>> I did a quick experiment with C name managing with GCC and -flto. It turns 
>> out the `DW_AT_linkage_name` field of `DW_TAG_subprogram` is never set for C 
>> programs. If set, the gdb debugger will use that field to match the user 
>> input and set breakpoints. Therefore, giving `DW_AT_linkage_name` a 
>> uniquefied name prevents the debugger from setting a breakpoint based on 
>> source names unless the user specifies a decorated name.
>>
>> Hence, this approach sounds like a workaround for us when the profile 
>> quality matters more than debugging experience. I'm inclined to have it 
>> under a switch. What do you think?
>
> Just a thought, we could always check if rawLinkageName is set and only set 
> it when it is not null.  That seems safe without needing the option. Not a 
> strong opinion.

Was this thread concluded? Doesn't look like any check was added?


Repository:
  rG LLVM Github 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

This change looks good to me but I would like @rnk to sign off.  My opinion is 
that the change is simple enough.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Hongtao Yu via Phabricator via cfe-commits]

hoy updated this revision to Diff 313607.
hoy added a comment.

Undoing changes to the clang test.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

Files:
  llvm/include/llvm/IR/DebugInfoMetadata.h
  llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
  llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll


Index: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
===
--- llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
+++ llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
@@ -32,5 +32,6 @@
 !17 = !{!13}
 !18 = !DILocation(line: 6, column: 3, scope: !15)
 
-; CHECK: define internal i32 @foo.__uniq.{{[0-9a-f]+}}()
+; CHECK: define internal i32 @foo.__uniq.{{[0-9a-f]+}}() !dbg ![[#DBG:]]
 ; CHECK: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
+; CHECK: ![[#DBG]] = distinct !DISubprogram(name: "foo", linkageName: 
"foo.__uniq.{{[0-9a-f]+}}"
Index: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
===
--- llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
+++ llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
@@ -13,6 +13,8 @@
 
 #include "llvm/Transforms/Utils/UniqueInternalLinkageNames.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/IR/DebugInfoMetadata.h"
+#include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Module.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Support/MD5.h"
@@ -31,11 +33,16 @@
   // this symbol is of internal linkage type.
   std::string ModuleNameHash = (Twine(".__uniq.") + Twine(Str)).str();
   bool Changed = false;
+  MDBuilder MDB(M.getContext());
 
   // Append the module hash to all internal linkage functions.
   for (auto  : M) {
 if (F.hasInternalLinkage()) {
   F.setName(F.getName() + ModuleNameHash);
+  if (DISubprogram *SP = F.getSubprogram()) {
+auto *Name = MDB.createString(F.getName());
+SP->replaceRawLinkageName(Name);
+  }
   Changed = true;
 }
   }
Index: llvm/include/llvm/IR/DebugInfoMetadata.h
===
--- llvm/include/llvm/IR/DebugInfoMetadata.h
+++ llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -2053,6 +2053,10 @@
 return getNumOperands() > 10 ? getOperandAs(10) : nullptr;
   }
 
+  void replaceRawLinkageName(MDString *LinkageName) {
+replaceOperandWith(3, LinkageName);
+  }
+
   /// Check if this subprogram describes the given function.
   ///
   /// FIXME: Should this be looking through bitcasts?


Index: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
===
--- llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
+++ llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
@@ -32,5 +32,6 @@
 !17 = !{!13}
 !18 = !DILocation(line: 6, column: 3, scope: !15)
 
-; CHECK: define internal i32 @foo.__uniq.{{[0-9a-f]+}}()
+; CHECK: define internal i32 @foo.__uniq.{{[0-9a-f]+}}() !dbg ![[#DBG:]]
 ; CHECK: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
+; CHECK: ![[#DBG]] = distinct !DISubprogram(name: "foo", linkageName: "foo.__uniq.{{[0-9a-f]+}}"
Index: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
===
--- llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
+++ llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
@@ -13,6 +13,8 @@
 
 #include "llvm/Transforms/Utils/UniqueInternalLinkageNames.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/IR/DebugInfoMetadata.h"
+#include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Module.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Support/MD5.h"
@@ -31,11 +33,16 @@
   // this symbol is of internal linkage type.
   std::string ModuleNameHash = (Twine(".__uniq.") + Twine(Str)).str();
   bool Changed = false;
+  MDBuilder MDB(M.getContext());
 
   // Append the module hash to all internal linkage functions.
   for (auto  : M) {
 if (F.hasInternalLinkage()) {
   F.setName(F.getName() + ModuleNameHash);
+  if (DISubprogram *SP = F.getSubprogram()) {
+auto *Name = MDB.createString(F.getName());
+SP->replaceRawLinkageName(Name);
+  }
   Changed = true;
 }
   }
Index: llvm/include/llvm/IR/DebugInfoMetadata.h
===
--- llvm/include/llvm/IR/DebugInfoMetadata.h
+++ llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -2053,6 +2053,10 @@
 return getNumOperands() > 10 ? getOperandAs(10) : nullptr;
   }
 
+  void replaceRawLinkageName(MDString *LinkageName) {
+replaceOperandWith(3, LinkageName);
+  }
+
   /// Check if this subprogram describes the given function.
   ///
   /// FIXME: Should this be 

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D93747#2470387 , @dblaikie wrote:

> Please remove the clang test change - if this is an LLVM change with LLVM 
> test coverage, that's enough. (we don't generally test every LLVM change from 
> both LLVM and Clang)

Sounds good.

> I'd still be curious if you could look around to see whether there are other 
> cases of function splitting/cloning/etc to see how they deal with updating 
> the DISubprograms, to see if there's some prior art that should be used here.

To the best of my knowledge, existing code does not change the linkage name 
field of a DISubprogram once created. You can create a new DISubprogram record 
with any linkage name you want but bear in mind how the debugger will consume 
the record. Looks like we are the first one to change existing record which 
will confuse the debugger.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[David Blaikie via Phabricator via cfe-commits]

dblaikie added a comment.

Please remove the clang test change - if this is an LLVM change with LLVM test 
coverage, that's enough. (we don't generally test every LLVM change from both 
LLVM and Clang)

I'd still be curious if you could look around to see whether there are other 
cases of function splitting/cloning/etc to see how they deal with updating the 
DISubprograms, to see if there's some prior art that should be used here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D93747#2470189 , @hoy wrote:

> In D93747#2470178 , @tmsriram wrote:
>
>> In D93747#2469556 , @hoy wrote:
>>
 In D93656 , @dblaikie wrote:
 Though the C case is interesting - it means you'll end up with C functions 
 with the same DWARF 'name' but different linkage name. I don't know what 
 that'll do to DWARF consumers - I guess they'll probably be OK-ish with 
 it, as much as they are with C++ overloading. I think there are some cases 
 of C name mangling so it's probably supported/OK-ish with DWARF Consumers. 
 Wouldn't hurt for you to take a look/see what happens in that case with a 
 debugger like gdb/check other cases of C name mangling to see what DWARF 
 they end up creating (with both GCC and Clang).
>>>
>>> I did a quick experiment with C name managing with GCC and -flto. It turns 
>>> out the `DW_AT_linkage_name` field of `DW_TAG_subprogram` is never set for 
>>> C programs. If set, the gdb debugger will use that field to match the user 
>>> input and set breakpoints. Therefore, giving `DW_AT_linkage_name` a 
>>> uniquefied name prevents the debugger from setting a breakpoint based on 
>>> source names unless the user specifies a decorated name.
>>>
>>> Hence, this approach sounds like a workaround for us when the profile 
>>> quality matters more than debugging experience. I'm inclined to have it 
>>> under a switch. What do you think?
>>
>> Just a thought, we could always check if rawLinkageName is set and only set 
>> it when it is not null.  That seems safe without needing the option. Not a 
>> strong opinion.
>
> It seems that the demangler of the debugger is not able to handle an 
> uniquefied name, even if the debug record originally comes with a linkage 
> name.

Yep, the demangler (which can be checked with c++filt) is very restricted on 
what comes after the "."  It can be either numbers or characters but not a mix 
of the two.  I guess we can enhance it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

In D93747#2470178 , @tmsriram wrote:

> In D93747#2469556 , @hoy wrote:
>
>>> In D93656 , @dblaikie wrote:
>>> Though the C case is interesting - it means you'll end up with C functions 
>>> with the same DWARF 'name' but different linkage name. I don't know what 
>>> that'll do to DWARF consumers - I guess they'll probably be OK-ish with it, 
>>> as much as they are with C++ overloading. I think there are some cases of C 
>>> name mangling so it's probably supported/OK-ish with DWARF Consumers. 
>>> Wouldn't hurt for you to take a look/see what happens in that case with a 
>>> debugger like gdb/check other cases of C name mangling to see what DWARF 
>>> they end up creating (with both GCC and Clang).
>>
>> I did a quick experiment with C name managing with GCC and -flto. It turns 
>> out the `DW_AT_linkage_name` field of `DW_TAG_subprogram` is never set for C 
>> programs. If set, the gdb debugger will use that field to match the user 
>> input and set breakpoints. Therefore, giving `DW_AT_linkage_name` a 
>> uniquefied name prevents the debugger from setting a breakpoint based on 
>> source names unless the user specifies a decorated name.
>>
>> Hence, this approach sounds like a workaround for us when the profile 
>> quality matters more than debugging experience. I'm inclined to have it 
>> under a switch. What do you think?
>
> Just a thought, we could always check if rawLinkageName is set and only set 
> it when it is not null.  That seems safe without needing the option. Not a 
> strong opinion.

It seems that the demangler of the debugger is not able to handle an uniquefied 
name, even if the debug record originally comes with a linkage name.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Sriraman Tallam via Phabricator via cfe-commits]

tmsriram added a comment.

In D93747#2469556 , @hoy wrote:

>> In D93656 , @dblaikie wrote:
>> Though the C case is interesting - it means you'll end up with C functions 
>> with the same DWARF 'name' but different linkage name. I don't know what 
>> that'll do to DWARF consumers - I guess they'll probably be OK-ish with it, 
>> as much as they are with C++ overloading. I think there are some cases of C 
>> name mangling so it's probably supported/OK-ish with DWARF Consumers. 
>> Wouldn't hurt for you to take a look/see what happens in that case with a 
>> debugger like gdb/check other cases of C name mangling to see what DWARF 
>> they end up creating (with both GCC and Clang).
>
> I did a quick experiment with C name managing with GCC and -flto. It turns 
> out the `DW_AT_linkage_name` field of `DW_TAG_subprogram` is never set for C 
> programs. If set, the gdb debugger will use that field to match the user 
> input and set breakpoints. Therefore, giving `DW_AT_linkage_name` a 
> uniquefied name prevents the debugger from setting a breakpoint based on 
> source names unless the user specifies a decorated name.
>
> Hence, this approach sounds like a workaround for us when the profile quality 
> matters more than debugging experience. I'm inclined to have it under a 
> switch. What do you think?

Just a thought, we could always check if rawLinkageName is set and only set it 
when it is not null.  That seems safe without needing the option. Not a strong 
opinion.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Hongtao Yu via Phabricator via cfe-commits]

hoy added a comment.

> In D93656 , @dblaikie wrote:
> Though the C case is interesting - it means you'll end up with C functions 
> with the same DWARF 'name' but different linkage name. I don't know what 
> that'll do to DWARF consumers - I guess they'll probably be OK-ish with it, 
> as much as they are with C++ overloading. I think there are some cases of C 
> name mangling so it's probably supported/OK-ish with DWARF Consumers. 
> Wouldn't hurt for you to take a look/see what happens in that case with a 
> debugger like gdb/check other cases of C name mangling to see what DWARF they 
> end up creating (with both GCC and Clang).

I did a quick experiment with C name managing with GCC and -flto. It turns out 
the `DW_AT_linkage_name` field of `DW_TAG_subprogram` is never set for C 
programs. If set, the gdb debugger will use that field to match the user input 
and set breakpoints. Therefore, giving `DW_AT_linkage_name` a uniquefied name 
prevents the debugger from setting a breakpoint based on source names unless 
the user specifies a decorated name.

Hence, this approach sounds like a workaround for us when the profile quality 
matters more than debugging experience. I'm inclined to have it under a switch. 
What do you think?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93747

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

[PATCH] D93747: Rename debug linkage name with -funique-internal-linkage-names

[Hongtao Yu via Phabricator via cfe-commits]

hoy created this revision.
Herald added subscribers: dexonsmith, wenlei, hiraditya.
hoy requested review of this revision.
Herald added projects: clang, LLVM.
Herald added subscribers: llvm-commits, cfe-commits.

Funtions that are renamed under -funique-internal-linkage-names have their 
debug linkage name updated as well.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D93747

Files:
  clang/test/CodeGen/unique-internal-linkage-names.cpp
  llvm/include/llvm/IR/DebugInfoMetadata.h
  llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
  llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll


Index: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
===
--- llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
+++ llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
@@ -32,5 +32,6 @@
 !17 = !{!13}
 !18 = !DILocation(line: 6, column: 3, scope: !15)
 
-; CHECK: define internal i32 @foo.__uniq.{{[0-9a-f]+}}()
+; CHECK: define internal i32 @foo.__uniq.{{[0-9a-f]+}}() !dbg ![[#DBG:]]
 ; CHECK: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
+; CHECK: ![[#DBG]] = distinct !DISubprogram(name: "foo", linkageName: 
"foo.__uniq.{{[0-9a-f]+}}"
Index: llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
===
--- llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
+++ llvm/lib/Transforms/Utils/UniqueInternalLinkageNames.cpp
@@ -13,6 +13,8 @@
 
 #include "llvm/Transforms/Utils/UniqueInternalLinkageNames.h"
 #include "llvm/ADT/SmallString.h"
+#include "llvm/IR/DebugInfoMetadata.h"
+#include "llvm/IR/MDBuilder.h"
 #include "llvm/IR/Module.h"
 #include "llvm/InitializePasses.h"
 #include "llvm/Support/MD5.h"
@@ -31,11 +33,16 @@
   // this symbol is of internal linkage type.
   std::string ModuleNameHash = (Twine(".__uniq.") + Twine(Str)).str();
   bool Changed = false;
+  MDBuilder MDB(M.getContext());
 
   // Append the module hash to all internal linkage functions.
   for (auto  : M) {
 if (F.hasInternalLinkage()) {
   F.setName(F.getName() + ModuleNameHash);
+  if (DISubprogram *SP = F.getSubprogram()) {
+auto *Name = MDB.createString(F.getName());
+SP->replaceRawLinkageName(Name);
+  }
   Changed = true;
 }
   }
Index: llvm/include/llvm/IR/DebugInfoMetadata.h
===
--- llvm/include/llvm/IR/DebugInfoMetadata.h
+++ llvm/include/llvm/IR/DebugInfoMetadata.h
@@ -2053,6 +2053,10 @@
 return getNumOperands() > 10 ? getOperandAs(10) : nullptr;
   }
 
+  void replaceRawLinkageName(MDString *LinkageName) {
+replaceOperandWith(3, LinkageName);
+  }
+
   /// Check if this subprogram describes the given function.
   ///
   /// FIXME: Should this be looking through bitcasts?
Index: clang/test/CodeGen/unique-internal-linkage-names.cpp
===
--- clang/test/CodeGen/unique-internal-linkage-names.cpp
+++ clang/test/CodeGen/unique-internal-linkage-names.cpp
@@ -5,6 +5,7 @@
 // RUN: %clang_cc1 -triple x86_64 -x c++ -O1 -S -emit-llvm 
-funique-internal-linkage-names -o - < %s | FileCheck %s --check-prefix=UNIQUEO1
 // RUN: %clang_cc1 -triple x86_64 -x c++ -O0 -S -emit-llvm 
-fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | 
FileCheck %s --check-prefix=UNIQUE
 // RUN: %clang_cc1 -triple x86_64 -x c++ -O1 -S -emit-llvm 
-fexperimental-new-pass-manager -funique-internal-linkage-names -o - < %s | 
FileCheck %s --check-prefix=UNIQUEO1
+// RUN: %clang_cc1 -triple x86_64 -x c++ -O2 -S -emit-llvm 
-fexperimental-new-pass-manager -funique-internal-linkage-names 
-debug-info-kind=limited -o - < %s | FileCheck %s --check-prefix=UNIQUEDEBUG
 
 static int glob;
 static int foo() {
@@ -65,3 +66,5 @@
 // UNIQUEO1: define weak_odr i32 ()* @_ZL4mverv.resolver()
 // UNIQUEO1: define internal i32 @_ZL4mverv.__uniq.{{[0-9a-f]+}}()
 // UNIQUEO1: define internal i32 @_ZL4mverv.sse4.2.__uniq.{{[0-9a-f]+}}
+// UNIQUEDEBUG: define internal i32 @_ZL3foov.__uniq.{{[0-9a-f]+}}() 
[[ATTR:#[0-9]+]] !dbg ![[#DBG:]]
+// UNIQUEDEBUG: ![[#DBG]] = distinct !DISubprogram(name: "foo", linkageName: 
"_ZL3foov.__uniq.{{[0-9a-f]+}}"


Index: llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
===
--- llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
+++ llvm/test/Transforms/UniqueLinkageNames/unique-internal-linkage-names.ll
@@ -32,5 +32,6 @@
 !17 = !{!13}
 !18 = !DILocation(line: 6, column: 3, scope: !15)
 
-; CHECK: define internal i32 @foo.__uniq.{{[0-9a-f]+}}()
+; CHECK: define internal i32 @foo.__uniq.{{[0-9a-f]+}}() !dbg ![[#DBG:]]
 ; CHECK: ret {{.*}} @foo.__uniq.{{[0-9a-f]+}} {{.*}}
+; CHECK: ![[#DBG]] = distinct !DISubprogram(name: "foo",