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.

Sign up for GitHub

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

Jump to bottom

WIP: cc-wrapper: -nostdlib doessn't mean not C++ #85513

Closed
wants to merge 1 commit into from
Closed

WIP: cc-wrapper: -nostdlib doessn't mean not C++ #85513

wants to merge 1 commit into from

Conversation

Ericson2314
Copy link
Member

Motivation for this change

This was introduced in 87607af, which
made the wrapper aware of -x <lang> flags. I am not sure why this new
case was added, especially with the -l that looks like a mistake.

FWIW, https://github.com/briansmith/nostdlib is an example of writing C++
with -nosdlib.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@Ericson2314
cc-wrapper: -nostdlib doessn't mean not C++
ae175a8 
This was introduced in 87607af, which
made the wrapper aware of `-x <lang>` flags. I am not sure why this new
case was added, especially with the `-l` that looks like a mistake.

FWIW, https://github.com/briansmith/nostdlib is an example of writing C++
with -nosdlib.
@veprbl
Copy link
Member

veprbl commented Apr 18, 2020

I don't see the contradiction. "isCpp=-1" disables linking to the standard c++ libraries, you can write C++ code with -nosdlib, but you would not be able to use the libraries anymore. Perhaps, should rename the variable instead.

@Ericson2314 Ericson2314 changed the title cc-wrapper: -nostdlib doessn't mean not C++ WIP: cc-wrapper: -nostdlib doessn't mean not C++ Apr 18, 2020
@Ericson2314
Copy link
Member Author

Do those flags with GCC affect search paths? Right now we control some -l and -L together which also seems wrong. I will mark this WIP while we figure out what should happen.

@FRidh FRidh added this to WIP in Staging Apr 21, 2020
@Ericson2314
Copy link
Member Author

@Mic92 Might you have some insight here, since you just did the -nostdinc fix?

@Mic92
Copy link
Member

Mic92 commented Aug 3, 2020

Maybe it could set a noCppLink but still allow to treat the file as c++.

@Ericson2314
Copy link
Member Author

Well does -nostdlib actually affect lookup paths, or just what libraries are linked by the default? https://gcc.gnu.org/onlinedocs/gcc/Link-Options.html doen't mention search paths so I think it's just the latter.

@Mic92
Copy link
Member

Mic92 commented Aug 4, 2020 •
edited

Well does -nostdlib actually affect lookup paths, or just what libraries are linked by the default? gcc.gnu.org/onlinedocs/gcc/Link-Options.html doen't mention search paths so I think it's just the latter.

It skip linking startup files, search paths are kept intact. The following builds:

$ printf '#include <optional>\n extern \"C\" { void _start(){} }' | g++  -nostdlib -o foo -xc++ -

@Ericson2314
Copy link
Member Author

Great, thanks for checking! I think that means this PR is correct, do you?

@Mic92
Copy link
Member

Mic92 commented Aug 5, 2020 •
edited

Great, thanks for checking! I think that means this PR is correct, do you?

I don't think it is a complete fix for it. Since the wrapper adds NIX_CXXSTDLIB_LINK to NIX_CFLAGS_LINK it would still link those libraries since they have been passed manually from the compilers point of view. Therefore I would say we need a dedicated variable set that will not add those linker flags while still having c++ header in the search path.

@Ericson2314
Copy link
Member Author

Oh right! Because we manually do e.g. -lc++abi to force some odd combinations of GCC and LLVM stuff. Thanks! Now I know why this was written this way in the first place---as a crude fix around that.

@ryantm ryantm marked this pull request as draft October 23, 2020 03:01
@stale
Copy link

stale bot commented Jun 4, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jun 4, 2021
@veprbl
Copy link
Member

veprbl commented Jun 17, 2021

Fixed by #121527

@veprbl veprbl closed this Jun 17, 2021
Staging automation moved this from WIP to Done Jun 17, 2021
@veprbl veprbl removed this from Done in Staging Jun 17, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ 10.rebuild-darwin-stdenv 10.rebuild-linux: 501+ 10.rebuild-linux: 5001+ 10.rebuild-linux-stdenv
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@Ericson2314 @veprbl @Mic92