<div dir="ltr">Thanks Dan for the review comments.<br><div class="gmail_extra"><br><br><div class="gmail_quote">On Tue, Jul 16, 2013 at 2:12 PM, Malea, Daniel <span dir="ltr"><<a href="mailto:daniel.malea@intel.com" target="_blank">daniel.malea@intel.com</a>></span> wrote:<br>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">



<div style="font-size:14px;font-family:Calibri,sans-serif;word-wrap:break-word">
<div>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:</div>
<ul>
<li>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.</li></ul></div></blockquote>

<div>Fixed </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="font-size:14px;font-family:Calibri,sans-serif;word-wrap:break-word"><ul><li>You may want to remove the "(FileSpec *)" cast in ProcessLinux.cpp in ::CreateInstance().</li>

</ul></div></blockquote><div>If the cast is removed then It would cause a compilation failure. </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="font-size:14px;font-family:Calibri,sans-serif;word-wrap:break-word">

<ul><li>Any reason to not use the initializer list in ProcessLinux to set m_core_file?</li></ul></div></blockquote><div>Done </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div style="font-size:14px;font-family:Calibri,sans-serif;word-wrap:break-word"><ul><li>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").</li></ul></div></blockquote><div>Done </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div style="font-size:14px;font-family:Calibri,sans-serif;word-wrap:break-word"><ul><li>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.</li></ul></div></blockquote><div>My understanding is ReadAllRegisterValues() is used for checkpoint and for corefiles we dont need that. That is why it is not implemented. </div>

<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="font-size:14px;font-family:Calibri,sans-serif;word-wrap:break-word"><ul><li>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?</li>

</ul></div></blockquote><div>I didnt find any structures with PRSTATUS and PSINFO that is why created new ones.</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="font-size:14px;font-family:Calibri,sans-serif;word-wrap:break-word">

<ul><li>Why make ThreadElfCore and ProcessElfCore friends? Everything seems to build OK without the friend declarations in those two classes..</li></ul></div></blockquote><div>Fixed.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div style="font-size:14px;font-family:Calibri,sans-serif;word-wrap:break-word">
<div><br>
</div>
<div>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.</div>

</div></blockquote><div><br></div><div>I actually copied mach-core plugin and modified functions as needed and preserved formatting. </div><div>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.</div>

<div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="font-size:14px;font-family:Calibri,sans-serif;word-wrap:break-word">
<div><br>
</div>
<div>Thanks again for working on this -- I'm very excited to see this functionality precipitate :)</div></div></blockquote><div>Thank you.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

<div style="font-size:14px;font-family:Calibri,sans-serif;word-wrap:break-word">
<div><br>
</div>
<div>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.)</div>
<div><br>
</div>
<div><br>
</div>
<div>Cheers,</div>
<div>Dan</div>
<div><br>
</div>
<span>
<div style="border-right:medium none;padding-right:0in;padding-left:0in;padding-top:3pt;text-align:left;font-size:11pt;border-bottom:medium none;font-family:Calibri;border-top:#b5c4df 1pt solid;padding-bottom:0in;border-left:medium none">

<br></div></span></div></blockquote></div></div></div>