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

Add regression test for a spurious import #88482

Merged
merged 1 commit into from
Sep 4, 2021

Conversation

athei
Copy link
Contributor

@athei athei commented Aug 30, 2021

This PR adds a test that verifies that the bug described in the linked issue does not creep back into the code. In essence it checks that compiling some specific code (that uses 128 bit multiplication) with a specific set of compiler options does not lead to a spurious import of a panic function.

I noticed that other wasm tests use # only-wasm32-bare in their Makefile. This will skip the test for me. I did not find out how to run this test locally. Maybe someone can help.

closes #78744
r? @jyn514

@rust-highfive
Copy link
Collaborator

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

Please see the contribution instructions for more information.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 30, 2021
@rust-log-analyzer

This comment has been minimized.

Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Hmm, this makes me nervous that it will fail the same way #88269 did. Could you maybe try adding wasm to the targets in config.toml so you can test locally?

@athei
Copy link
Contributor Author

athei commented Sep 1, 2021

Hmm, this makes me nervous that it will fail the same way #88269 did.

I tried to understand what you mean by reading the linked issue but I don't get it. Sorry I am a complete noob when it comes to the rustc code base.

Could you maybe try adding wasm to the targets in config.toml so you can test locally?

This is the hint I was looking for. I guess I need to change this line?:

# Which triples to build libraries (core/alloc/std/test/proc_macro) for. Each of
# these triples will be bootstrapped from the build triple themselves.
#
# Defaults to `host`. If you set this explicitly, you likely want to add all
# host triples to this list as well in order for those host toolchains to be
# able to compile programs for their native target.
#target = ["x86_64-unknown-linux-gnu"] (as an example)

Doesn't the CI run wasm tests? 😱

@jyn514
Copy link
Member

jyn514 commented Sep 1, 2021

Doesn't the CI run wasm tests? 😱

Yes, but only on merge, not for PRs. So the three green checks it currently shows don't mean anything.

@jyn514
Copy link
Member

jyn514 commented Sep 1, 2021

I guess I need to change this line?

Yes, that looks right.

@jyn514
Copy link
Member

jyn514 commented Sep 1, 2021

@the8472 suggested running ./src/ci/docker/run.sh wasm32, that might also work.

@athei
Copy link
Contributor Author

athei commented Sep 1, 2021

@the8472 suggested running ./src/ci/docker/run.sh wasm32, that might also work.

