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

[Jason Molenda via lldb-commits]

> On Oct 26, 2017, at 3:21 PM, Jason Molenda <jmolenda at apple.com> wrote:
> 
> 
> 
>> On Oct 26, 2017, at 3:12 PM, Davide Italiano <dccitaliano at gmail.com> wrote:
>> 
>> On Thu, Oct 26, 2017 at 3:04 PM, Jason Molenda <jmolenda at apple.com> wrote:
>>> 
>>> 
>>>> 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.
>>> 
>>> 
>> 
>> Thanks for your reply, Jason. I'm not sure how to test this either,
>> but I'll take a look.
>> In theory, (or at lesat what I have in mind :)) you should be able to
>> have a unit test that just allocates an object and calls
>> createInstance() directly [if possible], then checks that the result
>> is of the right type? [ABISysV_i386 vs ABIDarwin_i386 or something
>> like that?]
>> That won't of course take care of the race, but the test will break in
>> case somebody deletes code from the function (and/or allocates an
>> object with the wrong ABI).
>> I think it's not testing this feature entirely (and I think testing
>> the lack of races might be hard, but at least should give us some
>> coverage [if nothing, to discriminate dead code VS non-dead code].
>> To be fair, I haven't looked into how hard this is to get working, but
>> I might. CC:ing Zachary, maybe he has some ideas.
> 
> 
> 
> Yep agree, this should be easy to do in a unit test.  I think it could be as simple as
> 
> ArchSpec arch("apple-i386-macosx");
> ABISP m_abi_sp = ABI::FindPlugin(ProcessSP(), archspec);
> 
> if (m_abi_sp.get() == nullptr || m_abi_sp->GetPluginName() != ConstString("abi.macosx-i386"))
>  fail;
> 
> 
> I don't think any ABI actually needs a live Process object for their CreateInstance() methods today.  If it becomes necessary in the future, then that would need to be done for real in the unit test.
> 


(ugh, I got the order of the triple wrong above.  i386-apple-macosx of course.)