Set storage class of julia globals to dllimport on windows to avoid auto-import weirdness#54572
Conversation
a53e430 to
35ec93c
Compare
|
Just to clarify: This PR targets the backports-release-1.10 branch. Does that means 1.11 and 1.12 are not affected? Or will they get separate fixes? |
|
This is mostly for testing on 1.10 but it's likely this need a similar fix on 1.11 and up |
cada51b to
a81055d
Compare
topolarity
left a comment
There was a problem hiding this comment.
LGTM! A few style nits, mostly.
I can confirm this resolves #54550 for me locally 🎉
| $(WHOLE_ARCHIVE) $< $(NO_WHOLE_ARCHIVE) \ | ||
| $(if $(findstring -debug,$(notdir $@)),-ljulia-internal-debug -ljulia-debug,-ljulia-internal -ljulia) \ | ||
| $$([ $(OS) = WINNT ] && echo '' -lssp)) | ||
| $$([ $(OS) = WINNT ] && echo '' -lssp --disable-auto-import --disable-runtime-pseudo-reloc)) |
There was a problem hiding this comment.
For the backport, I think we should enable only --disable-runtime-pseudo-reloc, since it's a smaller change and should be sufficient to prevent issues like the originating bug.
For future releases, I definitely support including both flags though.
There was a problem hiding this comment.
This is already set on the cli, so the executable doesn't have it anyway, this just prevents silent failure like before
There was a problem hiding this comment.
I was mostly thinking about the case for pkgimages in case we still have a rare dllimport problem anywhere
In that case, precompilation would fail due to --disable-auto-import even if the linker was able to avoid the pseudo relocs - but a pkgeval is probably the best way to handle that concern tbh
There was a problem hiding this comment.
We don't have a windows pkgeval :(. But i'd rather this error then potentially silently fail.
There was a problem hiding this comment.
I think the --disable-runtime-pseudo-reloc is enough to prevent the silent failures, right? The --disable-auto-import only makes the error more eager in case we got our import discipline wrong
Anyway, I'm OK with this change as-is
| proto->copyAttributesFrom(G); | ||
| // DLLImport only needs to be set for the shadow module | ||
| // it just gets annoying in the JIT | ||
| proto->setDLLStorageClass(GlobalValue::DefaultStorageClass); |
There was a problem hiding this comment.
@vtjnash It'd be good to get a confirmation that this deletion is OK
Keno
left a comment
There was a problem hiding this comment.
In the master version of this, you should also be able to turn on the sysimage building test on windows now.
6832cfb to
e1b2099
Compare
|
What happened to the history of this PR? |
|
I believe it was the force pushes to the base branch that this targets? |
|
Hm, didn't mean to cause that. There were a few commits on the backport branch that were erroneous. |
5c2040c to
036add1
Compare
…uto-import weirdness. Also make the compiler error instead of emitting pseudo-relocations when encountering a reference to a non-exported symbol in a dll. Co-authored-by: Keno Fischer <keno@juliacomputing.com> Co-authored-by: Cody Tapscott <84105208+topolarity@users.noreply.github.com>
036add1 to
f1e4a28
Compare
Backported PRs: - [x] #54361 <!-- [LBT] Upgrade to v5.9.0 --> - [x] #54474 <!-- Unalias source from dest in copytrito --> - [x] #54548 <!-- Fixes for bitcast bugs with LLVM 17 / opaque pointers --> - [x] #54191 <!-- make `AbstractPipe` public --> - [x] #53402 <!-- Add `jl_getaffinity` and `jl_setaffinity` --> - [x] #53356 <!-- Rename at-scriptdir project argument to at-script and search upwards for Project.toml --> - [x] #54545 <!-- typeintersect: fix incorrect innervar handling under circular env --> - [x] #54586 <!-- Set storage class of julia globals to dllimport on windows to avoid auto-import weirdness. Forward port of #54572 --> - [x] #54587 <!-- Accomodate for rectangular matrices in `copytrito!` --> - [x] #54617 <!-- CLI: Use `GetModuleHandleExW` to locate libjulia.dll --> - [x] #54605 <!-- Allow libquadmath to also fail as it is not available on all systems --> - [x] #54634 <!-- Fix trampoline assembly for build on clang 18 on apple silicon --> - [x] #54635 <!-- Aggressive constprop in trevc! to stabilize triangular eigvec --> - [x] #54645 <!-- ensure we set the right value to gc_first_tid --> - [x] #54554 <!-- make elsize public --> - [x] #54648 <!-- Construct LazyString in error paths for tridiag --> - [x] #54658 <!-- fix missing uuid check on extension when finding the location of an extension --> - [x] #54594 <!-- Switch to Pkg mode prompt immediately and load Pkg in the background --> - [x] #54669 <!-- Improve error message in inplace transpose --> - [x] #54671 <!-- Add boundscheck in bindingkey_eq to avoid OOB access due to data race --> - [x] #54672 <!-- make: Fix `sed` command for LLVM libraries with no symbol versioning --> - [x] #54624 <!-- more precise aliasing checks for SubArray --> - [x] #54679 <!-- 🤖 [master] Bump the Distributed stdlib from 6a07d98 to 6c7cdb5 --> - [x] #54604 <!-- Fix tbaa annotation on union selector bytes inside of structs --> - [x] #54690 <!-- Fix assertion/crash when optimizing function with dead basic block --> - [x] #54704 <!-- LazyString in reinterpretarray error messages --> - [x] #54718 <!-- fix prepend StackOverflow issue --> - [x] #54674 <!-- Reimplement dummy pkg prompt as standard prompt --> - [x] #54737 <!-- LazyString in interpolated error messages involving types --> - [x] #54642 <!-- Document GenericMemory and AtomicMemory --> - [x] #54713 <!-- make: use `readelf` for LLVM symbol version detection --> - [x] #54760 <!-- REPL: improve prompt! async function handler --> - [x] #54606 <!-- fix double-counting and non-deterministic results in `summarysize` --> - [x] #54759 <!-- REPL: Fully populate the dummy Pkg prompt --> - [x] #54702 <!-- lowering: Recognize argument destructuring inside macro hygiene --> - [x] #54678 <!-- Don't let setglobal! implicitly create bindings --> - [x] #54730 <!-- Fix uuidkey of exts in fast path of `require_stdlib` --> - [x] #54765 <!-- Handle no-postdominator case in finalizer pass --> - [x] #54591 <!-- Don't expose guard pages to malloc_stack API consumers --> - [x] #54755 <!-- [TOML] remove Dates hack, replace with explicit usage --> - [x] #54721 <!-- add try/catch around scheduler to reset sleep state --> - [x] #54631 <!-- Avoid concatenating LazyString in setindex! for triangular matrices --> - [x] #54322 <!-- effects: add new `@consistent_overlay` macro --> - [x] #54785 - [x] #54865 - [x] #54815 - [x] #54795 - [x] #54779 - [x] #54837 Contains multiple commits, manual intervention needed: - [ ] #52694 <!-- Reinstate similar for AbstractQ for backward compatibility --> - [ ] #54649 <!-- Less restrictive copyto! signature for triangular matrices --> Non-merged PRs with backport label: - [ ] #54779 <!-- make recommendation clearer on manifest version mismatch --> - [ ] #54739 <!-- finish implementation of upgradable stdlibs --> - [ ] #54738 <!-- serialization: fix relocatability bug --> - [ ] #54574 <!-- Make ScopedValues public --> - [ ] #54457 <!-- Make `String(::Memory)` copy --> - [ ] #53957 <!-- tweak how filtering is done for what packages should be precompiled --> - [ ] #53452 <!-- RFC: allow Tuple{Union{}}, returning Union{} --> - [ ] #53286 <!-- Raise an error when using `include_dependency` with non-existent file or directory --> - [ ] #51479 <!-- prevent code loading from lookin in the versioned environment when building Julia -->
…uto-import weirdness (JuliaLang#54586) Forward port of JuliaLang#54572
Fixes #54550 on 1.10. Master will require a similar but different PR.