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 by-value arrays to improper_ctypes lint #66305

Merged
merged 2 commits into from
Nov 28, 2019

Conversation

elichai
Copy link
Contributor

@elichai elichai commented Nov 11, 2019

Hi,
C doesn't have a notion of passing arrays by value, only by reference/pointer.
Rust currently will pass it correctly by reference by it looks very misleading, and can confuse the borrow checker to think a move had occurred.

Fixes #58905 and fixes #24578.

We could also improve the borrow checker here but I think it's kinda a waste of work if we instead just tell the user it's an invalid FFI call.

(My first PR to rustc so if I missed some test or formatting guideline please tell me :) )

@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 @zackmdavis (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. Due to 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 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 Nov 11, 2019
@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-llvm-6.0 of your PR failed (pretty log, 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.
2019-11-11T18:11:06.8138713Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-11-11T18:11:06.8338155Z ##[command]git config gc.auto 0
2019-11-11T18:11:06.8433578Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-11-11T18:11:06.8502595Z ##[command]git config --get-all http.proxy
2019-11-11T18:11:06.8662447Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/66305/merge:refs/remotes/pull/66305/merge
---
2019-11-11T19:08:14.1931230Z .................................................................................................... 1400/9228
2019-11-11T19:08:20.4348955Z .................................................................................................... 1500/9228
2019-11-11T19:08:26.5040132Z .................................................................................................... 1600/9228
2019-11-11T19:08:35.5535079Z .................................................................................................... 1700/9228
2019-11-11T19:08:43.9284278Z ..i................................................................................................. 1800/9228
2019-11-11T19:08:50.7290360Z ......................................................................................iiiii......... 1900/9228
2019-11-11T19:09:12.3672953Z .................................................................................................... 2100/9228
2019-11-11T19:09:14.6692301Z .................................................................................................... 2200/9228
2019-11-11T19:09:17.2176890Z .................................................................................................... 2300/9228
2019-11-11T19:09:26.8710046Z .................................................................................................... 2400/9228
---
2019-11-11T19:12:14.1052322Z ..................................................................................i...............i. 4700/9228
2019-11-11T19:12:20.9543509Z .................................................................................................... 4800/9228
2019-11-11T19:12:29.8962587Z .................................................................................................... 4900/9228
2019-11-11T19:12:35.2939177Z .................................................................................................... 5000/9228
2019-11-11T19:12:46.8256899Z .....................................................................................ii.ii.......... 5100/9228
2019-11-11T19:12:50.6750657Z .i.................................................................................................. 5200/9228
2019-11-11T19:13:05.4406167Z .................................................................................................... 5400/9228
2019-11-11T19:13:12.4404289Z ...................................................................i................................ 5500/9228
2019-11-11T19:13:19.9110464Z .................................................................................................... 5600/9228
2019-11-11T19:13:27.8414465Z .................................................................................................... 5700/9228
2019-11-11T19:13:27.8414465Z .................................................................................................... 5700/9228
2019-11-11T19:13:36.7118302Z ....................................................ii...i..ii...........i.......................... 5800/9228
2019-11-11T19:13:59.1432609Z .................................................................................................... 6000/9228
2019-11-11T19:14:06.3469352Z .................................................................................................... 6100/9228
2019-11-11T19:14:06.3469352Z .................................................................................................... 6100/9228
2019-11-11T19:14:11.1996424Z .......................................................................i..ii........................ 6200/9228
2019-11-11T19:14:39.4846716Z .................................................................................................... 6400/9228
2019-11-11T19:14:41.6326772Z .......................................i............................................................ 6500/9228
2019-11-11T19:14:43.9208457Z .................................................................................................... 6600/9228
2019-11-11T19:14:46.3148292Z .......................i............................................................................ 6700/9228
---
2019-11-11T19:19:30.8594775Z failures:
2019-11-11T19:19:30.8634234Z 
2019-11-11T19:19:30.8635069Z ---- [ui] ui/lint/lint-ctypes.rs stdout ----
2019-11-11T19:19:30.8635286Z 
2019-11-11T19:19:30.8635842Z error: /checkout/src/test/ui/lint/lint-ctypes.rs:68: unexpected error: '68:27: 68:34: `extern` block uses type `u8`, which is not FFI-safe [improper_ctypes]'
2019-11-11T19:19:30.8636477Z error: 1 unexpected errors found, 0 expected errors not found
2019-11-11T19:19:30.8636639Z status: exit code: 1
2019-11-11T19:19:30.8636639Z status: exit code: 1
2019-11-11T19:19:30.8637608Z command: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "/checkout/src/test/ui/lint/lint-ctypes.rs" "-Zthreads=1" "--target=x86_64-unknown-linux-gnu" "--error-format" "json" "-Zui-testing" "-C" "prefer-dynamic" "--out-dir" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/lint/lint-ctypes" "-Crpath" "-O" "-Cdebuginfo=0" "-Zunstable-options" "-Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "-L" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/lint/lint-ctypes/auxiliary" "-A" "unused"
2019-11-11T19:19:30.8637864Z unexpected errors (from JSON output): [
2019-11-11T19:19:30.8638165Z         line_num: 68,
2019-11-11T19:19:30.8638301Z         kind: Some(
2019-11-11T19:19:30.8638435Z             Error,
2019-11-11T19:19:30.8638594Z         ),
2019-11-11T19:19:30.8638594Z         ),
2019-11-11T19:19:30.8639088Z         msg: "68:27: 68:34: `extern` block uses type `u8`, which is not FFI-safe [improper_ctypes]",
2019-11-11T19:19:30.8639433Z ]
2019-11-11T19:19:30.8639548Z 
2019-11-11T19:19:30.8640176Z thread '[ui] ui/lint/lint-ctypes.rs' panicked at 'explicit panic', src/tools/compiletest/src/runtest.rs:1520:13
2019-11-11T19:19:30.8640414Z note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace.
---
2019-11-11T19:19:30.8641984Z 
2019-11-11T19:19:30.8674889Z thread 'main' panicked at 'Some tests failed', src/tools/compiletest/src/main.rs:537:22
2019-11-11T19:19:30.8689991Z 
2019-11-11T19:19:30.8690506Z 
2019-11-11T19:19:30.8695093Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/compiletest" "--compile-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib" "--run-lib-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/lib/rustlib/x86_64-unknown-linux-gnu/lib" "--rustc-path" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage2/bin/rustc" "--src-base" "/checkout/src/test/ui" "--build-base" "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui" "--stage-id" "stage2-x86_64-unknown-linux-gnu" "--mode" "ui" "--target" "x86_64-unknown-linux-gnu" "--host" "x86_64-unknown-linux-gnu" "--llvm-filecheck" "/usr/lib/llvm-6.0/bin/FileCheck" "--host-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--target-rustcflags" "-Crpath -O -Cdebuginfo=0 -Zunstable-options  -Lnative=/checkout/obj/build/x86_64-unknown-linux-gnu/native/rust-test-helpers" "--docck-python" "/usr/bin/python2.7" "--lldb-python" "/usr/bin/python2.7" "--gdb" "/usr/bin/gdb" "--quiet" "--llvm-version" "6.0.0\n" "--system-llvm" "--cc" "" "--cxx" "" "--cflags" "" "--llvm-components" "" "--llvm-cxxflags" "" "--adb-path" "adb" "--adb-test-dir" "/data/tmp/work" "--android-cross-path" "" "--color" "always"
2019-11-11T19:19:30.8695630Z 
2019-11-11T19:19:30.8695707Z 
2019-11-11T19:19:30.8708810Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test
2019-11-11T19:19:30.8709104Z Build completed unsuccessfully in 1:02:02
2019-11-11T19:19:30.8709104Z Build completed unsuccessfully in 1:02:02
2019-11-11T19:19:30.8765206Z == clock drift check ==
2019-11-11T19:19:30.8779455Z   local time: Mon Nov 11 19:19:30 UTC 2019
2019-11-11T19:19:31.1453145Z   network time: Mon, 11 Nov 2019 19:19:31 GMT
2019-11-11T19:19:31.1458360Z == end clock drift check ==
2019-11-11T19:19:31.8768222Z 
2019-11-11T19:19:31.8878668Z ##[error]Bash exited with code '1'.
2019-11-11T19:19:31.8920938Z ##[section]Starting: Checkout
2019-11-11T19:19:31.8923106Z ==============================================================================
2019-11-11T19:19:31.8923162Z Task         : Get sources
2019-11-11T19:19:31.8923209Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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)

