Windows patches

Hi,

I haven’t heard back about some of the patches I’ve posted over the last few weeks. Some did get some discussion and I reworked them, but the final patch hasn’t made it in yet. Here’s a big patch file with my accumulated changes, which I will summarize below, fresh after an update.

Could someone help me out by evaluating them, and submit them, or give me some feedback so I can rework them?

Basically my goal is to get all the tests passing on Windows, and most of these relate to that. There are still 23 failing tests, but some of them have similar problems, so the feedback from these might help me know the most appropriate way to deal with them, and avoid wasting time doing stuff that’s not appropriate.

Also, if you’d prefer I do patches in a different way, i.e. do one file at a time, groups of similar changes, or just one big one for the remaining 23 failures, or post these to cfe-commits instead, please let me know what works best for you.

The summaries:

test/CodeGenCXX/predefined-expr.cpp:
CodeGenObjC/predefined-expr.m:
test/CodeGen/stack-protector.c:

The above all comment out the stdio.h inclusion and instead declare printf, to avoid problems parsing VC++'s stdio.h, particularly if a different target triple is specified.

test/Driver/darwin-ld.c:
test/Driver/darwin-as.c:

Add a “*” to the grep or FileCheck string to account for the “.EXE” on windows.

test/CodeGen/address-space-field2.c:
test/CodeGen/address-space-field3.c:
test/CodeGen/address-space-field4.c:

Converted to FileCheck to avoid problems with grep on Windows.

test/CodeGen/const-init.c:
test/CodeGen/cast-to-union.c:

In VC++, the *printf functions put an extra “0” in the exponent part of a floating point number. This add regular expressions to account for this.

include/clang/Frontend/InitHeaderSearch.h:
lib/Frontend/InitHeaderSearch.cpp:
tools/clang-cc/clang-cc.cpp:

This is a slight revision to the patch I posted yesterday to have the default include path code use the target triple, as the line numbers were off due to some other changes. It also sets up include paths for VC++ and Cygwin headers, along with the existing MinGW headers, plus the newer 4.4.0 version. Note that running the tests with the MinGW headers results in 40 or more failed tests. Currently, with this patch, only 23 tests are failing with the VC++ headers. I did leave in some disabled code regarding the question I posted yesterday about including windows.h for the registry stuff.

include/clang/Lex/LiteralSupport.h:
lib/Lex/LiteralSupport.cpp:

This fixes support for complex literals, reworked to avoid a goto, and to add a flag noting the presence of a Microsoft extension suffix (i8, i16, i32, i64).

lib/Basic/Targets.cpp:

Someone pointed out that in my previous Targets.cpp patch I didn’t handle the long size difference for one of the Windows targets. This fixes that.

lib/Headers/stdint.h:

I think I got this from Daniel. I believe it fixes some failing tests.

-John

P.S. Is there some reason why the VC++ buildbot is off-line?

win32all.patch (33 KB)

I checked these bits in.

And this bit. :slight_smile:

I don’t know why, but Daniel (who administers the VC++ buildbot) is on vacation for another week or so. I expect that he’ll bring it back online once he gets back.

  • Doug

For the change to VER, this isn't quite right. svn should be used to manage the line encodings. I double checked the file and it seems to be correct in svn? If you remove the file and check it out again, does it come out correctly? If it doesn't work, maybe a windows svn person could comment on the right fix. svn:mime-type text would be the only other fix I know.

Checked in. Give darwin-ld.c a try, I changed it to .* instead of just *. I'd expect that should work as well.

include/clang/Lex/LiteralSupport.h:
lib/Lex/LiteralSupport.cpp:

This fixes support for complex literals, reworked to avoid a goto, and to add a flag noting the presence of a Microsoft extension suffix (i8, i16, i32, i64).

Checked in.

lib/Basic/Targets.cpp:

Someone pointed out that in my previous Targets.cpp patch I didn't handle the long size difference for one of the Windows targets. This fixes that.

Checked in.

I put these in to speed up testing, which is always nice.

I've checked this in, along with some minor whitespacing issues. Please keep lines to 80 columns. Also, be sure to detab the files as well. In some editors, there is a setting to avoid the use of hard tabs.

I think the windows code should be boosted out to a new file, and hidden away from us mere mortals. In general #if in files is to be discouraged.

Ick. You'll have to discover some other way to do this. If there is a #define that is specific to this environment, you can test that. You can read through:

   $ clang -E -dD -xc /dev/null

