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

[David Blaikie]

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...

>
> 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
>>>
>