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

cleanup julia_init options #9266

Merged
merged 7 commits into from
Dec 14, 2014
Merged

cleanup julia_init options #9266

merged 7 commits into from
Dec 14, 2014

Conversation

vtjnash
Copy link
Member

@vtjnash vtjnash commented Dec 7, 2014

this attempts to be more consistent and centralized about initializing julia paths (build_path, image_file, julia_home). this is in an attempt to make embedding julia easier. also, it should help with spawning child processes (by providing more reliable access to the parameters, without dependence of the working directory)

  1. moves all configuration options into jl_compileropts
  2. computes the abspath/realpath of file paths in the config to avoid issues with chdir causing files to end up in the wrong location

@tkelman @ihnorton @staticfloat

moves all configuration options into jl_compileropts
compute the abspath/realpath of file paths in the config to avoid issues with chdir
@StefanKarpinski
Copy link
Member

💯

@staticfloat
Copy link
Member

I like this. +1

@@ -1002,20 +1107,17 @@ void jl_compile_all(void);

DLLEXPORT int julia_trampoline(int argc, char **argv, int (*pmain)(int ac,char *av[]))
{
#if defined(_OS_WINDOWS_)
SetUnhandledExceptionFilter(exception_handler);
Copy link
Member

Choose a reason for hiding this comment

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

Is this change related?

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 moved this line to be with the rest of the signal handlers (https://github.com/JuliaLang/julia/pull/9266/files#diff-885908120835b66f90958a39baeb2018R1073). i think that should be more consistent with what these functions are intended to do (and hence falls in line with "cleanup of julia init")

@vtjnash
Copy link
Member Author

vtjnash commented Dec 13, 2014

@JeffBezanson these latests commits add a few more initialization-related cleanup items (also driven by #8745-related work)

DLLEXPORT jl_jmp_buf jl_base_ctx;
static jl_jmp_buf * volatile jl_jmp_target;

#if defined(_CPU_X86_64_) || defined(_CPU_X86_)
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 probably be #if (defined(_CPU_X86_64_) || defined(_CPU_X86_)) && !defined(_MSC_VER) since inline assembly doesn't appear to be supported in MSVC for 64 bit

Also note the appveyor failure with mingw64, I'll see if I can reproduce that locally when I get a chance.

Copy link
Member Author

Choose a reason for hiding this comment

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

that's incredibly annoying.

at some point, someone will have to copy the ASM instructions to a MASM file, but for now, that's why I left the option here (plus I don't have an ARM computer to test on)

@JeffBezanson
Copy link
Member

The embedding docs should be updated; they mention JL_SET_STACK_BASE.

{
// this runs the first time we switch to a task
jl_task_t *t = jl_current_task;
jl_value_t *arg = jl_task_arg_in_transit;
Copy link
Member

Choose a reason for hiding this comment

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

Might as well git rid of this while you're at it?

Copy link
Member Author

Choose a reason for hiding this comment

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

will do. the diff in that other issue was actually against this branch, i just didn't want to include it if it wasn't correct (something about Jeff not wanting to have too many different changes happening in the same PR...)

Copy link
Member

Choose a reason for hiding this comment

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

This PR already entirely replaces the relevant code so it's kind of moot.

@vtjnash
Copy link
Member Author

vtjnash commented Dec 14, 2014

i think that addresses all comments thus far

return out;
}

static void jl_resolve_sysimg_location(JL_IMAGE_SEARCH rel)
Copy link
Member

Choose a reason for hiding this comment

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

Could you add a comment that describes what this function does? And that it operates by reading and writing jl_compileropts.

JeffBezanson added a commit that referenced this pull request Dec 14, 2014
@JeffBezanson JeffBezanson merged commit 502e5fd into master Dec 14, 2014
@vtjnash vtjnash deleted the jn/init_opt_cleanup branch December 14, 2014 18:11
@tkelman
Copy link
Contributor

tkelman commented Dec 14, 2014

This is broken on Win64! Debug builds don't work, and it segfaults in the tests. Can we pay attention to the statuses here?

@vtjnash
Copy link
Member Author

vtjnash commented Dec 14, 2014

agreed. i'm currently trying to look into why that win64 build is consistently failing

@JeffBezanson
Copy link
Member

Sorry. Will it be a quick fix or should I revert?

@tkelman
Copy link
Contributor

tkelman commented Dec 14, 2014

Not sure on the test issue. It's a MemoryError on the most recent appveyor builds but a segfault locally. The debug build fails with

    LINK usr/bin/julia-debug.exe
d000001.o:(.idata$5+0x0): multiple definition of `__imp___stack_chk_guard'
/usr/lib/gcc/x86_64-w64-mingw32/4.8/libssp.dll.a(d000008.o):(.idata$5+0x0): first defined here
d000001.o:(.idata$6+0x0): multiple definition of `__nm___stack_chk_guard'
/usr/lib/gcc/x86_64-w64-mingw32/4.8/libssp.dll.a(d000008.o):(.idata$6+0x0): first defined here
collect2: error: ld returned 1 exit status
make[2]: *** [/home/tkelman/Julia/julia-win64-seh/usr/bin/julia-debug.exe] Error 1
make[1]: *** [julia-debug] Error 2
make: *** [debug] Error 2

so I think stack_chk_guard will need to use a different name.

@vtjnash
Copy link
Member Author

vtjnash commented Dec 14, 2014

no, i can fix it quick. i'm just being too exact about the amount of stack I'm copying

on linux and win32, %RSP points to the top of the function stack frame

on win64, %RSP points to the top of the stack frame, but there's a spill area above that equal to max(4*sizeof(void*), stack space required for pushing all arguments to the function) which must be accounted for. ref http://msdn.microsoft.com/en-us/library/ew5tede7.aspx

@vtjnash
Copy link
Member Author

vtjnash commented Dec 14, 2014

__stack_chk_guard is a magic variable name that is understood and special cased by the compiler/linker. but I can work around that

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.

5 participants