Fix for issue #39827 #43836
Fix for issue #39827 #43836
Conversation
- adds handling of zero-sized types for volatile_store. - adds type size checks and warnigns for other volatile intrinsics. - adds a test to check warnings emitting. Cause of the issue While preparing for trans_intrinsic_call() invoke arguments are processed with trans_argument() method which excludes zero-sized types from argument list (to be more correct - all arguments for which ArgKind is Ignore are filtered out). As result volatile_store() intrinsic gets one argument instead of expected address and value. How it is fixed Modification of the trans_argument() method may cause side effects, therefore change was implemented in volatile_store() intrinsic building code itself. Now it checks function signature and if it was specialised with zero-sized type, then emits C_nil() instead of accessing non-existing second argument. Additionally warnings are added for all volatile operations which are specialised with zero-sized arguments. In fact, those operations are omitted in LLVM backend if no memory affected at all, e.g. number of elements is zero or type is zero-sized. This was not explicitly documented before and could lead to potential issues if developer expects volatile behaviour, but type has degraded to zero-sized.
- updates documentation on volatile memory intrinsics, now the case of zero-sized types is mentioned explicitly. Volatile memory operations which doesn't affect memory at all are omitted in LLVM backend, e.g. if number of elements is zero or type used in generic specialisation is zero-sized, then LLVM intrinsic or related code is not generated. This was not explicitly documented before in Rust documentation and potentially could cause issues.
|
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @pnkfelix (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. |
|
Nice PR! I'm not sure we want the "suspicious monomorphization" warning - it can spam the user in dead generic code. r=me without it. |
src/libcore/ptr.rs
Outdated
| @@ -384,6 +384,12 @@ pub unsafe fn write_unaligned<T>(dst: *mut T, src: T) { | |||
| /// over time. That being said, the semantics will almost always end up pretty | |||
| /// similar to [C11's definition of volatile][c11]. | |||
| /// | |||
| /// Compiler shouldn't change relative order or number of volatile memory | |||
There was a problem hiding this comment.
eng:
The compiler shouldn't change the relative order or number of volatile memory operations. However, volatile memory operations on zero-sized types (e.g. if a zero-sized type is passed to read_volatile) are no-ops and may be ignored.
- removes warnings introduced in changeset 0cd3587 - makes documentation more neat and grammatically correct
|
Thanks for comments. |
|
@bors r+ |
|
📌 Commit faf6b84 has been approved by |
Fix for issue #39827 *Cause of the issue* While preparing for `trans_intrinsic_call()` invoke arguments are processed with `trans_argument()` method which excludes zero-sized types from argument list (to be more correct - all arguments for which `ArgKind` is `Ignore` are filtered out). As result `volatile_store()` intrinsic gets one argument instead of expected address and value. *How it is fixed* Modification of the `trans_argument()` method may cause side effects, therefore change was implemented in `volatile_store()` intrinsic building code itself. Now it checks function signature and if it was specialised with zero-sized type, then emits `C_nil()` instead of accessing non-existing second argument.
|
☀️ Test successful - status-appveyor, status-travis |
Cause of the issue
While preparing for
trans_intrinsic_call()invoke arguments are processed withtrans_argument()method which excludes zero-sized types from argument list (to be more correct - all arguments for whichArgKindisIgnoreare filtered out). As resultvolatile_store()intrinsic gets one argument instead of expected address and value.How it is fixed
Modification of the
trans_argument()method may cause side effects, therefore change was implemented involatile_store()intrinsic building code itself. Now it checks function signature and if it was specialised with zero-sized type, then emitsC_nil()instead of accessing non-existing second argument.