This is an archive of the discontinued LLVM Phabricator instance.

Support Debugging TLS variable with lldb
Needs ReviewPublic

Authored by kkcode0 on Feb 9 2023, 7:40 PM.

Details

Summary

Implement qGetTLSAddr for accessing thread pointer and utilises existing facility to calculate TLS variable address (GetThreadLocalData in DYLD).

Diff Detail

Event Timeline

kkcode0 created this revision.Feb 9 2023, 7:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2023, 7:40 PM
kkcode0 requested review of this revision.Feb 9 2023, 7:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 9 2023, 7:40 PM
kkcode0 updated this revision to Diff 496321.Feb 9 2023, 7:44 PM

rebase all changes

kkcode0 updated this revision to Diff 496347.Feb 9 2023, 11:52 PM

mark some vars used

kkcode0 updated this revision to Diff 496360.Feb 10 2023, 12:51 AM

rebased with main

kkcode0 retitled this revision from Cleanedup and added comments to Support Debugging TLS variable with lldb.Feb 10 2023, 12:55 AM
kkcode0 edited the summary of this revision. (Show Details)
kkcode0 added reviewers: labath, richard.mitton.
kkcode0 updated this revision to Diff 496392.Feb 10 2023, 2:40 AM

enabled the tls test

A tiny quibble: GetThreadPointer seems like it should return a pointer to the thread that has this register set. I would not guess it's a pointer to the Thread's local storage. Maybe GetThreadLSPointer + a doc string saying what it does would clear this up?

kkcode0 updated this revision to Diff 496654.Feb 10 2023, 9:01 PM

added comments

kkcode0 updated this revision to Diff 496655.Feb 10 2023, 9:03 PM

added comments

There's one thing that's not clear to me about this patch. What is the relationship between qGetTLSAddr, as implemented here, and its implementation in GDB? I guess they're not compatible since we're not sending or using the offset and link map fields, but it's not clear to me whether we're at least implementing some subset of gdb functionality (e.g., what would gdbserver do if it received one of the packets that we're sending here). If we're not, then I don't think we should be using the same packet for this purpose.

lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
186–193

How is this related to the rest of the patch?

196

revert whitespace

767

I'm not sure what these marks are (tabs maybe), but could you figure out how to revert them?

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
116–119

Is this necessary, given that the function is already stubbed out in NativeRegisterContext?

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h
85

Return llvm::Expected<addr_t> perhaps?

lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp
380

Any reason to not use the PtraceWrapper function?

lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp
2123–2124

Might as well not bother parsing them, if they're going to be thrown away anyway.

lldb/source/Target/Thread.cpp
1627–1629

Could this be implemented in ThreadGDBRemote directly (without going through the GDBRemoteRegisterContext)?

kkcode0 updated this revision to Diff 555572.Sep 2 2023, 3:12 AM

formatting

kkcode0 updated this revision to Diff 555573.Sep 2 2023, 3:20 AM

formatting

kkcode0 updated this revision to Diff 555574.Sep 2 2023, 3:36 AM
kkcode0 marked an inline comment as done.

formatting

kkcode0 updated this revision to Diff 555575.Sep 2 2023, 3:38 AM

formatting

kkcode0 updated this revision to Diff 555591.Sep 2 2023, 6:59 AM
kkcode0 marked 3 inline comments as done.

address review comments

kkcode0 updated this revision to Diff 555592.Sep 2 2023, 7:02 AM
kkcode0 marked an inline comment as done.

spacing

kkcode0 added inline comments.Sep 2 2023, 7:03 AM
lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp
116–119

Thanks, it can be removed.

lldb/source/Target/Thread.cpp
1627–1629

Yes, Thanks moved it.

kkcode0 added inline comments.Sep 2 2023, 8:56 AM
lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
186–193

LoadVDSO is redundant , it is called via ProbeEntry->EntryBreakpointHit->LoadAllCurrentModules->LoadVDSO

kkcode0 updated this revision to Diff 555625.Sep 3 2023, 1:57 AM
kkcode0 marked an inline comment as done.

use ptracewrapper