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

[Bob Wilson]

> On Jun 10, 2014, at 6:07 PM, David Blaikie <dblaikie at gmail.com> wrote:
> 
> On Tue, Jun 10, 2014 at 6:04 PM, Bob Wilson <bob.wilson at apple.com> 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 <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
>>>> 
>>