On Oct 16, 2013, at 10:46 AM, Richard Mitton <[email protected]> wrote:
> Is there any situation in which a DWARF expression can exist outside of a > module? Surely they're only ever created from within .debug_info sections? I > agree that the check should only happen if TLS is used, so I'll fix that. Yes, but mostly for hand crafted expressions. I have been thinking that for conditional breakpoints it would be great if we could take an expression like "i==3" and turn that into a DWARF expression "DW_OP_reg22, DW_OP_constu(3), DW_OP_eq" and then ship that down to a remote GDB server and have it do the computation when breakpoints get hit to avoid the GDB remote packet traffic to speed things up a bit. This is future stuff, but something I have my eye on. You are correct in that all current DWARF expressions have a module context. > > The management of thread local storage is indeed implicitly tied directly to > the dynamic loader. Each module has it's own TLS section in the ELF, and so > requires the linker (or dynamic linker therefore for shared modules) to > arbitrate this. Sounds good. Hopefully other platforms do the same kind of thing. > > On systems that only support TLS via a get/set call (e.g. using the TlsAlloc > method on Windows, pthread_get_specific on OSX), these are not variables and > so the debugger is not concerned with them. I believe we have the old manual code for pthread_get_specific()/pthread_set_specific(), but we did recently add support for the __thread keyword and I am not sure how we implemented it. > > I agree with the rest of the comments and I'll update the patch to fix them. Great! It will be a great patch to get in, I am looking forward to adding support for this on Darwin. Greg > > Richard Mitton > [email protected] > > On 10/16/2013 10:29 AM, Greg Clayton wrote: >> 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
