> On Oct 26, 2017, at 3:12 PM, Davide Italiano <dccitali...@gmail.com> wrote:
>
> On Thu, Oct 26, 2017 at 3:04 PM, Jason Molenda <jmole...@apple.com> wrote:
>>
>>
>>> On Oct 26, 2017, at 10:24 AM, Davide Italiano via lldb-commits
>>> <lldb-commits@lists.llvm.org> wrote:
>>>
>>> On Thu, Oct 26, 2017 at 10:04 AM, Stephane Sezer via lldb-commits
>>> <lldb-commits@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.
J
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
