Skip to content

[LLDB] Enable TLS Variable Debugging Without Location Info on AArch64 - #110822

Closed
kamleshbhalui wants to merge 1 commit into
llvm:mainfrom
kamleshbhalui:kk/lldb_tls_fix_without_loc
Closed

kamleshbhalui wants to merge 1 commit into
llvm:mainfrom
kamleshbhalui:kk/lldb_tls_fix_without_loc

Conversation

@kamleshbhalui

Copy link
Copy Markdown
Contributor

On AArch64, TLS variables do not have DT_Location entries generated in the debug information due to the lack of dtpoff relocation support, unlike on x86_64. LLDB relies on this location info to calculate the TLS address, leading to issues when debugging TLS variables on AArch64.
However, GDB can successfully calculate the TLS address without relying on this debug info, by using the symbol’s address and manually calculating the offset. We adopt a similar approach for LLDB.

Fixes #71666

@llvmbot llvmbot added the lldb label Oct 2, 2024
@kamleshbhalui
kamleshbhalui requested review from DavidSpickett and labath and removed request for JDevlieghere October 2, 2024 10:45
@kamleshbhalui kamleshbhalui self-assigned this Oct 2, 2024
@llvmbot

llvmbot commented Oct 2, 2024

Copy link
Copy Markdown
Member

@llvm/pr-subscribers-lldb

Author: Kamlesh Kumar (kamleshbhalui)

Changes

On AArch64, TLS variables do not have DT_Location entries generated in the debug information due to the lack of dtpoff relocation support, unlike on x86_64. LLDB relies on this location info to calculate the TLS address, leading to issues when debugging TLS variables on AArch64.
However, GDB can successfully calculate the TLS address without relying on this debug info, by using the symbol’s address and manually calculating the offset. We adopt a similar approach for LLDB.

Fixes #71666


Full diff: https://github.com/llvm/llvm-project/pull/110822.diff

8 Files Affected:

  • (modified) lldb/include/lldb/Symbol/Variable.h (+2)
  • (modified) lldb/source/Core/ValueObjectVariable.cpp (+32-1)
  • (modified) lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp (+10-4)
  • (modified) lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp (+7-6)
  • (modified) lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h (+3-2)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp (+2-3)
  • (modified) lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp (+8)
  • (modified) lldb/source/Symbol/Variable.cpp (+11)
diff --git a/lldb/include/lldb/Symbol/Variable.h b/lldb/include/lldb/Symbol/Variable.h
index c437624d1ea6d7..4ad06baeb684a4 100644
--- a/lldb/include/lldb/Symbol/Variable.h
+++ b/lldb/include/lldb/Symbol/Variable.h
@@ -79,6 +79,8 @@ class Variable : public UserID, public std::enable_shared_from_this<Variable> {
     return m_location_list;
   }
 
+  bool IsThreadLocal() const;
+
   // When given invalid address, it dumps all locations. Otherwise it only dumps
   // the location that contains this address.
   bool DumpLocations(Stream *s, const Address &address);
diff --git a/lldb/source/Core/ValueObjectVariable.cpp b/lldb/source/Core/ValueObjectVariable.cpp
index 29aefb270c92c8..393ddc1960ab47 100644
--- a/lldb/source/Core/ValueObjectVariable.cpp
+++ b/lldb/source/Core/ValueObjectVariable.cpp
@@ -254,7 +254,38 @@ bool ValueObjectVariable::UpdateValue() {
       m_resolved_value.SetContext(Value::ContextType::Invalid, nullptr);
     }
   }
-
+  if (m_error.Fail() && variable->IsThreadLocal()) {
+    ExecutionContext exe_ctx(GetExecutionContextRef());
+    Thread *thread = exe_ctx.GetThreadPtr();
+    lldb::ModuleSP module_sp = GetModule();
+    if (!thread) {
+      m_error = Status::FromErrorString("no thread to evaluate TLS within");
+      return m_error.Success();
+    }
+    std::vector<uint32_t> symbol_indexes;
+    module_sp->GetSymtab()->FindAllSymbolsWithNameAndType(
+        ConstString(variable->GetName()), lldb::SymbolType::eSymbolTypeAny,
+        symbol_indexes);
+    Symbol *symbol = module_sp->GetSymtab()->SymbolAtIndex(symbol_indexes[0]);
+    lldb::addr_t tls_file_addr =
+        symbol->GetAddress().GetOffset() +
+        symbol->GetAddress().GetSection()->GetFileAddress();
+    const lldb::addr_t tls_load_addr =
+        thread->GetThreadLocalData(module_sp, tls_file_addr);
+    if (tls_load_addr == LLDB_INVALID_ADDRESS)
+      m_error = Status::FromErrorString(
+          "no TLS data currently exists for this thread");
+    else {
+      Value old_value(m_value);
+      m_value.GetScalar() = tls_load_addr;
+      m_value.SetContext(Value::ContextType::Variable, variable);
+      m_value.SetValueType(Value::ValueType::LoadAddress);
+      m_error = m_value.GetValueAsData(&exe_ctx, m_data, GetModule().get());
+      SetValueDidChange(m_value.GetValueType() != old_value.GetValueType() ||
+                        m_value.GetScalar() != old_value.GetScalar());
+      SetValueIsValid(m_error.Success());
+    }
+  }
   return m_error.Success();
 }
 
diff --git a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
index 51e4b3e6728f23..5a78ad2286f87a 100644
--- a/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
+++ b/lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
@@ -771,9 +771,12 @@ DynamicLoaderPOSIXDYLD::GetThreadLocalData(const lldb::ModuleSP module_sp,
             "GetThreadLocalData info: link_map=0x%" PRIx64
             ", thread info metadata: "
             "modid_offset=0x%" PRIx32 ", dtv_offset=0x%" PRIx32
-            ", tls_offset=0x%" PRIx32 ", dtv_slot_size=%" PRIx32 "\n",
+            ", tls_offset=0x%" PRIx32 ", dtv_slot_size=%" PRIx32
+            ", tls_file_addr=0x%" PRIx64 ", module name=%s "
+            "\n",
             link_map, metadata.modid_offset, metadata.dtv_offset,
-            metadata.tls_offset, metadata.dtv_slot_size);
+            metadata.tls_offset, metadata.dtv_slot_size, tls_file_addr,
+            module_sp->GetFileSpec().GetFilename().AsCString());
 
   // Get the thread pointer.
   addr_t tp = thread->GetThreadPointer();
@@ -790,9 +793,12 @@ DynamicLoaderPOSIXDYLD::GetThreadLocalData(const lldb::ModuleSP module_sp,
     LLDB_LOGF(log, "GetThreadLocalData error: fail to read modid");
     return LLDB_INVALID_ADDRESS;
   }
-
+  const llvm::Triple &triple_ref =
+      m_process->GetTarget().GetArchitecture().GetTriple();
   // Lookup the DTV structure for this thread.
-  addr_t dtv_ptr = tp + metadata.dtv_offset;
+  addr_t dtv_ptr = tp;
+  if (triple_ref.getArch() != llvm::Triple::aarch64)
+    dtv_ptr = dtv_ptr + metadata.dtv_offset;
   addr_t dtv = ReadPointer(dtv_ptr);
   if (dtv == LLDB_INVALID_ADDRESS) {
     LLDB_LOGF(log, "GetThreadLocalData error: fail to read dtv");
diff --git a/lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp b/lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
index f51a93e1b2dcbd..5f80265feda70c 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
+++ b/lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp
@@ -73,19 +73,20 @@
 #undef DECLARE_REGISTER_INFOS_ARM64_STRUCT
 
 static lldb_private::RegisterInfo g_register_infos_pauth[] = {
-    DEFINE_EXTENSION_REG(data_mask), DEFINE_EXTENSION_REG(code_mask)};
+    DEFINE_EXTENSION_REG(data_mask, LLDB_INVALID_REGNUM),
+    DEFINE_EXTENSION_REG(code_mask, LLDB_INVALID_REGNUM)};
 
 static lldb_private::RegisterInfo g_register_infos_mte[] = {
-    DEFINE_EXTENSION_REG(mte_ctrl)};
+    DEFINE_EXTENSION_REG(mte_ctrl, LLDB_INVALID_REGNUM)};
 
 static lldb_private::RegisterInfo g_register_infos_tls[] = {
-    DEFINE_EXTENSION_REG(tpidr),
+    DEFINE_EXTENSION_REG(tpidr, LLDB_REGNUM_GENERIC_TP),
     // Only present when SME is present
-    DEFINE_EXTENSION_REG(tpidr2)};
+    DEFINE_EXTENSION_REG(tpidr2, LLDB_INVALID_REGNUM)};
 
 static lldb_private::RegisterInfo g_register_infos_sme[] = {
-    DEFINE_EXTENSION_REG(svcr),
-    DEFINE_EXTENSION_REG(svg),
+    DEFINE_EXTENSION_REG(svcr, LLDB_INVALID_REGNUM),
+    DEFINE_EXTENSION_REG(svg, LLDB_INVALID_REGNUM),
     // 16 is a default size we will change later.
     {"za", nullptr, 16, 0, lldb::eEncodingVector, lldb::eFormatVectorOfUInt8,
      KIND_ALL_INVALID, nullptr, nullptr, nullptr}};
