This is an archive of the discontinued LLVM Phabricator instance.

[LLD][COFF] Move debug info for thread-local variables into PDB global stream
ClosedPublic

Authored by aganea on Apr 28 2020, 7:17 AM.

Details

Summary

Before this patch, the debug record S_GTHREAD32 which represents global thread_local symbols, was emitted by LLD into the respective module stream. This makes Visual Studio unable to display thread_local symbols in the debugger.

After this patch, S_GTHREAD32 is moved into the globals stream. This matches MSVC behavior.

Diff Detail

Event Timeline

aganea created this revision.Apr 28 2020, 7:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 28 2020, 7:17 AM

This seems to make sense.

I see you changed the test from a 32-bit one to a 64-bit one, which seems to be the bulk of this patch. Is bitness relevant to the problem being solved? Do we know thread-local variables are in the globals stream regardless of bitness?

rnk added inline comments.Apr 29 2020, 3:22 PM
lld/test/COFF/Inputs/pdb-globals.yaml
30

Do you mind checking what S_LTHREAD32 does? You probably just have to make a static thread local var.

aganea updated this revision to Diff 261312.Apr 30 2020, 12:08 PM
aganea marked an inline comment as done.
  • Generate yaml with the 32-bit toolchain to reduce variability.
  • Added local thread_local to cover S_LTHREAD32.

Is bitness relevant to the problem being solved? Do we know thread-local variables are in the globals stream regardless of bitness?

In this case, the globals stream is the same for 32-bit and 64-bit builds (except maybe for file offsets).

rnk added inline comments.Apr 30 2020, 1:13 PM
lld/COFF/PDB.cpp
806

S_LTHREAD32 doesn't follow this pattern? I was kind of expecting it to show up wherever S_LDATA32 is.

lld/test/COFF/Inputs/pdb-globals.yaml
44

Huh, static thread_local int at global scope doesn't do it? And these don't show up in the PDB's global stream?

rnk added inline comments.Apr 30 2020, 1:31 PM
lld/COFF/PDB.cpp
805

Looks like we need to send S_LDATA32 to here.

lld/test/COFF/Inputs/pdb-globals.yaml
44

The example I came up with probably makes a better test case here, and we can handle function local globals (thread local or regular globals) in a follow-up.

$ 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 -Z7 t.cpp && link -dll -force t.obj  -debug && llvm-pdbutil dump -globals t.pdb | grep S_[LG]THREAD
Microsoft (R) C/C++ Optimizing Compiler Version 19.25.28612 for x64
Copyright (C) Microsoft Corporation.  All rights reserved.

t.cpp
Microsoft (R) Incremental Linker Version 14.25.28612.0
Copyright (C) Microsoft Corporation.  All rights reserved.

     384 | S_LTHREAD32 [size = 32] `foo::static_tls`
     352 | S_GTHREAD32 [size = 32] `foo::global_tls`

I think we need to reuse the local S_UDT logic here, and return isGlobalScope from symbolGoesInGlobalsStream. Perhaps S_LDATA32 needs the same treatment.

aganea updated this revision to Diff 261551.May 1 2020, 2:37 PM
aganea marked 6 inline comments as done.
aganea added inline comments.
lld/COFF/PDB.cpp
805

Not exactly. S_LDATA32 always remains in the module stream, but is duplicated into the globals stream only if it's a global.

806

It is indeed following the same pattern!

aganea added a comment.May 5 2020, 6:24 AM

Ping! Is there anything else to be done here?

amccarth accepted this revision.May 5 2020, 9:16 AM

LGTM, but I think we should hear again from rnk before landing this.

This revision is now accepted and ready to land.May 5 2020, 9:16 AM
rnk accepted this revision.May 5 2020, 4:09 PM

lgtm with a comment update, thanks!

lld/COFF/PDB.cpp
805

Ah, yes, this comment should've been in symbolGoesInGlobalsStream.

828

UDTs aren't global variables. I'd change the qualification to "unless they are function-local". At the end of the day, the linker only knows if the record is inside a matched pair of S_[LG]PROC32 / S_END records, i.e. function local. Maybe isGlobalScope should be renamed to isFunctionScope and inverted.

This revision was automatically updated to reflect the committed changes.