[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

tra added a comment. This patch appears to break CUDA compilation: https://github.com/llvm/llvm-project/issues/58491 We apparently emit the symbol with a character (`.`) that can't be used on the target. Normally such characters get renamed/escaped, as you can see in the generated function nam

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

dblaikie added a comment. Might be worth a note in the Release Notes given that it's at least caused issues with one DWARF consumer. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123534/new/ https://reviews.llvm.org/D123534 __

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim added a comment. In D123534#3525533 , @paulkirth wrote: > We noticed when building Fuchsia using ToT LLVM that CGo has trouble with the > new changes to DWARF introduced in this patch. It appears as if CGo is > confused because it found a `DW_TAG_

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

paulkirth added a comment. We noticed when building Fuchsia using ToT LLVM that CGo has trouble with the new changes to DWARF introduced in this patch. It appears as if CGo is confused because it found a `DW_TAG_variable` that doesn't have a name, which is allowed by the DWARF standard. This

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG7aa1fa0a0a07: Reland "[dwarf] Emit a DIGlobalVariable for constant strings." (authored by hctim). Repository: rG LLVM Github Monorepo CHANGES SIN

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim updated this revision to Diff 430494. hctim added a comment. Fix the fuchsia windows bot by making the DILocalVariables in the test order-independent. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123534/new/ https://reviews.llvm.org/D123534

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim added a comment. Why, on Fuchsia, on WIndows, the DIs are in the order `q -> p -> r` is beyond me (instead of the file order and my order `p -> q -> r`. Looking into it. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123534/new/ https://revie

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

paulkirth added a comment. Thanks for the quick response. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123534/new/ https://reviews.llvm.org/D123534 ___ cfe-commits mailing list cfe-commits@lists.llvm.or

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim added a comment. Done, I'll take a look and resubmit later. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123534/new/ https://reviews.llvm.org/D123534 ___ cfe-commits mailing list cfe-commits@lists

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

paulkirth added a comment. Hi, we're seeing a breakage in Fuchsia's clang CI for x64 windows that I think is related to this patch. https://luci-milo.appspot.com/ui/p/fuchsia/builders/toolchain.ci/clang-windows-x64/b8813962058917346337/overview We see a test failure in Clang :: CodeGen/debug-in

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG4680982b36a8: [dwarf] Emit a DIGlobalVariable for constant strings. (authored by hctim). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST A

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

dblaikie accepted this revision. dblaikie added a comment. Seems good - if folks find the growth too much, or it creates weird perf issues otherwise, we can discuss that if/when it comes up. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123534/new/

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim updated this revision to Diff 429799. hctim added a comment. Touch ups from comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123534/new/ https://reviews.llvm.org/D123534 Files: clang/lib/CodeGen/CGDebugInfo.cpp clang/lib/CodeGen/C

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

rnk accepted this revision. rnk added a comment. This revision is now accepted and ready to land. Looks good to me. I generally agree with Adrian's suggestions. Comment at: llvm/test/DebugInfo/COFF/global-no-strings.ll:1 +; RUN: llc < %s | FileCheck %s +; RUN: llc < %s -filetyp

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

aprantl added a comment. I think this mostly looks good. I left comment about the test inline. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123534/new/ https://reviews.llvm.org/D123534 ___ cfe-commits m

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

aprantl added inline comments. Comment at: clang/lib/CodeGen/CGDebugInfo.h:536 + /// Create and attach debuginfo to a the provided string literal GV. + void AddStringLiteralDebugInfo(llvm::GlobalVariable *GV, StringRef Name, I would appreciate a comment expla

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim updated this revision to Diff 429276. hctim added a comment. Remove test based on removed invariant (DIGlobalVariables no longer need a name). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123534/new/ https://reviews.llvm.org/D123534 Files:

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim updated this revision to Diff 429121. hctim added a comment. Herald added a subscriber: ormris. update patch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123534/new/ https://reviews.llvm.org/D123534 Files: clang/lib/CodeGen/CGDebugInfo.cp

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim added a comment. In D123534#3509667 , @rnk wrote: > That's fine, a Windows machine shouldn't be necessary. This patch has to > change the AsmPrinter for DWARF, so it makes sense that it should do the same > for CodeView. I think it should be suffi

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

rnk added a comment. In D123534#3507891 , @hctim wrote: > Not sure about PDB. I did run a quick test with `gdb`, and very > unscientifically, didn't notice any difference in usability or errors between > pre- and post-this patch on a `clang` invocation

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim added a comment. In D123534#3504789 , @rnk wrote: > This seems reasonable, but I worry about the consequences of creating lots of > unnamed global variables. What will gdb do with so many unnamed globals? What > will the PDB linker do with all the

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

rnk added a comment. This seems reasonable, but I worry about the consequences of creating lots of unnamed global variables. What will gdb do with so many unnamed globals? What will the PDB linker do with all these unnamed globals? I can't answer these questions, and you're welcome to try and f

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim added a comment. In D123534#3489262 , @dblaikie wrote: > @rnk @aprantl @probinson: this looks plausible to me to take as a general > change to the emitted DWARF, but I'm open to ideas/suggestions if folks find > this to be too much growth for not

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

dblaikie added subscribers: aprantl, rnk. dblaikie added a comment. Tried this patch on some internal targets and metrics and it seems actually quite reasonable. Tested with split DWARF, on Linux, with compressed debug info in .o/.dwo files, uncompressed in exe/dwp files. With -O0 and -O3, roug

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim updated this revision to Diff 426175. hctim added a comment. (restoring after uploading the diff for D123538 here accidentally) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123534/new/ https://reviews.llvm

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim updated this revision to Diff 426172. hctim marked an inline comment as done. hctim added a comment. Herald added subscribers: rupprecht, MaskRay, emaste. Herald added a reviewer: jhenderson. Update test, rebase off of string debuginfo. Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

dblaikie added a comment. In D123534#3475790 , @hctim wrote: >> summary of DWARF: >> & how many of these descriptions get added to the debug info? > > afaict, there is now: > 1x .debug_addr entry for each string > 1x. debug_info DW_TAG_variable for each

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim marked an inline comment as done. hctim added inline comments. Comment at: clang/test/CodeGen/debug-info-variables.c:1 +// RUN: %clang_cc1 %s -debug-info-kind=standalone -S -emit-llvm -o - | FileCheck %s + dblaikie wrote: > Presumably named variables are a

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim added a comment. > summary of DWARF: > & how many of these descriptions get added to the debug info? afaict, there is now: 1x .debug_addr entry for each string 1x. debug_info DW_TAG_variable for each string 1x. DW_TAG_array_type + DW_TAG_subrange_type for each unique sizeof(string) i tr

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

dblaikie added a comment. Size still seems moderately concerning, but might be acceptable. Got a summary of what the DWARF looks like now (without names)? (maybe there's something else we can strip/optimize) & how many of these descriptions get added to the debug info? Numbers for Split DWARF

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim added a comment. I just did an experiment with the following patch that punts everything to `const char*` types to try and get better folding onto a single type: The change didn't make a significant difference, with the clang binary size of 1263433512 (126384B reduction, or 1.224% -> 1.21

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim added a comment. 1263559896 (new) - 1248280328 (old) = 15279568 B (1.22% increase). Seems more reasonable to me. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123534/new/ https://reviews.llvm.org/D123534

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim added a comment. Yeah, as I drove away from my desk I realized my mistake. Will get new accurate numbers. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123534/new/ https://reviews.llvm.org/D123534 ___

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim added a comment. Confirmed again with `-DCMAKE_BUILD_TYPE=RelWithDebInfo -DLLVM_ENABLE_ASSERTIONS=On`: 1510883864 (new) - 1510881800 (old) = 2064 bytes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123534/new/ https://reviews.llvm.org/D123

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim updated this revision to Diff 424055. hctim added a comment. Herald added subscribers: llvm-commits, hiraditya. Herald added a project: LLVM. Patch update. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D123534/new/ https://reviews.llvm.org/D12

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim added a comment. In D123534#3462551 , @dblaikie wrote: > What's the DW_AT_type referring to? (why is there a type called ".str"?) > > For your particular purposes, it seems like the name/type/linkage name could > all be omitted & that might be OK,

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

probinson added a comment. > What's the DW_AT_type referring to? (why is there a type called ".str"?) I'd expect it to point to a base type, in this case character string. A typeless variable with a location would be pretty weird. But I agree the name/linkage_name would be unnecessary. Reposi

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

dblaikie added a comment. In D123534#3454749 , @hctim wrote: > In D123534#3454354 , @dblaikie > wrote: > >> This seems like it would significantly introduce debug info size for at >> least some kinds of code - h

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim added a comment. section : increase in bytes for clang built with full debuginfo : %% of total binary size .debug_loclists 317782 0.0250% .debug_abbrev 88590 0.0070% .debug_info 100708340.7929% .debug_rnglists

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim added a comment. In D123534#3454354 , @dblaikie wrote: > This seems like it would significantly introduce debug info size for at least > some kinds of code - have you done any size measurements of this change? With `-DCMAKE_BUILD_TYPE=RelWithDebIn

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

dblaikie added a comment. This seems like it would significantly introduce debug info size for at least some kinds of code - have you done any size measurements of this change? What does the resulting DWARF look like? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://r

[PATCH] D123534: [dwarf] Emit a DIGlobalVariable for constant strings.

hctim created this revision. hctim added reviewers: echristo, cmtice. Herald added a project: All. hctim requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. An upcoming patch will extend llvm-symbolizer to provide the source line information fo