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

Replace sizeof(jl_tagged_value_t) with sizeof(void*) for MSVC #11776

Merged
merged 1 commit into from
Jun 21, 2015

Conversation

tkelman
Copy link
Contributor

@tkelman tkelman commented Jun 20, 2015

This (along with one small int128 patch, not included with this PR) results in a successful MSVC-built julia repl, see discussion at 8b8b261#commitcomment-11018031

MSVC was getting a different answer for sizeof(jl_taggedvalue_t) because there are some GNU extension tricks that make the current "types as C structs" arrangement work.

Amazingly all the linalg tests pass (ccalling into a mingw-built openblas dll), there's a failure in numbers that's likely due to missing int128 intrinsics, and I still need to run the rest of the tests to get an idea what other failures there are several other failures to look into. See #7761 (comment) for instructions on how to build with MSVC - it currently downloads mingw-built dependency dll's from a nightly julia binary, but leveraging #11754 should eventually be able to make the build process less hacky, including building as many deps as possible with MSVC to make things msys and mingw-independent.

Should probably open an MSVC support tracking issue after this gets merged, to figure out how to manage any MSVC-only changes in base, get an MSVC-built Julia to properly identify itself as such, etc.

this (along with one small int128 patch) results in a successful
msvc-built julia repl, see discussion at 8b8b261#commitcomment-11018031
@tkelman tkelman mentioned this pull request Jun 20, 2015
@JeffBezanson
Copy link
Member

Awesome!

@tkelman
Copy link
Contributor Author

tkelman commented Jun 20, 2015

(many thanks to @vtjnash for repeated debugging help and answering my various dumb questions over several months)

@vtjnash
Copy link
Member

vtjnash commented Jun 20, 2015

lgtm

(edit: looks like travis passed, but someone canceled the mac build causing the status here to look bad)

tkelman added a commit that referenced this pull request Jun 21, 2015
Replace sizeof(jl_tagged_value_t) with sizeof(void*) for MSVC
@tkelman tkelman merged commit 19de000 into master Jun 21, 2015
@tkelman tkelman deleted the tk/msvc-success branch June 21, 2015 05:24
@tkelman
Copy link
Contributor Author

tkelman commented Jul 2, 2015

MSVC build was broken again by the removal of sys.ji, this patch can bring it back https://gist.github.com/f8bf6baf23b28839ea30 when run with -J usr/lib/julia/sys.ji. That's not really suitable for committing though, a better fix involving using the MSVC linker to link the dll's should work when using recent LLVM (for comdat) but I don't have a build of that anywhere. I just sent a message to llvmdev asking whether their MSVC Windows binaries could include more than just clang in the future.

@ihnorton
Copy link
Member

ihnorton commented Jul 2, 2015

I just sent a message to llvmdev asking whether their MSVC Windows
binaries could include more than just clang in the future.

Maybe if we ask often enough :)
http://llvm.1065342.n5.nabble.com/ORC-jit-example-was-refs-to-LLVM-consultants-td80154.html#a80198

On Thu, Jul 2, 2015 at 12:40 AM, Tony Kelman notifications@github.com
wrote:

MSVC build was broken again by the removal of sys.ji, this patch can bring
it back https://gist.github.com/f8bf6baf23b28839ea30 when run with -J
usr/lib/julia/sys.ji. That's not really suitable for committing though, a
better fix involving using the MSVC linker to link the dll's should work
when using recent LLVM (for comdat) but I don't have a build of that
anywhere. I just sent a message to llvmdev asking whether their MSVC
Windows binaries could include more than just clang in the future.


Reply to this email directly or view it on GitHub
#11776 (comment).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants