[Lldb-commits] [lldb] r316673 - Allow SysV-i386 ABI on everything other than Apple targets

[Jason Molenda via lldb-commits]

> On Oct 26, 2017, at 10:24 AM, Davide Italiano via lldb-commits <lldb-commits at lists.llvm.org> wrote:
> 
> On Thu, Oct 26, 2017 at 10:04 AM, Stephane Sezer via lldb-commits
> <lldb-commits at lists.llvm.org> wrote:
>> Author: sas
>> Date: Thu Oct 26 10:04:20 2017
>> New Revision: 316673
>> 
>> URL: http://llvm.org/viewvc/llvm-project?rev=316673&view=rev
>> Log:
>> Allow SysV-i386 ABI on everything other than Apple targets
>> 
>> Summary:
>> This matches other SysV ABIs that are different on Apple and non-Apple targets,
>> like `ABISysV_arm.cpp` for instance.
>> 
>> Reviewers: clayborg, emaste
>> 
>> Subscribers: aemerson, kristof.beyls, lldb-commits
>> 
>> Differential Revision: https://reviews.llvm.org/D39335
>> 
>> Modified:
>>    lldb/trunk/source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
>> 
>> Modified: lldb/trunk/source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp
>> URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp?rev=316673&r1=316672&r2=316673&view=diff
>> ==============================================================================
>> --- lldb/trunk/source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp (original)
>> +++ lldb/trunk/source/Plugins/ABI/SysV-i386/ABISysV_i386.cpp Thu Oct 26 10:04:20 2017
>> @@ -205,11 +205,12 @@ ABISysV_i386::GetRegisterInfoArray(uint3
>> ABISP
>> ABISysV_i386::CreateInstance(lldb::ProcessSP process_sp, const ArchSpec &arch) {
>>   static ABISP g_abi_sp;
>> -  if ((arch.GetTriple().getArch() == llvm::Triple::x86) &&
>> -      (arch.GetTriple().isOSLinux() || arch.GetTriple().isOSFreeBSD())) {
>> -    if (!g_abi_sp)
>> -      g_abi_sp.reset(new ABISysV_i386(process_sp));
>> -    return g_abi_sp;
>> +  if (arch.GetTriple().getVendor() != llvm::Triple::Apple) {
>> +    if (arch.GetTriple().getArch() == llvm::Triple::x86) {
>> +      if (!g_abi_sp)
>> +        g_abi_sp.reset(new ABISysV_i386(process_sp));
>> +      return g_abi_sp;
>> +    }
>>   }
>>   return ABISP();
>> }
>> 
> 
> This seems to change a fairly fundamental function for lldb-i386.
> I think we should have an unit-test for this. Sorry for being
> pedantic, I promise I'll stop after this one.


It's a good suggestion, and not something we test today.  Right now there are two i386 ABIs that lldb supports:  Darwin and SysV.  This patch implements that correctly -- but the obvious problem is if a third i386 ABI is added in the future.  Now it's a race to see whether SysV-i386 or CrazyOtherABI-i386 gets CreateInstance'd, depending on the order they're registered or something.  And I'm not sure how you write a test today that would test a new target that uses CrazyOtherABI-i386 is getting the correct plugin activated.



J