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

derive: use discriminant_value in #[derive(Hash)] #32252

Merged
merged 5 commits into from
Mar 27, 2016

Conversation

durka
Copy link
Contributor

@durka durka commented Mar 14, 2016

derive: use discriminant_value in #[derive(Hash)]

Fixes #21714.

Spawned from #32139.

r? @alexcrichton

@alexcrichton
Copy link
Member

@bors: r+ b938cf96e0dbf8928833e823572e4461ee9975ac

eh we'll see how bors orders this

@bors
Copy link
Contributor

bors commented Mar 15, 2016

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

@durka
Copy link
Contributor Author

durka commented Mar 15, 2016

My pre-emptive efforts to avoid this merge conflict failed.

@sfackler
Copy link
Member

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Mar 15, 2016

📌 Commit 23aa5f1 has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Mar 16, 2016

⌛ Testing commit 23aa5f1 with merge 5058d71...

@bors
Copy link
Contributor

bors commented Mar 16, 2016

💔 Test failed - auto-linux-32-opt

@durka
Copy link
Contributor Author

durka commented Mar 16, 2016

@alexcrichton do you think it's real this time?

On Wed, Mar 16, 2016 at 2:43 AM, bors notifications@github.com wrote:

[image: 💔] Test failed - auto-linux-32-opt
http://buildbot.rust-lang.org/builders/auto-linux-32-opt/builds/8443


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#32252 (comment)

@alexcrichton
Copy link
Member

Hm yes, that looks legit... not sure how though?

@durka
Copy link
Contributor Author

durka commented Mar 16, 2016

I mean, start is a lang item, and lang items are stored in an enum that
derives Hash, so it's somewhat possible-ish? Am I barking up the right tree?

On Wed, Mar 16, 2016 at 12:36 PM, Alex Crichton notifications@github.com
wrote:

Hm yes, that looks legit... not sure how though?


You are receiving this because you authored the thread.
Reply to this email directly or view it on GitHub
#32252 (comment)

@alexcrichton
Copy link
Member

Perhaps, yeah. It seems specifically related to the c-dynamic-dylib test as it looked like that was also failing on OSX. I'd start out by poking around at the symbols and such and just see what's up

@durka
Copy link
Contributor Author

durka commented Mar 18, 2016

More data! I ran check-stage2-rmake TESTNAME=c-dynamic-dylib on a 64-bit Arch system, configured with --build=i686-unknown-linux-gnu --host=i686-unknown-linux-gnu. On master the test passes; on this branch (rebased on master) it doesn't.

Master:

# nm i686-unknown-linux-gnu/test/run-make/c-dynamic-dylib/bar | grep lang_start
         U _ZN2rt10lang_start20h853db53228ff7fabATzE

Branch:

# nm i686-unknown-linux-gnu/test/run-make/c-dynamic-dylib/bar | grep lang_start
         U _ZN2rt10lang_start20hdfbcc23f8b0cf0d20UzE

(note: I commented out this line so that I could inspect the symbols)

cc @eddyb (we talked on IRC) any thoughts?

@eddyb
Copy link
Member

eddyb commented Mar 18, 2016

@durka The symbols would differ anyway, had you changed a single non-whitespace thing in any library.

@durka
Copy link
Contributor Author

durka commented Mar 18, 2016

@eddyb ok. So do you know what "symbol lookup error" actually means? Who is
looking up the symbol and why would they have the wrong hash?

On Fri, Mar 18, 2016 at 4:58 PM, Eduard-Mihai Burtescu <
notifications@github.com> wrote:

@durka https://github.com/durka The symbols would differ anyway, had
you changed a single non-whitespace thing in any library.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#32252 (comment)

@eddyb
Copy link
Member

eddyb commented Mar 18, 2016

@durka The dynamic linker is looking up symbols and it only reports the first failure. Most likely none of the symbols in there match the libstd it's trying to use (run ldd on it to see a bunch of info).

rt::lang_start is a common first symbol to fail looking up, it usually means "you're using the wrong library".

Now, since this is a hashing change, it could mean "you were always using the wrong library but somehow stage1 libstd and stage2 libstd don't have the same symbol names".

Which, well, totally makes sense! You're changing how libsyntax behaves, but that only kicks in when building stage1 librustc and friends, so the stage1 compiler uses the old scheme, while stage2 has the new scheme, therefore the libs don't match.

The only thing you need to do is get this into a snapshot (sad that I haven't looked into this in-depth and right now we're building one...) with the test disabled, and re-enable it afterwards.

@eddyb
Copy link
Member

eddyb commented Mar 18, 2016

@durka Alternatively, you could try looking at the invocation that compiles bar and what LD_LIBRARY_PATH is set to when it runs - they will likely be different.
If it's an obvious mismatch (stage1 vs stage2), you could maybe try fixing the one that is (mistakenly) using stage1.

@durka
Copy link
Contributor Author

durka commented Mar 18, 2016

@eddyb yep, you're right. Here is the log:

make[1]: Entering directory '/root/rust/rust/work-derive/src/test/run-make/c-dynamic-dylib'
ccache gcc -Wall -Werror -g -fPIC -m32  -c -o /root/rust/rust/work-derive/i686-unknown-linux-gnu/test/run-make/c-dynamic-dylib/libcfoo.o cfoo.c
ccache gcc -Wall -Werror -g -fPIC -m32  -o /root/rust/rust/work-derive/i686-unknown-linux-gnu/test/run-make/c-dynamic-dylib/libcfoo.so /root/rust/rust/work-derive/i686-unknown-linux-gnu/test/run-make/c-dynamic-dylib/libcfoo.o -shared
LD_LIBRARY_PATH="/root/rust/rust/work-derive/i686-unknown-linux-gnu/test/run-make/c-dynamic-dylib:/root/rust/rust/work-derive/i686-unknown-linux-gnu/stage2/lib:/root/rust/rust/work-derive/i686-unknown-linux-gnu/llvm/Release/lib:" /root/rust/rust/work-derive/i686-unknown-linux-gnu/stage2/bin/rustc --out-dir /root/rust/rust/work-derive/i686-unknown-linux-gnu/test/run-make/c-dynamic-dylib -L /root/rust/rust/work-derive/i686-unknown-linux-gnu/test/run-make/c-dynamic-dylib -C codegen-units=8 foo.rs -C prefer-dynamic
LD_LIBRARY_PATH="/root/rust/rust/work-derive/i686-unknown-linux-gnu/test/run-make/c-dynamic-dylib:/root/rust/rust/work-derive/i686-unknown-linux-gnu/stage2/lib:/root/rust/rust/work-derive/i686-unknown-linux-gnu/llvm/Release/lib:" /root/rust/rust/work-derive/i686-unknown-linux-gnu/stage2/bin/rustc --out-dir /root/rust/rust/work-derive/i686-unknown-linux-gnu/test/run-make/c-dynamic-dylib -L /root/rust/rust/work-derive/i686-unknown-linux-gnu/test/run-make/c-dynamic-dylib -C codegen-units=8 bar.rs
LD_LIBRARY_PATH="/root/rust/rust/work-derive/i686-unknown-linux-gnu/test/run-make/c-dynamic-dylib:/root/rust/rust/work-derive/i686-unknown-linux-gnu/stage1/lib/rustlib/i686-unknown-linux-gnu/lib:" /root/rust/rust/work-derive/i686-unknown-linux-gnu/test/run-make/c-dynamic-dylib/bar

So it compiles everything with stage2 libs and then runs with stage1 libs.

@durka
Copy link
Contributor Author

durka commented Mar 18, 2016

And

[root@li1424-173 work-derive]# nm i686-unknown-linux-gnu/stage1/lib/rustlib/i686-unknown-linux-gnu/lib/libstd-9026086f.so | grep lang_start
000db210 T _ZN2rt10lang_start20hbfee00d943992dc10UzE
[root@li1424-173 work-derive]# nm i686-unknown-linux-gnu/stage2/lib/rustlib/i686-unknown-linux-gnu/lib/libstd-9026086f.so | grep lang_start
000db280 T _ZN2rt10lang_start20hdfbcc23f8b0cf0d20UzE

clearly it's looking for the stage2 symbol. So the question is where the mistake is in the maze of makefiles.

@durka
Copy link
Contributor Author

durka commented Mar 19, 2016

I think I found the mistake in the maze of makefiles. In this block of vars, shouldn't it be TLIB$(1) instead of TLIB1?

@durka
Copy link
Contributor Author

durka commented Mar 19, 2016

With the above change there are 19 failures in a full make check. Running it again to find out what they are.

@durka
Copy link
Contributor Author

durka commented Mar 19, 2016

Failures:

