Patch to add two new classes - RegisterContextLinux_x86_64 and RegisterContextFreeBSD_x86_64

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_RegisterContextx86-64.diff (26.7 KB)

Somebody please review and commit.

Thanks
Samuel

I will have to defer to someone in the linux community. Can someone on the linux front take a look at this patch?

Greg

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

And this time with the patch!

PlatformRegisterContext_x86-64.patch (27.2 KB)

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

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

log-regs.patch (9.67 KB)

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

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

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.

Agree with the downsides. Slight correction m_register_infos is
allocated only once per platform.

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.

Good idea it will make RegisterContext classes neat.

Thanks
Samuel

m_register_infos is allocated only once per platform.

Well, a single instance of RegisterContext will allocate a single copy of g_register_infos. However, we have one instance of RegisterContext per POSIXThread. This is required to have a register set per thread, but not to have static metadata like g_register_infos,

- Ashok

m_register_infos is allocated only once per platform.

Well, a single instance of RegisterContext will allocate a single copy of g_register_infos. However, we have one instance of RegisterContext per POSIXThread. This is required to have a register set per thread, but not to have static metadata like g_register_infos,

Good point, I lost track of where trunk was.

The attached plugin removes the requirement for static methods like GetRegisterIndexFromOffset() in the base class. There is also a minor cleanup of the platform-specific plugins. I removed m_register_infos from the base class so that there is no stale copy of this variable. I also modified the test as I don't see any evidence that "log enable Darwin registers" is supported. In summary:

Fixed "log enable linux registers" and added a test.
- Eliminated the use of static for methods that read m_register_infos, so that these routines can be implemented in the base class.
- Eliminated m_register_infos in the base class because this is not used when derived classes call UpdateRegisterInfo.
- Also moved the namespace using declarations from headers to source files.

- Ashok

log-regs-4.diff (23.5 KB)