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

David Blaikie dblaikie at gmail.com
Tue Jun 10 18:00:04 PDT 2014


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
>




More information about the cfe-commits mailing list