test [pretty] pretty/derive-totalsum-attr.rs ... FAILED
test [pretty] pretty/derive-totalsum.rs ... FAILED
test [pretty] pretty/issue-15778-pass.rs ... FAILED
test [pretty] pretty/issue_16723_multiple_items_syntax_ext.rs ... FAILED
test [pretty] pretty/lint-plugin-cmdline-allow.rs ... FAILED
test [pretty] pretty/llvm-pass-plugin.rs ... FAILED
test [pretty] pretty/macro-crate-does-hygiene-work.rs ... FAILED
test [pretty] pretty/macro-crate-multi-decorator.rs ... FAILED
test [pretty] pretty/macro-crate-outlive-expansion-phase.rs ... FAILED
test [pretty] pretty/macro-crate.rs ... FAILED
test [pretty] pretty/mbe_matching_test_macro.rs ... FAILED
test [pretty] pretty/mir-pass.rs ... FAILED
test [pretty] pretty/plugin-args-1.rs ... FAILED
test [pretty] pretty/plugin-args-2.rs ... FAILED
test [pretty] pretty/plugin-args-3.rs ... FAILED
test [pretty] pretty/plugin-lib-ok-in-plugin.rs ... FAILED
test [pretty] pretty/plugin-plus-extern-crate.rs ... FAILED
test [pretty] pretty/roman-numerals-macro.rs ... FAILED
test [pretty] pretty/syntax-extension-with-dll-deps.rs ... FAILED
test result: FAILED. 32 passed; 19 failed; 7 ignored; 0 measured

More output.

@eddyb
Copy link
Member

eddyb commented Mar 19, 2016

@durka Those are the "pretty" versions of run-pass-fulldeps aka rpass-full. If make check-stage2-rpass-full still works, that means the pretty tests are compiled/ran incorrectly.

@durka
Copy link
Contributor Author

durka commented Mar 19, 2016

Look at the output though, it's complaining about plugins not found which
seems worrying.
On Mar 19, 2016 11:48, "Eduard-Mihai Burtescu" notifications@github.com
wrote:

@durka https://github.com/durka Those are the "pretty" versions of
run-pass-fulldeps aka rpass-full. If make check-stage2-rpass-full still
works, that means the pretty tests are compiled/ran incorrectly.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#32252 (comment)

@eddyb
Copy link
Member

eddyb commented Mar 19, 2016

No, they're complaining about plugins having mismatched symbols (i.e. mismatched dynamic libraries), which is quite common - make check-stage1-rpass-full always fails because stage0 and stage1 don't have matching libraries.

@durka
Copy link
Contributor Author

durka commented Mar 19, 2016

Yeah, this doesn't work.

make clean && make check-stage2-rpass-full

@eddyb
Copy link
Member

eddyb commented Mar 19, 2016

@durka That works if you do check-stage2 because it builds liblog from another dependency.
Might as well fix that missing explicit dependency on liblog bug in mk/crates.mk.

@bors
Copy link
Contributor

bors commented Mar 26, 2016

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

@durka
Copy link
Contributor Author

durka commented Mar 27, 2016

#32293 landed so I undid the rebase. Let's see if I did it right.

@eddyb
Copy link
Member

eddyb commented Mar 27, 2016

@bors r=alexcrichton

@bors
Copy link
Contributor

bors commented Mar 27, 2016

📌 Commit 1e67d8a has been approved by alexcrichton

@bors
Copy link
Contributor

bors commented Mar 27, 2016

⌛ Testing commit 1e67d8a with merge 4369eef...

bors added a commit that referenced this pull request Mar 27, 2016
derive: use discriminant_value in #[derive(Hash)]

derive: use discriminant_value in #[derive(Hash)]

Fixes #21714.

Spawned from #32139.

r? @alexcrichton
@bors
Copy link
Contributor

bors commented Mar 27, 2016

💔 Test failed - auto-win-gnu-32-opt-rustbuild

@eddyb
Copy link
Member

eddyb commented Mar 27, 2016

@bors clean retry

@bors
Copy link
Contributor

bors commented Mar 27, 2016

⌛ Testing commit 1e67d8a with merge deee0f7...

bors added a commit that referenced this pull request Mar 27, 2016
derive: use discriminant_value in #[derive(Hash)]

derive: use discriminant_value in #[derive(Hash)]

Fixes #21714.

Spawned from #32139.

r? @alexcrichton
@bors
Copy link
Contributor

bors commented Mar 27, 2016

💔 Test failed - auto-win-msvc-64-opt-rustbuild

@durka
Copy link
Contributor Author

durka commented Mar 27, 2016

I got a couple of failures in unrelated tests when running make check locally. A bunch of debuginfo failures on x64-darwin. But the bots seem to have not even gotten that far.

@eddyb
Copy link
Member

eddyb commented Mar 27, 2016

@bors retry

@Manishearth
Copy link
Member

@bors force

Manishearth added a commit to Manishearth/rust that referenced this pull request Mar 27, 2016
derive: use discriminant_value in #[derive(Hash)]

derive: use discriminant_value in #[derive(Hash)]

Fixes rust-lang#21714.

Spawned from rust-lang#32139.

r? @alexcrichton
@bors
Copy link
Contributor

bors commented Mar 27, 2016

⛄ The build was interrupted to prioritize another pull request.

@bors bors merged commit 1e67d8a into rust-lang:master Mar 27, 2016
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.

7 participants