[Lldb-commits] [PATCH] 32-bit target support on x86_64 Linux

[Greg Clayton]

On Sep 11, 2012, at 5:05 PM, "Kaylor, Andrew" <andrew.kaylor at intel.com> wrote:

> Yeah, the Linux-specific modification is OK.  I'll do a little more poking to see if I can figure out how my default triple got to be so odd.

Sounds good. Fixing the comparisong sounds like the right move IMHO.
> 
> BTW, I have llvm commit access.  Does that automatically confer lldb commit-after-review access upon me?  

Yep!

> I appreciated your having expedited the commits from my recent changes, but I'm curious as to what my status is in this regard.

Yep, we all review and then you can commit will work for us.

> 
> -Andy
> 
> -----Original Message-----
> From: Greg Clayton [mailto:gclayton at apple.com] 
> Sent: Tuesday, September 11, 2012 4:24 PM
> To: Kaylor, Andrew
> Cc: lldb-commits at cs.uiuc.edu
> Subject: Re: [Lldb-commits] [PATCH] 32-bit target support on x86_64 Linux
> 
> 
> On Sep 7, 2012, at 2:32 PM, "Kaylor, Andrew" <andrew.kaylor at intel.com> wrote:
> 
>> Hi Greg,
>> 
>> I wasn't sure about the "TripleVendorWasSpecified" part, and I'm still not sure I understand it.  Let me tell you about how I got in the position of needing that change, and maybe you can tell me what I should have done to fix it correctly.
>> 
>> I'm not sure exactly what led to this situation, but I think it has something to do with how I built lldb.  When I run lldb on my 64-bit Linux system with a 64-bit app on the command line ("lldb myapp64"), in the course of PlatformLinux::ResolveExecutable execution a call is made to lldb_private::operator==(const ArchSpec& lhs, const ArchSpec& rhs).  In this case the lhs triple (derived from llvm::sys::getDefaultTargetTriple()) is "x86_64-unknown-linux-gnu" and the rhs triple (based on examination of the executable) is "x86_64-gnu-linux".  
>> 
>> It seemed to me that these two should be reported as a match.  This conclusion also seemed to be supported by the code in lldb_private::operator==(const ArchSpec& lhs, const ArchSpec& rhs), which looks like this (in part):
>> 
>>       if (lhs_triple_vendor != rhs_triple_vendor)
>>       {
>>           const bool rhs_vendor_specified = rhs.TripleVendorWasSpecified();
>>           const bool lhs_vendor_specified = lhs.TripleVendorWasSpecified();
>>           // Both architectures had the vendor specified, so if they aren't
>>           // equal then we return false
>>           if (rhs_vendor_specified && lhs_vendor_specified)
>>               return false;
>> 
>>           // Only fail if both vendor types are not unknown
>>           if (lhs_triple_vendor != llvm::Triple::UnknownVendor &&
>>               rhs_triple_vendor != llvm::Triple::UnknownVendor)
>>               return false;
>>       }
>> 
>> In my case the original implementation concluded that both rhs and lhs vendors were specified and so returned false before reaching the lines where the comment says "Only fail if both vendor types are not unknown."
>> 
>> Based on this, I thought that the intention of the code was to treat the case where both vendors are specified but one is specified as "unknown" as a match, and so I also jumped to the conclusion (apparently incorrect) that "unknown" was a variety of "unspecified" (at least in the eyes of this code).
>> 
>> The result of this case being reported as a non-match, BTW, was that lldb reported that it didn't have a matching platform for which to load the executable.
>> 
>> It isn't clear to me how my system default target triple got to be "x86_64-unknown-linux-gnu".  I don't recall specifying anything explicitly during the configure and build process.
>> 
>> So, what's the correct fix here?
> 
> It sounds like we need to verify if "llvm::sys::getDefaultTargetTriple()" is correct or not. If it is correct, then this is the triple that LLVM/clang are going to expect when we run expression so we would want to chang the object file parser triple which was based on examination of the executable to match llvm::sys::getDefaultTargetTriple().
> 
> "x86_64-unknown-linux-gnu" just seems wrong. I just spoke with Daniel Dunbar on the clang team and he said "x86_64-pc-linux" or "x86_64-pc-linux-gnu" are fine defaults. Also for clang, they don't really do much with the vendor field for linux, so I would be ok with modifying the "lldb_private::operator==(const ArchSpec& lhs, const ArchSpec& rhs)" to ignore vendor when the OS is set to "linux". That would probably solve your problem with the least number of modification in LLDB. Does that sound ok?
> 
> Greg
> 
>> 
>> -Andy
>> 
>> -----Original Message-----
>> From: Greg Clayton [mailto:gclayton at apple.com] 
>> Sent: Friday, September 07, 2012 10:51 AM
>> To: Kaylor, Andrew
>> Cc: lldb-commits at cs.uiuc.edu
>> Subject: Re: [Lldb-commits] [PATCH] 32-bit target support on x86_64 Linux
>> 
>> Andrew:
>> 
>> This patch looks ok except this part:
>> 
>> Index: include/lldb/Core/ArchSpec.h
>> ===================================================================
>> --- include/lldb/Core/ArchSpec.h	(revision 163394)
>> +++ include/lldb/Core/ArchSpec.h	(working copy)
>> @@ -210,7 +210,10 @@
>>    bool
>>    TripleVendorWasSpecified() const
>>    {
>> -        return !m_triple.getVendorName().empty();
>> +        llvm::StringRef vendor = m_triple.getVendorName();
>> +        if (vendor.empty())
>> +            return false;
>> +        return vendor != "unknown";
>>    }
>> 
>>    bool
>> 
>> 
>> When you give an llvm::Triple just a raw architecture like "x86_64", it will always return an "unknown" enumeration when asked what its vendor and OS is, but in LLDB we want to know when the user specified "unknown" explicity since we use this when looking for the right plug-in. There currently isn't a "none" enumeration or name that is accepted by the triple. So if the user currently specifies:
>> 
>> (lldb) target create --arch x86_64-unknown-unknown /tmp/a.out
>> 
>> We will know that the user specified "unknown" with:
>> 
>>    bool
>>    TripleVendorWasSpecified() const
>>    {
>>        return !m_triple.getVendorName().empty();
>>    }
>> 
>> This causes no vendor or OS to be used when debugging and it is handy when you are debugging something on a bare board. It allows us to select the DynamicLoaderStatic to be used which just loads files at the addresses that are contained in the files themselves (absolute addressing).
>> 
>> So the "bool ArchSpec::TripleVendorWasSpecified() const" is really trying to say "did the user type something for the vendor" (which inludes "unknown" as a valid thing the user typed). Does that make sense? 
>> 
>> I committed the other changes though:
>> 
>> % svn commit
>> Sending        source/Host/common/Host.cpp
>> Sending        source/Plugins/Platform/Linux/PlatformLinux.cpp
>> Sending        source/Plugins/Process/Linux/ProcessMonitor.cpp
>> Transmitting file data ...
>> Committed revision 163398.
>> 
>> On Sep 6, 2012, at 2:45 PM, "Kaylor, Andrew" <andrew.kaylor at intel.com> wrote:
>> 
>>> The attached patch adds support for debugging 32-bit processes when running a 64-bit lldb on an x86_64 Linux system.
>>> 
>>> Making this work required two basic changes:
>>> 
>>> 1)      Getting lldb to report that it could debug 32-bit processes
>>> 2)      Changing an assumption about how ptrace works when debugging cross-platform
>>> 
>>> For the first change, I took a conservative approach and only enabled this for x86_64 Linux platforms.  It may be that the change I made in Host.cpp could be extended to other 64-bit Linux platforms, but I'm not familiar enough with the other platforms to know for sure.
>>> 
>>> For the second change, the Linux ProcessMonitor class was assuming that ptrace(PTRACE_[PEEK|POKE]DATA...) would read/write a "word" based on the child process word size.  However, the ptrace documentation says that the "word" size read or written is "determined by the OS variant."  I verified experimentally that when ptracing a 32-bit child from a 64-bit parent a 64-bit word is read or written.
>>> 
>>> This patch also modifies ArchSpec.h to recognize that "unknown" indicates an unspecified vendor choice.  Whether or not this comes into play seems to depend on how you build lldb.
>>> 
>>> -Andy
>>> <linux-32-bit-support.patch>_______________________________________________
>>> lldb-commits mailing list
>>> lldb-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/lldb-commits
>> 
>