r210584 - Fix crash with x86_64-pc-win32-macho target. <rdar://problem/17235840>

[Bob Wilson]

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.

On Jun 10, 2014, at 6:00 PM, David Blaikie <dblaikie at gmail.com> 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 <bob.wilson at apple.com> 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 <rnk at google.com> 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 <bob.wilson at apple.com> 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 <echristo at gmail.com>
>>>> 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 <bob.wilson at apple.com>
>>>> 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
>>>>> cfe-commits at cs.uiuc.edu
>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>> 
>>> 
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>> 
>> 
>> 
>> 
>> _______________________________________________
>> cfe-commits mailing list
>> cfe-commits at cs.uiuc.edu
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>