-
Notifications
You must be signed in to change notification settings - Fork 451
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
fix: do not dllexport symbols in core static libraries #3601
Conversation
TODO to definitely resolve #2346: do the same for user code in Lake |
endif | ||
ifdef CMAKE_LIKE_OUTPUT | ||
@echo "[ ] Building $<" | ||
endif | ||
@mkdir -p "$(@D)" | ||
ifdef PROFILE | ||
$(LEANC) -c -o $@ $< $(LEANC_OPTS) -DLEAN_EXPORTING |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I noticed we are running this for both the C and LLVM backend. I don't imagine this -D
actually does anything when compiling the bc
file. We will need to change the LLVM emitter instead:
lean4/src/Lean/Compiler/IR/EmitLLVM.lean
Lines 496 to 507 in 5302b78
-- we must now set symbol visibility for global. | |
if ps.isEmpty then | |
if isClosedTermName env decl.name then LLVM.setVisibility global LLVM.Visibility.hidden -- static | |
else if isExternal then pure () -- extern (Recall that C/LLVM funcs are extern linkage by default.) | |
else LLVM.setDLLStorageClass global LLVM.DLLStorageClass.export -- LEAN_EXPORT | |
else if !isExternal | |
-- An extern decl might be linked in from a different translation unit. | |
-- Thus, we cannot export an external declaration as we do not define it, | |
-- only declare its presence. | |
-- So, we only export non-external definitions. | |
then LLVM.setDLLStorageClass global LLVM.DLLStorageClass.export | |
return global |
However, this may be a TODO for later, as configuring this to emit both the export file and the non-export is more difficult as this is part of Lean elaboration rather than a separate step.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tydeu I believe the only remaining issue is that |
Mathlib CI status (docs):
|
I believe I have fixed everything I can on the Lake side and everything now can function, so I will hand this back to you @Kha. |
@Kha Added the release note. Feel free to merge when convenient. |
On Windows, we now compile all core
.o
s twice, once with and withoutdllexport
, for use in the shipped dynamic and static libraries, respectively. On other platforms, we export always as before to avoid the duplicate work.