[lldb-dev] Patch to add two new classes - RegisterContextLinux_x86_64 and RegisterContextFreeBSD_x86_64

[Samuel Jacob]

Hi Ashok,

Thanks for quick review and fixing the memory leak and dangling pointer.
Your change looks good to me. Please commit.

I would prefer "m_register_infos = NULL" rather than "m_register_infos
= 0" since m_register_infos is a pointer.
However I am fine with zero assignment also.

Thanks
Samuel

On Wed, May 1, 2013 at 7:51 AM, Thirumurthi, Ashok
<ashok.thirumurthi at intel.com> wrote:
> And this time with the patch!
>
> -----Original Message-----
> From: lldb-dev-bounces at cs.uiuc.edu [mailto:lldb-dev-bounces at cs.uiuc.edu] On Behalf Of Thirumurthi, Ashok
> Sent: Wednesday, May 01, 2013 10:33 AM
> To: Samuel Jacob; lldb-dev
> Subject: Re: [lldb-dev] Patch to add two new classes - RegisterContextLinux_x86_64 and RegisterContextFreeBSD_x86_64
>
> Hi Samuel,
>
> Thanks for preparing the patch.  I ran the 64-bit test suite on Ubuntu 12.04 with no regressions.
>
> I noticed there is no matching delete for m_register_infos (Linux/FreeBSD).  The attached patch proposes a fix by deleting on destruction and allocating on demand.  Note that EntityRegister::Materialize makes calls to ReadRegister which relies on m_register_infos in the call to GetRegisterOffset.  Previously, the following tests relied on m_register_infos living past the destruction (i.e. a side-effect of the unmatched new).  The attached patch handles this case while avoiding the memory leak:
>     tools/lldb/test$ python dotest.py --executable /path/to/lldb expression_command/issue_11588
>     tools/lldb/test$ python dotest.py --executable /path/to/lldb functionalities/register
>
> I also added a comment for DEFINE_GPR to explain that the 0 size and offset will be filled in by platform-specific classes.  I'll commit this patch based on your review,
>
> - Ashok
>
>
> -----Original Message-----
> From: lldb-dev-bounces at cs.uiuc.edu [mailto:lldb-dev-bounces at cs.uiuc.edu] On Behalf Of Samuel Jacob
> Sent: Tuesday, April 30, 2013 6:31 PM
> To: lldb-dev
> Subject: Re: [lldb-dev] Patch to add two new classes - RegisterContextLinux_x86_64 and RegisterContextFreeBSD_x86_64
>
> Somebody please review and commit.
>
> Thanks
> Samuel
>
> On Fri, Apr 26, 2013 at 3:17 PM, Samuel Jacob <samueldotj at gmail.com> wrote:
>> RegisterContext_x86_64->GPR is defined based on the host. This is done
>> at compile time by including OS specific files.
>>
>> This caused a problem in elf-coredump plugin - RegisterContext_x86_64
>> with FreeBSD GPR format cant be instantiated in Linux and vice versa.
>> The need for this is based on Greg's suggestion - coredump files
>> should be platform independent.(Refer
>> http://lists.cs.uiuc.edu/pipermail/lldb-dev/2013-February/001477.html)
>>
>> This patch adds two new classes RegisterContextLinux_x86_64 and
>> RegisterContextFreeBSD_x86_64 which can instantiated on any platform.
>>
>> I verified it compiles on a Ubuntu and on a MacMini. I also verified
>> it shows correct backtrace and correct register content on a simple
>> program. I can run more test cases for that somebody please point me
>> how do I run test suites.
>>
>> Once this is done I will send a new elf-core patch based on this.
>>
>> Thanks
>> Samuel
> _______________________________________________
> lldb-dev mailing list
> lldb-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
>
> _______________________________________________
> lldb-dev mailing list
> lldb-dev at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev