search

terralang / terra

Terra is a low-level system programming language that is embedded in and meta-programmed by the Lua programming language.
terralang.org
Other
2.72k stars 201 forks source link

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

Closed ErikMcClure closed 5 years ago

ErikMcClure commented 5 years ago

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.

elliottslaughter commented 5 years ago

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.

ErikMcClure commented 5 years ago

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 commented 5 years ago

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

ErikMcClure commented 5 years ago

@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 commented 5 years ago

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.

elliottslaughter commented 5 years ago

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.

ErikMcClure commented 5 years ago

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 commented 5 years ago

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?

ErikMcClure commented 5 years ago

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 commented 5 years ago

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 commented 5 years ago

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.