and see if any spring to mind. If not, you can add one or a feature test for this and use it.

Aside from that, I think the rest of the patch has now been checked in. If there are any pieces I missed, please svn up, and send in what's missing. And welcome.

Thanks, Mike.

Can you give me some pointers on how to do a platform-specific file? I don’t see any examples, and I don’t know cmake. For example, have a “windows” subdirectory, and some conditional in the CMakeLists.txt file? And how to make sure make doesn’t pick up the file.

I figure the Basic library might be a good place for it.

Note that I found some errors in the registry part of it, and I’m experimenting with getting the windows.h include to work.

Aside from that, I think the rest of the patch has now been checked in. If there are any pieces I missed, please svn up, and send in what’s missing. And welcome.

You got them all, thanks!

-John

Oh, there are lots of ways and llvm doesn't tend to have a beautiful way to do this... but one way would be to arrange it like so:

#ifdef _MSC_VER
code
#endif

You can then just have it built normally, and call the routines from the file from the other parts of the file.

So, for example, take a concepts called filename path canonicalization (just to pick a windows difference between unix).

Support/Paths.h:

   char *canonicalize(char *);

Support/UnixPaths.cpp:

   #ifndef _MSC_VER
   char *canonicalize(char *c) {
     while (c[0] == '/' && c[1] == '/') ++c;
     return c;
   }
   #endif

Support/WinPaths.cpp:

   #ifdef _MSC_VER
   char *canonicalize(char *c) {
     // ...
   }
   #endif

another approach would be llvm/lib/System/Atomic.cpp, though, I'm not sure I really like that style as much. In general, one wraps up the system specific bits they want behind a system independent api, use that api in the main code, and then in the implementation, one can hide the details.

Here’s another crack at the windows include paths patch, using the registry this time, with a fall back to the environment (if perhaps run on a non-Windows system), with another fall back to hard-coded paths (if the env vars are not set). I also added an extra path that is typically defaulted on Windows, the PlatformSDK/Include path, where all the Windows headers are.

I rearranged the code to try to localize the #ifdefs a bit more, but rather than try to create a common place for system related things, I punted and just left them in the file for now.

On thing you might not like is the hack to the CMakeLists.txt files I put in, for working around the problem including windows.h because of the /Za option. But if anyone knows a better way…

-John

windows_include_paths_3.patch (10.5 KB)

-/// RemoveDuplicates - If there are duplicate directory entries in the specified
-/// search list, remove the later (dead) ones.
+/// RemoveDuplicates - If there are duplicate directory entries in the
+/// specified search list, remove the later (dead) ones.

No, this was fine before. Be sure to set your reflow column to be 80.

+bool getSystemRegistryString(const char *keyPath, const char *valueName,
+ char *value, size_t maxLength) {
+ (void)keyPath;
+ (void)valueName;
+ (void)value;
+ (void)maxLength;
+ return(false);
+}

No, just omit the parameter name in the definition instead of cast to (void).

About the CMakeLists.txt change, ick. Too much cut-n-paste programming. If there is a way to say add this flag for this translation unit, that'd be best, second best would be to say, remove this flag for just this unit. I was hoping there'd be a way to do that in just a few lines (like two, something like remove_flag(InitHeaderSearch.cpp, /Za)) or some such. I'h hoping a cmake person can suggest something better. Barring that, can you set a flag, check that in that flag in add_clang_library, and omit adding the /Za option?

Mike,

After reading some cmake docs, tutorials, and experimenting, here’s the best I could come up with.

There is a set_source_file_properties option, but unfortunately it doesn’t seem to let me undo the /Za option set in the macro via set_target_properties, though it does let me add additional options. Therefore, I have it re-call set_target_properties again, with the /Za option edited out, thus affecting the whole library, which I hope won’t be a problem down the road.

There was a fixme comment about using AddGnuCPlusPlusIncludePaths, but MinGW is not using the same directory structure. Therefore, I added a helper function specific to MinGW. The MinGW64 stuff I guessed at, as I don’t have it installed, but I’ll get it shortly and revise if needed.

If this is acceptable, should this be my test check-in? Or should I try something smaller first?

I’ll move to cfe-commits after this.

-John

windows_include_paths_4.patch (11.5 KB)

After reading some cmake docs, tutorials, and experimenting, here's the best I could come up with.

Looks better... I checked it in, after fixing up some of the temporary issues.

If this is acceptable, should this be my test check-in? Or should I try something smaller first?

Ah, oh well. I'll let you check in the next one. :slight_smile: