[Lldb-commits] [PATCH] Fixes for 32-bit linux ptrace and RegisterContext issues

[Greg Clayton]

One question: is source/Plugins/Process/Linux/ProcessMonitor.cpp only compiled natively on systems that can use it? If so, the the "pid_t" argument will probably work. Otherwise, you might want to change it to use a explicitly sized int type (int32_t?).

I worry that using RegisterValue::GetAsUInt32() on a 64 bit register value and truncating the data with you change: "case eTypeUInt64:   return m_data.uint32;" will cause problems. Can you just switch clients over to use RegisterValue::GetAsUInt64() for those that need to? That would seem like a better fix.

Thanks for taking to time to get the 32 bit linux build working, that will be a great benefit to all LLDB users.

Greg

On Feb 27, 2014, at 5:04 AM, Matthew Gardiner <mg11 at csr.com> wrote:

> Hi Folks,
> 
> I have been struggling with trying to get a 32-bit linux build of lldb to debug
> a simple program. My primary angst was the false reporting of the presence of
> a watchpoint, resulting in an assertion fail. I've tracked that done now, and
> the implication of my findings are that any usage of the PTRACE/PtraceWrapper
> code in Linux/ProcessMonitor.cpp for 32-bit linux is buggy, if
> LLDB_CONFIGURATION_BUILDANDINTEGRATION is not defined.
> 
> The crux of the problem is this prototype
> 
> extern long
> PtraceWrapper(int req, lldb::pid_t pid, void *addr, void *data, size_t data_size,
>              const char* reqName, const char* file, int line)
> 
> using lldb::pid_t as pid which 64-bit;
> 
> when this is fed into ptrace which is prototyped as:
> 
> extern long int ptrace (enum __ptrace_request __request, ...);
> 
> so badness occurs when a 64-bit pid is pulled off the stack by an
> implementation expecting 32-bits. Obviously the addr and data parameters
> are corrupted...
> 
> Another problem I've encountered occurs because the RegisterValue class
> returns fail value for GetAsUInt32 if the object is originally constructed
> using a 64-bit integer.
> 
> Finally, the 64-bit register context was being used to debug 32-bit cores.
> 
> The following patch fixes those issues and the result is that 32-bit linux
> lldb can debug 32-bit linux applications.
> 
> Index: source/Core/RegisterValue.cpp
> ===================================================================
> --- source/Core/RegisterValue.cpp	(revision 202380)
> +++ source/Core/RegisterValue.cpp	(working copy)
> @@ -706,6 +706,7 @@
>         case eTypeUInt8:    return m_data.uint8;
>         case eTypeUInt16:   return m_data.uint16;
>         case eTypeUInt32:   return m_data.uint32;
> +        case eTypeUInt64:   return m_data.uint32;
>         case eTypeFloat:
>             if (sizeof(float) == sizeof(uint32_t))
>                 return m_data.uint32;
> Index: source/Plugins/Process/Linux/ProcessMonitor.cpp
> ===================================================================
> --- source/Plugins/Process/Linux/ProcessMonitor.cpp	(revision 202380)
> +++ source/Plugins/Process/Linux/ProcessMonitor.cpp	(working copy)
> @@ -157,7 +157,7 @@
> // Wrapper for ptrace to catch errors and log calls.
> // Note that ptrace sets errno on error because -1 can be a valid result (i.e. for PTRACE_PEEK*)
> extern long
> -PtraceWrapper(int req, lldb::pid_t pid, void *addr, void *data, size_t data_size,
> +PtraceWrapper(int req, pid_t pid, void *addr, void *data, size_t data_size,
>               const char* reqName, const char* file, int line)
> {
>     long int result;
> @@ -171,10 +171,10 @@
>         result = ptrace(static_cast<__ptrace_request>(req), pid, *(unsigned int *)addr, data);
>     else
>         result = ptrace(static_cast<__ptrace_request>(req), pid, addr, data);
> -
> +	
>     if (log)
>         log->Printf("ptrace(%s, %" PRIu64 ", %p, %p, %zu)=%lX called from file %s line %d",
> -                    reqName, pid, addr, data, data_size, result, file, line);
> +                    reqName, static_cast<lldb::pid_t>(pid), addr, data, data_size, result, file, line);
> 
>     PtraceDisplayBytes(req, data, data_size);
> 
> Index: source/Plugins/Process/POSIX/POSIXThread.cpp
> ===================================================================
> --- source/Plugins/Process/POSIX/POSIXThread.cpp	(revision 202380)
> +++ source/Plugins/Process/POSIX/POSIXThread.cpp	(working copy)
> @@ -190,7 +190,9 @@
>                         reg_interface = new RegisterContextFreeBSD_x86_64(target_arch);
>                         break;
>                     case llvm::Triple::Linux:
> -                        reg_interface = new RegisterContextLinux_x86_64(target_arch);
> +                        reg_interface = ArchSpec::eCore_x86_64_x86_64 == target_arch.GetCore() ?
> +							static_cast<RegisterInfoInterface*>(new RegisterContextLinux_x86_64(target_arch)) :
> +							static_cast<RegisterInfoInterface*>(new RegisterContextLinux_i386(target_arch));
>                         break;
>                     default:
>                         assert(false && "OS not supported");
> 
> 
> Would someone be able to test that this patch is sane for 64-bit linux,
> then commit it to SVN?
> 
> thanks
> Matthew Gardiner
> 
> 
> Member of the CSR plc group of companies. CSR plc registered in England and Wales, registered number 4187346, registered office Churchill House, Cambridge Business Park, Cowley Road, Cambridge, CB4 0WZ, United Kingdom
> More information can be found at www.csr.com. Keep up to date with CSR on our technical blog, www.csr.com/blog, CSR people blog, www.csr.com/people, YouTube, www.youtube.com/user/CSRplc, Facebook, www.facebook.com/pages/CSR/191038434253534, or follow us on Twitter at www.twitter.com/CSR_plc.
> New for 2014, you can now access the wide range of products powered by aptX at www.aptx.com.
> _______________________________________________
> lldb-commits mailing list
> lldb-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits