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

Append freebsd-version to all --target=*-freebsd if executed on FreeBSD. #788

Merged
merged 1 commit into from
Feb 5, 2023

Conversation

dot-asm
Copy link
Contributor

@dot-asm dot-asm commented Feb 5, 2023

It's a formally ambiguous thing to do, to assume that FreeBSD-on-FreeBSD cross-compilation targets the same version, but without it it doesn't work at all...

…eBSD.

It's a formally ambiguous thing to do, to assume that FreeBSD-on-FreeBSD
cross-compilation targets the same version, but without it it doesn't
work at all...
@dot-asm
Copy link
Contributor Author

dot-asm commented Feb 5, 2023

Some additional information. The problem appears to be C++-specific and depends on an include C++ search path being omitted if clang++ is invoked with version-less triplet.

EDIT. And for this reason one can wonder if it would be appropriate to extend the if condition with check for self.cpp.

Copy link
Member

@thomcc thomcc left a comment

Choose a reason for hiding this comment

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

I wonder if the reason it doesn't happen for C is just because who provides the C++ headers. Anyway, I'm a little concerned that we don't fully understand what's going on here, but I'm fine landing this in the meantime, since it's consistent/coherent with the other change I accepted for freebsd versions.

Ideally I would like a better solution here but I don't know what one looks like. Maybe checking clang's version, but that's pretty gnarly too, tbh. I'll try and look around to see what some programs do to solve this, but this is fine in the meantime.

@thomcc thomcc merged commit 0d9a0f8 into rust-lang:main Feb 5, 2023
@dot-asm
Copy link
Contributor Author

dot-asm commented Feb 5, 2023

I wonder if the reason it doesn't happen for C is just because who provides the C++ headers.

The only observable difference in C is __FreeBSD__ macro. Just '*-freebsd' yields 8, '*-freebsdN' - N. The C header search path always lists those provided by the compiler itself and system /usr/include.

Anyway, I'm a little concerned that we don't fully understand what's going on here,

It should be noted that as long as CXX is not set to clang++, everything works out fine. Because cc-rs handles default c++, even though it's clang behind the FreeBSD curtains, by passing -m32/-m64 instead of --target. It should also be noted that if the last else was else if target != host, we wouldn't be having these conversations :-)

@dot-asm
Copy link
Contributor Author

dot-asm commented Feb 5, 2023

It should be noted that as long as CXX is not set to clang++, everything works out fine.

Hmmm, I realized that it might happen that it holds true only for Tier 1 and Tier 2 FreeBSD platforms out-of-the-box. Though Tier 3 would require a cross-linker specified, which should show up as a compiler through RUSTC_LINKER, in which case not passing --target would work out. [And as already mentioned elsewhere I would advocate for a warning in case prefixed compiler command, a-b-c-cc, does not match Rust x-y-z target.]

@thomcc
Copy link
Member

thomcc commented Feb 6, 2023

The only observable difference in C is __FreeBSD__ macro

Hmm, I'm still somewhat concerned given that BSDs are often not ABI-stable across major versions. Also, I do not know what things in system headers might change based on the value of __FreeBSD__.

I guess I should set up a couple FreeBSD VMs of different versions to check some of this and verify my own assumptions.

And as already mentioned elsewhere I would advocate for a warning in case prefixed compiler command, a-b-c-cc, does not match Rust x-y-z target

This is tricky because it requires a more complete matching impl than we have. It's very common for cross toolchains to end up with a slightly weird CROSS_COMPILE prefix, even if they're actually the same target.

In a lot of cases folks won't see warnings we emit anyway, since it won't be in the same workspace as e.g. the build script running cc. I would expect this to be especially common for something like atypical cross-compile configurations, which is probably not common for developers.

I suppose warning in obviously wrong cases isn't unreasonable though.

@dot-asm
Copy link
Contributor Author

dot-asm commented Feb 6, 2023

The only observable difference in C is __FreeBSD__ macro

Hmm, I'm still somewhat concerned given that BSDs are often not ABI-stable across major versions.

Of course. I merely attempted to provide some colour to the answer to the original what's-up-with-C question. Recall that my suggestion is to avoid passing --target whenever possible, most notably in non-cross-compile cases and to prefixed command lines.

And as already mentioned elsewhere I would advocate for a warning in case >> prefixed compiler command, a-b-c-cc, does not match Rust x-y-z target

This is tricky because it requires a more complete matching impl than we have.

I'm not saying that it's trivial, but that it would be a sensible thing to do. The "warning in obviously wrong cases isn't unreasonable though" remark seems to indicate that we agree :-)

@mqudsi
Copy link
Contributor

