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.

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

[msvc] Don't use -M* flags. #723

Closed
wants to merge 1 commit into from
Closed

Conversation

dot-asm
Copy link
Contributor

@dot-asm dot-asm commented Sep 17, 2022

Since this crate doesn't actually make decisions about final application linking, it's only appropriate to not interfere with the process. This can be achieved with -MT -Zl, because object modules compiled with these flags can be linked by relying on default MSVC behaviour that allows linking with either VCCRT version, static or dynamic, debug or release...

It's suggested to use -Zl when building Rust static library. For reference, -Zl doesn't affect linking directives set with in Windows headers to add references to not-so-common libraries like mfc.lib, windowscodecs.lib to name just a pair.

[Even an alternative solution to #717.]

src/lib.rs Outdated
// interfere with the process. This can be achieved with
// -MT -Zl, because object modules compiled with these flags
// can be linked with either VCCRT version, static or dynamic,
// debug or release...
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, was it actually non-trivial to find the -Zl documentation? It's available even in cl /? output... I'd argue that if something needs to be added, then it would be more appropriate to discuss the subtlety of the -MT in the context. The symbol decoration part... Either way, I'd wait till maintainers make the choice which direction will be preferred, this, #717 or #725 ;-)

@thomcc thomcc added the O-windows Windows targets and toolchains label Oct 26, 2022
@ChrisDenton
Copy link
Member

I worry that this solution may not address the core issue. Yes, omitting the CRT library name from a library will get rid of linker errors. But does this also hide potential conflicts that should be handled? For example, do the CRT (or even C++) headers care about static/dynamic or debug/no-debug? If they do then using /MT unconditionally may cause much subtler problems. Note that the documentation says the compiler will define one or more of _MT, _DLL and/or _DEBUG depending on the option chosen here.

@dot-asm
Copy link
Contributor Author

dot-asm commented Nov 8, 2022

... do the CRT (or even C++) headers care about static/dynamic or debug/no-debug?

As for static/dynamic, the only difference is when _DLL is defined the declarations get additional declspec(dllimport) qualifier. In practical terms it means that the corresponding symbol gets decorated with __imp_ prefix and compile emits an indirect call instruction. Now, the prefixed symbols are available only in the shared library, but it's perfectly possible to link the undecorated symbols with the shared library. In this case the linker will resolve the direct call [generated in case _DLL is not defined] to an indirect jump instruction that brings you to the same entry point as the indirect call would. In other words, a call compiled with /MT and linked with the dynamic CRT costs you one extra instruction.

As for debug/non-debug. Well, it would only be a problem if a function prototype would change depending on _DEBUG. I can't imagine that it would be the case. Macros can depend on it though, but we're talking about linking subtleties here. But if macros are considered a problem, we can just as well pass explicit -D_DEBUG in a debug build...

Now an observation. Let's say you add cc.static_crt(true) to your build.rs. It dutifully passes /MT to cl, which leaves reference to LIBCMT, but does it have an effect? No, because rustc passes the /NODEFAULTLIB flag, which is functionally equivalent to using /Zl, and links with the shared CRT library. By default that is. And it works. Because of the above explanation. In other words, behind the curtains the suggested method is not actually a stranger to Rust :-)

I worry that this solution may not address the core issue.

Well, doesn't it depend on definition of the core issue? :-) :-) :-) Formally speaking cc-rs's duty is to produce static libraries for rustc to link with, period. The 'period' implies that it should abstain from interfering with choices rustc makes at link time. In other words, one can say that the core issue is that cc-rs attempts to interfere. It doesn't actually work, but it shouldn't actually try.

@ChrisDenton
Copy link
Member

rustc passes the /NODEFAULTLIB

To my knowledge, rustc does not pass the /NODEFAULTLIB flag.

You have outlined some ways in which C/C++ compilations changes depending on the options chosen, which on their own suggest /MT does in fact have an effect outside of just the linker. There is also at least the potential for C/C++ code to be compiled differently in other ways depending on /MT or /MD. They may or may not but cc-rs is not in the position of being able to make assumptions about third party code.

@dot-asm
Copy link
Contributor Author

dot-asm commented Nov 9, 2022

To my knowledge, rustc does not pass the /NODEFAULTLIB flag.

It does not? But... How..................... Hmm... You're right!!! I must have mixed it up with windows-gnu. But again, how?! How does it work as if /NODEFAULTLIB is passed? Ah! Rust simply discards the linker warnings? Just swipe[s] the library mismatch problems under the rug, nothing to see here. Oh, this is just...

/MT or /MD

It should be noted that /MT has been the default for at least 10 years now, older than cc-rs. "Default" means that not specifying any /M option is equivalent to passing /MT. Again, /MD is really about just symbol decoration.

... cc-rs is not in the position of being able to make assumptions about third party code.

At the same time it's not formally reasonable to expect that arbitrary code would work without any adaptation. After all, it has to play on the host's, Rust's in this case, terms. And hence it's not unreasonable to limit the playfield. But I hear you! On the other hand, I'd say that the abovementioned swiping-under-the-rug is not exactly the definition of not-making-assumptions...

@dot-asm
Copy link
Contributor Author

dot-asm commented Nov 9, 2022

Let's re-frame the problem description. Is cc-rs entitled to control if the final application is linked with shared or dynamic vccrt? No. Right? It should follow Rust's lead, but not try to steer. Besides, it can't as it is now, and probably won't ever be able to. With this in mind, formally speaking there should be no .static_crt() method. One can make a case that CARGO_CFG_TARGET_FEATURE should be evaluated for presence of "crt-static". And one can also make a case for making users aware of situations when it might be appropriate to use /Zl in their build scripts.

@dot-asm
Copy link
Contributor Author

dot-asm commented Nov 9, 2022

Just in case for reference. To the question about "efficacy" of the .static_crt() method. It's perfectly possible to trigger linking errors by calling .static_crt(false) and passing -C target-feature=+crt-static through %RUSTFLAGS%. (Keep in mind that .static_crt() can be called in a crate provider's script, not your application :-)

