[Lldb-commits] [lldb] r186207 - Introduces core file support for Linux x86-64 using 'lldb a.out -c core'.
ashok.thirumurthi at intel.com
Wed Jul 17 09:25:41 PDT 2013
Thanks for the input, Greg, good to know there's a practical way forwards to using the ELF core files on more platforms.
Samuel, I tested/committed the patch that includes Daniel's review feedback in r186516, and so far it looks green on the buildbots. Cheers,
From: lldb-commits-bounces at cs.uiuc.edu [mailto:lldb-commits-bounces at cs.uiuc.edu] On Behalf Of Samuel Jacob
Sent: Tuesday, July 16, 2013 7:08 PM
To: Malea, Daniel
Cc: 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'.
Thanks Dan for the review comments.
On Tue, Jul 16, 2013 at 2:12 PM, Malea, Daniel <daniel.malea at intel.com<mailto: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().
If the cast is removed then It would cause a compilation failure.
* 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.
My understanding is ReadAllRegisterValues() is used for checkpoint and for corefiles we dont need that. That is why it is not implemented.
* 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?
I didnt find any structures with PRSTATUS and PSINFO that is why created new ones.
* 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.
I actually copied mach-core plugin and modified functions as needed and preserved formatting.
This is the first time I hearing about clang-format, so unless otherwise it is too much whitespace issue I like to fix it manually rather than running a automated tool.
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.)
-------------- next part --------------
An HTML attachment was scrubbed...
More information about the lldb-commits