Tried that one. It ran for hours. It ignored the wasm test :(

test [run-make] run-make/wasm-abi ... ignored
test [run-make] run-make/wasm-custom-section ... ignored
test [run-make] run-make/wasm-custom-sections-opt ... ignored
test [run-make] run-make/wasm-export-all-symbols ... ignored
test [run-make] run-make/wasm-import-module ... ignored
test [run-make] run-make/wasm-panic-small ... ignored
test [run-make] run-make/wasm-spurious-import ... ignored
test [run-make] run-make/wasm-stringify-ints-small ... ok
test [run-make] run-make/wasm-symbols-different-module ... ignored
test [run-make] run-make/wasm-symbols-not-exported ... ignored
test [run-make] run-make/wasm-symbols-not-imported ... ignored

I also tried running this

./x.py test src/test/run-make/wasm-spurious-import 

with this config.toml

# Includes one of the default files in src/bootstrap/defaults
profile = "compiler"
changelog-seen = 2

[rust]
lld = true

[build]
target = ["wasm32-unknown-unknown"]

It can't build lld:

Updating only changed submodules
Submodules updated in 0.02 seconds
    Finished dev [unoptimized + debuginfo] target(s) in 0.39s
Building stage0 std artifacts (x86_64-apple-darwin -> x86_64-apple-darwin)
    Finished release [optimized] target(s) in 0.16s
Copying stage0 std from stage0 (x86_64-apple-darwin -> x86_64-apple-darwin / x86_64-apple-darwin)
Building stage0 compiler artifacts (x86_64-apple-darwin -> x86_64-apple-darwin)
    Finished release [optimized] target(s) in 0.39s
Copying stage0 rustc from stage0 (x86_64-apple-darwin -> x86_64-apple-darwin / x86_64-apple-darwin)
Building LLD for x86_64-apple-darwin
running: "cmake" "/Users/alexander/Developer/rust/src/llvm-project/lld" "-G" "Ninja" "-DCMAKE_INSTALL_MESSAGE=LAZY" "-DCMAKE_C_COMPILER=cc" "-DCMAKE_CXX_COMPILER=c++" "-DCMAKE_ASM_COMPILER=cc" "-DCMAKE_C_FLAGS=-ffunction-sections -fdata-sections -fPIC -m64 -arch x86_64 -stdlib=libc++" "-DCMAKE_CXX_FLAGS=-ffunction-sections -fdata-sections -fPIC -m64 -arch x86_64 -stdlib=libc++" "-DLLVM_CONFIG_PATH=/Users/alexander/Developer/rust/build/bootstrap/debug/llvm-config-wrapper" "-DLLVM_INCLUDE_TESTS=OFF" "-DCMAKE_CXX_STANDARD=14" "-DCMAKE_INSTALL_PREFIX=/Users/alexander/Developer/rust/build/x86_64-apple-darwin/lld" "-DCMAKE_ASM_FLAGS= -ffunction-sections -fdata-sections -fPIC -m64 -arch x86_64" "-DCMAKE_BUILD_TYPE=Release"
CMake Error at CMakeLists.txt:41 (message):
  LLVMConfig.cmake not found


-- Configuring incomplete, errors occurred!
See also "/Users/alexander/Developer/rust/build/x86_64-apple-darwin/lld/build/CMakeFiles/CMakeOutput.log".
thread 'main' panicked at '
command did not execute successfully, got: exit status: 1

build script failed, must exit now', /Users/alexander/.cargo/registry/src/github.com-1ecc6299db9ec823/cmake-0.1.44/src/lib.rs:885:5
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
	finished in 0.100 seconds
Build completed unsuccessfully in 0:00:02

@the8472
Copy link
Member

the8472 commented Sep 1, 2021

LLVMConfig.cmake not found

You'll have to install additional dependencies.

https://rustc-dev-guide.rust-lang.org/building/prerequisites.html#dependencies

@athei
Copy link
Contributor Author

athei commented Sep 2, 2021

LLVMConfig.cmake not found

You'll have to install additional dependencies.

https://rustc-dev-guide.rust-lang.org/building/prerequisites.html#dependencies

I can't find the information how to use a OS installed LLVM. I tried this way:

PATH=/usr/local/opt/llvm/bin:$PATH ./x.py test src/test/run-make/wasm-spurious-import

But this still yields the LLVMConfig.cmake not found error. This comes from cmake and I think I need to maybe specify that in another way.

Another thing that puzzles me is that in the config.toml it is specified that llvm should be downloaded from CI. Why do I need to do this then?

@jyn514
Copy link
Member

jyn514 commented Sep 2, 2021

r? @the8472 - I don't have much experience with cross compiling

@rust-highfive rust-highfive assigned the8472 and unassigned jyn514 Sep 2, 2021
@the8472
Copy link
Member

the8472 commented Sep 2, 2021

Oh you're on osx. I don't know if the dependencies are different there. Normally the build tools equivalent to what you can find in the docker file will do the job. If llvm build acts up then removing the build directory can help too.

Anyway, I checked out the commit, ran ./x.py test --stage 1 --target wasm32-unknown-unknown src/test/run-make/wasm-spurious-import and it passed.

Since that was @jyn514's main concern

@bors r+

@bors
Copy link
Contributor

bors commented Sep 2, 2021

📌 Commit 14cbb4b has been approved by the8472

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 2, 2021
m-ou-se added a commit to m-ou-se/rust that referenced this pull request Sep 2, 2021
Add regression test for a spurious import

This PR adds a test that verifies that the bug described in the linked issue does not creep back into the code. In essence it checks that compiling some specific code (that uses 128 bit multiplication) with a specific set of compiler options does not lead to a spurious import of a panic function.

I noticed that other wasm tests use `# only-wasm32-bare` in their `Makefile`. This will skip the test for me. I did not find out how to run this test locally. Maybe someone can help.

closes rust-lang#78744
r? `@jyn514`
@athei
Copy link
Contributor Author

athei commented Sep 3, 2021

@the8472 Thanks for helping me out here. I find it concerning that the docker run did skip the test. That should be independent of macOS. Do you have an explanation for that?

@the8472
Copy link
Member

the8472 commented Sep 3, 2021

Ah sorry, that was the wrong job. That CI job tests wasm32-unknown-emscripten. wasm32-unknown-unknown is tested by the test-various job.

@bors
Copy link
Contributor

bors commented Sep 3, 2021

⌛ Testing commit 14cbb4b with merge 03c775c...

@bors
Copy link
Contributor

bors commented Sep 4, 2021

☀️ Test successful - checks-actions
Approved by: the8472
Pushing 03c775c to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 4, 2021
@bors bors merged commit 03c775c into rust-lang:master Sep 4, 2021
@rustbot rustbot added this to the 1.56.0 milestone Sep 4, 2021
@athei athei deleted the add-import-test branch September 4, 2021 10:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Non existing function imported when compiling for wasm
7 participants