Since this crate doesn't actually make decisions about
final application linking, it's only appropriate to not
interfere with the process. This can be achieved by
relying on default MSVC behaviour that allows linking
with either VCCRT version, static or dynamic...
@dot-asm dot-asm changed the title [msvc] Use -MT -Zl unconditionally. [msvc] Don't use -M* flags. Nov 22, 2022
@dot-asm
Copy link
Contributor Author

dot-asm commented Nov 22, 2022

Complete make-over. Have a look, @ChrisDenton.

@dot-asm
Copy link
Contributor Author

dot-asm commented Nov 22, 2022

One can make a case that CARGO_CFG_TARGET_FEATURE should be evaluated for presence of "crt-static".

Just in case, I'm not making this case.

@ChrisDenton
Copy link
Member

Other issues aside, I'm not convinced we should be changing the defaults here (see also the recent issues with changing asm defaults). Currently the import libraries Rust uses depends on crt-static and cc-rs matches that with its choice of -MT/-MD. There is a good justification for matching Rust by default (mixing crt libraries is bad). Not doing so means that the user of cc-rs now has one more thing to be aware of (and potentially get wrong).

The desire to have a more versatile way to override the defaults provided by cc-rs is something we should support but not at the expense of doing the wrong thing by default.

@thomcc
Copy link
Member

thomcc commented Nov 23, 2022

One can make a case that CARGO_CFG_TARGET_FEATURE should be evaluated for presence of "crt-static".

Just in case, I'm not making this case.

Do you think it shouldn't? That sounds pretty reasonable behavior to me, and is consistent with how we set many other flags...

@dot-asm
Copy link
Contributor Author

dot-asm commented Nov 23, 2022

(mixing crt libraries is bad)

Which is not actually happening. Attempt to mix makes the linker issue warnings (which rustc just swipes under the rug), but no actual mixing takes place. The ambiguity is resolved by prioritizing the command line over /defaultlib in the first object file with linker directive. Rustc passes the crt library on the command line which takes the priority, hence it's always in control. And it's only appropriate if cc-rs stays out of the way...

@dot-asm
Copy link
Contributor Author

dot-asm commented Nov 23, 2022

Do you think it shouldn't? That sounds pretty reasonable behavior to me, and is consistent with how we set many other flags...

Correct, I think it shouldn't. But it's just me :-) Just to rehash. The cc-rs's purpose is to produce .a files for rustc to link with. It's not its business to try to interfere with linking decisions. And there is a way to achieve the goal. I admit that omitting /M* goes against tradition, so to say, but it does work, reliably, and would actually deliver even here. Well, it's not time yet :-) Just keep the possibility in mind...

As for other flags. I don't know if you're talking about what I think you would, but there are a number of places where I feel the project fell for suboptimal solutions in a heat of a moment, so to say :-) Well, I'm just rambling... Cheers.

@dot-asm dot-asm closed this Nov 23, 2022
@ChrisDenton
Copy link
Member

(mixing crt libraries is bad)

Which is not actually happening. Attempt to mix makes the linker issue warnings (which rustc just swipes under the rug), but no actual mixing takes place. The ambiguity is resolved by prioritizing the command line over /defaultlib in the first object file with linker directive. Rustc passes the crt library on the command line which takes the priority, hence it's always in control. And it's only appropriate if cc-rs stays out of the way...

With the current behaviour (if you don't override anything) cc-rs matches what rustc does. If you set crt-static in rustc then rustc with set libcmt.lib and cc-rs will use -MT. If you don't then rustc will use msvcrt.lib and cc-rs will use -MD. So they match up.

I have a number of issues with the way Rust handles linking the C runtime and the UCRT but I do think cc-rs should match it as a default.

@dot-asm
Copy link
Contributor Author

dot-asm commented Nov 24, 2022

With the current behaviour (if you don't override anything) cc-rs matches what rustc does.

Does it though? As already discussed, the difference between /MT and /MD is a) /defaultlib directives in .drective section, and b) __imp_ symbol decoration in /MD case. Does rustc emit linker directives? No. Does the libc crate decorate referred CRT symbols? ... No. In other words if cc-rs was to actually match what rustc and libc do, it's the /MT /Zl combination that takes you there. It just works!

@ChrisDenton
Copy link
Member

If passing /MT to cl.exe you do not get _DLL defined in the C/C++ code. This is an observable difference between /MT and /MD beyond just linking (and that's even without the possibility of undocumented differences).

Using /MT /Zl silences linker warnings by also removing the directives that will cause incompatibility warnings. But it doesn't stop those incompatibilities from existing in compiled code.

@dot-asm
Copy link
Contributor Author

dot-asm commented Nov 24, 2022

If passing /MT to cl.exe you do not get _DLL defined in the C/C++ code.

As already discussed, _DLL translates exclusively to declspec(dllimport) attributes, which in turn translate to __imp_ symbol decorations. That is all to it.

/MT /Zl ... doesn't stop those incompatibilities from existing in compiled code.

Can you give a concrete example of one? As none can be observed on libc crate's boundary. And my assertion is that there are actually none. Again, the lack of the abovementioned symbol decoration is not an issue, it's dealt with, and libc crate depends on this fact.

@ChrisDenton
Copy link
Member

_DLL is a define. You can test it with an #ifdef.

@dot-asm
Copy link
Contributor Author

dot-asm commented Nov 24, 2022

_DLL is a define. You can test it with an #ifdef.

So you mean that you expect that application code somebody compiles with cc-rs could check for it. I've already addressed this hypothetical as "At the same time it's not formally reasonable to expect that arbitrary code would work without any adaptation. After all, it has to play on the host's, Rust's in this case, terms." In other words, if somebody checks for it and it turns to be a problem, it would be for them to resolve :-)

@ChrisDenton
Copy link
Member

Any code, including the C/C++ CRT headers themselves, can check defines, now or in the future. It would be unreasonable to expect everyone else to audit 30 years of C/C++ code and remove and fixup _DLL checks just to make us happy. Especially as it's documented behaviour.

Yes, we can say people should adapt their C/C++ code to work with Rust tooling, but why should they when simply passing the right cl flag avoids the issue entirely?

@ChrisDenton
Copy link
Member

(I'd also note that people do also use cc-rs to compile dlls, though technically it's geared towards making static libs. In that case the issue is even more concerning)

@dot-asm
Copy link
Contributor Author

dot-asm commented Nov 24, 2022

the C/C++ CRT headers themselves,

And assertion is that all C CRT headers do is symbol decoration. But there you actually found stronger argument in your favour:-) Even a defeater:-) The C++ CRT. Trouble is that /Zl doesn't affect the reference to C++ CRT. And it seems that the dynamic C++ CRT can be linked only with dynamic C CRT. Moreover, the compiler leaves /failifmismatch directives preventing the kind of thing one can pull off with C, i.e. actually postpone the CRT flavour choice till the link time...

@dot-asm
Copy link
Contributor Author

dot-asm commented Nov 24, 2022

I'd also note that people do also use cc-rs to compile dlls

??? Where does cc-rs do that? Or are you referring to cdylib as crate type? In this case it's [still] rustc that calls the shorts at link time, and not a problem to link -MT -Zl C[!] into a .dll [in general or cdylib in particular]...

@dot-asm
Copy link
Contributor Author

dot-asm commented Nov 24, 2022

postpone the CRT flavour choice till the link time...

Well, it should still be possible to use -MT -Zl with C files and follow Rust's crt-static when compiling C++ files... But it surely would be even more controversial :-) :-) :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-windows Windows targets and toolchains
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants