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

[Greg Clayton]

LLDB does not use the LLVM coding style. I would prefer that any checkins adhere to the LLDB coding conventions. I know we have a mix of styles in LLDB so far, but we do prefer the LLDB coding style that is prevalent on most of the LLDB code.

Greg

On Jul 16, 2013, at 4:07 PM, Samuel Jacob <samueldotj at gmail.com> wrote:

> Thanks Dan for the review comments.
> 
> 
> On Tue, 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.
> 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
> 
> 
> <lldb_elf-core.patch>