Before this patch, global variables didn't have their namespace prepended in the Codeview debug symbol stream. This prevented Visual Studio from displaying them in the debugger (they appeared as 'unspecified error')
Details
Details
Diff Detail
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Comment Actions
lgtm
| llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp | ||
|---|---|---|
| 3146 | Looks like @akhuang got this right recently for S_CONSTANT, but we've had the bug with global names since forever. :( | |
| llvm/test/DebugInfo/COFF/globals.ll | ||
| 145 | I'm not sure why we check the relocations here in the first place. They use offsets, so they tend to be fragile. I think we ought to remove them from this test. We have enough coverage from the ASM check lines above. As long as we have confidence that .secrel and .secidx directives work and are tested elsewhere, this seems redundant. | |
Comment Actions
Fix local thread_local which shouldn't generate a scoped name in S_LTHREAD32.
| llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp | ||
|---|---|---|
| 3146 | I've updated the code above a bit. There was an edge case with S_LTHREAD32 which needed to emit a unscoped name. Could you please take a quick look again? | |
Comment Actions
I'm noting by the way that extern thread_local int A; does not generate a debug symbol :( I haven't checked why yet.
| llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp | ||
|---|---|---|
| 3116 | I think this isn't quite right. Consider: $ cat t.cpp namespace foo { thread_local int global_tls; int global_int() { return ++global_tls; } static thread_local int static_tls; int static_int() { return ++static_tls; } int func_int() { thread_local int func_tls; return ++func_tls; } int func_static_int() { static int func_static; return ++func_static; } } $ cl -c t.cpp -Z7 && llvm-pdbutil dump -symbols t.obj | grep 'THREA\|DATA' Microsoft (R) C/C++ Optimizing Compiler Version 19.25.28612 for x64 Copyright (C) Microsoft Corporation. All rights reserved. t.cpp 0 | S_LTHREAD32 [size = 23] `func_tls` 0 | S_LDATA32 [size = 26] `func_static` 0 | S_GTHREAD32 [size = 30] `foo::global_tls` 0 | S_LTHREAD32 [size = 30] `foo::static_tls` It's function local TLS that shouldn't receive a fully qualified name. Let's handle that separately. Those are probably less common. I'm OK committing the previous patchset and handling function local globals separately. To fix function local globals (thread local or otherwise), we have to walk up the DIGV scope and see if any of them is a subprogram. We have some existing logic for this for UDTs (typedefs). | |
Comment Actions
Thanks @amccarth ! Would you mind taking a second look please? The stream now matches MSVC behavior.
| llvm/lib/CodeGen/AsmPrinter/CodeViewDebug.cpp | ||
|---|---|---|
| 3116 | I've updated to only handle namespaces. I'll write a follow-up patch to fix the subprogram prefix after landing this. I've also noted that statics inside a class don't seem to generate the right qualifier either in the debug stream: $ cat a.cpp struct Data { inline static thread_local int DataStaticTLS = 5; inline static int DataGlobalStatic = 7; }; int bar() { foo::Data D; return D.DataStaticTLS + D.DataGlobalStatic; } $ cl /Z7 /GS- /c a.cpp /Od /std:c++17 && link /DEBUG /INCREMENTAL:NO a.obj /NODEFAULTLIB /FORCE /ENTRY:bar /SUBSYSTEM:console && llvm-pdbutil.exe dump --all a.pdb | grep S_G.*Data 132 | S_GTHREAD32 [size = 36] `Data::DataStaticTLS` 168 | S_GDATA32 [size = 40] `Data::DataGlobalStatic` With clang-cl (this patch applied): 132 | S_GTHREAD32 [size = 28] `DataStaticTLS` 160 | S_GDATA32 [size = 32] `DataGlobalStatic` | |
I think this isn't quite right. Consider:
It's function local TLS that shouldn't receive a fully qualified name. Let's handle that separately. Those are probably less common. I'm OK committing the previous patchset and handling function local globals separately.
To fix function local globals (thread local or otherwise), we have to walk up the DIGV scope and see if any of them is a subprogram. We have some existing logic for this for UDTs (typedefs).