> On Jun 10, 2014, at 6:07 PM, David Blaikie <[email protected]> wrote: > > 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...
No, I’m not personally using it at all. I know there are some people at Apple who do. I received a bug report saying that it crashes, so I fixed the crash. I agree that it would be very useful to have some good tests, but I don’t know enough about it to do that. > >> >> 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
