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. Thanks! -- Davide _______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
