Use sess.opts.optimize instead of sess.opts.cg.opt_level for LTO optlevel #22530

rprichard · 2015-02-19T11:22:05Z

I wasn't sure if I should reuse write::get_llvm_opt_level or not. It returns an llvm::CodeGenOptLevel, which is the Rust binding for CodeGenOpt::Level. lto.rs is passing an optlevel to LLVM's PassManagerBuilder, which takes an unsigned int. PassManagerBuilder's optlevel uses essentially the same enumeration (i.e. 0-3 with 2 as default), but it's implicit.

…evel Fixes rust-lang#22525

rust-highfive · 2015-02-19T11:22:18Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @alexcrichton (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. The way Github handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see CONTRIBUTING.md for more information.

dotdash · 2015-02-19T12:38:36Z

@bors r+ dc3bc90

Fixes rust-lang#22525 I wasn't sure if I should reuse `write::get_llvm_opt_level` or not. It returns an `llvm::CodeGenOptLevel`, which is the Rust binding for `CodeGenOpt::Level`. `lto.rs` is passing an optlevel to LLVM's `PassManagerBuilder`, which takes an unsigned int. `PassManagerBuilder`'s optlevel uses essentially the same enumeration (i.e. 0-3 with 2 as default), but it's implicit.

Manishearth · 2015-02-21T05:23:48Z

I have a suspicion this breaks the android build. See http://buildbot.rust-lang.org/builders/auto-linux-64-x-android-t/builds/3689

(Not yet confirmed if it really is due to this PR)

rprichard · 2015-02-21T07:16:13Z

That seems plausible. The failing test run-pass/sepcomp-lib-lto.rs is built with -C lto -O, whose behavior changed with this PR. I wonder if the test would also fail with -C lto -C opt-level=2. If so, that would indicate this PR more strongly, but it'd also imply that LTO on Android was already broken, I think.

AFAICT, it seems that the Android NDK does not have pthread_atfork in any static or dynamic library for ARM. Maybe jemalloc is supposed to use the stub in src/rt/rust_android_dummy.c?

Manishearth · 2015-02-21T07:23:38Z

I think @alexcrichton mentioned something about a bunch of missing symbols being hacked around on Android. I guess you should discuss with him?

Yeah, it seems like LTO on Android is partially broken. I guess we could land this and put up an issue for investigating this. Not sure.

rprichard · 2015-02-21T07:47:25Z

I'm currently building Rust for Android so I can take a closer look at it.

Manishearth · 2015-02-21T21:31:19Z

I'm able to build it at stage1 with no errors.

I'll need to try doing it for stage2, one moment.

(Moving the test from run-pass to compile-fail means that you don't need an android device to test it)

Manishearth · 2015-02-21T22:15:50Z

Stage2 passes too. Hmm.

(I'm only testing compilation)

rprichard · 2015-02-21T22:24:43Z

I'm able to reproduce the pthread_atfork build error on my machine now. I
built Rust with my patch. The test fails with either -C opt-level=2 -C
lto or with -O -C lto, but passes with -C opt-level=0 -C lto.

On Sat, Feb 21, 2015 at 2:16 PM, Manish Goregaokar <[email protected]

wrote:

Stage2 passes too. Hmm.

—
Reply to this email directly or view it on GitHub
#22530 (comment).

Manishearth · 2015-02-21T22:25:53Z

Hm, I'd included your patch too, but I might have done it wrong.

Suggestions on how to proceed?

rprichard · 2015-02-21T22:33:14Z

Here's the command-line I'm using to compile the test case:

RUSTC="/home/rprichard/work/rust-android-local/bin/rustc \
    --target=arm-linux-androideabi \
    -C linker=/home/rprichard/android-ndk-standalone-19/bin/arm-linux-androideabi-gcc"

$RUSTC ~/work/rust/src/test/auxiliary/sepcomp_lib.rs \
    -C codegen-units=3 --crate-type=rlib,dylib

$RUSTC ~/work/rust/src/test/run-pass/sepcomp-lib-lto.rs -C opt-level=2 -C lto -L.

It fails with this output:

error: linking with `/home/rprichard/android-ndk-standalone-19/bin/arm-linux-androideabi-gcc` failed: exit code: 1
note: "/home/rprichard/android-ndk-standalone-19/bin/arm-linux-androideabi-gcc" '"-Wl,--as-needed"' '"-Wl,--allow-multiple-definition"' '"-L"' '"/home/rprichard/work/rust-android-local/lib/rustlib/arm-linux-androideabi/lib"' '"-o"' '"sepcomp-lib-lto"' '"sepcomp-lib-lto.o"' '"-Wl,--whole-archive"' '"-lmorestack"' '"-Wl,--no-whole-archive"' '"-Wl,--gc-sections"' '"-Wl,-O1"' '"-nodefaultlibs"' '"/tmp/rustc.D4hSH49lQNTN/libstd-4e7c5e5c.rlib"' '"/tmp/rustc.D4hSH49lQNTN/liballoc-4e7c5e5c.rlib"' '"-L"' '"."' '"-L"' '"/home/rprichard/work/rust-android-local/lib/rustlib/arm-linux-androideabi/lib"' '"-L"' '"/home/rprichard/mess/android-test/.rust/lib/arm-linux-androideabi"' '"-L"' '"/home/rprichard/mess/android-test/lib/arm-linux-androideabi"' '"-Wl,--whole-archive"' '"-Wl,-Bstatic"' '"-Wl,--no-whole-archive"' '"-Wl,-Bdynamic"' '"-ldl"' '"-llog"' '"-lgcc"' '"-lc"' '"-lm"' '"-lcompiler-rt"'
note: /home/rprichard/android-ndk-standalone-19/bin/../lib/gcc/arm-linux-androideabi/4.6/../../../../arm-linux-androideabi/bin/ld: /tmp/rustc.D4hSH49lQNTN/liballoc-4e7c5e5c.rlib(r-jemalloc-jemalloc.pic.o): in function malloc_init_hard:jemalloc.c(.text.unlikely.malloc_init_hard+0x280): error: undefined reference to 'pthread_atfork'
collect2: ld returned 1 exit status

Manishearth · 2015-02-21T23:09:12Z

@bors: r-

Getting this out of the queue until we solve the failure :)

Manishearth · 2015-02-21T23:13:21Z

Ah, okay, I was trying to compile it a bit differently (by copying the invocation provided by compiletest and tweaking).

rprichard · 2015-02-21T23:16:57Z

I think I've figured it out.

pthread_atfork comes from r-rust_builtin-rust_android_dummy.o, which is linked into libstd-4e7c5e5c.rlib. It's needed in r-jemalloc-jemalloc.pic.o in liballoc-4e7c5e5c.rlib. However, libstd comes before liballoc on the linker command-line. The linker examines libstd first, decides that r-rust_builtin-rust_android_dummy.o is unneeded and leaves it out. Then it examines liballoc and sees that pthread_atfork is missing. It doesn't revisit libstd.

With -C opt-level=0 -C lto, it happens to work because sepcomp-lib-lto.o pulls in other symbols from r-rust_builtin-rust_android_dummy.o. Namely, it pulls in __errno_location, log2, and log2f.

To help debug this issue, pass -C link-args="-Wl,-Map=output.map -Wl,--cref" -Z print-link-args -C save-temps to rustc.

-C opt-level=0 -C lto:

/home/rprichard/android-ndk-standalone-19/bin/arm-linux-androideabi-gcc
    -Wl,--as-needed
    -Wl,--allow-multiple-definition
    -L /home/rprichard/work/rust-android-local/lib/rustlib/arm-linux-androideabi/lib
    -o sepcomp-lib-lto
    sepcomp-lib-lto.o
    -Wl,--whole-archive -lmorestack -Wl,--no-whole-archive
    -Wl,--gc-sections
    -nodefaultlibs
    /tmp/rustc.2wZNcIqEwb2J/libstd-4e7c5e5c.rlib
    /tmp/rustc.2wZNcIqEwb2J/liballoc-4e7c5e5c.rlib
    -L .
    -L /home/rprichard/work/rust-android-local/lib/rustlib/arm-linux-androideabi/lib
    -L /home/rprichard/mess/android-test/.rust/lib/arm-linux-androideabi
    -L /home/rprichard/mess/android-test/lib/arm-linux-androideabi
    -Wl,--whole-archive
    -Wl,-Bstatic
    -Wl,--no-whole-archive
    -Wl,-Bdynamic
    -ldl
    -llog
    -lgcc
    -lc
    -lm
    -Wl,-Map=output.map
    -Wl,--cref
    -lcompiler-rt

cross-reference output:

# __errno_location                                  /tmp/rustc.X0fdWX76ezor/libstd-4e7c5e5c.rlib(r-rust_builtin-rust_android_dummy.o)
#                                                   sepcomp-lib-lto.o
# log2                                              /tmp/rustc.X0fdWX76ezor/libstd-4e7c5e5c.rlib(r-rust_builtin-rust_android_dummy.o)
#                                                   sepcomp-lib-lto.o
#                                                   /home/rprichard/android-ndk-standalone-19/bin/../sysroot/usr/lib/libm.so
# log2f                                             /tmp/rustc.X0fdWX76ezor/libstd-4e7c5e5c.rlib(r-rust_builtin-rust_android_dummy.o)
#                                                   sepcomp-lib-lto.o
#                                                   /home/rprichard/android-ndk-standalone-19/bin/../sysroot/usr/lib/libm.so
# pthread_atfork                                    /tmp/rustc.X0fdWX76ezor/libstd-4e7c5e5c.rlib(r-rust_builtin-rust_android_dummy.o)
#                                                   /tmp/rustc.X0fdWX76ezor/liballoc-4e7c5e5c.rlib(r-jemalloc-jemalloc.pic.o)

rprichard · 2015-02-21T23:24:34Z

I think the fix is to move rust_android_dummy.o out of libstd into its own library, then either:

Move it later in the linker command-line, or maybe,
Wrap it with -Wl,--whole-archive -Wl,--no-whole-archive (like we do with -lmorestack).

rprichard · 2015-02-23T00:10:47Z

The Android LTO problem was already reported as #18800. We need to break out pthread_atfork from librust_builtin. Prior to finding that issue, I split it out into a "rust_android_alloc_dummy" library, which works, but it's ugly.

Can we merge this PR without first fixing the Android LTO problem? This PR isn't really breaking Android LTO, it's just making the existing brokenness more obvious.

Manishearth · 2015-02-23T02:04:50Z

That sounds good to me, though I'd want @dotdash or @alexcrichton to confirm if that's what we want to do.

vadimcn · 2015-02-23T03:01:57Z

src/librustc_trans/back/lto.rs

@@ -167,7 +167,12 @@ pub fn run(sess: &session::Session, llmod: ModuleRef,
        llvm::LLVMRustAddAnalysisPasses(tm, pm, llmod);
        llvm::LLVMRustAddPass(pm, "verify\0".as_ptr() as *const _);

-        let opt = sess.opts.cg.opt_level.unwrap_or(0) as libc::c_uint;
+        let opt = match sess.opts.optimize {
+            config::No => 0,


It looks like -Clto without optimizations doesn't even perform dead code elimination, which makes LTO rather pointless, IMO.
Can we please make -Clto imply -Copt-level=2? (unless specified explicitly, of course)

While perhaps not super useful, it does allow varying axes of configuration as well as perhaps a smoke test of some form. I would be ok, however, to have -Clto switch the default optimization level as ell though.

alexcrichton · 2015-02-23T08:07:17Z

Can we merge this PR without first fixing the Android LTO problem?

Unfortunately this PR cannot land until all tests are passing, we won't merge in-progress patches which have known failing tests.

Manishearth · 2015-02-23T08:09:42Z

@alexcrichton Apparently the Android LTO problem is basically that LTO is broken on Android -- this PR just uncovers another place where it's broken. I wouldn't call it an in-progress patch, it's more of a patch that disturbs a test in a broken area.

alexcrichton · 2015-02-23T08:13:29Z

That's not really the point at hand though. This test is currently running on android and passing. With this PR it looks like the test starts failing. We can't merge until that's dealt with one way or another.

Manishearth · 2015-02-23T08:14:21Z

Alright.

pnkfelix · 2015-02-23T08:19:04Z

The old school way to address link errors like this was to list the offending input multiple times on the linker invocation. Can we do that here, get libstd added a second time to to the end?

rprichard · 2015-02-23T10:19:35Z

That's not really the point at hand though. This test is currently running on android and passing. With this PR it looks like the test starts failing. We can't merge until that's dealt with one way or another.

That makes sense. I suppose I was wondering if there were some way to disable the test for Android, because we already know that the Android-LTO combination is currently broken.

rprichard · 2015-02-23T11:07:51Z

FWIW, here's my fix for the Android-LTO issue. rprichard@9511c06. It just splits out pthread_atfork from librust_builtin.a. I verified that it fixed the sepcomp-lib-lto.rs failure.

rprichard · 2015-02-23T11:49:42Z

It looks like it's possible to disable a test using a comment:

// ignore-android

alexcrichton · 2015-02-24T01:00:02Z

Yes adding // ignore-android is fine.

Manishearth · 2015-02-24T01:01:31Z

Yeah, that's sort of what I meant when I said that we can merge this PR without having the test pass. Sorry if I wasn't clear :)

…is fixed

dotdash · 2015-02-24T08:50:22Z

@bors r+ 9167c62

bors · 2015-02-24T14:45:25Z

⌛ Testing commit 9167c62 with merge 308b79f...

bors · 2015-02-24T14:51:03Z

💔 Test failed - auto-linux-32-nopt-t

Manishearth · 2015-02-24T14:51:35Z

(Not a real failure, I'm trying to get eddy's UFCS patch to land first so I canceled the build. Sorry.)

Manishearth · 2015-02-24T15:22:59Z

@bors: retry

Manishearth · 2015-02-24T20:50:04Z

@bors: force

Manishearth · 2015-02-24T20:56:29Z

@bors: retry

Manishearth · 2015-02-24T20:57:16Z

@bors retry clean force

bors · 2015-02-24T20:57:32Z

⌛ Testing commit 9167c62 with merge ad04cce...

Fixes #22525 I wasn't sure if I should reuse `write::get_llvm_opt_level` or not. It returns an `llvm::CodeGenOptLevel`, which is the Rust binding for `CodeGenOpt::Level`. `lto.rs` is passing an optlevel to LLVM's `PassManagerBuilder`, which takes an unsigned int. `PassManagerBuilder`'s optlevel uses essentially the same enumeration (i.e. 0-3 with 2 as default), but it's implicit.

alexcrichton · 2015-02-25T04:27:07Z

Tests all passed but didn't get picked up by homu, merging manually.

Use sess.opts.optimize instead of sess.opts.cg.opt_level for LTO optl…

dc3bc90

…evel Fixes rust-lang#22525

rust-highfive assigned alexcrichton Feb 19, 2015

dotdash mentioned this pull request Feb 22, 2015

-O -C lto is *not* same to -C opt-level=2 -C lto #22677

Closed

vadimcn reviewed Feb 23, 2015
View reviewed changes

Disable run-pass/sepcomp-lib-lto.rs on Android until rust-lang#18800 …

9167c62

…is fixed

alexcrichton merged commit 9167c62 into rust-lang:master Feb 25, 2015

Use sess.opts.optimize instead of sess.opts.cg.opt_level for LTO optlevel #22530

Use sess.opts.optimize instead of sess.opts.cg.opt_level for LTO optlevel #22530

Conversation

rprichard commented Feb 19, 2015

rust-highfive commented Feb 19, 2015

dotdash commented Feb 19, 2015

Manishearth commented Feb 21, 2015

rprichard commented Feb 21, 2015

Manishearth commented Feb 21, 2015

rprichard commented Feb 21, 2015

Manishearth commented Feb 21, 2015

Manishearth commented Feb 21, 2015

rprichard commented Feb 21, 2015

Manishearth commented Feb 21, 2015

rprichard commented Feb 21, 2015

Manishearth commented Feb 21, 2015

Manishearth commented Feb 21, 2015

rprichard commented Feb 21, 2015

rprichard commented Feb 21, 2015

rprichard commented Feb 23, 2015

Manishearth commented Feb 23, 2015

vadimcn Feb 23, 2015

Choose a reason for hiding this comment

alexcrichton Feb 23, 2015

Choose a reason for hiding this comment

alexcrichton commented Feb 23, 2015

Manishearth commented Feb 23, 2015

alexcrichton commented Feb 23, 2015

Manishearth commented Feb 23, 2015

pnkfelix commented Feb 23, 2015

rprichard commented Feb 23, 2015

rprichard commented Feb 23, 2015

rprichard commented Feb 23, 2015

alexcrichton commented Feb 24, 2015

Manishearth commented Feb 24, 2015

dotdash commented Feb 24, 2015

bors commented Feb 24, 2015

bors commented Feb 24, 2015

Manishearth commented Feb 24, 2015

Manishearth commented Feb 24, 2015

Manishearth commented Feb 24, 2015

Manishearth commented Feb 24, 2015

Manishearth commented Feb 24, 2015

bors commented Feb 24, 2015

alexcrichton commented Feb 25, 2015