mqudsi commented Feb 7, 2023

My preference would be to err on the side of caution and having the compiler error out (as it did before this) and the user realizing (somehow.. because we don't emit a warning!) they need to override the (compiler's, not cargo's) --target parameter when cross-compiling rather than silently assume something that might not be the case (i.e. we end up enabling a FreeBSD 14 API when the user is targeting FreeBSD 13 i686 on a FreeBSD 14 x86_64 host).

This patch "just works" in more cases than before, but at the cost of some guesswork - unless I'm misunderstanding something, of course.

#785 was a lot cleaner because 99.9% of the cases it affected were no cross-compilation at all; the edge case would be "cross compiling for FreeBSD 13 x86_64 on a FreeBSD 14 x86_64 host" and if there were a clean way to detect at the patch site whether or not cargo was executed with a cross-compilation option at all or not, I would use it to make even that error out.

@thomcc
Copy link
Member

thomcc commented Feb 8, 2023

Yeah, so I think I've become kind of convinced this whole line of fixes is wrong. The first clue being that clang++ 14's fix was not to use the host's freebsd-version.

A particularly damning thing is that it seems that we're the reason clang had to tweak in 14 this in the first place (https://bugzilla.mozilla.org/show_bug.cgi?id=1628567 is linked from that clang patch, and is an error coming from cc-rs... target doesn't have a version either, so it's not really like we had the info and lost it).

In either case, I think given that clang went out of it's way to unbreak us, and even to push us to this behavior, we should have a good reason for not following their approach. Thankfully, following the logic in https://reviews.llvm.org/D77776 seems straightforward:

  • If we're compiling C++ with clang++ (do nothing when compiling C, or using GCC)
  • If the target we have is not already versioned target.
  • If the c++ stdlib has not been explicitly configured already (unsure about this one...)
  • Default to using libc++ (call build.cpp_set_stdlib("c++"))

Thoughts?

@mqudsi
Copy link
Contributor

mqudsi commented Feb 8, 2023

I think your sequence is correct. I agree with the "if the c++ stdlib has not been explicitly configured" for the reasons below. (unless there's a chance that some other tool might incorrectly configure the stdlib before cc-rs enters the picture?)

As I understand it, clang++ (versions 13 and below for the sake of discussion) default to libstdc++ unless FreeBSD major is 10 or above. libstdc++ will work so long as you're compiling C++98 (not C++11 or greater), though perhaps suboptimally as it isn't updated. But if you're using C++11 or newer, you need to be using libstdc++.

  • The fix in Fix --target cflag on FreeBSD when compiling against clang #785 was actually one of the proposed solutions in the LLVM issue (use the host FreeBSD major version at build time), but, as we noted in that issue, cannot be safely assumed to extend to cross-compilation cases. I don't question its correctness but only its efficacy (in that it doesn't extend to cross-compilation cases).
  • Another (similar) suggestion was to use the lowest supported version that ships with libc++ (xxx-unknown-freebsd10), which would be a valid cross-compilation solution (less preferable than using the host version if you know you're not cross-compiling, in case some of the C/C++ code does in fact test the FreeBSD version preprocessor macro).
  • The fix in this PR isn't correct, both by extension and as pointed out in the discussion above, because you could be cross-compiling to a lower FreeBSD major version.

In both these cases, all that does is get clang++ to use libc++ instead of libstdc++.

Explicitly passing in -std=libc++ would side-step the whole thing and just make clang++ work without needing to infer anything, and without overriding the FreeBSD version macro. It would also work for both native and cross-compilation scenarios (with the caveat of not supporting FreeBSD 9 and below, because they don't ship with libc++).

That last caveat is why we should (if we can) include your "If the c++ stdlib has not been explicitly configured already" step - one could theoretically cross-compile C++98 code on a FreeBSD 14 host with cc-rs, targeting FreeBSD 9, and have it work ok by explicitly specifying -std=libstdc++ (though this is assuming rust code itself can be targeted against FreeBSD 9 in the first place).

(I can confirm that replacing the recently added "append freebsd-version to triple" patch with "add -std=libc++" passes the same tests that failed before #785.)

For reference: https://wiki.freebsd.org/NewC++Stack

@dot-asm
Copy link
Contributor Author

dot-asm commented Feb 8, 2023

  • The fix in this PR isn't correct, both by extension and as pointed out in the discussion above, because you could be cross-compiling to a lower FreeBSD major version.

Well, it's not more incorrect than the originating #785 :-) It's arguably less incorrect, because the originating PR left even the simplest case of cross-compilation in the cold :-)

Either way, I would still argue for not passing any --target assuming that the commands found on user $PATH meet the user's expectations. And if the user wants finer control, it would be up to the user. Want to target FreeBSD-N on FreeBSD-M? Pass corresponding --target in the build script or adjust C[XX]FLAGS accordingly. As for switching downgrading from libc++ to libstdc++, it's probably not inappropriate to put it on the user as well(*). The build script would be the only option. But if one was to make the choice automatic, most reliable way would be to execute the compiler with -dM -E -x c++ /dev/null flags and examine the __FreeBSD__ definition...

(*) After all, the corresponding FreeBSD versions went out of support prior this project's MSRV was released.

@mqudsi
Copy link
Contributor

mqudsi commented Feb 9, 2023

I'm of the opinion that "always erroring out" is more correct than "sometimes works, sometimes introduces subtle issues," so perhaps you're right if you look at it from your perspective and not mine. But in this case, we actually do know the right thing to do so there's no reason to punt on this.

In all cases, not passing in anything (neither --target nor -std=...) is a fine alternative to the patch in #785 but again doesn't work for cross compilation (since you need to pass in a --target so you either need to pass in a target triple with the version number or without it but w/ -std=...). As mentioned from the start, conceptually I prefer not passing anything where possible as it results in the least surprise to the user. Repeating myself from the other issue to emphasize that I agree with you here, breaking the build if the user specified a triple-prefixed binary as CC is probably acceptable, even if technically a regression.

mqudsi added a commit to mqudsi/cc-rs that referenced this pull request Feb 9, 2023
This avoids certain corner cases where supplying the same llvm triple used by
rust for clang++ can cause native (host == target) C++ builds to break.

This fix supersedes rust-lang#785. See relevant discussions in rust-lang#785 and rust-lang#788.
mqudsi added a commit to mqudsi/cc-rs that referenced this pull request Feb 9, 2023
When cross compiling, we are forced to emit the `--target=<triple>` compiler
flag, but this can break clang++ C++ builds on FreeBSD if the triple isn't
set to a value that causes clang++ to infer the use of libc++ instead of
libstdc++ (the latter of which does not support C++11 and beyond on FreeBSD).

By manually specifying the usage of `-std=libc++` we avoid the need to know
which version of FreeBSD we are targeting (as we do not have that information
from the rust target triple and cannot infer it from the host as we are
cross-compiling).

Note that we were already specifying a default stdlib value libc++ for
`cpp_link_stdlib` under FreeBSD but that is not sufficient.

This fix supersedes rust-lang#788. See relevant discussions in rust-lang#785 and rust-lang#788.
@mqudsi
Copy link
Contributor

mqudsi commented Feb 9, 2023

See #794

mqudsi added a commit to mqudsi/cc-rs that referenced this pull request Feb 10, 2023
When cross compiling, we are forced to emit the `--target=<triple>` compiler
flag, but this can break clang++ C++ builds on FreeBSD if the triple isn't
set to a value that causes clang++ to infer the use of libc++ instead of
libstdc++ (the latter of which does not support C++11 and beyond on FreeBSD).

By manually specifying the usage of `-std=libc++` we avoid the need to know
which version of FreeBSD we are targeting (as we do not have that information
from the rust target triple and cannot infer it from the host as we are
cross-compiling).

Note that we were already specifying a default stdlib value libc++ for
`cpp_link_stdlib` under FreeBSD but that is not sufficient.

There are other solutions that would work when not cross compiling such as
omitting the `--target` altogether, but there's no compelling reason to add more
code if this solution works for both native and cross-compiling cases.

This fix supersedes rust-lang#788. See relevant discussions in rust-lang#785 and rust-lang#788.
mqudsi added a commit to mqudsi/cc-rs that referenced this pull request Feb 10, 2023
When cross compiling, we are forced to emit the `--target=<triple>` compiler
flag, but this can break clang++ C++ builds on FreeBSD if the triple isn't
set to a value that causes clang++ to infer the use of libc++ instead of
libstdc++ (the latter of which does not support C++11 and beyond on FreeBSD).

By manually specifying the usage of `-std=libc++` we avoid the need to know
which version of FreeBSD we are targeting (as we do not have that information
from the rust target triple and cannot infer it from the host as we are
cross-compiling).

Note that we were already specifying a default stdlib value libc++ for
`cpp_link_stdlib` under FreeBSD but that is not sufficient.

There are other solutions that would work when not cross compiling such as
omitting the `--target` altogether, but there's no compelling reason to add more
code if this solution works for both native and cross-compiling cases.

This fix supersedes rust-lang#788. See relevant discussions in rust-lang#785 and rust-lang#788.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants