Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Steps towards MSVC compatibility #6230

Merged
merged 10 commits into from
Apr 1, 2014
Merged

Steps towards MSVC compatibility #6230

merged 10 commits into from
Apr 1, 2014

Conversation

tkelman
Copy link
Contributor

@tkelman tkelman commented Mar 21, 2014

Add USEMSVC variable to Make.inc for handling different flags and toolchain

Fix LLVM header problem in codegen.cpp due to define on min/max

Use __cpuid intrinsic for msvc instead of inline assembly

Add explicit casts, fixup ifdefs (_WIN32 instead of __WIN32__)

The changes to the Makefiles assume you're using the compile and ar-lib wrapper scripts from Automake, explicitly overriding CC and AR. This gets to flisp and then runs into trouble: https://ci.appveyor.com/project/tkelman/julia/build/1.0.127

@tknopp
Copy link
Contributor

tknopp commented Mar 21, 2014

Great to have you working on this!

@jiahao jiahao added the windows label Mar 21, 2014
@tkelman tkelman changed the title Steps towards MSVC compatibility WIP: Steps towards MSVC compatibility Mar 22, 2014
@tkelman tkelman changed the title WIP: Steps towards MSVC compatibility Steps towards MSVC compatibility Mar 22, 2014
@tkelman
Copy link
Contributor Author

tkelman commented Mar 22, 2014

Some progress here: after updating the nmake files a bit, MSVC can build flisp but has trouble with julia_flisp.boot.inc apparently due to line endings? https://ci.appveyor.com/project/tkelman/julia/build/1.0.158

In parallel pursuing the gmake approach (just to see which gets further sooner), there is some trouble at jl_uv.c: https://ci.appveyor.com/project/tkelman/julia/build/1.0.163

The way the makefiles worked out, nmake is doing flisp first and gmake is doing libuv first. It may be that flisp line endings and libuv linking are problems both methods would run into, not sure.

@vtjnash
Copy link
Member

vtjnash commented Mar 23, 2014

Some progress here: after updating the nmake files a bit, MSVC can build flisp but has trouble with julia_flisp.boot.inc apparently due to line endings?

My initial guess is that Windows is doing a \n -> \r\n -> \r\r\n standard corruption sanitization, and then rejecting the resulting file as invalid.

In parallel pursuing the gmake approach (just to see which gets further sooner), there is some trouble at jl_uv.c: https://ci.appveyor.com/project/tkelman/julia/build/1.0.163

Again just an initial guess, but the error message looks like you are building against the upstream master libuv, rather than julia's custom-patched libuv

@tkelman
Copy link
Contributor Author

tkelman commented Mar 23, 2014

I was doing git config --global core.autocrlf input before clone so a patch to utf8proc would apply cleanly, but there might be a better way of doing that with some flag to git apply.

I may have screwed up which libuv branch the submodule was pointing to, I'll look into it.

@tkelman
Copy link
Contributor Author

tkelman commented Mar 23, 2014

Yep, I had the submodule wrong, sorry.

What do we make of this? https://ci.appveyor.com/project/tkelman/julia/build/1.0.170

/c/projects/julia/ar-lib lib -rcs libflisp.a flisp.o builtins.o string.o equalhash.o table.o iostream.o julia_extensions.o basename.o dirname.o
link flisp.o builtins.o string.o equalhash.o table.o iostream.o julia_extensions.o basename.o dirname.o flmain.o -OUT:flisp libflisp.a ../support/libsupport.a /c/projects/julia/usr/lib/libuv.a /c/projects/julia/usr/lib/libutf8proc.a kernel32.lib ws2_32.lib psapi.lib advapi32.lib iphlpapi.lib
Microsoft (R) Incremental Linker Version 12.00.21005.1
Copyright (C) Microsoft Corporation.  All rights reserved.

   Creating library flisp.lib and object flisp.exp
./flisp unittest.lsp
eval: variable 0x1.8p3 has no value

in file unittest.lsp

#0 (lambda)

