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

[Davide Italiano via lldb-commits]

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.

Thanks!

--
Davide