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 <[email protected]> wrote: > And this time with the patch! > > -----Original Message----- > From: [email protected] [mailto:[email protected]] 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: [email protected] [mailto:[email protected]] 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 <[email protected]> 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 > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev > > _______________________________________________ > lldb-dev mailing list > [email protected] > http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev _______________________________________________ lldb-dev mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/lldb-dev
