[Lldb-commits] [lldb] r186207 - Introduces core file support for Linux x86-64 using 'lldb a.out -c core'.
Greg Clayton
gclayton at apple.com
Tue Jul 16 14:43:11 PDT 2013
I spent some time modifying the patch to correctly abstract the register contexts so we don't have this incorrect RegisterContext base class that uses ProcessMonitor... I wasn't able to finish it, but I did get it close. I have attached the patch for you guys to check out and hopefully fix it up and get it working for normal linux/freebsd, and for ELF core files for both linux and freebsd.
The basics are, the following classes contain the register context data, but don't do any of the reading/writing registers (they have pure virtual functions for that):
RegisterContextPOSIX_i386
RegisterContextPOSIX_x86_64
Then there are classes that inherit from these and each contain grab the process monitor:
RegisterContextPOSIXProcessMonitor_i386
RegisterContextPOSIXProcessMonitor_x86_64
Then there should also be classes that do this for elf:
RegisterContextELFLinux_XXX
RegisterContextELFFreeBSD_XXX
Please try and modify the existing patch to make it work so that we can use ELF core files on all systems.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: elfcore.patch
Type: application/octet-stream
Size: 335477 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20130716/16446b44/attachment.obj>
-------------- next part --------------
On Jul 16, 2013, at 2:12 PM, "Malea, Daniel" <daniel.malea at intel.com> wrote:
> Hi Samuel, thanks for the updated patch! I had a chance to apply your patch and give it a quick look…I have a few comments:
>
> * In source/Plugins/Process/CMakeLists.txt I noticed an extra "add_subdirectory(elf-core)" in the common section — I had to remove this before cmake would process the file without errors.
> * You may want to remove the "(FileSpec *)" cast in ProcessLinux.cpp in ::CreateInstance().
> * Any reason to not use the initializer list in ProcessLinux to set m_core_file?
> * Please replace "assert(0);" in RegisterContext_x86_64.cpp with a more meaningful error — like llvm_fatal_error("reason") for potentially user-visible situations, or llvm_unreachable("reason here") for code that is not meant to ever execute. Another pattern I see is widely used in lldb is the assert(0 && "the reason for asserting").
> * In RegisterContextCoreLinux_x86_64, any reason to not implement ::ReadAllRegisterValues()? It seems that if ::ReadRegister() is implemented, then so should ::ReadAllRegisterValues(). It's OK to leave this as a FIXME for a later date, but a comment indicating the intention to implement it would be helpful. Or, if it can't be implemented, a comment indicating why not would be good.
> * Are the structures ElfPrStatus and ElfPsInfo defined in some ELF header file? If so, would it be better to re-use that header rather than duplicating the structures in LLDB?
> * Why make ThreadElfCore and ProcessElfCore friends? Everything seems to build OK without the friend declarations in those two classes..
>
> Also, I noticed some random whitespace issues — the best way I found to handle these kinds of annoyances is by running clang-format on any new files you're adding. The other benefit of that tool is that it keeps things close to the LLVM style guide.
>
> Thanks again for working on this -- I'm very excited to see this functionality precipitate :)
>
> I can confirm running the test suite indicates no regressions with the latest version of the patch (with clang-3.4 on Ubuntu 13.04.)
>
>
> Cheers,
> Dan
>
> From: Samuel Jacob <samueldotj at gmail.com<mailto:samueldotj at gmail.com>>
> Date: Monday, 15 July, 2013 8:04 PM
> To: Greg Clayton <gclayton at apple.com<mailto:gclayton at apple.com>>
> Cc: "lldb-commits at cs.uiuc.edu<mailto:lldb-commits at cs.uiuc.edu>" <lldb-commits at cs.uiuc.edu<mailto:lldb-commits at cs.uiuc.edu>>
> Subject: Re: [Lldb-commits] [lldb] r186207 - Introduces core file support for Linux x86-64 using 'lldb a.out -c core'.
>
> Greg,
>
> I modified the patch to compile only on Linux/FreeBSD system. I did a quick compilation on a borrowed MacMini and it passed. Please let me know what you think of the attached patch.
>
> Thanks
> Samuel
>
>
> On Fri, Jul 12, 2013 at 4:04 PM, Greg Clayton <gclayton at apple.com<mailto:gclayton at apple.com>> wrote:
>
> On Jul 12, 2013, at 3:59 PM, Greg Clayton <gclayton at apple.com<mailto:gclayton at apple.com>> wrote:
>
>> I reverted the ELF core file support until we can get the darwin build working. I could have commented out the initialization in lldb.cpp, but there were changes in AuxVector that used the ELF core file process plug-in name, so the ELF core file process plug-in needed to be compiled in in order to link.
>>
>> It seems like we need to cleanup the register context classes to make sure ones that are used under ProcessPOSIX are completely separate from the ones that ProcessElfCore will use. The reverted patch had the base RegisterContext class with a GetMonitor() accessor that required ProcessPOSIX to be compiled in which doesn't work for darwin builds since ProcessPOSIX is only compiled in on systems that natively use it.
>
> Also the base register context class can't have this because the process will either be ProcessPOSIX or ProcessElfCore. If the GetMonitor() function got calls when ProcessElfCore was the process we could have issues.
>
> To work around this on MacOSX, we have a base RegisterContextDarwin_<ARCH> which must be subclassed and it has pure virtual functions:
>
> virtual int
> DoReadGPR (lldb::tid_t tid, int flavor, GPR &gpr) = 0;
>
> virtual int
> DoReadFPU (lldb::tid_t tid, int flavor, FPU &fpu) = 0;
>
> virtual int
> DoReadEXC (lldb::tid_t tid, int flavor, EXC &exc) = 0;
>
> virtual int
> DoWriteGPR (lldb::tid_t tid, int flavor, const GPR &gpr) = 0;
>
> virtual int
> DoWriteFPU (lldb::tid_t tid, int flavor, const FPU &fpu) = 0;
>
> virtual int
> DoWriteEXC (lldb::tid_t tid, int flavor, const EXC &exc) = 0;
>
> The mach core files will override these functions and hook them up to the core file register states, and other targets can do what they need to.
>
>
>> I will be happy to get the Xcode project building with any additional source files once they build and link for darwin.
>>
>> % svn commit
>> Sending lib/Makefile
>> Sending source/CMakeLists.txt
>> Sending source/Plugins/DynamicLoader/POSIX-DYLD/AuxVector.cpp
>> Sending source/Plugins/Makefile
>> Sending source/Plugins/Process/CMakeLists.txt
>> Sending source/Plugins/Process/Linux/ProcessLinux.cpp
>> Sending source/Plugins/Process/Linux/ProcessLinux.h
>> Sending source/Plugins/Process/POSIX/RegisterContext_x86_64.cpp
>> Sending source/Plugins/Process/elf-core/CMakeLists.txt
>> Sending source/Plugins/Process/elf-core/Makefile
>> Sending source/Plugins/Process/elf-core/ProcessElfCore.cpp
>> Sending source/Plugins/Process/elf-core/ProcessElfCore.h
>> Sending source/Plugins/Process/elf-core/RegisterContextCoreFreeBSD_x86_64.cpp
>> Sending source/Plugins/Process/elf-core/RegisterContextCoreFreeBSD_x86_64.h
>> Sending source/Plugins/Process/elf-core/RegisterContextCoreLinux_x86_64.cpp
>> Sending source/Plugins/Process/elf-core/RegisterContextCoreLinux_x86_64.h
>> Sending source/Plugins/Process/elf-core/ThreadElfCore.cpp
>> Sending source/Plugins/Process/elf-core/ThreadElfCore.h
>> Sending source/lldb.cpp
>> Transmitting file data ...................
>> Committed revision 186223.
>>
>>
>>
>> On Jul 12, 2013, at 3:30 PM, Greg Clayton <gclayton at apple.com<mailto:gclayton at apple.com>> wrote:
>>
>>> RegisterContextCoreLinux_x86_64 inherits from RegisterContextLinux_x86_64 which inherits from RegisterContext_x86_64 which uses has:
>>>
>>> ProcessMonitor &GetMonitor();
>>>
>>> This register context used by the core file can't use this since the process plug-in will be ProcessElfCore and the implementation of GetMonitor() does:
>>>
>>>
>>> ProcessMonitor &
>>> RegisterContext_x86_64::GetMonitor()
>>> {
>>> ProcessSP base = CalculateProcess();
>>> ProcessPOSIX *process = static_cast<ProcessPOSIX*>(base.get());
>>> return process->GetMonitor();
>>> }
>>>
>>> ProcessELFCore doesn't, nor should it inherit from ProcessPOSIX:
>>>
>>>
>>> class ProcessElfCore : public lldb_private::Process
>>> {
>>> public:
>>
>> _______________________________________________
>> lldb-commits mailing list
>> lldb-commits at cs.uiuc.edu<mailto:lldb-commits at cs.uiuc.edu>
>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>
>
More information about the lldb-commits
mailing list