Remove dependency on libgcc-dw2-1.dll from win32 executables. #29177

vadimcn · Oct 20, 2015

Note: for now, this change only affects -windows-gnu builds.

So why was this libgcc dylib dependency needed in the first place?
The stack unwinder needs to know about locations of unwind tables of all the modules loaded in the current process. The easiest portable way of achieving this is to have each module register itself with the unwinder when loaded into the process. All modules compiled by GCC do this by calling the __register_frame_info() in their startup code (that's crtbegin.o and crtend.o, which are automatically linked into any gcc output).
Another important piece is that there should be only one copy of the unwinder (and thus unwind tables registry) in the process. This pretty much means that the unwinder must be in a shared library (unless everything is statically linked).

Now, Rust compiler tries very hard to make sure that any given Rust crate appears in the final output just once. So if we link the unwinder statically to one of Rust's crates, everything should be fine.

Unfortunately, GCC startup objects are built under assumption that libgcc is the one true place for the unwind info registry, so I couldn't find any better way than to replace them. So out go crtbegin/crtend, in come rsbegin/rsend!

A side benefit of this change is that rustc is now more in control of the command line that goes to the linker, so we could stop using gcc as the linker driver and just invoke ld directly.

rust-highfive · Oct 20, 2015

r? @arielb1

(rust_highfive has picked a reviewer for you, use r? to override)

vadimcn · Oct 20, 2015

r? @alexcrichton

alexcrichton · Oct 20, 2015

@vadimcn you never cease to amaze me, fantastic work!

I'll try to get around to this tomorrow, and you may be met with a barrage of questions :)

retep998 · Oct 20, 2015

retep998 reviewed Oct 20, 2015

View changes

retep998 · Oct 20, 2015

src/librustc_back/target/windows_base.rs

+            "rsbegin.o".to_string(),
+        ),
+        late_link_args: vec!(
+            "-lmingwex".to_string(),


Just wondering, what do we rely on mingwex for anyway?

alexcrichton · Oct 20, 2015

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton · Oct 20, 2015

mk/target.mk

+# $(2) - object name
+define COPY_LIBC_STARTUP
+
+$(1)/$(2) : $$(CFG_LIBC_DIR)/$(2)


Perhaps instead of CFG_LIBC_DIR this could use gcc -print-file-name=$(2) to figure out the location of the object?

Certainly! I missed this gcc option somehow.

alexcrichton · Oct 20, 2015

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton · Oct 20, 2015

mk/cfg/i686-pc-windows-gnu.mk

@@ -22,3 +22,4 @@ CFG_LDPATH_i686-pc-windows-gnu :=
 CFG_RUN_i686-pc-windows-gnu=$(2)
 CFG_RUN_TARG_i686-pc-windows-gnu=$(call CFG_RUN_i686-pc-windows-gnu,,$(2))
 CFG_GNU_TRIPLE_i686-pc-windows-gnu := i686-w64-mingw32
+CFG_LIBC_STARTUP_OBJECTS_i686-pc-windows-gnu := crt2.o dllcrt2.o


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)

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.

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.

alexcrichton · Oct 20, 2015

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton · Oct 20, 2015

mk/target.mk

+	$$(eval $$(call TARGET_RUSTRT_STARTUP_OBJ,$(1),$(2),$(3),$$(obj))) )
+
+$$(foreach obj,$$(CFG_LIBC_STARTUP_OBJECTS_$(2)), \
+	$$(eval $$(TLIB$(1)_T_$(2)_H_$(3))/stamp.core : $$(TLIB$(1)_T_$(2)_H_$(3))/$$(obj)) \


Oh wow interesting, I had no idea this was even valid syntax... Could this perhaps use something like:

$$(TLIB$(1)_T_$(2)_H_$(3))/stamp.core : $$(foreach obj,..,$$(TLIB$(1)_T_$(2)_H_$(3))/$$(obj))

That'd at least keep it a little more conventional with the rest of the makefiles

alexcrichton · Oct 20, 2015

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton · Oct 20, 2015

src/doc/trpl/custom-allocators.md

@@ -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 () {}


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!

alexcrichton · Oct 20, 2015

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton · Oct 20, 2015

src/librustc_back/target/windows_base.rs

@@ -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(),


Was this necessary? I thought that -nodefaultlibs would do what this comment indicates, and the man pages at least indicate that -nostdlib means that only the directories on the command line will be searched (e.g. doesn't mention anything about startup files or objects).

My man page's section for -nostdlib begins with this exact sentence . I could have used -nostartfiles since we are also passing -nodefaultlibs, but just to make sure...

Oh wait I was looking at the man page for ld not gcc, nevermind!

alexcrichton · Oct 20, 2015

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton · Oct 20, 2015

src/librustc_back/target/windows_base.rs

+            "-ladvapi32".to_string(),
+            "-lshell32".to_string(),
+            "-luser32".to_string(),
+            "-lkernel32".to_string(),


This is an interesting list! Were these added because no_default_libraries is now true? In the spirit of "no default libraries" none of these should actually be specified (e.g. they have to be explicitly linked in). This has the risk of being a breaking change, however, but it's one that I think I'd be fine stomaching.

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 compiler-rt should also go here, btw.

Most of our other platforms don't implicitly pass -l arguments because they pass -nodefaultlibs to gcc, and when that started I just didn't take care of MinGW at the time as I didn't really fully understand it. So in theory libraries like libstd and liblibc and such are the ones that pass these -l flags, and they can control the ordering via the normal methods (e.g. in the source or via the crate DAG).

The compiler-rt library is passed by the compiler at the very end of the linker, and it's got a flag in the custom target spec as well to say "don't pass this as it's not available on this platform"

I meant that we should just stick compiler-rt into late_link_args and eliminate its special status.

Ah yeah that's a good point, but probably good to consider separately!

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:

-lmingw32 -lmingwex -lkernel32 -lmsvcrt -lgcc

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?

Ah, hm. I stand corrected!

Well, on second look, there is a small chance of backcompat fall-out:
nm -A -u libmingwex.a | grep __imp__ mostly shows symbols from kernel32, but not all of them are:
lib32_libmingwex_a-getlogin.o: U __imp__GetUserNameA@8 is from advapi32.dll,
lib32_libmingwex_a-wassert.o: U __imp__MessageBoxW@16 is from user32.dll

The last one actually breaks linking when building libflate, because I guess miniz uses assert internally.
There might be other crates who are in the same boat.

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?

libflate does not depend on user32 directly, it depends on mingwex, which depends on user32.
Assert is actually a bit of a weird case, because normally assert's helper function lives libc (I think). Linking to mingwex by default papers over the fact that that on mingw it isn't provided by libc (though msvcrt does export assert, not sure why mingw folks were not happy with it).
I worry that this might be rather common to have crates that depend on a C library, that uses assert (and thus depends on mingwex), which don't specify mingwex as a #[link] dependency, because on all other platforms linking to libc is enough.
How about we drop advapi32 and shell32 but keep user32? (see my last commit)

OK, sounds good to me!

alexcrichton · Oct 20, 2015

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton · Oct 20, 2015

src/librustc_back/target/windows_base.rs

+            // Do not use the standard system startup files or libraries when linking
+            "-nostdlib".to_string(),
+        ),
+        pre_link_objects_exe: vec!(


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.

They are explained in the struct definition. Is that not enough?

Ah I was looking more of a "why are these passed on this platform" moreso than "what does this field do"

alexcrichton · Oct 20, 2015

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton · Oct 20, 2015

src/librustc_trans/trans/intrinsic.rs

@@ -1315,7 +1315,7 @@ fn trans_msvc_try<'blk, 'tcx>(bcx: Block<'blk, 'tcx>,
        // The "catch-resume" block is where we're running this landing pad but
        // we actually need to not catch the exception, so just resume the
        // exception to return.
-        Resume(catch_resume, vals);
+        trans_unwind_resume(catch_resume, vals);


This in theory isn't necessary, right? (it's only used for MSVC targets which don't have custom_unwind_resume set?

Hmm, true. Still, it's probably a good thing to centralize checking of the custom_unwind_resume flag?

Yeah makes sense, thanks for the clarification!

alexcrichton · Oct 20, 2015

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton · Oct 20, 2015

src/libstd/lib.rs

+#[cfg(all(target_os="windows", target_arch = "x86", target_env="gnu"))]
+pub mod __frame_registry {
+    pub use sys_common::unwind::imp::eh_frame_registry::*;
+}


Could this be moved into src/libstd/rt.rs? That's got other public symbols like some unwinding ones, and it's already auto-unstable as well.

alexcrichton · Oct 20, 2015

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton · Oct 20, 2015

src/libstd/sys/common/libunwind.rs

+#[cfg_attr(target_os = "bitrig",
+           link(name = "c++abi"))]
+#[cfg_attr(all(target_os = "windows", target_env="gnu"),
+           link(name = "gcc_eh"))]


So just to clarify gcc_eh is a static library which contains all these unwinding functions? We were just previously linking to the dynamic version and using those instead?

Er actually... let me rephrase, can you clarify what gcc_eh is?

Yes, gcc_eh exports all those _Unwind_XXX functions. The dynamic version of libgcc, is actually a bundle of libgcc.a and libgcc_eh.a.

Ah ok, so I know that with MinGW you can sometimes get seh, dwarf, or sjlj exceptions, so does that matter here? For example I think we're basically only compatible with the dwarf strategy on 32-bit and seh on 64 (right?) and is gcc_eh just whatever the installation happens to be? Or is it guaranteed to always be the one we use?

Yes, gcc_eh depends on the flavor of gcc installed. If it's the sjlj one, we'll just fail to link, I think.

Ah and as one final clarification, after talking with @brson, so this means that we are statically linking the unwinding support into the standard library? If so, should this be kind = "static"? Also if so, we may want to checkup on the licenses to ensure that we're covered to statically link this code from gcc (we probably are though)

If so, should this be kind = "static"?

It probably should. However this doesn't work right now, because it's in the system libs directory, which rustc is not aware of, so it fails to find it.

Also if so, we may want to checkup on the licenses to ensure that we're covered to statically link this code from gcc (we probably are though)

Yes, I believe so. libgcc_eh is covered by the linking exception.

If so, should this be kind = "static"?

It probably should. However this doesn't work right now, because it's in the system libs directory, which rustc is not aware of, so it fails to find it.

This is the same issue that I've been complaining about with getting dllimport fixed.

alexcrichton · Oct 20, 2015

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton · Oct 20, 2015

src/libstd/sys/common/unwind/gcc.rs

+#[lang = "eh_unwind_resume"]
+#[unwind]
+unsafe extern fn rust_eh_unwind_resume(panic_ctx: *mut u8) -> ! {
+    uw::_Unwind_Resume(panic_ctx as *mut uw::_Unwind_Exception);


If this just calls the standard _Unwind_Resume, how come a custom lang item is needed here? Shouldn't the gnu-32 target not need this?

I didn't want to leave it up to the linker to choose which _Unwind_Resume this gets resolved to (what if -lgcc_s is added by the user?).
Having this as a lang item seems safer (also conceptually tidier, IMO, since this is a part of language runtime, just like eh_personality).

Hm ok, adding -lgcc_s is a good point!

alexcrichton · Oct 20, 2015

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton · Oct 20, 2015

src/libstd/sys/common/unwind/gcc.rs

+}
+
+#[cfg(all(target_os="windows", target_arch = "x86", target_env="gnu"))]
+pub mod eh_frame_registry {


Can you be sure to add a comment here pointing to a location which documents what this all is?

alexcrichton · Oct 20, 2015

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton · Oct 20, 2015

src/rtstartup/rsbegin.rs

+// <LICENSE-MIT or http://opensource.org/licenses/MIT>, at your
+// option. This file may not be copied, modified, or distributed
+// except according to those terms.
+


Ah ok, so this is about the location that it'd be awesome to have a nice long doc block explaining what this is all doing. Could you add some docs here basically describing the whole scheme for why this works and such? Things like:

Why we have our custom startup objects (e.g. why the standard ones aren't sufficient).

What these startup objects are doing, e.g. what's happening in the standard library and what's happening in MinGW and where it's all getting linked.

What each item is in these objects.

etc

alexcrichton · Oct 20, 2015

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton · Oct 20, 2015

src/libstd/sys/common/unwind/gcc.rs

+    }
+    #[cfg(not(test))]
+    #[no_mangle]
+    pub unsafe extern fn rust_eh_register_frames(eh_frame_begin: *const u8,


Could the symbol references in rsstart.o just point to __register_frame_info directly instead of going through the standard library? Or is there a downside to that?

I wanted these symbols to be Rust-specific for more control over linkage. __register_frame_info should be completely encapsulated in libstd. We could replace gcc unwinder with some other implementation after all.

Ah ok, could you be sure to add this in the doc comment down below? Sounds fine to me!

vadimcn · Oct 21, 2015

Looks better?

alexcrichton · Oct 21, 2015

alexcrichton reviewed Oct 21, 2015

View changes

alexcrichton · Oct 21, 2015

src/rtstartup/rsbegin.rs

+    #[no_mangle]
+    #[link_section = ".eh_frame"]
+    // Marks beginning of the stack frame unwind info section
+    pub static __EH_FRAME_BEGIN__: [u8; 0] = [];


So just to clarify, this works because:

The linker will construct the .eh_frame section from the list of items contained within it on the command line. This is, however, guarantee to be the first, so &__EH_FRAME_BEGIN__ is guaranteed to be a pointer to the start of the section.

Similarly, rsend.o is guarnateed to be at the end, so it's guaranteed that &__EH_FRAME_END__ is at the end of the section.

Does that sound right? Also out of curiosity, what is the __EH_FRAME_END__ symbol used for? It's not explicitly referenced here, so is it referenced or dynamically loaded from the CRT or elsewhere?

Yes, that's my understanding of how this works.

Also out of curiosity, what is the EH_FRAME_END symbol used for?

It's used to support the weight of eh_frame section, thus the extra wide base.

Hm ok, and by "weight" and "extra wide base" do you mean how __EH_FRAME_END__ is a u32 while the start is a [u8; 0]? (a fact I just realized)

...the other reason is that you can't have a static variable (the zero footer) without a name (at least that I know of).

By extra wide base I meant the underscores :) just kidding :)

Oh dear it looks like my ignorance is leaking, a zero footer makes sense to me!

alexcrichton · Oct 23, 2015

cc @brson, just wanna give you a heads up about the we-are-building-our-own-object-files strategy. It seems fine to me but figured you may want to know!

alexcrichton · Oct 24, 2015

This is basically r+ from me, but I think I'd prefer to hold this off until after the 1.5 beta is released so this lands in 1.6 instead (just so we have some extra time to weed out any problems here). Does that sound ok?

vadimcn · Oct 25, 2015

That's next Thursday, right? Fine by me.

alexcrichton · Oct 25, 2015

OK cool, now I just wish I could bors r+ date=2015-...

alexcrichton · Oct 28, 2015

@bors: r+ afc3046

bors · Oct 28, 2015

⌛️ Testing commit afc3046 with merge ccc9ff2...

bors · Oct 28, 2015

💔 Test failed - auto-win-gnu-64-opt

vadimcn · Oct 29, 2015

Uh-oh, that's a stage0 ICE. Crashes in llvm::AttributeSet::removeAttributes

vadimcn · Nov 1, 2015

Looks like this was my own fault. The tricks I played with injection of the _Unwind_Resume symbol somehow caused it to turn into a "constant expression" value (but only on x86_64, which doesn't even use this symbol ), which did not sit well with LLVM functions like LLVMRemoveAttribute. Hiding it from stage0 resolves the issue.
r? @alexcrichton

alexcrichton · Nov 1, 2015

@bors: r+ 0332ee9

bors · Nov 1, 2015

⌛️ Testing commit 0332ee9 with merge 7140918...

bors · Nov 1, 2015

☀️ Test successful - auto-linux-32-nopt-t, auto-linux-32-opt, auto-linux-64-debug-opt, auto-linux-64-nopt-t, auto-linux-64-opt, auto-linux-64-x-android-t, auto-linux-cross-opt, auto-linux-musl-64-opt, auto-mac-32-opt, auto-mac-64-nopt-t, auto-mac-64-opt, auto-win-gnu-32-nopt-t, auto-win-gnu-32-opt, auto-win-gnu-64-nopt-t, auto-win-gnu-64-opt, auto-win-msvc-32-opt, auto-win-msvc-64-opt

rust-highfive assigned arielb1 Oct 20, 2015

rust-highfive assigned alexcrichton and unassigned arielb1 Oct 20, 2015

vadimcn added some commits Oct 21, 2015

Use `gcc -print-file-name` for finding C runtime startup objects:
reverted changes in configure, refactored target.mk

9a71c5c

Moar comments.

Loading status checks…

d710f8b

Drop `advapi32` and `shell32` from late_link_args.

Loading status checks…

afc3046

Fix stage0 ICE caused by the old _Unwind_Resume override trickery.

Loading status checks…

0332ee9

bors merged commit 0332ee9 into rust-lang:master Nov 1, 2015
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

alexcrichton referenced this pull request Nov 5, 2015
Closed
Build failure with MinGW-w64 gcc 5.2.0 #29649

rust-lang/rust

Join GitHub today

Remove dependency on libgcc-dw2-1.dll from win32 executables. #29177

Conversation

vadimcn commented Oct 20, 2015

vadimcn added some commits Oct 18, 2015

rust-highfive assigned arielb1 Oct 20, 2015

This comment has been minimized.

rust-highfive commented Oct 20, 2015

This comment has been minimized.

vadimcn commented Oct 20, 2015

rust-highfive assigned alexcrichton and unassigned arielb1 Oct 20, 2015

This comment has been minimized.

alexcrichton commented Oct 20, 2015

retep998 reviewed Oct 20, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Oct 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Oct 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Oct 20, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Oct 20, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Oct 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Oct 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Oct 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Oct 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Oct 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Oct 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Oct 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

alexcrichton reviewed Oct 20, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Oct 20, 2015 View changes

This comment has been minimized.

alexcrichton reviewed Oct 20, 2015 View changes

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

vadimcn added some commits Oct 21, 2015

This comment has been minimized.

vadimcn commented Oct 21, 2015

alexcrichton reviewed Oct 21, 2015 View changes

This comment has been minimized.

This comment has been minimized.

retep998 reviewed Oct 20, 2015

View changes

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton reviewed Oct 20, 2015

View changes

alexcrichton reviewed Oct 21, 2015

View changes