make[3]: *** [flisp] Error 1
make[3]: Leaving directory `/c/projects/julia/src/flisp'

This is with the gmake approach, but nmake-compiled flisp has the same issue if you try to run unittest.lsp manually.

@tknopp
Copy link
Contributor

tknopp commented Mar 23, 2014

This smells like windows strtoddoes not support hex values...

I had similar issues when playing around with strtod and despite this unit test failing, I was able to use Julia. So maybe one can skip this for the moment and solve strtod in a separate issue.

@tknopp
Copy link
Contributor

tknopp commented Mar 23, 2014

And out of curious, you still use the C compiler right? I thought that flisp would already require C++ wham using MSVC

@tkelman
Copy link
Contributor Author

tkelman commented Mar 23, 2014

C for libuv, C++ (/TP flag) for Julia.

I was able to turn off the line endings error by adding

#ifdef _MSC_VER
#pragma warning(disable:4335)
#endif

to ast.c, and since the nmake files were skipping flisp's unit test, I tweaked the gmake version to do the same with MSVC for now. Now for gmake I need to work out the right way to link libjulia.dll, since cl doesn't work very well as a linker driver if you're only linking object and static library files.

With nmake I need to find a MSVC-compiled binary download of LLVM and get it installed in appveyor.yml.

@tkelman
Copy link
Contributor Author

tkelman commented Mar 24, 2014

That last commit touches almost every C file, adding extern "C" when using a C++ compiler. This gets the number of unresolved externals with MSVC from 69, all the way down to 4.

Two of the remaining undefined references are setjmp and longjmp, which are getting preprocessed and assembled okay by the MSVC toolchain, but the object files are not publicly exporting their symbols according to dumpbin. See the warning

/c/projects/julia/ar-lib lib -rcs libsupport.a hashing.o timefuncs.o ptrhash.o operators.o utf8.o ios.o htable.o bitvector.o int2str.o libsupportinit.o arraylist.o strtod.o asprintf.o wcwidth.o _setjmp.win32.o _longjmp.win32.o
_longjmp.win32.o : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library
_setjmp.win32.o : warning LNK4221: This object file does not define any previously undefined public symbols, so it will not be used by any link operation that consumes this library

@vtjnash, can you help on this one?

The other two are confusing, looks like maybe a problem with C vs C++ linkage?

profile.o : error LNK2019: unresolved external symbol __imp__timeGetDevCaps@8 referenced in function _profile_bt@4
LLVMSupport.lib(Path.obj) : error LNK2019: unresolved external symbol __imp__SHGetFolderPathA@20 referenced in function "public: static void __cdecl llvm::sys::Path::GetSystemLibraryPaths

@tknopp
Copy link
Contributor

tknopp commented Mar 24, 2014

This is pretty exciting. Cool!

@ihnorton
Copy link
Member

@tkelman for the first one, you could try changing the order of the object files so setjmp and longjmp are first (see eg: http://blogs.msdn.com/b/vcblog/archive/2009/07/21/linker-warning-lnk4221-and-some-tips-to-avoid-it.aspx)

For the last two, I think you need to link Shell32.lib and Winmm.lib

@tkelman
Copy link
Contributor Author

tkelman commented Mar 24, 2014

I tried changing the order around, same warning about no public symbols. If I use dumpbin -symbols to look at the MinGW-compiled version of _setjmp.win32.o, I see an External for _jl_setjmp. It's not there for the ml-assembled version.

Will try adding shell32.lib and winmm.lib to OSLIBS, thanks for that. Edit: indeed that worked, now we're down to just the assembly functions.

@tknopp
Copy link
Contributor

tknopp commented Mar 24, 2014

shouldn't it be possible to use the MinGW compiled object files for the jmp functions? Correct me if I am wrong but these should be rather plattform independent (on x68 and x64).
Of course it would be better to get ml to generate the correct files.

@vtjnash
Copy link
Member

vtjnash commented Mar 24, 2014

Assembly files are, by virtue of not using a compiler, compiler independent.

One of the things the CNAME macro does is add the dllexport flag. Perhaps ml uses a different name for this command, or can't handle multiple commands on one line

@vtjnash
Copy link
Member

vtjnash commented Mar 25, 2014

why would microsoft pick ; for their MASM comment character? (although it seems in-line with their other annoying choices: \ for path separators, ^ for escape -- sometimes, REM or ' for comment). that explains why it couldn't see the code generated from my multiline _ENTRY macro. anyone know a way to emit multiple lines from a CPP macro? (other than passing the result through s/;/\n/g, since windows doesn't have sed)

@tkelman
Copy link
Contributor Author

tkelman commented Mar 25, 2014

Hah, that explains it. If I manually split up the .code; .p2align 2,0x90; .globl _jl_setjmp; .section .drectve; .ascii " -export:" "jl_setjmp"; .section .text; .def _jl_setjmp; .scl 2; .type 32; .endef; _jl_setjmp: onto separate lines, MASM complains about the syntax, so I don't think that's the only thing that needs to be changed.

I thought having \ at the end of the line would do it, but I guess not. FWIW there is a PowerShell equivalent of sed:

(Get-Content file) | ForEach-Object { $_ -replace ";", "`n" } | Set-Content file

@vtjnash
Copy link
Member

vtjnash commented Mar 25, 2014

maybe we should just copy all of that template code into the individual files and provide translations of each line

@tkelman
Copy link
Contributor Author

tkelman commented Mar 25, 2014

Maybe? I haven't tried yet, but it may end up being necessary to get openlibm compiling here too, since it's linked statically into the REPL exe's (on Windows anyway).

@vtjnash
Copy link
Member

vtjnash commented Mar 25, 2014

It's not technically necessary to have libopenlibm, it just makes the math more accurate. I was hoping to get both working by patching the bsd_asm header as I did, but now I'm somewhat at a loss

@tkelman
Copy link
Contributor Author

tkelman commented Mar 25, 2014

I may have it,

.model small,C

.code
jl_setjmp proc
    mov    eax,DWORD PTR [esp+4]
    mov    edx,DWORD PTR [esp+0]
    mov    DWORD PTR [eax+0],ebp  
    mov    DWORD PTR [eax+4],ebx
    mov    DWORD PTR [eax+8],edi
    mov    DWORD PTR [eax+12],esi
    mov    DWORD PTR [eax+16],esp
    mov    DWORD PTR [eax+20],edx
    xor    eax,eax
    ret
jl_setjmp endp
end

gives me an External for _jl_setjmp.

This was thanks to http://kipirvine.com/asm/gettingStartedVS2012/index.htm:

@vtjnash
Copy link
Member

vtjnash commented Mar 25, 2014

Awesome. We could probably use an include file as a multiline macro, if you would like to make this general for openlibm too (thanks to your picture above for the idea)

@tkelman
Copy link
Contributor Author

tkelman commented Mar 25, 2014

Good idea, my first cut in e97ef83 is very preliminary but I believe it gives a successfully linked libjulia.dll: https://ci.appveyor.com/project/tkelman/julia/build/1.0.215

I don't have a local copy of MSVC 2012 to match the version of LLVM I'm downloading, but anyone who does should be able to replicate this from my appveyor2 branch (it's messy and has way too many commits to get lots of AppVeyor builds, I'll occasionally cherry-pick, rebase, and squash into the cleaner msvc PR branch).

It got out of src, and is now trying base, AFAIK further than anyone has gotten with MSVC before (the nmake files never tried anything more than libjulia, did they?). Next we have fenv.h which might be tricky, since MSVC isn't C99-compliant.

@vtjnash
Copy link
Member

vtjnash commented Mar 25, 2014

Not sure is this helps, but the fenv functions are provided by openlibm

It look like cl doesnt understand the -dM option we use to invoke the preprocessing step only (in $(CPP)). But that should be roughly the same as how we preprocessing the .S files

@vtjnash
Copy link
Member

vtjnash commented Mar 25, 2014

I don't think you have the right value for HUGE_VALF, but otherwise looks good

@tkelman
Copy link
Contributor Author

tkelman commented Mar 25, 2014

Thanks for looking it over. HUGE_VALF should be a single-precision positive infinity, right? Largest representable single is around 3.4e38?

@tkelman
Copy link
Contributor Author

tkelman commented Mar 25, 2014

So, tried to figure out what the -dM preprocessor flag actually does. Wandering through the git history I find it traces back to 2010: 95a480c

According to http://gcc.gnu.org/onlinedocs/gcc/Preprocessor-Options.html, -dM does this:

Instead of the normal output, generate a list of ‘#define’ directives for all the macros
defined during the execution of the preprocessor, including predefined macros.
This gives you a way of finding out what is predefined in your version of the preprocessor.

Any way to get pcre_h.jl or errno_h.jl without that capability in the preprocessor?

@tknopp
Copy link
Contributor

tknopp commented Mar 25, 2014

This is really awesome. Have you tried to take the MSVC libjulia.dll and replace it with one in a MinGW build, in order to see if julia can already be executed?

@tknopp
Copy link
Contributor

tknopp commented Mar 29, 2014

@tkelman Sorry, I forgot about that. I am currently uploading to

https://dl.dropboxusercontent.com/u/10376457/llvm3.3msvc.zip

Its the release build and only the folder with the *.lib + the include folder.

@tkelman
Copy link
Contributor Author

tkelman commented Mar 29, 2014

Thanks, cool I'll give it a try. Is it 32 or 64 bit? I have the infrastructure in place in my appveyor2 branch (everything here, plus the assembly fixes, plus MSVC additions to msys_build.sh) to get everything compiled with 32 bit, but no idea where to look about the flisp unit test failure or the overflow in numeric constant "0" (other than that it's happening in https://github.com/JuliaLang/julia/blob/master/src/julia-parser.scm#L195).

