[Lldb-commits] [lldb] r186207 - Introduces core file support for Linux x86-64 using 'lldb a.out -c core'.
gclayton at apple.com
Wed Jul 17 10:49:10 PDT 2013
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.
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.
> • 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 :)
> 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.)
More information about the lldb-commits