[lldb-dev] i386 support for linux

[Stephen Wilson]

Hi Marco,

On Wed, Feb 23, 2011 at 08:21:02PM +0100, Marco Minutoli wrote:
> Hello everybody,
> 
> some days ago I noticed that lldb at the moment is missing the support
> for i386 debugging on Linux and having some spare time I tried to
> modify the plugin for the x86_64 architecture. In the attachment you
> can find the patch. Any comments/suggestions or whatever are welcome,
> you know I am a student and I have a lot to learn.

This looks really good!  Thanks so much for working on this!

> Before updating from svn the patch was able to compile and lldb was
> able to run programs on my 32bit installation of Ubuntu. Now It seems
> that something is broken due to a missing method. The exact error is:
> 
> LinuxThread.cpp:69:22: error: no member named 'GetGenericCPUType' in
> 'lldb_private::ArchSpec'
>         switch (arch.GetGenericCPUType())
> 
> What has changed?

We have a slightly different interface to the ArchSpec class committed in 
r126278.  I just sent out a patch series to address this here:

  http://lists.cs.uiuc.edu/pipermail/lldb-commits/Week-of-Mon-20110221/002257.html

This change means your patches will need to be updated slightly.


I have a few more comments inline below:



> Index: include/lldb/Expression/ClangExpressionVariable.h
> ===================================================================
> --- include/lldb/Expression/ClangExpressionVariable.h	(revision 126320)
> +++ include/lldb/Expression/ClangExpressionVariable.h	(working copy)
> @@ -117,7 +117,7 @@
>      EnableParserVars()
>      {
>          if (!m_parser_vars.get())
> -            m_parser_vars.reset(new struct ParserVars);
> +            m_parser_vars.reset(new class ParserVars);
>      }


Can we just do "new ParserVars" here?


> Index: source/Plugins/Process/Linux/LinuxThread.cpp
> ===================================================================
> --- source/Plugins/Process/Linux/LinuxThread.cpp	(revision 126320)
> +++ source/Plugins/Process/Linux/LinuxThread.cpp	(working copy)
> @@ -19,6 +19,7 @@
>  #include "LinuxThread.h"
>  #include "ProcessLinux.h"
>  #include "ProcessMonitor.h"
> +#include "RegisterContextLinux_i386.h"
>  #include "RegisterContextLinux_x86_64.h"
>  #include "UnwindLLDB.h"
>  
> @@ -71,6 +72,10 @@
>              assert(false && "CPU type not supported!");
>              break;
>  
> +        case ArchSpec::eCPU_i386:
> +            m_reg_context_sp.reset(new RegisterContextLinux_i386(*this, 0));
> +            break;
> +
>          case ArchSpec::eCPU_x86_64:
>              m_reg_context_sp.reset(new RegisterContextLinux_x86_64(*this, 0));
>              break;


You will need to update this to use the new ArchSpec::Core enumeration
values.  It should be OK to use your register context for i486
as well. 


> +
> +    bool
> +    HardwareSingleStep(bool enable);
> +
> +    bool
> +    UpdateAfterBreakpoint();
> +
> +    struct GPR
> +    {
> +        uint32_t eax;
> +        uint32_t ebx;
> +        uint32_t ecx;
> +        uint32_t edx;
> +        uint32_t edi;
> +        uint32_t esi;
> +        uint32_t ebp;
> +        uint32_t esp;
> +        uint32_t ss;
> +        uint32_t eflags;
> +        uint32_t eip;
> +        uint32_t cs;
> +        uint32_t ds;
> +        uint32_t es;
> +        uint32_t fs;
> +        uint32_t gs;
> +    };
> +
> +    struct MMSReg
> +    {
> +        uint8_t bytes[10];
> +        uint8_t pad[6];
> +    };
> +
> +    struct XMMReg
> +    {
> +        uint8_t bytes[16];
> +    };
> +
> +    struct FPU
> +    {
> +        uint32_t    pad[2];
> +        uint16_t    fcw;
> +        uint16_t    fsw;
> +        uint8_t     ftw;
> +        uint8_t     pad1;
> +        uint16_t    fop;
> +        uint32_t    ip;
> +        uint16_t    cs;
> +        uint16_t    pad2;
> +        uint32_t    dp;
> +        uint16_t    ds;
> +        uint16_t    pad3;
> +        uint32_t    mxcsr;
> +        uint32_t    mxcsrmask;
> +        MMSReg      stmm[8];
> +        XMMReg      xmm[8];
> +        uint8_t     pad4[14*16];
> +        int         pad5;
> +    };
> +
> +    struct UserArea
> +    {
> +        GPR      regs;          // General purpose registers.
> +        int32_t  fpvalid;       // True if FPU is being used.
> +        int32_t  pad0;
> +        FPU      i387;          // FPU registers.
> +        uint32_t tsize;         // Text segment size.
> +        uint32_t dsize;         // Data segment size.
> +        uint32_t ssize;         // Stack segment size.
> +        uint32_t start_code;    // VM address of text.
> +        uint32_t start_stack;   // VM address of stack bottom (top in rsp).
> +        int32_t  signal;        // Signal causing core dump.
> +        int32_t  reserved;      // Unused.
> +        int32_t  pad1;
> +        uint32_t ar0;           // Location of GPR's.
> +        FPU*     fpstate;       // Location of FPR's.
> +        uint32_t magic;         // Identifier for core dumps.
> +        char     u_comm[32];    // Command causing core dump.
> +        uint32_t u_debugreg[8]; // Debug registers (DR0 - DR7).
> +        uint32_t error_code;    // CPU error code.
> +        uint32_t fault_address; // Control register CR3.
> +    };
> +private:
> +    UserArea user;
> +
> +    ProcessMonitor &GetMonitor();
> +
> +    bool ReadGPR();
> +    bool ReadFPR();
> +};
> +
> +#endif // #ifndef liblldb_RegisterContextLinux_i386_h_


The ordering for the GPR and FPU register structures looks off here.
They need to be binary compatible with the layout generated as a result of
ptrace GETFPREGS and GETREGS operations.

To discover the layout you need to read the kernel source.  To the best
of my knowledge it is not documented anywhere else.  In this case
the information is under arch/x86/include/asm/user_32.h.


If you don't mind respinning the patch to address the above I would be
more than happy to give it a good testing.


Thanks again for working on this!


-- 
steve