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.

Sign up for GitHub

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

win: precompile=yes #17179

Merged
merged 4 commits into from
Jul 3, 2016
Merged

win: precompile=yes #17179

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
558f44a
win: cleanup build warnings
vtjnash Jun 29, 2016
bf3b97e
win64: add Julia personality
vtjnash Jun 29, 2016
9ad1357
set precompiled=yes by default on windows
vtjnash Jun 29, 2016
5f31073
Fix assertion failure on LLVM 3.8+
Keno Jul 1, 2016
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 3 additions & 4 deletions .travis.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,7 +56,6 @@ notifications:
- http://julia.mit.edu:8000/travis-hook
before_install:
- make check-whitespace
- JULIA_SYSIMG_BUILD_FLAGS="--output-ji ../usr/lib/julia/sys.ji"
- if [ `uname` = "Linux" ]; then
Copy link
Contributor

@tkelman tkelman Jun 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bootstrapping is NOT testing the case where you have a [sys.]ji file but not a dll file, which systems without linkers need to continue testing

and I added a separate test of --precompiled=no in cmdlineargs, these are testing separate things

changes to tests and CI configuration really do not belong in the same commit as changing the default value here

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bootstrapping is NOT testing the case where you have a ji file but not a dll file

that's exactly what bootstrapping tests.

which systems without linkers need to continue testing

now that's just a red herring, since this is make, we just ran the linker

changes to tests and CI configuration really do not belong in the same commit as changing the default value here

the changes are inter-related (aside from the part where this test has severely bit-rotted)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bootstrapping doesn't have a sys.ji - this test is to be sure build_sysimg keeps working, where we may not have a linker

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm well aware of how bootstrapping works, and that it cycles through several names (not currently including sys.ji). The codepath that was being tested for in build_sysimg doesn't exist anymore. But forcing a non-default build configuration here has the potential to cause us to miss bugs (#16907) and is preventing me from testing other more interesting code paths (#16059).

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Now that --precompiled=no is tested separately we wouldn't have missed #16907 locally. If this doesn't need to be deleted to add new functionality or fix the exact bug that this PR fixes, don't delete it now, in this PR. The use case of running build_sysimg from a binary without having a toolchain present certainly still exists, and should be tested. We could instead add a separate makefile target for a ji-only sys.ji output and have that build in parallel on CI if that's in any way better. Can you leave this alone for now and address it elsewhere?

Copy link
Contributor

