-
Notifications
You must be signed in to change notification settings - Fork 13k
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
Remove dependency on libgcc-dw2-1.dll from win32 executables. #29177
Changes from 8 commits
1da4662
def917a
c807ff3
bd0cf1b
adce670
145b843
9a71c5c
d710f8b
afc3046
0332ee9
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -140,6 +140,8 @@ pub extern fn __rust_usable_size(size: usize, _align: usize) -> usize { | |
# #[lang = "panic_fmt"] fn panic_fmt() {} | ||
# #[lang = "eh_personality"] fn eh_personality() {} | ||
# #[lang = "eh_unwind_resume"] extern fn eh_unwind_resume() {} | ||
# #[no_mangle] pub extern fn rust_eh_register_frames () {} | ||
# #[no_mangle] pub extern fn rust_eh_unregister_frames () {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm, these are so volatile that it seems like a pretty good indication to me that we either need to start ignoring these tests or figure out a better way to deal with this, it's unfortunate to have these keep getting duplicated... Not critical though! |
||
``` | ||
|
||
After we compile this crate, it can be used as follows: | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -23,10 +23,7 @@ pub fn opts() -> TargetOptions { | |
exe_suffix: ".exe".to_string(), | ||
staticlib_prefix: "".to_string(), | ||
staticlib_suffix: ".lib".to_string(), | ||
// Unfortunately right now passing -nodefaultlibs to gcc on windows | ||
// doesn't work so hot (in terms of native dependencies). This flag | ||
// should hopefully be removed one day though! | ||
no_default_libraries: false, | ||
no_default_libraries: true, | ||
is_like_windows: true, | ||
archive_format: "gnu".to_string(), | ||
pre_link_args: vec!( | ||
|
@@ -63,7 +60,32 @@ pub fn opts() -> TargetOptions { | |
|
||
// Always enable DEP (NX bit) when it is available | ||
"-Wl,--nxcompat".to_string(), | ||
|
||
// Do not use the standard system startup files or libraries when linking | ||
"-nostdlib".to_string(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Was this necessary? I thought that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. My man page's section for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh wait I was looking at the man page for |
||
), | ||
pre_link_objects_exe: vec!( | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Could you be sure to add a comment for these fields? They'll probably want to point somewhere else, but just an indication about where to find info about what they're doing. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. They are explained in the struct definition. Is that not enough? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah I was looking more of a "why are these passed on this platform" moreso than "what does this field do" |
||
"crt2.o".to_string(), // mingw C runtime initialization for executables | ||
"rsbegin.o".to_string(), // Rust compiler runtime initialization, see rsbegin.rs | ||
), | ||
pre_link_objects_dll: vec!( | ||
"dllcrt2.o".to_string(), // mingw C runtime initialization for dlls | ||
"rsbegin.o".to_string(), | ||
), | ||
late_link_args: vec!( | ||
"-lmingwex".to_string(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just wondering, what do we rely on |
||
"-lmingw32".to_string(), | ||
"-lgcc".to_string(), // alas, mingw* libraries above depend on libgcc | ||
"-lmsvcrt".to_string(), | ||
"-ladvapi32".to_string(), | ||
"-lshell32".to_string(), | ||
"-luser32".to_string(), | ||
"-lkernel32".to_string(), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is an interesting list! Were these added because There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The trouble is, they have to come after all other libs, and be in this specific sequence. Just consider them a part of the platform runtime. I think There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Most of our other platforms don't implicitly pass The There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I meant that we should just stick There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah yeah that's a good point, but probably good to consider separately! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm it may be the case that some parts of those previous libraries depend on things like advapi32, but I was able to succesfully compile an executable with just:
If we include mingwex or mingw32 I think we must include gcc because they're probably generated by gcc (and need the intrinsics). I think we can successfully link without advapi32, shell32, and user32, though? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, hm. I stand corrected! Well, on second look, there is a small chance of backcompat fall-out: The last one actually breaks linking when building libflate, because I guess There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In terms of intrinsically required libraries by the compiler, however, I don't these may want to not be in this list. These libraries are injected into all compilations ever for this target, and it can end up creating hidden dependencies and such (like in this case), when the "most correct" thing to do would probably be to only include those the compiler itself requires. I think we need mingw32/mingwex to build executables as crt2.o references some symbols from those, and then kernel32/msvcrt/gcc are dependencies of those libraries, so those are all required for dlls/exes. If libflate is the one, though, that depends on advapi32 or user32, then libflate itself should declare that I think (it should also in theory declare that it depends on mingwex). That being said, though, we probably want to just link these into the standard library (e.g. add -luser32 which I think may be missing today) and we should already be covered by -ladvapi32 in the standard library? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. libflate does not depend on user32 directly, it depends on mingwex, which depends on user32. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. OK, sounds good to me! |
||
), | ||
post_link_objects: vec!( | ||
"rsend.o".to_string() | ||
), | ||
custom_unwind_resume: true, | ||
exe_allocation_crate: super::maybe_jemalloc(), | ||
|
||
.. Default::default() | ||
|
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.
Could you add a comment here as to why these two objects are being included? It's fine to be a "pointer comment" to somewhere else as well (same with x86_64 below)
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 have no strong objection... it just... doesn't seem scalable to comment on a variable at every point of use. Eventually, when we get rid of "gcc as a linker driver" of all platforms (we want that, right?), this line will be repeated in every one of those .mk files...
The purpose is explained in targets.mk, where one can arrive via a simple grep search.
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.
Ah yeah that's fine, just want to make sure that there's enough data here for someone to figure out why all this exists in the future.