src/librustc_lint/types.rs Outdated Show resolved Hide resolved
src/librustc_lint/types.rs Outdated Show resolved Hide resolved
src/librustc_lint/types.rs Outdated Show resolved Hide resolved
@hanna-kruppe
Copy link
Contributor

Don't really have the time to review this but from a quick glance: AFAICT this won't interact as desired with transparent newtypes (e.g., passing #[repr(transparent)] struct TenInts([c_uint; 10]); by value won't warn). The same problem exists for other special cases (cc #66202) so maybe it should be addressed holistically in a separate PR, but I wanted to note it for the record.

@elichai
Copy link
Contributor Author

elichai commented Nov 11, 2019

Right I forgot I wanted to ask about transparent structs.
should we lint them too?

although technically with a single value struct I'm pretty sure repr(C) and repr(transparent) have the same layout (https://play.rust-lang.org/?gist=4721ce8ea427b0edac3a1f47dbca2635)

@hanna-kruppe
Copy link
Contributor

hanna-kruppe commented Nov 11, 2019

C structs with a single field have the same memory layout as their field, but they are not always treated the same by calling conventions. The whole point of repr(transparent) is to ensure that, and calling convention is a key consideration for this lint, so it seems pretty clear-cut to me that transparent newtypes should get the exact same treatment as the type they wrap.

@elichai
Copy link
Contributor Author

elichai commented Nov 11, 2019

@rkruppe anyhow. I don't mind adding the lint for transparent too, but if there's an inherent problem with the linting of repr(transparent) then I think it's best for a later PR that will rework some of that logic
(I can try to do it after this is merged if I can understand exactly the problem)

@eddyb
Copy link
Member

eddyb commented Nov 13, 2019

Rust currently will pass it correctly by reference

I should note that this is entirely coincidental, and depends on the architecture.

This is just like MyStruct and *const MyStruct can happen be passed similarly depending on the contents of MyStruct, there's nothing "correct" about it, nor can it be relied upon.

@elichai
Copy link
Contributor Author

elichai commented Nov 13, 2019

Rust currently will pass it correctly by reference

I should note that this is entirely coincidental, and depends on the architecture.

This is just like MyStruct and *const MyStruct can happen be passed similarly depending on the contents of MyStruct, there's nothing "correct" about it, nor can it be relied upon.

Thanks! Good to know.
It just means this lint is more important than I thought :)