@tknopp
Copy link
Contributor

tknopp commented Mar 29, 2014

Its up now. And its 64bit

@tknopp
Copy link
Contributor

tknopp commented Mar 29, 2014

Or more precisely: I think its 64 bit :-)
Not entirely sure what I did at that time when compiling

@tkelman
Copy link
Contributor Author

tkelman commented Mar 30, 2014

Rebased for the removal of readline, and squashed a few of the minor changes to get rid of some places where I changed my mind on how to do things (couldn't get rc to work right in place of windres, travesty of travesties that the MSVC julia exe doesn't have an icon right now).

MSVC appears to be miscompiling something in flisp, but I don't know what.

64 bit is awaiting confirmation on exception-handling enums in headers.

The setjmp/longjmp assembly could be fixed quick-and-dirty here, or more elegantly (but more work that hasn't been done yet) in the preprocessor macros over in openlibm.

I've had to introduce MSVC-only dependencies, not included in the PR at this point (the getopt port, and ar-lib and compile from automake - libuv has versions of these two but they're not recent enough) that should be properly handled in deps/Makefile instead of in msys_build.sh in my personal branch.

tkelman added 10 commits March 31, 2014 06:01
Add USEMSVC variable to Make.inc for handling different flags and toolchain

Fix LLVM header problem in codegen.cpp due to define on min/max

Use __cpuid intrinsic for msvc instead of inline assembly

Add explicit casts, fixup ifdefs (_WIN32 instead of __WIN32__)
patch utf8proc so it can compile in either C or C++ mode

dont define USE_COMPUTED_GOTO for MSVC
(might be an odd double-definition problem from some header)

add utf8proc info to src/flisp/Windows.mk
(needs some improvement on handling of utf8proc
version number and assumption of cl vs icl)

add strtod to Windows.mk

add DLLEXPORT before jl_init_frontend(void)

modify OSLIBS and CONFIGURE_COMMON for msvc
compile basename and dirname for msvc

disable line endings error

define HUGE_VALF if needed

add shell32.lib and winmm.lib to OSLIBS

modify preprocessing for MSVC

add include path to openlibm for fenv.h
in order to compile Julia with a C++ compiler (specifically MSVC)
skip flisp unit test with msvc (for now?)

skip a few headers (will need to find replacements for these)
explicit casts and fix ifdefs (__WIN32__ => _WIN32)

remove unistd header already included in repl.h

remove DLLEXPORT from repl.h

use hex code for escape

put back getopt.h

define dirname and PATH_MAX for repl

fix LDFLAGS that MSVC doesn't understand

skip julia_res.o with msvc

extern C linkage on repl
crashes immediately, but it's a start

extern C on basename, dirname, jlapi

DLLEXPORT on basename, dirname

add export to link command for jl_setjmp and jl_longjmp

get uv to export symbols
@tkelman
Copy link
Contributor Author

tkelman commented Mar 31, 2014

@vtjnash Do we actually expect the EXCEPTION_EXECUTE_HANDLER case to ever occur? It doesn't look like the implementation of _exception_handler in init.c ever returns that value. If I #ifndef _MSC_VER it out, along with 2 other tiny new commits above, then 64 bit in MSVC gets exactly as far as 32 bit.

tkelman referenced this pull request Mar 31, 2014
…broken for me right now, so this only works for libjulia-release
@vtjnash
Copy link
Member

vtjnash commented Mar 31, 2014

we have full control over _exception_handler, so no, that value will never occur.

@vtjnash
Copy link
Member

vtjnash commented Mar 31, 2014

I see that travis passed the last build, so is this ready to merge?

@tkelman
Copy link
Contributor Author

tkelman commented Mar 31, 2014

I'm happy with it at the moment, so unless anyone has any review comments, yes?

vtjnash added a commit that referenced this pull request Apr 1, 2014
Steps towards MSVC compatibility
@vtjnash vtjnash merged commit 1a37f0a into JuliaLang:master Apr 1, 2014
@ihnorton
Copy link
Member

ihnorton commented Apr 1, 2014

Awesome 👍

@tknopp tknopp mentioned this pull request Apr 1, 2014
@tkelman tkelman mentioned this pull request Jul 28, 2014
@tkelman tkelman deleted the msvc branch July 4, 2015 02:45
tkelman added a commit that referenced this pull request Aug 5, 2015
I added this for MSVC in #6230 but it's not actually necessary with MSVC 2013.
Maybe it was a 2012-or-earlier thing that got fixed as they added support for more of C99.
In a `NO_GIT` build openlibm will be at a different location, so let's not hard-code it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
system:windows Affects only Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants