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

[Samuel Jacob]

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
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20130716/be43a994/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: lldb_elf-core.patch
Type: application/octet-stream
Size: 54671 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20130716/be43a994/attachment.obj>