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

Jump to bottom

Set storage class of julia globals to dllimport on windows to avoid auto-import weirdness #54572

Merged
merged 1 commit into from
May 28, 2024
Merged

Set storage class of julia globals to dllimport on windows to avoid auto-import weirdness #54572

merged 1 commit into from
May 28, 2024

Conversation

gbaraldi
Copy link
Member

@gbaraldi gbaraldi commented May 24, 2024 •
edited
Loading

Fixes #54550 on 1.10. Master will require a similar but different PR.

@gbaraldi gbaraldi force-pushed the gb/why-windows-why branch 2 times, most recently from a53e430 to 35ec93c Compare May 24, 2024 12:48
@fingolfin
Copy link
Member

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?

@gbaraldi
Copy link
Member Author

This is mostly for testing on 1.10 but it's likely this need a similar fix on 1.11 and up

@gbaraldi gbaraldi added DO NOT MERGE Do not merge this PR! labels May 24, 2024
@gbaraldi gbaraldi force-pushed the gb/why-windows-why branch 4 times, most recently from cada51b to a81055d Compare May 24, 2024 19:22
topolarity
topolarity approved these changes May 24, 2024
View reviewed changes
Copy link
Member

@topolarity topolarity left a comment

Choose a reason for hiding this comment

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

LGTM! A few style nits, mostly.

I can confirm this resolves #54550 for me locally 🎉

src/staticdata.c Outdated Show resolved Hide resolved
sysimage.mk
@@ -16,7 +16,7 @@ $(build_private_libdir)/%.$(SHLIB_EXT): $(build_private_libdir)/%-o.a
@$(call PRINT_LINK, $(CXX) $(LDFLAGS) -shared $(fPIC) -L$(build_private_libdir) -L$(build_libdir) -L$(build_shlibdir) -o $@ \
$(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))
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is already set on the cli, so the executable doesn't have it anyway, this just prevents silent failure like before

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@gbaraldi gbaraldi May 28, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

We don't have a windows pkgeval :(. But i'd rather this error then potentially silently fail.

Copy link
Member

Choose a reason for hiding this comment

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

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

base/linking.jl Show resolved Hide resolved
src/codegen.cpp Outdated Show resolved Hide resolved
topolarity
topolarity reviewed May 24, 2024
View reviewed changes
src/codegen.cpp
@@ -1786,9 +1789,6 @@ static inline GlobalVariable *prepare_global_in(Module *M, GlobalVariable *G)
G->isConstant(), GlobalVariable::ExternalLinkage,
nullptr, G->getName(), nullptr, G->getThreadLocalMode());
proto->copyAttributesFrom(G);
// DLLImport only needs to be set for the shadow module
// it just gets annoying in the JIT
proto->setDLLStorageClass(GlobalValue::DefaultStorageClass);
Copy link
Member

Choose a reason for hiding this comment

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

@vtjnash It'd be good to get a confirmation that this deletion is OK

Keno
Keno approved these changes May 24, 2024
View reviewed changes
Copy link
Member

@Keno Keno left a comment

Choose a reason for hiding this comment

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

In the master version of this, you should also be able to turn on the sysimage building test on windows now.

Keno
Keno reviewed May 24, 2024
View reviewed changes
src/aotcompile.cpp Outdated Show resolved Hide resolved
@gbaraldi gbaraldi removed the DO NOT MERGE Do not merge this PR! label May 24, 2024
@KristofferC KristofferC force-pushed the backports-release-1.10 branch 2 times, most recently from 6832cfb to e1b2099 Compare May 25, 2024 17:37
@ChrisRackauckas ChrisRackauckas mentioned this pull request May 26, 2024
Closed
@giordano
Copy link
Contributor

What happened to the history of this PR?

@gbaraldi
Copy link
Member Author

I believe it was the force pushes to the base branch that this targets?

@KristofferC
Copy link
Member

Hm, didn't mean to cause that. There were a few commits on the backport branch that were erroneous.

@gbaraldi gbaraldi force-pushed the gb/why-windows-why branch 3 times, most recently from 5c2040c to 036add1 Compare May 27, 2024 15:48
@gbaraldi @Keno @topolarity
Set storage class of julia globals to dllimport on windows to avoid a…
f1e4a28 
…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>
@gbaraldi gbaraldi added the backport 1.10 Change should be backported to the 1.10 release label May 27, 2024
@topolarity topolarity merged commit 007d8de into backports-release-1.10 May 28, 2024
7 checks passed
@topolarity topolarity deleted the gb/why-windows-why branch May 28, 2024 14:34
topolarity pushed a commit that referenced this pull request May 29, 2024
@gbaraldi
Set storage class of julia globals to dllimport on windows to avoid a…
efc35a6 
…uto-import weirdness (#54586)

Forward port of #54572
@KristofferC KristofferC removed the backport 1.10 Change should be backported to the 1.10 release label May 30, 2024
DilumAluthge pushed a commit that referenced this pull request Jun 3, 2024
@gbaraldi @DilumAluthge
Set storage class of julia globals to dllimport on windows to avoid a…
196b924 
…uto-import weirdness (#54586)

Forward port of #54572
KristofferC pushed a commit that referenced this pull request Jun 7, 2024
@gbaraldi
Set storage class of julia globals to dllimport on windows to avoid a…
90acf2a 
…uto-import weirdness (#54586)

Forward port of #54572

(cherry picked from commit efc35a6)
KristofferC added a commit that referenced this pull request Jun 25, 2024
@KristofferC
Backports for 1.11.0-rc1 (#54611)
ec84807 
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 -->
lazarusA pushed a commit to lazarusA/julia that referenced this pull request Jul 12, 2024
@gbaraldi @lazarusA
Set storage class of julia globals to dllimport on windows to avoid a…
dd44df7 
…uto-import weirdness (JuliaLang#54586)

Forward port of JuliaLang#54572
@mofeing mofeing mentioned this pull request Oct 27, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@topolarity topolarity topolarity approved these changes

@Keno Keno Keno approved these changes

@vtjnash vtjnash vtjnash approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@gbaraldi @fingolfin @giordano @KristofferC @vtjnash @Keno @topolarity