A few comments:
1 - A module should not be required in DWARFExpression. The current code will
fail to evaluate if the DWARF expression has no module. If your changed from
DWARFExpression::Evaluate():
ModuleSP module_sp = m_module_wp.lock();
if (!module_sp.get())
{
if (error_ptr)
error_ptr->SetErrorString("Module was unloaded.");
return false;
}
The check would be better made as:
if (m_module_wp.expired())
{
if (error_ptr)
error_ptr->SetErrorString("Module was unloaded.");
return false;
}
2 - We should only fail to evaluate the expression for a DW_OP_XXX that
_requires_ the module. The only thing that currently requires the module is
DW_OP_GNU_push_tls_address. So we should only check the module in the
DW_OP_GNU_push_tls_address case.
3 - You will need to modify the DWARFExpression contractor:
DWARFExpression::DWARFExpression(lldb::ModuleSP module_sp, const DataExtractor&
data, lldb::offset_t data_offset, lldb::offset_t data_length) :
m_module_wp(module_sp),
m_data(data, data_offset, data_length),
m_reg_kind (eRegisterKindDWARF),
m_loclist_slide(LLDB_INVALID_ADDRESS)
{
}
If "module_sp" is empty, it will throw an exception. So this needs to be:
DWARFExpression::DWARFExpression(lldb::ModuleSP module_sp, const DataExtractor&
data, lldb::offset_t data_offset, lldb::offset_t data_length) :
m_module_wp(),
m_data(data, data_offset, data_length),
m_reg_kind (eRegisterKindDWARF),
m_loclist_slide(LLDB_INVALID_ADDRESS)
{
if (module_sp)
m_module_wp = module_sp;
}
We shouldn't require a module.
4 - We should be asking the lldb_private::Thread object itself for the thread
local data, not the process on the thread's behalf. There can still be
functions in lldb_private::Process if required, but they should be protected
and only lldb_private::Thread should have access.
5 - Does the functionality of determining the thread local storage really
depend on the DYLD plug-in? Is it specific to the dynamic loader, or specific
to the system itself? I know MacOSX can create processes that don't use dyld
and they might have thread local storage. I will have to look into how we do
this on MacOSX to be able to more intelligently speak to this.
6 - The changes in source/Plugins/Process/Utility/RegisterContextLLDB.cpp will
crash unless you fix #1
On Oct 15, 2013, at 3:39 PM, Richard Mitton <[email protected]> wrote:
> Added support for reading thread-local storage variables, as defined using
> the __thread modifier. I think this should be fine as a first pass, but I'm
> throwing this out there for review.
>
> To make this work this patch extends LLDB to:
>
> - Explicitly track the link_map address for each module. This is effectively
> the module handle, not sure why it wasn't already being stored off anywhere.
> As an extension later, it would be nice if someone were to add support for
> printing this as part of the modules list.
>
> - Allow reading the per-thread data pointer via ptrace. I have added support
> for Linux here. I'll be happy to add support for FreeBSD once this is
> reviewed. OS X does not appear to have __thread variables, so maybe we don't
> need it there. Windows support should eventually be workable along the same
> lines.
>
> - Make DWARF expressions track which module they originated from.
>
> - Add support for the DW_OP_GNU_push_tls_address DWARF opcode, as generated
> by gcc and recent versions of clang. Earlier versions of clang (such as 3.2,
> which is default on Ubuntu right now) do not generate TLS debug info
> correctly so can not be supported here.
>
> - Understand the format of the pthread DTV block. This is where it gets
> tricky. We have three basic options here:
>
> 1) Call "dlinfo" or "__tls_get_addr" on the inferior and ask it directly.
> However this won't work on core dumps, and generally speaking it's not a good
> idea for the debugger to call functions itself, as it has the potential to
> not work depending on the state of the target.
>
> 2) Use libthread_db. This is what GDB does. However this option requires
> having a version of libthread_db on the host cross-compiled for each
> potential target. This places a large burden on the user, and would make it
> very hard to cross-debug from Windows to Linux, for example. Trying to build
> a library intended exclusively for one OS on a different one is not pleasant.
> GDB sidesteps the problem and asks the user to figure it out.
>
> 3) Parse the DTV structure ourselves. On initial inspection this seems to be
> a bad option, as the DTV structure (the format used by the runtime to manage
> TLS data) is not in fact a kernel data structure, it is implemented entirely
> in useerland in libc. Therefore the layout of it's fields are version and OS
> dependent, and are not standardized.
>
> However, it turns out not to be such a problem. All OSes use basically the
> same algorithm (a per-module lookup table) as detailed in Ulrich Drepper's
> TLS ELF ABI document, so we can easily write code to decode it ourselves. The
> only question therefore is the exact field layouts required. Happily, the
> implementors of libpthread expose the structure of the DTV via metadata
> exported as symbols from the .so itself, designed exactly for this kind of
> thing. So this patch simply reads that metadata in, and re-implements
> libthread_db's algorithm itself. We thereby get cross-platform TLS lookup
> without either requiring third-party libraries, while still being independent
> of the version of libpthread being used.
>
> Test case included.
>
>
> http://llvm-reviews.chandlerc.com/D1944
>
> Files:
> include/lldb/Expression/DWARFExpression.h
> include/lldb/Target/DynamicLoader.h
> include/lldb/Target/Process.h
> source/Expression/DWARFExpression.cpp
> source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.cpp
> source/Plugins/DynamicLoader/POSIX-DYLD/DYLDRendezvous.h
> source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp
> source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.h
> source/Plugins/Process/Linux/ProcessMonitor.cpp
> source/Plugins/Process/Linux/ProcessMonitor.h
> source/Plugins/Process/POSIX/ProcessPOSIX.cpp
> source/Plugins/Process/POSIX/ProcessPOSIX.h
> source/Plugins/Process/Utility/RegisterContextLLDB.cpp
> source/Plugins/SymbolFile/DWARF/DWARFDebugInfoEntry.cpp
> source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
> source/Target/Process.cpp
> test/lang/c/tls_globals/Makefile
> test/lang/c/tls_globals/TestTlsGlobals.py
> test/lang/c/tls_globals/a.c
> test/lang/c/tls_globals/main.c
> <D1944.1.patch>_______________________________________________
> lldb-commits mailing list
> [email protected]
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
_______________________________________________
lldb-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