reason: "passing raw arrays by value is not FFI-safe",
help: Some("consider passing a pointer to the array"),
}
}
Copy link
Member

Choose a reason for hiding this comment

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

I think it's really easy to poison the cache, so maybe add a method that wraps check_type_for_ffi (say, check_top_level_type_for_ffi?) and checks for ty::Array before calling check_type_for_ffi?

Copy link
Contributor Author

@elichai elichai Nov 14, 2019

Choose a reason for hiding this comment

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

Hmm, the point of the cache is to prevent infinite recursion.
I'm not sure I'm seeing how is this any different than other checks.

Copy link
Member

Choose a reason for hiding this comment

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

top_level isn't in the hash key, so whichever comes first will stay in the cache and affect any other occurrences of the same type.

Also, overall the diff would be simpler if the top-level check was separate, and check_type_for_ffi didn't have an extra parameter.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated the name of the inner one, otherwise calling check_top_level_type_for_ffi would've been misleading, because it checks both top level and lower levels.

@bors
Copy link
Contributor

bors commented Nov 14, 2019

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

@elichai elichai force-pushed the 2019-11-array_ffi branch 2 times, most recently from 61b1517 to 159aa11 Compare November 17, 2019 08:41
src/librustc_lint/types.rs Outdated Show resolved Hide resolved
@JohnCSimon JohnCSimon added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 24, 2019
@JohnCSimon
Copy link
Member

Ping from triage:
@elichai can you please address the comments from @eddyb ?
Thank you!

@eddyb
Copy link
Member

eddyb commented Nov 27, 2019

r=me if nobody else has any concerns (cc @oli-obk @Centril for lint change)

@bors
Copy link
Contributor

bors commented Nov 27, 2019

📌 Commit b3666b6 has been approved by eddyb

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 27, 2019
@bors
Copy link
Contributor

bors commented Nov 27, 2019

⌛ Testing commit b3666b6 with merge 4fc8f2ced225b73fc28307b7c96ed0880dd707e3...

@rust-highfive
Copy link
Collaborator

The job x86_64-gnu-distcheck of your PR failed (pretty log, 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.
2019-11-27T17:09:52.5881972Z     Updating crates.io index
2019-11-27T17:09:53.6544510Z error: failed to fetch `https://github.com/rust-lang/crates.io-index`
2019-11-27T17:09:53.6544643Z 
2019-11-27T17:09:53.6544706Z Caused by:
2019-11-27T17:09:53.6544831Z   SSL error: error:140E0197:SSL routines:SSL_shutdown:shutdown while in init; class=Ssl (16)
2019-11-27T17:09:53.6551725Z failed to run: /checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo build --manifest-path /checkout/src/bootstrap/Cargo.toml --locked
2019-11-27T17:09:53.6598553Z Makefile:67: recipe for target 'prepare' failed
2019-11-27T17:09:53.6605967Z make: *** [prepare] Error 1
2019-11-27T17:09:54.6620738Z Command failed. Attempt 2/5:
2019-11-27T17:09:54.7839995Z     Updating crates.io index
---
2019-11-27T17:43:10.1200237Z [  0%] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/APInt.cpp.o
2019-11-27T17:43:10.3387861Z [  0%] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/APSInt.cpp.o
2019-11-27T17:43:10.5370548Z [  0%] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/ARMBuildAttrs.cpp.o
2019-11-27T17:43:10.7525556Z [  0%] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/ARMAttributeParser.cpp.o
2019-11-27T17:43:10.7934245Z sccache: encountered fatal error
2019-11-27T17:43:10.7939056Z sccache: error : Invalid checksum
2019-11-27T17:43:10.7944047Z sccache:  cause: Invalid checksum
2019-11-27T17:43:10.7954620Z lib/Demangle/CMakeFiles/LLVMDemangle.dir/build.make:86: recipe for target 'lib/Demangle/CMakeFiles/LLVMDemangle.dir/ItaniumDemangle.cpp.o' failed
2019-11-27T17:43:10.7959162Z make[3]: *** [lib/Demangle/CMakeFiles/LLVMDemangle.dir/ItaniumDemangle.cpp.o] Error 254
2019-11-27T17:43:10.7965040Z make[3]: Leaving directory '/checkout/obj/build/tmp/distcheck/build/x86_64-unknown-linux-gnu/llvm/build'
2019-11-27T17:43:10.7983586Z CMakeFiles/Makefile2:780: recipe for target 'lib/Demangle/CMakeFiles/LLVMDemangle.dir/all' failed
2019-11-27T17:43:10.7988141Z make[2]: *** [lib/Demangle/CMakeFiles/LLVMDemangle.dir/all] Error 2
2019-11-27T17:43:10.7993047Z make[2]: *** Waiting for unfinished jobs....
2019-11-27T17:43:11.0255235Z [  0%] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/Allocator.cpp.o
2019-11-27T17:43:11.2290046Z [  0%] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/BinaryStreamError.cpp.o
2019-11-27T17:43:11.4334886Z [  0%] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/BinaryStreamReader.cpp.o
2019-11-27T17:43:11.6328825Z [  0%] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/BinaryStreamRef.cpp.o
---
2019-11-27T17:43:16.3772278Z [  2%] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/InitLLVM.cpp.o
2019-11-27T17:43:16.4794656Z [  2%] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/IntEqClasses.cpp.o
2019-11-27T17:43:16.6190546Z [  2%] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/IntervalMap.cpp.o
2019-11-27T17:43:16.7166853Z [  2%] Building CXX object lib/Support/CMakeFiles/LLVMSupport.dir/ItaniumManglingCanonicalizer.cpp.o
2019-11-27T17:43:16.8691832Z sccache: encountered fatal error
2019-11-27T17:43:16.8693433Z sccache: error : corrupt deflate stream
2019-11-27T17:43:16.8693575Z sccache:  cause: corrupt deflate stream
2019-11-27T17:43:16.8694458Z lib/Support/CMakeFiles/LLVMSupport.dir/build.make:1190: recipe for target 'lib/Support/CMakeFiles/LLVMSupport.dir/IntervalMap.cpp.o' failed
2019-11-27T17:43:16.8694687Z make[3]: *** [lib/Support/CMakeFiles/LLVMSupport.dir/IntervalMap.cpp.o] Error 254
2019-11-27T17:43:16.8697439Z make[3]: *** Waiting for unfinished jobs....
2019-11-27T17:43:18.4346776Z sccache: encountered fatal error
2019-11-27T17:43:18.4347461Z sccache: error : Invalid checksum
2019-11-27T17:43:18.4347647Z sccache:  cause: Invalid checksum
2019-11-27T17:43:18.4357903Z lib/Support/CMakeFiles/LLVMSupport.dir/build.make:1214: recipe for target 'lib/Support/CMakeFiles/LLVMSupport.dir/ItaniumManglingCanonicalizer.cpp.o' failed
2019-11-27T17:43:18.4358742Z make[3]: Leaving directory '/checkout/obj/build/tmp/distcheck/build/x86_64-unknown-linux-gnu/llvm/build'
2019-11-27T17:43:18.4359319Z CMakeFiles/Makefile2:901: recipe for target 'lib/Support/CMakeFiles/LLVMSupport.dir/all' failed
2019-11-27T17:43:18.4360311Z Makefile:149: recipe for target 'all' failed
2019-11-27T17:43:18.4360800Z make[1]: Leaving directory '/checkout/obj/build/tmp/distcheck/build/x86_64-unknown-linux-gnu/llvm/build'
2019-11-27T17:43:18.4360800Z make[1]: Leaving directory '/checkout/obj/build/tmp/distcheck/build/x86_64-unknown-linux-gnu/llvm/build'
2019-11-27T17:43:18.4361046Z make[3]: *** [lib/Support/CMakeFiles/LLVMSupport.dir/ItaniumManglingCanonicalizer.cpp.o] Error 254
2019-11-27T17:43:18.4361547Z make[2]: *** [lib/Support/CMakeFiles/LLVMSupport.dir/all] Error 2
2019-11-27T17:43:18.4361722Z make[1]: *** [all] Error 2
2019-11-27T17:43:18.4379239Z command did not execute successfully, got: exit code: 2
2019-11-27T17:43:18.4379420Z 
2019-11-27T17:43:18.4379420Z 
2019-11-27T17:43:18.4379972Z build script failed, must exit now', /checkout/obj/build/tmp/distcheck/vendor/cmake/src/lib.rs:813:5
2019-11-27T17:43:18.4380542Z  finished in 23.509
2019-11-27T17:43:18.4399445Z failed to run: /checkout/obj/build/tmp/distcheck/build/bootstrap/debug/bootstrap test
2019-11-27T17:43:18.4399809Z Build completed unsuccessfully in 0:25:26
2019-11-27T17:43:18.4399809Z Build completed unsuccessfully in 0:25:26
2019-11-27T17:43:18.4456069Z Makefile:48: recipe for target 'check' failed
2019-11-27T17:43:18.4456576Z 
2019-11-27T17:43:18.4456787Z command did not execute successfully: "make" "check"
2019-11-27T17:43:18.4457340Z expected success, got: exit code: 2
2019-11-27T17:43:18.4457745Z 
---
2019-11-27T17:43:18.4547522Z   local time: Wed Nov 27 17:43:18 UTC 2019
2019-11-27T17:43:18.4855158Z   network time: Wed, 27 Nov 2019 17:43:18 GMT
2019-11-27T17:43:18.4856158Z == end clock drift check ==
2019-11-27T17:43:21.8684174Z 
2019-11-27T17:43:21.8794898Z ##[error]Bash exited with code '1'.
2019-11-27T17:43:21.8832040Z ##[section]Starting: Checkout
2019-11-27T17:43:21.8834612Z ==============================================================================
2019-11-27T17:43:21.8834731Z Task         : Get sources
2019-11-27T17:43:21.8834819Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

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)

@bors
Copy link
Contributor

bors commented Nov 27, 2019

💔 Test failed - checks-azure

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 27, 2019
@elichai
Copy link
Contributor Author

elichai commented Nov 27, 2019

I don't think this error is related to the PR here

@eddyb
Copy link
Member

eddyb commented Nov 27, 2019

@bors retry (spurious network error?)

@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 Nov 27, 2019
@tmandry tmandry added A-spurious Area: Spurious failures in builds (spuriously == for no apparent reason) I-nominated T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Nov 27, 2019
tmandry added a commit to tmandry/rust that referenced this pull request Nov 27, 2019
Add by-value arrays to `improper_ctypes` lint

Hi,
C doesn't have a notion of passing arrays by value, only by reference/pointer.
Rust currently will pass it correctly by reference by it looks very misleading, and can confuse the borrow checker to think a move had occurred.

Fixes rust-lang#58905 and fixes rust-lang#24578.

We could also improve the borrow checker here but I think it's kinda a waste of work if we instead just tell the user it's an invalid FFI call.

(My first PR to `rustc` so if I missed some test or formatting guideline please tell me :) )
bors added a commit that referenced this pull request Nov 27, 2019
Rollup of 17 pull requests

Successful merges:

 - #64325 (Stabilize nested self receivers in 1.41.0)
 - #66222 (Use `eq_opaque_type_and_type` when type-checking closure signatures)
 - #66305 (Add by-value arrays to `improper_ctypes` lint)
 - #66399 (rustc_metadata: simplify the interactions between Lazy and Table.)
 - #66534 (Allow global references via ForeignItem and Item for the same symbol name during LLVM codegen)
 - #66700 (Fix pointing at arg for fulfillment errors in function calls)
 - #66704 (Intra doc enum variant field)
 - #66718 (Refactor `parse_enum_item` to use `parse_delim_comma_seq`)
 - #66722 (Handle non_exhaustive in borrow checking)
 - #66744 (Fix shrink_to panic documentation)
 - #66761 (Use LLVMDisposePassManager instead of raw delete in rustllvm)
 - #66769 (Add core::{f32,f64}::consts::TAU.)
 - #66774 (Clean up error codes)
 - #66777 (Put back tidy check on error codes)
 - #66797 (Fixes small typo in array docs r? @steveklabnik)
 - #66798 (Fix spelling typos)
 - #66800 (Combine similar tests for const match)

Failed merges:

r? @ghost
@bors bors merged commit b3666b6 into rust-lang:master Nov 28, 2019
@elichai elichai deleted the 2019-11-array_ffi branch November 28, 2019 09:29
@jplatte
Copy link
Contributor

jplatte commented Dec 12, 2019

I'm guilty of assuming this would "just work" and just got hit by this warning. How dangerous would it be to ignore this warning? Is it likely that array layout is at some point going to be defined and officially usable in FFI?

@elichai
Copy link
Contributor Author

elichai commented Dec 12, 2019

@jplatte no.
C doesn't support by value arrays.
AFAIK The only way this might(does?) work is by MIR/llvm optimizing this to pass by pointer in practice and not by value

@eddyb
Copy link
Member

eddyb commented Dec 12, 2019

@jplatte If you (want to) have a T[N] argument on the C side, use a pointer to the array.
If, on the other hand, you (want to) have a struct Array { T field[N]; }, wrap it in a #[repr(C)] struct on the Rust side as well.

If we are ever going to define [T; N] for FFI it will likely be C++'s std::array<T, N> (if that even has defined layout), or the equivalent array-wrapped-in-struct (the second option above).

AFAIK The only way this might(does?) work is by MIR/llvm optimizing this to pass by pointer in practice and not by value

That's more of an ABI detail, rather than an optimization, and it's most likely to happen for types where e.g. in C struct Foo is passed like struct Foo*, on that specific target.
I'm pretty sure it's also UB in practice to get that type wrong, due to things like ownership of the value, and including ABI details around stack management.

@jplatte
Copy link
Contributor

jplatte commented Dec 12, 2019

@jplatte If you (want to) have a T[N] argument on the C side, use a pointer to the array.
If, on the other hand, you (want to) have a struct Array { T field[N]; }, wrap it in a #[repr(C)] struct on the Rust side as well.

Sorry for being unclear about what it is I'm currently doing.

If we are ever going to define [T; N] for FFI it will likely be C++'s std::array<T, N> (if that even has defined layout), or the equivalent array-wrapped-in-struct (the second option above).

This is in fact what I'm doing currently. I've got array<reference_wrapper<Sth>, N> as the return type of a function implemented in C++ and [*mut Sth; N] as the return type on the Rust side. On the C++ side I've added a few static_asserts that check is_standard_layout for the return type and also that local arrays of pointers have the same size as it.

Thanks for the very quick reply!

@elichai
Copy link
Contributor Author

elichai commented Dec 12, 2019

AFAIU std::array is a struct wrapper around the array.
So to make an equivalent in rust you need a repr C struct with an array inside

@eddyb
Copy link
Member

eddyb commented Dec 13, 2019

With const generics this should work:

#[repr(C)]
struct CppArray<T, const N: usize> {
    data: [T; N],
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-spurious Area: Spurious failures in builds (spuriously == for no apparent reason) S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Rust arrays on C FFI are super confusing Don't allow array types in extern "C" functions.