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

[Thirumurthi, Ashok]

I see.  Note that there are a few downsides to the current implementation.  First, there is a need for about 6Kb of data for m_register_infos that is currently allocated per thread.  Secondly, it's not possible to have static methods that use m_register_infos, and we have a regression in logging because of static methods that use the stale m_register_infos like GetRegisterIndexFromOffset().  Third, the base class is brittle because it has stale m_register_infos.

One possibility is for POSIXThread to maintain a new static class instance that wraps m_register_infos for each platform (let's call this RegisterLayout).  This would allow the plugins to be recoded without static methods while keeping the leaf classes lightweight.  Currently, we couple the register set with the layout in RegisterContext, whereas we need one register set per thread and one register layout per platform.

Another possibility is to have static arrays in the platform-specific classes and move all the code that uses them into the platform-specific classes.  I'll give the first approach a try,

- Ashok

-----Original Message-----
From: Samuel Jacob [mailto:samueldotj at gmail.com] 
Sent: Monday, May 06, 2013 6:31 PM
To: Thirumurthi, Ashok
Cc: lldb-dev
Subject: Re: [lldb-dev] Patch to add two new classes - RegisterContextLinux_x86_64 and RegisterContextFreeBSD_x86_64

Hi Ashok,

Any update to RegisterContext_x86_64::m_register_infos should be isolated to a particular platform(Linux, FreeBSD...).
That is why a copy was made for each platform.

With this patch, creating RegisterContext for multiple platforms would become problem.
For example in the following code, both r1 and r2 would point to same register info structure(which is wrong).
RegisterContextFreeBSD_x86_64 r1;
RegisterContextLinux_x86_64 r2;

Thanks
Samuel

On Mon, May 6, 2013 at 3:04 PM, Thirumurthi, Ashok <ashok.thirumurthi at intel.com> wrote:
> Hi Samuel,
>
> I missed the fact that the derived classes create copies of m_register_infos that should be used by non-static methods of the base class.  The attached patch addresses this point while aiming to simplify the derived class.  Note that we will have a matrix of derived classes for os x arch that can get large, so a simpler derivation has advantages.  This approach does update the register info every time that a specialization is constructed, but I think that is defensible since any bugs will be easier to reproduce.
>
> FYI, I also added a test so that asserts for invariants will have some 
> coverage in our test suite,
>
> - Ashok
>
>
> -----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:51 AM
> To: Samuel Jacob; lldb-dev
> Subject: Re: [lldb-dev] Patch to add two new classes - 
> RegisterContextLinux_x86_64 and RegisterContextFreeBSD_x86_64
>
> 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