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

Fix compilation on windows and detect visual studio developer console #400

Merged
merged 11 commits into from Sep 24, 2019

Conversation

@blackhole12
Copy link
Contributor

commented Sep 24, 2019

  • Fixes an include bug in tdebug.cpp that breaks compilation on Visual Studio 2019
  • Completely rewrites the MSVC makefile to make it work with a wider range of visual studio versions
  • Updated readme to reflect that build.bat must now be run inside a Visual Studio Native Tools Command Prompt
  • Terra itself detects if it is running inside the Native Tools Command Prompt and uses those environment variables instead. This allows it to be run on any visual studio installation beyond 2015.

I have only tested this on Visual Studio 2019 16.3, but I'm reasonably confidant it should work with 2019, 2017, and 2015. The previous incarnation of the Makefile made incorrect assumptions about Microsoft knowing how to properly name things. The version of the toolset being used has nothing to do with visual studio itself (it's currently at 14.2, while Visual Studio is at 16.3), and furthermore, the minor version changes are totally ignored, because all 14.x toolsets use msvcp140.dll. These changes should ensure that building continues to work even if a different toolset is installed, regardless of what version of Visual Studio is installed. The Makefile was also modified to allow spaces in the current directory (LuaJIT unfortunately refuses to exist in a path with spaces). In theory, it should also allow you to compile for x86, but a source code error currently prevents that from happening.

The current Terra detection code for finding the visual studio compiler settings only works for Visual Studio 2015. Properly fixing this to work for any visual studio version is an enormous pain because the registry entries used here no longer exist in recent versions, and the Visual Studio 2019 Command Prompt no longer uses them to figure out where Visual Studio is installed - the batch files take advantage of the fact that it's inside the visual studio directory and simply uses relative paths to populate environment variables. For now, I have included code that detects when terra is run inside a Visual Studio Native Tools Command Prompt and instead uses the environment variables already populated by it, which provides a workaround allowing you to use terra with any visual studio version. It is important that the architecture of the command prompt matches the architecture you are compiling for - an x86 tools command prompt can't compile x64.

blackhole12 added 2 commits Sep 24, 2019
* Fixes an include bug that breaks compilation on Visual Studio 2019
* Completely rewrites the MSVC makefile to make it work with a wider range of visual studio versions
* build.bat must now be run inside a Visual Studio Native Tools Command Prompt
* Terra itself detects if it is running inside the Native Tools Command Prompt and uses those environment variables instead.
@elliottslaughter

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2019

Wow, huge thanks for putting in the time and effort to do this! I will try to look at this soon, though assuming it passes CI I expect I will pull this into master pretty soon.

If it's not too much to ask, would it be possible to take a look at #322 at some point as well? It's failing 8 tests at the moment, but the process of debugging through AppVeyor is painful enough that I'm not likely to finish it any time soon on my own. And if we get that working we can have a unified build system across Windows, Mac and Linux.

@blackhole12

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

This is failing the CI checks because the CI scripts have not been updated to reflect the new usage, I will need to look into this before it can be merged.

@elliottslaughter

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2019

Skimming through the changes so far, they look fine, so if the CI can be updated I will plan to merge this. Thanks.

blackhole12 added 6 commits Sep 24, 2019
@blackhole12

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

@elliottslaughter This change now compiles correctly and passes most of the tests, but I had to make windows-specific fixes for 5 of the tests.

This is because terra is making the poor assumption that the printf symbol actually exists. It does not and it hasn't for quite some time. Unfortunately, including stdio.h does not resolve this issue, because terra can't actually reference inline functions - it compiles the header code and then tells LLVM to look for a symbol corresponding to the function name. That symbol doesn't exist if the function is inlined!

Normally this isn't a problem, but windows uses inlined functions and macros all over the place. I don't actually know how the old tests were compiling at all, because this change was made way back in Visual Studio 2015. I've "fixed" this by including \\legacy_stdio_definitions.lib, but anyone who attempts to use printf in terra from windows is also going to run into this problem. This is a serious problem and I am likely going to file a separate issue on it.

Two tests are still marked as "failing" even though they actually succeed, due to spurious output from LLD:

class2.t:
set      =      &{int32} -> {}
get      =      &{} -> int32
0   C                                   0x00007ffab36d8573 lua_rawgeti + 51
1   unknown                             0x000000003c6303b8

I suspect this is related to a bug I fixed in my fork of LLD but never actually attempted a pull request (because I figured nobody cares). Unfortunately, because this is a bug in LLVM that hasn't been fixed yet, it is impossible to fix these tests for old LLVM versions. Furthermore, the spurious output includes a memory address, which is random and therefore can't be included in "expected output" so I'm not sure what we're going to do about it.

@elliottslaughter

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2019

Using legacy_stdio_definitions.lib sounds fine for now. Agree on starting a new issue to track that.

I'd be fine with disabling the two spurious tests on Windows. The alternative I suppose would be to add a more complicated check logic that knows what is expected, but that seems like more trouble than it's worth.

@@ -85,72 +76,77 @@ LIBS= clangAnalysis.lib clangARCMigrate.lib clangAST.lib clangASTMatchers.lib
LLVMVectorize.lib LLVMX86AsmParser.lib LLVMX86AsmPrinter.lib \
LLVMX86CodeGen.lib LLVMX86Desc.lib LLVMX86Disassembler.lib \
LLVMX86Info.lib LLVMX86Utils.lib \
$(LUAJIT_DIR)\src\lua51.lib Shlwapi.lib
"$(LUAJIT_DIR)\src\lua51.lib Shlwapi.lib"

This comment has been minimized.

Copy link
@elliottslaughter

elliottslaughter Sep 24, 2019

Collaborator

I think there's a typo on this line.

@elliottslaughter

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2019

Ok, CI seems to be passing except LLVM 3.5 on Windows hits a link error. I marked one line that looks suspicious in the diff.

@blackhole12

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

So, the tests are still failing, but now it's because it literally can't find legacy_stdio_definitions.lib because Visual Studio 2013 is actually too old to have it.

I do not know how to detect this scenario in a sane manner. There doesn't seem to be any way to detect "is the current compiler insane or not" because I'm not sure terra actually knows the specific version. This is leaning into "we have to actually modify saveobj() itself" territory.

@elliottslaughter

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2019

Do you understand how these tests currently pass under on master? Perhaps there was an intentional or unintentional change that caused them to start failing?

@blackhole12

This comment has been minimized.

Copy link
Contributor Author

commented Sep 24, 2019

These tests should never have been able to pass on master. It is also impossible to debug, because the old build environment requires that Visual Studio 2015 be installed in a specific location, and in fact some strange artifact of the precise build environment that is on appveyor may have been the only reason they were passing at all. This is not something I can reasonably debug.

@elliottslaughter

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2019

Ok, in the interest of not getting stalled on this, let's disable those tests on Windows and add a note in #401 that they need to be re-enabled once that issue is fixed.

@elliottslaughter elliottslaughter merged commit ef00caa into zdevito:master Sep 24, 2019
1 of 2 checks passed
1 of 2 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
@elliottslaughter

This comment has been minimized.

Copy link
Collaborator

commented Sep 24, 2019

The GNU mirror seems to be down, but the tests are otherwise passing, so I pulled this into master. Thanks again for working on this!

If you're not too burned out after all that, I rebased #322, and we can see how happy AppVeyor is after the rebase. I'm hoping the remaining issues in that branch will be relatively small ones since most of the tests were already passing.

@PyryM PyryM referenced this pull request Oct 6, 2019
0 of 7 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.