@tkelman tkelman Jun 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we're talking past each other. I don't care how it gets built, we can remove support for JULIA_SYSIMG_BUILD_FLAGS if you want (better in #16059 though, not here). As long as a no-linker .ji sys.ji file gets built somehow, separately from sys.dll is fine (parallel make to save CI time), and we test that Julia can start with it, I'm satisfied.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We do. It's a standard step in the build process.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's also not what this test was written to test (since that's unnecessarily redundant and not quite correct). This was originally written to test the --precompile=no configuration, but was later mistakenly altered to its current form instead.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm confused - we only emit inference.ji and sys.o in a default build. I was specifically referring to sys.ji and should have spelled that out.

Copy link
Contributor

@tkelman tkelman Jun 29, 2016 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, when the --precompiled=no configuration changed forms to being bundled inside sys.dll (and introduced as that flag) in a default build, the remaining purpose of this test was then only about the no-linker build_sysimg use case.

contrib/travis_fastfail.sh || exit 1;
mkdir -p $HOME/bin;
Expand Down Expand Up @@ -93,7 +92,7 @@ script:
- make -C moreutils mispipe
- make $BUILDOPTS -C base version_git.jl.phony
- moreutils/mispipe "make $BUILDOPTS NO_GIT=1 -C deps" bar > deps.log || cat deps.log
- make $BUILDOPTS NO_GIT=1 JULIA_SYSIMG_BUILD_FLAGS="$JULIA_SYSIMG_BUILD_FLAGS" prefix=/tmp/julia install | moreutils/ts -s "%.s"
- make $BUILDOPTS NO_GIT=1 prefix=/tmp/julia install | moreutils/ts -s "%.s"
- make $BUILDOPTS NO_GIT=1 build-stats
- du -sk /tmp/julia/*
- if [ `uname` = "Darwin" ]; then
Expand All @@ -102,8 +101,8 @@ script:
done;
fi
- cd .. && mv julia julia2
- cp /tmp/julia/lib/julia/sys.ji local.ji && /tmp/julia/bin/julia -J local.ji -e 'true' &&
/tmp/julia/bin/julia-debug -J local.ji -e 'true' && rm local.ji
- /tmp/julia/bin/julia --precompiled=no -e 'true' &&
/tmp/julia/bin/julia-debug --precompiled=no -e 'true'
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

leave the indent otherwise it's not immediately obvious this is a line continuation

- /tmp/julia/bin/julia -e 'versioninfo()'
- export JULIA_CPU_CORES=2 && export JULIA_TEST_MAXRSS_MB=600 && cd /tmp/julia/share/julia/test &&
/tmp/julia/bin/julia --check-bounds=yes runtests.jl $TESTSTORUN &&
Expand Down
2 changes: 1 addition & 1 deletion appveyor.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,6 @@ build_script:

test_script:
- usr\bin\julia -e "versioninfo()"
- copy usr\lib\julia\sys.ji local.ji && usr\bin\julia -J local.ji -e "true" && del local.ji
- usr\bin\julia --precompiled=no -e "true"
- cd test && ..\usr\bin\julia --check-bounds=yes runtests.jl all &&
..\usr\bin\julia --check-bounds=yes runtests.jl libgit2-online pkg
1 change: 0 additions & 1 deletion contrib/windows/msys_build.sh
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -166,7 +166,6 @@ for lib in SUITESPARSE ARPACK BLAS LAPACK FFTW \
done
echo 'override LIBLAPACK = $(LIBBLAS)' >> Make.user
echo 'override LIBLAPACKNAME = $(LIBBLASNAME)' >> Make.user
echo 'JULIA_SYSIMG_BUILD_FLAGS=--output-ji ../usr/lib/julia/sys.ji' >> Make.user

# Remaining dependencies:
# libuv since its static lib is no longer included in the binaries
Expand Down
13 changes: 11 additions & 2 deletions src/codegen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,13 +3,14 @@
#include "llvm-version.h"
#include "platform.h"
#include "options.h"
#if defined(_OS_WINDOWS_) && !defined(LLVM38)
// trick pre-llvm38 into skipping the generation of _chkstk calls
#if defined(_OS_WINDOWS_) && !defined(LLVM39)
// trick pre-llvm39 into skipping the generation of _chkstk calls
// since it has some codegen issues associated with them:
// (a) assumed to be within 32-bit offset
// (b) bad asm is generated for certain code patterns:
// see https://github.com/JuliaLang/julia/pull/11644#issuecomment-112276813
// also, use ELF because RuntimeDyld COFF I686 support didn't exist
// also, use ELF because RuntimeDyld COFF X86_64 doesn't seem to work (fails to generate function pointers)?
#define FORCE_ELF
#endif
#if defined(_CPU_X86_)
Expand Down Expand Up @@ -413,6 +414,9 @@ static Function *jlgetnthfieldchecked_func;
//static Function *jlsetnthfield_func;
#ifdef _OS_WINDOWS_
static Function *resetstkoflw_func;
#if defined(_CPU_X86_64_)
static Function *juliapersonality_func;
#endif
#endif
static Function *diff_gc_total_bytes_func;
static Function *jlarray_data_owner_func;
Expand Down Expand Up @@ -5410,6 +5414,11 @@ static void init_julia_llvm_env(Module *m)
resetstkoflw_func = Function::Create(FunctionType::get(T_int32, false),
Function::ExternalLinkage, "_resetstkoflw", m);
add_named_global(resetstkoflw_func, &_resetstkoflw);
#if defined(_CPU_X86_64_)
juliapersonality_func = Function::Create(FunctionType::get(T_int32, true),
Function::ExternalLinkage, "__julia_personality", m);
add_named_global(juliapersonality_func, &__julia_personality);
#endif
#ifndef FORCE_ELF
#if defined(_CPU_X86_64_)
#if defined(_COMPILER_MINGW_)
Expand Down
9 changes: 3 additions & 6 deletions src/debuginfo.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -128,9 +128,6 @@ void jl_add_linfo_in_flight(StringRef name, jl_lambda_info_t *linfo, const DataL
}

#if defined(_OS_WINDOWS_)
#if defined(_CPU_X86_64_)
extern "C" EXCEPTION_DISPOSITION _seh_exception_handler(PEXCEPTION_RECORD ExceptionRecord,void *EstablisherFrame, PCONTEXT ContextRecord, void *DispatcherContext);
#endif
static void create_PRUNTIME_FUNCTION(uint8_t *Code, size_t Size, StringRef fnname,
uint8_t *Section, size_t Allocated, uint8_t *UnwindData)
{
Expand All @@ -143,7 +140,7 @@ static void create_PRUNTIME_FUNCTION(uint8_t *Code, size_t Size, StringRef fnnam
if (!catchjmp[0]) {
catchjmp[0] = 0x48;
catchjmp[1] = 0xb8; // mov RAX, QWORD PTR [...]
*(uint64_t*)(&catchjmp[2]) = (uint64_t)&_seh_exception_handler;
*(uint64_t*)(&catchjmp[2]) = (uint64_t)&__julia_personality;
catchjmp[10] = 0xff;
catchjmp[11] = 0xe0; // jmp RAX
UnwindData[0] = 0x09; // version info, UNW_FLAG_EHANDLER
Expand Down Expand Up @@ -421,9 +418,9 @@ class JuliaJITEventListener: public JITEventListener
assert(SectionAddrCheck);
assert(SectionLoadOffset != 1);
catchjmp[SectionLoadOffset] = 0x48;
catchjmp[SectionLoadOffset + 1] = 0xb8; // mov RAX, QWORD PTR [&_seh_exception_handle]
catchjmp[SectionLoadOffset + 1] = 0xb8; // mov RAX, QWORD PTR [&__julia_personality]
*(uint64_t*)(&catchjmp[SectionLoadOffset + 2]) =
(uint64_t)&_seh_exception_handler;
(uint64_t)&__julia_personality;
catchjmp[SectionLoadOffset + 10] = 0xff;
catchjmp[SectionLoadOffset + 11] = 0xe0; // jmp RAX
UnwindData[SectionLoadOffset] = 0x09; // version info, UNW_FLAG_EHANDLER
Expand Down
5 changes: 0 additions & 5 deletions src/init.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,12 +73,7 @@ jl_options_t jl_options = { 0, // quiet
JL_OPTIONS_FAST_MATH_DEFAULT,
0, // worker
JL_OPTIONS_HANDLE_SIGNALS_ON,
#ifdef _OS_WINDOWS_
// TODO remove this when using LLVM 3.5+
JL_OPTIONS_USE_PRECOMPILED_NO,
#else
JL_OPTIONS_USE_PRECOMPILED_YES,
#endif
JL_OPTIONS_USE_COMPILECACHE_YES,
NULL, // bindto
NULL, // outputbc
Expand Down
16 changes: 12 additions & 4 deletions src/jitlayers.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -532,29 +532,37 @@ ExecutionEngine *jl_ExecutionEngine;
#endif

// MSVC's link.exe requires each function declaration to have a Comdat section
// rather than litter the code with conditionals,
// So rather than litter the code with conditionals,
// all global values that get emitted call this function
// and it decides whether the definition needs a Comdat section and adds the appropriate declaration
// TODO: consider moving this into jl_add_to_shadow or jl_dump_shadow? the JIT doesn't care, so most calls are now no-ops
template<class T> // for GlobalObject's
static T *addComdat(T *G)
{
#if defined(_OS_WINDOWS_) && defined(_COMPILER_MICROSOFT_)
#if defined(_OS_WINDOWS_) && defined(LLVM35)
if (imaging_mode && !G->isDeclaration()) {
#ifdef LLVM35
// Add comdat information to make MSVC link.exe happy
// it's valid to emit this for ld.exe too,
// but makes it very slow to link for no benefit
if (G->getParent() == shadow_output) {
#if defined(_COMPILER_MICROSOFT_)
Comdat *jl_Comdat = G->getParent()->getOrInsertComdat(G->getName());
// ELF only supports Comdat::Any
jl_Comdat->setSelectionKind(Comdat::NoDuplicates);
G->setComdat(jl_Comdat);
#endif
#if defined(_CPU_X86_64_)
// Add unwind exception personalities to functions to handle async exceptions
assert(!juliapersonality_func || juliapersonality_func->getParent() == shadow_output);
if (Function *F = dyn_cast<Function>(G))
F->setPersonalityFn(juliapersonality_func);
#endif
}
// add __declspec(dllexport) to everything marked for export
if (G->getLinkage() == GlobalValue::ExternalLinkage)
G->setDLLStorageClass(GlobalValue::DLLExportStorageClass);
else
G->setDLLStorageClass(GlobalValue::DefaultStorageClass);
#endif
}
#endif
return G;
Expand Down
2 changes: 2 additions & 0 deletions src/julia_internal.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -343,6 +343,8 @@ typedef struct {
uint64_t jl_getUnwindInfo(uint64_t dwBase);
#ifdef _OS_WINDOWS_
#include <dbghelp.h>
JL_DLLEXPORT EXCEPTION_DISPOSITION __julia_personality(
PEXCEPTION_RECORD ExceptionRecord, void *EstablisherFrame, PCONTEXT ContextRecord, void *DispatcherContext);
extern HANDLE hMainThread;
typedef CONTEXT bt_context_t;
#if defined(_CPU_X86_64_)
Expand Down
3 changes: 1 addition & 2 deletions src/signals-win.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,8 +278,7 @@ static LONG WINAPI exception_handler(struct _EXCEPTION_POINTERS *ExceptionInfo)
}

#if defined(_CPU_X86_64_)
EXCEPTION_DISPOSITION _seh_exception_handler(PEXCEPTION_RECORD ExceptionRecord, void *EstablisherFrame, PCONTEXT ContextRecord, void *DispatcherContext)
{
JL_DLLEXPORT EXCEPTION_DISPOSITION __julia_personality(PEXCEPTION_RECORD ExceptionRecord, void *EstablisherFrame, PCONTEXT ContextRecord, void *DispatcherContext) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should be wrapped

EXCEPTION_POINTERS ExceptionInfo;
ExceptionInfo.ExceptionRecord = ExceptionRecord;
ExceptionInfo.ContextRecord = ContextRecord;
Expand Down
6 changes: 4 additions & 2 deletions src/threading.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <inttypes.h>

#include "julia.h"
#include "julia_internal.h"
Expand Down Expand Up @@ -89,11 +90,12 @@ BOOLEAN WINAPI DllMain(IN HINSTANCE hDllHandle, IN DWORD nReason,
TlsFree(jl_tls_key);
break;
}
return 1; // success
}

JL_DLLEXPORT JL_CONST_FUNC jl_tls_states_t *(jl_get_ptls_states)(void)
{
return TlsGetValue(jl_tls_key);
return (jl_tls_states_t*)TlsGetValue(jl_tls_key);
}

jl_get_ptls_states_func jl_get_ptls_states_getter(void)
Expand Down Expand Up @@ -545,7 +547,7 @@ JL_DLLEXPORT void jl_threading_profile(void)
if (!fork_ns) return;

printf("\nti profile:\n");
printf("prep: %g (%llu)\n", NS_TO_SECS(prep_ns), (unsigned long long)prep_ns);
printf("prep: %g (%" PRIu64 ")\n", NS_TO_SECS(prep_ns), prep_ns);

uint64_t min, max, avg;
ti_timings(fork_ns, &min, &max, &avg);
Expand Down