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

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
2 changes: 1 addition & 1 deletion Make.inc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1365,7 +1365,7 @@ ifeq ($(OS), WINNT)
HAVE_SSP := 1
OSLIBS += -Wl,--export-all-symbols -Wl,--version-script=$(BUILDROOT)/src/julia.expmap \
$(NO_WHOLE_ARCHIVE) -lpsapi -lkernel32 -lws2_32 -liphlpapi -lwinmm -ldbghelp -luserenv -lsecur32 -latomic
JLDFLAGS += -Wl,--stack,8388608
JLDFLAGS += -Wl,--stack,8388608 --disable-auto-import --disable-runtime-pseudo-reloc
ifeq ($(ARCH),i686)
JLDFLAGS += -Wl,--large-address-aware
endif
Expand Down
2 changes: 1 addition & 1 deletion base/linking.jl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ function ld()
# LLD supports mingw style linking
flavor = "gnu"
m = Sys.ARCH == :x86_64 ? "i386pep" : "i386pe"
default_args = `-m $m -Bdynamic --enable-auto-image-base --allow-multiple-definition`
default_args = `-m $m -Bdynamic --enable-auto-image-base --allow-multiple-definition --disable-auto-import --disable-runtime-pseudo-reloc`
topolarity marked this conversation as resolved.
Show resolved Hide resolved
elseif Sys.isapple()
flavor = "darwin"
arch = Sys.ARCH == :aarch64 ? :arm64 : Sys.ARCH
Expand Down
3 changes: 1 addition & 2 deletions cli/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,7 @@ LOADER_CFLAGS += -DGLIBCXX_LEAST_VERSION_SYMBOL=\"$(shell echo "$(CSL_NEXT_GLIBC
endif

ifeq ($(OS),WINNT)
LOADER_LDFLAGS += -municode -mconsole -nostdlib --disable-auto-import \
--disable-runtime-pseudo-reloc -lntdll -lkernel32 -lpsapi
LOADER_LDFLAGS += -municode -mconsole -nostdlib -lntdll -lkernel32 -lpsapi
else ifeq ($(OS),Linux)
LOADER_LDFLAGS += -Wl,--no-as-needed -ldl -lpthread -rdynamic -lc -Wl,--as-needed
else ifeq ($(OS),FreeBSD)
Expand Down
18 changes: 11 additions & 7 deletions src/aotcompile.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1166,6 +1166,8 @@ static void materializePreserved(Module &M, Partition &partition) {
GV.setInitializer(nullptr);
GV.setLinkage(GlobalValue::ExternalLinkage);
GV.setVisibility(GlobalValue::HiddenVisibility);
if (GV.getDLLStorageClass() != GlobalValue::DLLStorageClassTypes::DefaultStorageClass)
continue; // Don't mess with exported or imported globals
GV.setDSOLocal(true);
}

Expand Down Expand Up @@ -1645,6 +1647,7 @@ void jl_dump_native_impl(void *native_code,
if (jl_small_typeof_copy) {
jl_small_typeof_copy->setVisibility(GlobalValue::HiddenVisibility);
jl_small_typeof_copy->setDSOLocal(true);
jl_small_typeof_copy->setDLLStorageClass(GlobalValue::DLLStorageClassTypes::DefaultStorageClass);
}
}

Expand Down Expand Up @@ -1673,16 +1676,17 @@ void jl_dump_native_impl(void *native_code,
// reflect the address of the jl_RTLD_DEFAULT_handle variable
// back to the caller, so that we can check for consistency issues
GlobalValue *jlRTLD_DEFAULT_var = jl_emit_RTLD_DEFAULT_var(&metadataM);
addComdat(new GlobalVariable(metadataM,
jlRTLD_DEFAULT_var->getType(),
true,
GlobalVariable::ExternalLinkage,
jlRTLD_DEFAULT_var,
"jl_RTLD_DEFAULT_handle_pointer"), TheTriple);

Type *T_size = DL.getIntPtrType(Context);
Type *T_psize = T_size->getPointerTo();

auto FT = FunctionType::get(Type::getInt8Ty(Context)->getPointerTo()->getPointerTo(), {}, false);
auto F = Function::Create(FT, Function::ExternalLinkage, "get_jl_RTLD_DEFAULT_handle_addr", metadataM);
llvm::IRBuilder<> builder(BasicBlock::Create(Context, "top", F));
builder.CreateRet(jlRTLD_DEFAULT_var);
F->setLinkage(GlobalValue::ExternalLinkage);
if (TheTriple.isOSBinFormatCOFF())
F->setDLLStorageClass(GlobalValue::DLLStorageClassTypes::DLLExportStorageClass);

if (TheTriple.isOSWindows()) {
// Windows expect that the function `_DllMainStartup` is present in an dll.
// Normal compilers use something like Zig's crtdll.c instead we provide a
Expand Down
8 changes: 4 additions & 4 deletions src/codegen.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -493,9 +493,12 @@ struct JuliaVariable {
if (GlobalValue *V = m->getNamedValue(name))
return cast<GlobalVariable>(V);
auto T_size = m->getDataLayout().getIntPtrType(m->getContext());
return new GlobalVariable(*m, _type(T_size),
auto var = new GlobalVariable(*m, _type(T_size),
isconst, GlobalVariable::ExternalLinkage,
NULL, name);
if (Triple(m->getTargetTriple()).isOSWindows())
var->setDLLStorageClass(GlobalValue::DLLStorageClassTypes::DLLImportStorageClass); // Cross-library imports must be explicit for COFF (Windows)
return var;
}
GlobalVariable *realize(jl_codectx_t &ctx);
};
Expand Down Expand Up @@ -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

return proto;
}
return cast<GlobalVariable>(local);
Expand Down
6 changes: 3 additions & 3 deletions src/staticdata.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2838,9 +2838,9 @@ JL_DLLEXPORT void jl_preload_sysimg_so(const char *fname)
// Allow passing in a module handle directly, rather than a path
JL_DLLEXPORT void jl_set_sysimg_so(void *handle)
{
void* *jl_RTLD_DEFAULT_handle_pointer;
int symbol_found = jl_dlsym(handle, "jl_RTLD_DEFAULT_handle_pointer", (void **)&jl_RTLD_DEFAULT_handle_pointer, 0);
if (!symbol_found || (void*)&jl_RTLD_DEFAULT_handle != *jl_RTLD_DEFAULT_handle_pointer)
void** (*get_jl_RTLD_DEFAULT_handle_addr)(void) = NULL;
int symbol_found = jl_dlsym(handle, "get_jl_RTLD_DEFAULT_handle_addr", (void **)&get_jl_RTLD_DEFAULT_handle_addr, 0);
if (!symbol_found || (void*)&jl_RTLD_DEFAULT_handle != (get_jl_RTLD_DEFAULT_handle_addr()))
jl_error("System image file failed consistency check: maybe opened the wrong version?");
if (jl_options.cpu_target == NULL)
jl_options.cpu_target = "native";
Expand Down
2 changes: 1 addition & 1 deletion sysimage.mk
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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

@$(INSTALL_NAME_CMD)$(notdir $@) $@
@$(DSYMUTIL) $@

Expand Down