> On Oct 26, 2017, at 3:21 PM, Jason Molenda <jmole...@apple.com> wrote: > > > >> 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. >
(ugh, I got the order of the triple wrong above. i386-apple-macosx of course.) _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