diff --git a/lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h b/lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h
index c9c4d7ceae5573..29f3bddef317f7 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h
+++ b/lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h
@@ -535,10 +535,11 @@ static uint32_t g_d31_invalidates[] = {fpu_v31, fpu_s31, LLDB_INVALID_REGNUM};
   }
 
 // Defines pointer authentication mask registers
-#define DEFINE_EXTENSION_REG(reg)                                              \
+#define DEFINE_EXTENSION_REG(reg, kind)                                        \
   {                                                                            \
     #reg, nullptr, 8, 0, lldb::eEncodingUint, lldb::eFormatHex,                \
-        KIND_ALL_INVALID, nullptr, nullptr, nullptr,                           \
+        LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, kind,                        \
+        LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM , nullptr, nullptr, nullptr,  \
   }
 
 static lldb_private::RegisterInfo g_register_infos_arm64_le[] = {
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp b/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
index 887983de2e8516..af63706c9e9e78 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp
@@ -270,8 +270,6 @@ void ManualDWARFIndex::IndexUnitImpl(DWARFUnit &unit,
       case DW_AT_location:
       case DW_AT_const_value:
         has_location_or_const_value = true;
-        is_global_or_static_variable = die.IsGlobalOrStaticScopeVariable();
-
         break;
 
       case DW_AT_specification:
@@ -363,7 +361,8 @@ void ManualDWARFIndex::IndexUnitImpl(DWARFUnit &unit,
       break;
 
     case DW_TAG_variable:
-      if (name && has_location_or_const_value && is_global_or_static_variable) {
+      is_global_or_static_variable = die.IsGlobalOrStaticScopeVariable();
+      if (name && is_global_or_static_variable) {
         set.globals.Insert(ConstString(name), ref);
         // Be sure to include variables by their mangled and demangled names if
         // they have any since a variable can have a basename "i", a mangled
diff --git a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
index a40d6d9e37978b..db8d0d5e889b1a 100644
--- a/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
+++ b/lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
@@ -3499,6 +3499,7 @@ VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
   DWARFFormValue type_die_form;
   bool is_external = false;
   bool is_artificial = false;
+  bool is_declaration = false;
   DWARFFormValue const_value_form, location_form;
   Variable::RangeList scope_ranges;
 
@@ -3545,6 +3546,7 @@ VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
       is_artificial = form_value.Boolean();
       break;
     case DW_AT_declaration:
+      is_declaration = form_value.Boolean();
     case DW_AT_description:
     case DW_AT_endianity:
     case DW_AT_segment:
@@ -3557,6 +3559,12 @@ VariableSP SymbolFileDWARF::ParseVariableDIE(const SymbolContext &sc,
     }
   }
 
+  // If It's a declaration then symbol not present in this symbolfile
+  // return early to try other linked objects.
+  if (is_declaration) {
+    return nullptr;
+  }
+
   // Prefer DW_AT_location over DW_AT_const_value. Both can be emitted e.g.
   // for static constexpr member variables -- DW_AT_const_value and
   // DW_AT_location will both be present in the DIE defining the member.
diff --git a/lldb/source/Symbol/Variable.cpp b/lldb/source/Symbol/Variable.cpp
index d4e1ce43ef1f16..8996846d42a6d8 100644
--- a/lldb/source/Symbol/Variable.cpp
+++ b/lldb/source/Symbol/Variable.cpp
@@ -438,6 +438,17 @@ Status Variable::GetValuesForVariableExpressionPath(
   return error;
 }
 
+bool Variable::IsThreadLocal() const {
+  ModuleSP module_sp(m_owner_scope->CalculateSymbolContextModule());
+  // Give the symbol vendor a chance to add to the unified section list.
+  module_sp->GetSymbolFile();
+  std::vector<uint32_t> symbol_indexes;
+  module_sp->GetSymtab()->FindAllSymbolsWithNameAndType(
+      ConstString(GetName()), lldb::SymbolType::eSymbolTypeAny, symbol_indexes);
+  Symbol *symbol = module_sp->GetSymtab()->SymbolAtIndex(symbol_indexes[0]);
+  return symbol->GetAddress().GetSection()->IsThreadSpecific();
+}
+
 bool Variable::DumpLocations(Stream *s, const Address &address) {
   SymbolContext sc;
   CalculateSymbolContext(&sc);

@labath

labath commented Oct 2, 2024

Copy link
Copy Markdown
Contributor

The implementation of Variable::IsThreadLocal is most likely a non-starter, as it introduces a relatively expensive operation on the common variable update path for all variables. Doing this (once) at variable creation would be better, but I still have very big reservations about this approach. AFAICT, there's no way to guarantee that the variable you find this way will be the one that is actually described by the DWARF. You could even have more than one variable with the same name if they're static thread_local.

This really sounds like something where the compiler should give us more to go on -- instead of us trying to divine this information out of thin air.

@kamleshbhalui
kamleshbhalui force-pushed the kk/lldb_tls_fix_without_loc branch from 899c26d to ba30715 Compare October 2, 2024 14:38
@github-actions

github-actions Bot commented Oct 2, 2024 •

Copy link
Copy Markdown

⚠️ C/C++ code formatter, clang-format found issues in your code. ⚠️

You can test this locally with the following command:
git-clang-format --diff e379094328e49731a606304f7e3559d4f1fa96f9 f36d8a2346be90b6bf5d68a7cec0bcf2d41cbd99 --extensions cpp,h -- lldb/include/lldb/Symbol/Variable.h lldb/source/Core/ValueObjectVariable.cpp lldb/source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp lldb/source/Plugins/Process/Utility/RegisterInfoPOSIX_arm64.cpp lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp lldb/source/Symbol/Variable.cpp
View the diff from clang-format here.
diff --git a/lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h b/lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h
index b1a0a1957e..002690690c 100644
--- a/lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h
+++ b/lldb/source/Plugins/Process/Utility/RegisterInfos_arm64.h
@@ -472,10 +472,8 @@ static uint32_t g_d31_invalidates[] = {fpu_v31, fpu_s31, LLDB_INVALID_REGNUM};
 
 // Generates register kinds array for registers with only generic kind
 #define GENERIC_KIND(generic_kind)                                             \
-  {                                                                            \
-    LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, generic_kind,                    \
-        LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM                               \
-  }
+  {LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM, generic_kind,                     \
+   LLDB_INVALID_REGNUM, LLDB_INVALID_REGNUM}
 
 // Generates register kinds array for registers with only lldb kind
 #define KIND_ALL_INVALID                                                       \

On AArch64, TLS variables do not have DT_Location entries generated in the
debug information due to the lack of dtpoff relocation support, unlike on x86_64.
LLDB relies on this location info to calculate the TLS address, leading to issues
when debugging TLS variables on AArch64.
However, GDB can successfully calculate the TLS address without relying on this debug info,
by using the symbol’s address and manually calculating the offset. We adopt a similar approach for LLDB.
@kamleshbhalui
kamleshbhalui force-pushed the kk/lldb_tls_fix_without_loc branch from ba30715 to f36d8a2 Compare October 2, 2024 15:37
@kamleshbhalui

Copy link
Copy Markdown
Contributor Author

The implementation of Variable::IsThreadLocal is most likely a non-starter, as it introduces a relatively expensive operation on the common variable update path for all variables. Doing this (once) at variable creation would be better, but I still have very big reservations about this approach. AFAICT, there's no way to guarantee that the variable you find this way will be the one that is actually described by the DWARF. You could even have more than one variable with the same name if they're static thread_local.

This really sounds like something where the compiler should give us more to go on -- instead of us trying to divine this information out of thin air.

I could pass that info to variable creation to simplify the check on update path and about second question even gdb could not debug static thread_local.

@labath

labath commented Oct 7, 2024

Copy link
Copy Markdown
Contributor

Not giving any results would actually be the better case here. It would be worse if it actually showed a random variable with that name, even if it was not the variable we were looking for (which is what I think this patch will do if there are multiple variables with that name).

@DavidSpickett

Copy link
Copy Markdown
Contributor

Thanks for working on this feature, I will find the time to review it properly today.

I don't remember whether the Arm ABI specifies a way to represent TLS variables, or whether it's the compilers that aren't producing it. If it's the former, we (Arm that is) do take proposals to our ABIs (https://github.com/ARM-software/abi-aa) so we could address that.

So there are other routes if you are not able to address Pavel's concerns about the accuracy of this.

@DavidSpickett DavidSpickett left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't want a fix for AArch64 TLS to break TLS support elsewhere, so this needs testing and we may need to add new tests for the existing support.

Could you:

  • Find out what TLS testing we have right now. (a really ugly way to do it is to delete the TLS handling code and see what fails)
  • If it doesn't cover what you're modifying, extend that and PR those changes.
  • Write a test case for this new code, which will:
    • Include the situation Pavel brought up.
    • Include the limitation around static thread local
    • Include any other assumptions you're making.

Another overall topic is, if we landed this then improved the compilers, would the new objects "just work" or would we need to remove this workaround? Which would then break debugging older binaries.

(this is a tradeoff we can make if it's worth it, but also we could choose to improve the compilers right now instead)

ExecutionContext exe_ctx(GetExecutionContextRef());
Thread *thread = exe_ctx.GetThreadPtr();
lldb::ModuleSP module_sp = GetModule();
if (!thread) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Get thread first and exit if it's nullptr, then get the others.

if (!thread) {
m_error = Status::FromErrorString("no thread to evaluate TLS within");
return m_error.Success();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I would appreciate a blank line in places like this where some part of the work has been done and the next stage begins.

}

if (m_error.Fail() && variable->IsThreadLocal()) {
ExecutionContext exe_ctx(GetExecutionContextRef());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A comment at the start of this if block like:

If the proper relocation information is provided, we will have <whatever verb it is> the symbol. If not, we will now try to <whatever it is we're doing here.

// Lookup the DTV structure for this thread.
addr_t dtv_ptr = tp + metadata.dtv_offset;
addr_t dtv_ptr = tp;
if (triple_ref.getArch() != llvm::Triple::aarch64)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The usual complaint here about architecture specific code being in generic paths. This at least needs a comment to explain why we're not doing this for AArch64. If not a better place to put it. ABI plugins is the usual place, this is part of the ABI after all.

Have a look at lldb/source/Plugins/ABI/AArch64/ABIAArch64.h and the others in that folder.

I also wonder if this will break horribly if we ever do emit proper information from the compiler. Does this code run before or after the code above in ValueObjectVariable.cpp? I wonder if we can from know already that we've been given the right information, and know whether to apply this workaround.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I second this motion and it matches my comments above. We can't just have arch specific code in a code path that is only used for a specific architecture.


static lldb_private::RegisterInfo g_register_infos_tls[] = {
DEFINE_EXTENSION_REG(tpidr),
DEFINE_EXTENSION_REG(tpidr, GENERIC_KIND(LLDB_REGNUM_GENERIC_TP)),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it would be simpler to just expand DEFINE_EXTENSION_REG just for tpidr, instead of changing it for every use. Only going to have one TLS pointer so it's justified I think.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Make a new #define if needed for this one.

// return early to try other linked objects.
if (is_declaration) {
return nullptr;
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Parts like this I'm bothered by the effect they may have on existing lookups, but testing will address that.

@clayborg clayborg left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How is the DWARF encoded differently for a thread local variable for arm64? Normally there is just one location attribute that specifies a TLS address. Why do we need to lookup symbols by name if the DWARF has TLS info in it already. Can you dump the DWARF for a simple example and paste it as comment so we can know what we are dealing with?

return m_error.Success();
}
std::vector<uint32_t> symbol_indexes;
module_sp->GetSymtab()->FindAllSymbolsWithNameAndType(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use Module::FindFirstSymbolWithNameAndType. No need to get every symbol for this if we just need the first one.

}
std::vector<uint32_t> symbol_indexes;
module_sp->GetSymtab()->FindAllSymbolsWithNameAndType(
ConstString(variable->GetName()), lldb::SymbolType::eSymbolTypeAny,

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we really want to get any symbol type? Won't a variable have a type of eSymbolTypeData?

Comment on lines +270 to +272
lldb::addr_t tls_file_addr =
symbol->GetAddress().GetOffset() +
symbol->GetAddress().GetSection()->GetFileAddress();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems very specify to the way that the current OS encodes thread local data. What does this work on right now? Linux only I assume? This seems like this functionality should be in OS specific code. Maybe the ABI plug-ins so that if this is a Linux thing, if the OS ABI defines a way to get variables for thread locals, we encapsulate this so this doesn't say try to run on windows, or other systems. So I would suggest using the target triple of the target to get an appropriate OS ABI or other architecture specific plug-in (ARM64) and then doing this work there only if the target triple matches arm64-linux-....

// Lookup the DTV structure for this thread.
addr_t dtv_ptr = tp + metadata.dtv_offset;
addr_t dtv_ptr = tp;
if (triple_ref.getArch() != llvm::Triple::aarch64)

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I second this motion and it matches my comments above. We can't just have arch specific code in a code path that is only used for a specific architecture.

Comment on lines +441 to +453
bool Variable::IsThreadLocal() const {
ModuleSP module_sp(m_owner_scope->CalculateSymbolContextModule());
// Give the symbol vendor a chance to add to the unified section list.
module_sp->GetSymbolFile();
std::vector<uint32_t> symbol_indexes;
module_sp->GetSymtab()->FindAllSymbolsWithNameAndType(
ConstString(GetName()), lldb::SymbolType::eSymbolTypeAny, symbol_indexes);
if (symbol_indexes.empty())
return false;
Symbol *symbol = module_sp->GetSymtab()->SymbolAtIndex(symbol_indexes[0]);
return symbol->GetAddress().GetSection()->IsThreadSpecific();
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't work for all architectures so this belongs in an OS ABI plug-in or something like that. We debug many different systems: linux, macOS, iOS, windows, Android etc, and the way variables are stored is an OS ABI thing.

// Give the symbol vendor a chance to add to the unified section list.
module_sp->GetSymbolFile();
std::vector<uint32_t> symbol_indexes;
module_sp->GetSymtab()->FindAllSymbolsWithNameAndType(

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you are going to do this code elsewhere, like in an OS ABI plug-in, please use FindFirstSymbolWithNameAndType if you only use the first one. Probably specify lldb::SymbolType::eSymbolTypeData as well


static lldb_private::RegisterInfo g_register_infos_tls[] = {
DEFINE_EXTENSION_REG(tpidr),
DEFINE_EXTENSION_REG(tpidr, GENERIC_KIND(LLDB_REGNUM_GENERIC_TP)),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Agreed. Make a new #define if needed for this one.

@xgupta

xgupta commented Apr 1, 2026

Copy link
Copy Markdown
Contributor

This PR can be closed since #146572 is committed which emits DW_AT_location for Aarch64 and LLDB able to use to debug TLS variables.

@DavidSpickett

Copy link
Copy Markdown
Contributor

As stated above, we do have a solution for AArch64 TLS so I will close this PR now.

@kamleshbhalui thank you for your time and effort nevertheless. If you or anyone else still wants to pursue this method to support older toolchains, you are free to do so.

For anyone coming here from Google, read #71666 to see details of the fix. You do need a recent clang and some extra options, at least at time of writing.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

lldb on AArch64 Linux cannot read thread local variables

6 participants