[Lldb-commits] [PATCH] D143698: Support Debugging TLS variable with lldb

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.



================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:186-193
-
-    if (!SetRendezvousBreakpoint()) {
-      // If we cannot establish rendezvous breakpoint right now we'll try again
-      // at entry point.
-      ProbeEntry();
-    }
-
----------------
How is this related to the rest of the patch?


================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:196
                                                   addr_t base_addr,
-                                                  bool base_addr_is_offset) {
+                                                  bool base_addr_is_offset) {             
   m_loaded_modules[module] = link_map_addr;
----------------
revert whitespace


================
Comment at: lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:731
 
-  addr_t link_map = it->second;
-  if (link_map == LLDB_INVALID_ADDRESS)
-    return LLDB_INVALID_ADDRESS;
-
-  const DYLDRendezvous::ThreadInfo &metadata = m_rendezvous.GetThreadInfo();
-  if (!metadata.valid)
-    return LLDB_INVALID_ADDRESS;
-
-  // Get the thread pointer.
-  addr_t tp = thread->GetThreadPointer();
-  if (tp == LLDB_INVALID_ADDRESS)
-    return LLDB_INVALID_ADDRESS;
-
-  // Find the module's modid.
-  int modid_size = 4; // FIXME(spucci): This isn't right for big-endian 64-bit
-  int64_t modid = ReadUnsignedIntWithSizeInBytes(
-      link_map + metadata.modid_offset, modid_size);
-  if (modid == -1)
-    return LLDB_INVALID_ADDRESS;
-
-  // Lookup the DTV structure for this thread.
-  addr_t dtv_ptr = tp + metadata.dtv_offset;
-  addr_t dtv = ReadPointer(dtv_ptr);
-  if (dtv == LLDB_INVALID_ADDRESS)
-    return LLDB_INVALID_ADDRESS;
-
-  // Find the TLS block for this module.
-  addr_t dtv_slot = dtv + metadata.dtv_slot_size * modid;
-  addr_t tls_block = ReadPointer(dtv_slot + metadata.tls_offset);
-
-  Log *log = GetLog(LLDBLog::DynamicLoader);
-  LLDB_LOGF(log,
-            "DynamicLoaderPOSIXDYLD::Performed TLS lookup: "
-            "module=%s, link_map=0x%" PRIx64 ", tp=0x%" PRIx64
-            ", modid=%" PRId64 ", tls_block=0x%" PRIx64 "\n",
-            module_sp->GetObjectName().AsCString(""), link_map, tp,
-            (int64_t)modid, tls_block);
-
-  if (tls_block == LLDB_INVALID_ADDRESS)
-    return LLDB_INVALID_ADDRESS;
-  else
-    return tls_block + tls_file_addr;
+   addr_t link_map = it->second;
+   if (link_map == LLDB_INVALID_ADDRESS)
----------------
I'm not sure what these marks are (tabs maybe), but could you figure out how to revert them?


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.cpp:114-117
+  tp = LLDB_INVALID_ADDRESS;
+  Status error;
+  return error;
+}
----------------
Is this necessary, given that the function is already stubbed out in `NativeRegisterContext`?


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux.h:85
 
+  virtual Status ReadThreadPointer(uint64_t &tp) override;
+
----------------
Return `llvm::Expected<addr_t>` perhaps?


================
Comment at: lldb/source/Plugins/Process/Linux/NativeRegisterContextLinux_x86_64.cpp:379
+  errno = 0;
+  auto ret= ptrace(static_cast<__ptrace_request>(PTRACE_ARCH_PRCTL), m_thread.GetID(),
+                   &tp, (void *)ARCH_GET_FS, 0);
----------------
Any reason to not use the `PtraceWrapper` function?


================
Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:2125-2126
+  lldb::tid_t tid =  StringExtractor(argv[0]).GetU64(LLDB_INVALID_ADDRESS, 16);
+  lldb::addr_t offset = StringExtractor(argv[1]).GetU64(LLDB_INVALID_ADDRESS, 16);
+  lldb::addr_t lm = StringExtractor(argv[2]).GetU64(LLDB_INVALID_ADDRESS, 16);
+  (void) offset;
----------------
Might as well not bother parsing them, if they're going to be thrown away anyway.


================
Comment at: lldb/source/Target/Thread.cpp:1651-1653
+  RegisterContext *reg_ctx = this->GetRegisterContext().get();
+  auto tp = reg_ctx->GetThreadPointer();
+  return tp;
----------------
Could this be implemented in ThreadGDBRemote directly (without going through the GDBRemoteRegisterContext)?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D143698