Skip to content

Fix issue 30063 (reset codegen-units to 1 when necessary) #30208

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

Merged
merged 2 commits into from
Dec 9, 2015

Conversation

pnkfelix
Copy link
Member

@pnkfelix pnkfelix commented Dec 4, 2015

When given rustc -C codegen-units=4 --emit=obj, reset units back to 1.

Fix #30063

Note: while this code is careful to handle the case of mutliple emit types (e.g. --emit=asm,obj) by reporting all the emit types that conflict with codegen units in its warnings, an invocation with multiple emit types and -o PATH will continue to ignore the requested target path (with a warning), as it already does today, since the code that checks for that is further downstream. (Multiple emit types without -o PATH will "work", though it will downgrade codegen-units to 1 just like all the other cases.)

r? @alexcrichton


// Issue #30063: if user requests llvm-related output to one
// particular path, disable codegen-units.
if matches.opt_present("o") && cg.codegen_units != 1 {
Copy link
Member

Choose a reason for hiding this comment

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

I'd personally prefer to omit the check here for -o and always execute this logic if codegen_units != 1, that way we maintain equivalence between rustc -o foo foo.rs and rustc foo.rs (for all emit types)

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay; I was trying to minimize the impact of the change over the current state of affairs, but I'm willing to concede that the principle of least surprise implies we should adopt the tack you suggest.

Copy link
Member Author

Choose a reason for hiding this comment

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

@alexcrichton (okay commit 24bc2d2 should address this concern.)

@alexcrichton
Copy link
Member

r=me, thanks @pnkfelix!

@pnkfelix pnkfelix changed the title Fix issue 30063 Fix issue 30063 (reset codegen-units to 1 when necessary) Dec 4, 2015
@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 4, 2015

@bors r=alexcrichton

@bors
Copy link
Collaborator

bors commented Dec 4, 2015

📌 Commit 199d2e4 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Dec 5, 2015

⌛ Testing commit 199d2e4 with merge ff6fa09...

@bors
Copy link
Collaborator

bors commented Dec 5, 2015

💔 Test failed - auto-mac-64-nopt-t

@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 5, 2015

Doh,I was running the Makefile by hand and forgot the driver will already add an --out-dir

@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 5, 2015

@bors r=alexcrichton

@bors
Copy link
Collaborator

bors commented Dec 5, 2015

📌 Commit d623dd9 has been approved by alexcrichton

@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 5, 2015

@bors r=alexcrichton

@bors
Copy link
Collaborator

bors commented Dec 5, 2015

📌 Commit d623dd9 has been approved by alexcrichton

@bors
Copy link
Collaborator

bors commented Dec 5, 2015

⌛ Testing commit d623dd9 with merge 0191c29...

@bors
Copy link
Collaborator

bors commented Dec 5, 2015

💔 Test failed - auto-mac-64-nopt-t

@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 7, 2015

Hmm, it seems that test/run-make/sepcomp-cci-copies is requiring that we support using -C codegen-units=N and --emit=llvm-ir.

@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 7, 2015

@alexcrichton given that at least one pre-existing test is relying on the ability to mix -C codegen-units and --emit=llvm-thingie, I'm tempted to go back to my previous more conservative approach of only erroring when one mixes -o <file> in the invocation as well.

What do you think?


  • Yet another option would be to support mixing -C codegen-units and --emit=llvm-thingie (when -o absent) only for certain llvm-thingie ... namely llvm-ir and a few others. (But I am not terribly happy with that, it sounds very ad hoc...)
  • Or we could add the "pluralized variants", as mentioned elsewhere

Update: @alexcrichton gave me a quicker solution to the problem here: don't use --emit=llvm-ir for the test in question; use -C save-temps instead.

@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 8, 2015

Update: @alexcrichton gave me a quicker solution to the problem here: don't use --emit=llvm-ir for the test in question; use -C save-temps instead.

This didn't work, since -C save-temps does not emit disassembled LLVM .ll files (just .bc bitcode files)


I discussed the matter face-to-face with @alexcrichton and @huonw , and @alexcrichton agreed that my original approach (of only downshifting to code-units=1 if one also passed -o) is the better way to go here.

…ts back to 1.

Fix rust-lang#30063

Note: while this code is careful to handle the case of mutliple emit
types (e.g. `--emit=asm,obj`) by reporting all the emit types that
conflict with codegen units in its warnings, an invocation with
multiple emit types *and* `-o PATH` will continue to ignore the
requested target path (with a warning), as it already does today,
since the code that checks for that is further downstream.
pnkfelix added a commit to pnkfelix/rust that referenced this pull request Dec 8, 2015
The `rsbegin.o` and `rsend.o` build products should not be generated
on non WinGnu platforms.

This is another path to resolving rust-lang#30063 for non win-gnu targets.
(And it won't require a snapshot, unlike PR rust-lang#30208.)
@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 8, 2015

@bors r=alexcrichton 9b5b2e3

bors added a commit that referenced this pull request Dec 9, 2015
 When given `rustc -C codegen-units=4 --emit=obj`, reset units back to 1.

Fix #30063

Note: while this code is careful to handle the case of mutliple emit types (e.g. `--emit=asm,obj`) by reporting all the emit types that conflict with codegen units in its warnings, an invocation with multiple emit types *and* `-o PATH` will continue to ignore the requested target path (with a warning), as it already does today, since the code that checks for that is further downstream.  (Multiple emit types without `-o PATH` will "work", though it will downgrade codegen-units to 1 just like all the other cases.)

r? @alexcrichton
@bors
Copy link
Collaborator

bors commented Dec 9, 2015

⌛ Testing commit 9b5b2e3 with merge 56a1f51...

@bors bors merged commit 9b5b2e3 into rust-lang:master Dec 9, 2015
bors added a commit that referenced this pull request Dec 9, 2015
…alexcrichton

The `rsbegin.o` and `rsend.o` build products should not be generated
on non WinGnu platforms.

This is another path to resolving #30063 for non win-gnu targets.
(And it won't require a snapshot, unlike PR #30208.)

r? @alexcrichton
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.

3 participants