On Tue, Jun 10, 2014 at 6:04 PM, Bob Wilson <[email protected]> wrote: > Thanks for fixing the output to the source tree. I should have caught that. > > I don’t especially know what to test for here. I wasn’t involved in setting > up this win32-macho stuff, and I don’t know much about how it works. > Apparently whoever did set it up neglected to add any tests. I won’t claim > this is a good test, but it is far better than what we had before (nothing!). > If someone who knows about this can provide a better test, then that would be > great.
So how'd you come across the issue? I assume you're /using/ it, in which case it might be useful to add some tests/understand how it works... > > On Jun 10, 2014, at 6:00 PM, David Blaikie <[email protected]> wrote: > >> I've modified this test so it doesn't write to the source tree (by >> adding -o /dev/null) in r210618, but I'll echo Ried and Eric's >> sentiments and add one of my own: >> >> "test this command doesn't crash" is a fairly poor test case. Please >> verify that whatever behavior you expect (and I'm sure it's something >> more precise than "does anything other than crash") actually occurs >> (using FileCheck, presumably). Crashes are a great way to find missing >> test coverage, and simply adding a "this doesn't crash" test is a real >> lost opportunity to actually test whatever we had failed to test >> previously. >> >> And, yes, as Eric/Reid said, if you've found missing test coverage in >> other areas, add that to the relevant test directories. The driver >> should be tested in test/Driver. >> >> On Tue, Jun 10, 2014 at 4:58 PM, Bob Wilson <[email protected]> wrote: >>> It would be sufficient, but since I think any future breakage of this is >>> equally likely to come from the driver, I intentionally made the test to run >>> through the driver. >>> >>> On Jun 10, 2014, at 3:09 PM, Reid Kleckner <[email protected]> wrote: >>> >>> This runline is sufficient, then: >>> // RUN: %clang_cc1 %s -triple x86_64-pc-win32-macho -emit-llvm-only >>> >>> >>> On Tue, Jun 10, 2014 at 2:46 PM, Bob Wilson <[email protected]> wrote: >>>> >>>> The most important check is for header search, but I was hoping it might >>>> serve as an overall sanity check for other issues. I already found one >>>> issue >>>> with i386 that is not exposed with -fsyntax-only. (I’m looking at that now >>>> and will fix it if there’s an easy solution.) I don’t feel strongly about >>>> this test, so in the interest of reducing testing time, I’ll change it to >>>> use -fsyntax-only. >>>> >>>>> On Jun 10, 2014, at 2:29 PM, Eric Christopher <[email protected]> >>>>> wrote: >>>>> >>>>> What are you trying to test with this test? It's currently invoking >>>>> the backend and I'm not sure I see a reason for it given the original >>>>> change is only to header search? >>>>> >>>>> -eric >>>>> >>>>> On Tue, Jun 10, 2014 at 2:07 PM, Bob Wilson <[email protected]> >>>>> wrote: >>>>>> Author: bwilson >>>>>> Date: Tue Jun 10 16:07:12 2014 >>>>>> New Revision: 210584 >>>>>> >>>>>> URL: http://llvm.org/viewvc/llvm-project?rev=210584&view=rev >>>>>> Log: >>>>>> Fix crash with x86_64-pc-win32-macho target. <rdar://problem/17235840> >>>>>> >>>>>> The changes in r204978 broke win32-macho targets. There were checks >>>>>> added for >>>>>> MSVC and Itanium environments as special cases, and win32-macho needs >>>>>> to be >>>>>> treated the same way. >>>>>> >>>>>> Added: >>>>>> cfe/trunk/test/Misc/win32-macho.c >>>>>> Modified: >>>>>> cfe/trunk/lib/Frontend/InitHeaderSearch.cpp >>>>>> >>>>>> Modified: cfe/trunk/lib/Frontend/InitHeaderSearch.cpp >>>>>> URL: >>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Frontend/InitHeaderSearch.cpp?rev=210584&r1=210583&r2=210584&view=diff >>>>>> >>>>>> ============================================================================== >>>>>> --- cfe/trunk/lib/Frontend/InitHeaderSearch.cpp (original) >>>>>> +++ cfe/trunk/lib/Frontend/InitHeaderSearch.cpp Tue Jun 10 16:07:12 >>>>>> 2014 >>>>>> @@ -472,7 +472,8 @@ void InitHeaderSearch::AddDefaultInclude >>>>>> >>>>>> case llvm::Triple::Win32: >>>>>> if (triple.getEnvironment() == llvm::Triple::MSVC || >>>>>> - triple.getEnvironment() == llvm::Triple::Itanium) >>>>>> + triple.getEnvironment() == llvm::Triple::Itanium || >>>>>> + triple.getObjectFormat() == llvm::Triple::MachO) >>>>>> return; >>>>>> break; >>>>>> } >>>>>> >>>>>> Added: cfe/trunk/test/Misc/win32-macho.c >>>>>> URL: >>>>>> http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Misc/win32-macho.c?rev=210584&view=auto >>>>>> >>>>>> ============================================================================== >>>>>> --- cfe/trunk/test/Misc/win32-macho.c (added) >>>>>> +++ cfe/trunk/test/Misc/win32-macho.c Tue Jun 10 16:07:12 2014 >>>>>> @@ -0,0 +1,2 @@ >>>>>> +// Check that basic use of win32-macho targets works. >>>>>> +// RUN: %clang -c -target x86_64-pc-win32-macho %s >>>>>> >>>>>> >>>>>> _______________________________________________ >>>>>> cfe-commits mailing list >>>>>> [email protected] >>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>>> >>>> >>>> _______________________________________________ >>>> cfe-commits mailing list >>>> [email protected] >>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> >>> >>> >>> >>> _______________________________________________ >>> cfe-commits mailing list >>> [email protected] >>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits >>> > _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
