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

Stop `-O`/`-C opt-level` and `-g`/`-C debuginfo` conflicting #60426

Merged
merged 7 commits into from May 6, 2019

Conversation

Projects
None yet
10 participants
@varkor
Copy link
Member

commented Apr 30, 2019

Allow -O and -C opt-level, and -g and -C debuginfo to be specified simultaneously without conflict. In such cases, the rightmost provided option is chosen.

Fixes #7493.
Fixes #32352.

Blocked on rust-lang-nursery/getopts#79.

r? @alexcrichton

@@ -0,0 +1,22 @@
-include ../../run-make-fulldeps/tools.mk

# FIXME: it would be good to check that it's actually the rightmost flags

This comment has been minimized.

Copy link
@varkor

varkor Apr 30, 2019

Author Member

Ideas for checking this in the test are welcome!

@varkor varkor changed the title Stop `-O` and `-C opt-level`, and `-g` and `-C debuginfo`, conflicting Stop `-O`/`-C opt-level` and `-g`/`-C debuginfo` conflicting Apr 30, 2019

@alexcrichton

This comment has been minimized.

Copy link
Member

commented May 1, 2019

Looks reasonable to me! I think testing manually here is fine for now, I also don't know of a great way to test that the right setting is applied.

Could this also include a small comment in the argument parsing about how it's finding the last option specified?

@varkor

This comment has been minimized.

Copy link
Member Author

commented May 1, 2019

Could this also include a small comment in the argument parsing about how it's finding the last option specified?

Done.

@varkor varkor referenced this pull request May 2, 2019

Merged

Update getopts #60423

@alexcrichton

This comment has been minimized.

Copy link
Member

commented May 2, 2019

r=me when this is ready to go

@varkor varkor force-pushed the varkor:fix-duplicate-arg-handling branch from 18ca3d8 to f6b5e8a May 3, 2019

@varkor varkor marked this pull request as ready for review May 3, 2019

@varkor

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

@bors r=alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

📌 Commit f6b5e8a has been approved by alexcrichton

@varkor

This comment has been minimized.

Copy link
Member Author

commented May 3, 2019

@bors rollup

@Centril

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

@bors rollup-

(don't rollup PRs that touch .lock files)

Centril added a commit to Centril/rust that referenced this pull request May 3, 2019

Rollup merge of rust-lang#60426 - varkor:fix-duplicate-arg-handling, …
…r=alexcrichton

Stop `-O`/`-C opt-level` and `-g`/`-C debuginfo` conflicting

Allow `-O` and `-C opt-level`, and `-g` and `-C debuginfo` to be specified simultaneously without conflict. In such cases, the rightmost provided option is chosen.

Fixes rust-lang#7493.
Fixes rust-lang#32352.

~Blocked on rust-lang-nursery/getopts#79

r? @alexcrichton

@Centril Centril referenced this pull request May 3, 2019

Closed

Rollup of 8 pull requests #60519

bors added a commit that referenced this pull request May 3, 2019

Auto merge of #60519 - Centril:rollup-jvrq9h1, r=Centril
Rollup of 8 pull requests

Successful merges:

 - #59520 (rustc_codegen_llvm: support 128-bit discriminants in debuginfo.)
 - #60426 (Stop `-O`/`-C opt-level` and `-g`/`-C debuginfo` conflicting)
 - #60429 (Account for paths in incorrect pub qualifier help)
 - #60449 (Constrain all regions in the concrete type for an opaque type)
 - #60476 (build dist-aarch64-linux with --enable-profiler)
 - #60486 (Place related refactors)
 - #60496 (Fix potential integer overflow in SGX memory range calculation.)
 - #60517 (Reword casting message)

Failed merges:

r? @ghost
@jethrogb

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

[00:59:29] ---- [run-make] run-make/override-aliased-flags stdout ----
[00:59:29] note: Run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
[00:59:29] 
[00:59:29] error: make failed
[00:59:29] status: exit code: 2
[00:59:29] command: "make"
[00:59:29] stdout:
[00:59:29] ------------------------------------------
[00:59:29] # Test that `-O` and `-C opt-level` can be specified multiple times.
[00:59:29] # The rightmost flag will be used over any previous flags.
[00:59:29] LD_LIBRARY_PATH="/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/override-aliased-flags/override-aliased-flags:/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib:/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-bootstrap-tools/x86_64-unknown-linux-gnu/release/deps:/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/lib" '/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc' --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/override-aliased-flags/override-aliased-flags -L /checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/override-aliased-flags/override-aliased-flags  -Clinker=arm-none-eabi-gcc -O -O main.rs
[00:59:29] Makefile:8: recipe for target 'all' failed
[00:59:29] 
[00:59:29] ------------------------------------------
[00:59:29] stderr:
[00:59:29] ------------------------------------------
[00:59:29] error: linking with `arm-none-eabi-gcc` failed: exit code: 1
[00:59:29]   |
[00:59:29]   = note: "arm-none-eabi-gcc" "-Wl,--as-needed" "-Wl,-z,noexecstack" "-m64" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/override-aliased-flags/override-aliased-flags/main.main.7rcbfp3g-cgu.0.rcgu.o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/override-aliased-flags/override-aliased-flags/main.main.7rcbfp3g-cgu.1.rcgu.o" "-o" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/override-aliased-flags/override-aliased-flags/main" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/override-aliased-flags/override-aliased-flags/main.4s37gsrti678ik8u.rcgu.o" "-Wl,--gc-sections" "-pie" "-Wl,-zrelro" "-Wl,-znow" "-Wl,-O1" "-nodefaultlibs" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/run-make/override-aliased-flags/override-aliased-flags" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "-Wl,--start-group" "-Wl,-Bstatic" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/libstd-36963fe6a2961b22.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/libpanic_unwind-bad302bb9c2be352.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/libbacktrace_sys-068d4c647d225b00.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/librustc_demangle-dd618d3a61d2285d.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/libhashbrown-08bd494bdfbe6ec5.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/librustc_std_workspace_alloc-f434e5c003b087ed.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/libunwind-4ad3589c043e8a63.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/liblibc-b93458722dc1ee7d.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/liballoc-cbc983130e35415c.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/librustc_std_workspace_core-b986af5e424776e6.rlib" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcore-08dafd64384d2a0a.rlib" "-Wl,--end-group" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib/libcompiler_builtins-bc40ea2e2336c57a.rlib" "-Wl,-Bdynamic" "-ldl" "-lrt" "-lpthread" "-lgcc_s" "-lc" "-lm" "-lrt" "-lpthread" "-lutil" "-lutil"
[00:59:29]   = note: arm-none-eabi-gcc: error: unrecognized command line option '-m64'
[00:59:29]           
[00:59:29] 
[00:59:29] error: aborting due to previous error
[00:59:29] 
[00:59:29] make: *** [all] Error 1
[00:59:29] 
[00:59:29] ------------------------------------------

https://travis-ci.com/rust-lang/rust/jobs/197566788 via #60519

@Centril

This comment has been minimized.

Copy link
Contributor

commented May 3, 2019

@bors r-

@bors

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

☔️ The latest upstream changes (presumably #60117) made this pull request unmergeable. Please resolve the merge conflicts.

@varkor varkor force-pushed the varkor:fix-duplicate-arg-handling branch from 2eec7f5 to 5ba5d35 May 4, 2019

@varkor

This comment has been minimized.

Copy link
Member Author

commented May 4, 2019

@bors r=alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

commented May 4, 2019

📌 Commit 5ba5d35 has been approved by alexcrichton

@varkor

This comment has been minimized.

Copy link
Member Author

commented May 5, 2019

@bors r=alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

📌 Commit 80f54ba has been approved by alexcrichton

@bors

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

⌛️ Testing commit 80f54ba with merge 231d2cd...

bors added a commit that referenced this pull request May 5, 2019

Auto merge of #60426 - varkor:fix-duplicate-arg-handling, r=alexcrichton
Stop `-O`/`-C opt-level` and `-g`/`-C debuginfo` conflicting

Allow `-O` and `-C opt-level`, and `-g` and `-C debuginfo` to be specified simultaneously without conflict. In such cases, the rightmost provided option is chosen.

Fixes #7493.
Fixes #32352.

~Blocked on rust-lang-nursery/getopts#79

r? @alexcrichton
@bors

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

💔 Test failed - checks-travis

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented May 5, 2019

The job dist-powerpc64-linux of your PR failed on Travis (raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
[00:34:54] [RUSTC-TIMING] syntax_ext test:false 39.731
[00:40:59] [RUSTC-TIMING] rustc test:false 405.515
[00:40:59]    Compiling rustc_mir v0.0.0 (/checkout/src/librustc_mir)
[00:40:59]    Compiling rustc_typeck v0.0.0 (/checkout/src/librustc_typeck)
No output has been received in the last 30m0s, this potentially indicates a stalled build or something wrong with the build itself.
Check the details on how to adjust your build configuration on: https://docs.travis-ci.com/user/common-build-problems/#Build-times-out-because-no-output-was-received
The build has been terminated

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@varkor

This comment has been minimized.

Copy link
Member Author

commented May 5, 2019

@bors retry

@Zoxc

This comment has been minimized.

Copy link
Contributor

commented May 5, 2019

These command line flags can conflict though. Why should we not emit an error?

Manishearth added a commit to Manishearth/rust that referenced this pull request May 5, 2019

Rollup merge of rust-lang#60426 - varkor:fix-duplicate-arg-handling, …
…r=alexcrichton

Stop `-O`/`-C opt-level` and `-g`/`-C debuginfo` conflicting

Allow `-O` and `-C opt-level`, and `-g` and `-C debuginfo` to be specified simultaneously without conflict. In such cases, the rightmost provided option is chosen.

Fixes rust-lang#7493.
Fixes rust-lang#32352.

~Blocked on rust-lang-nursery/getopts#79

r? @alexcrichton

bors added a commit that referenced this pull request May 5, 2019

Auto merge of #60567 - Manishearth:rollup-rjagqnw, r=Manishearth
Rollup of 5 pull requests

Successful merges:

 - #60131 (Fix broken link in rustc_plugin doc)
 - #60426 (Stop `-O`/`-C opt-level` and `-g`/`-C debuginfo` conflicting)
 - #60515 (use span instead of div for since version)
 - #60530 (rustc: rename all occurences of "freevar" to "upvar".)
 - #60536 (Correct code points to match their textual description)

Failed merges:

r? @ghost

@bors bors merged commit 80f54ba into rust-lang:master May 6, 2019

1 of 2 checks passed

homu Test failed
Details
Travis CI - Pull Request Build Passed
Details

@varkor varkor deleted the varkor:fix-duplicate-arg-handling branch May 6, 2019

@varkor

This comment has been minimized.

Copy link
Member Author

commented May 6, 2019

@Zoxc: the original issue threads contain motivation. Flags in general can be specified multiple times and -O and -C opt-level are supposed to act like synonyms, so it's surprising that they conflict with each other.

@Zoxc

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

I guess nonsensical things like -C opt-level=2 -C opt-level=3 is unfortunately allowed already.

@petrochenkov

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

It's not nonsensical, it's needed for hierarchical build systems.
The top level project sets default flags and sub-projects can override by appending to default flags (and sub-sub-projects can override by appending again).

@Zoxc

This comment has been minimized.

Copy link
Contributor

commented May 6, 2019

s/hierarchical/broken/


# FIXME: it would be good to check that it's actually the rightmost flags
# that are used when multiple flags are specified, but I can't think of a
# reliable way to check this.

This comment has been minimized.

Copy link
@Lonami

Lonami May 8, 2019

Can't we make rustc show or warn to stderr about which flag it chose? "Note: both foo and bar specified. Using bar", then, we could just capture its output and compare.

This comment has been minimized.

Copy link
@Lonami

Lonami May 8, 2019

(cc @varkor for his comment on #60426 (comment) which I read just now)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.