[Lldb-commits] [lldb] r186207 - Introduces core file support for Linux x86-64 using 'lldb a.out -c core'.

[Thirumurthi, Ashok]

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,


-        Ashok

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.
Fixed

  *   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?
Done

  *   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").
Done

  *   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..
Fixed.


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 :)
Thank you.


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


-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20130717/d41756fc/attachment.html>