use stores of the correct size to set discriminants #131698

the8472 · 2024-10-14T17:41:06Z

Resolves an old HACK /FIXME.

Note that I haven't worked much with codegen so I'm not sure if I'm using the functions correctly and I was surprised seeing out-of-range values being fed into const_uint_big but apparently they're wrapped implicitly? By making it explicit we can pass in-range values instead.

rustbot · 2024-10-14T17:41:14Z

r? @estebank

rustbot has assigned @estebank.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

the8472 · 2024-10-14T18:38:34Z

CC @adwinwhite

RalfJung · 2024-10-17T21:39:49Z

If this changes what codegen does, it should come with ui tests and/or codegen tests ensuring we don't regress this in the future.

bors · 2024-11-04T04:47:57Z

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

estebank

r=me after rebase + test

RalfJung · 2024-11-30T14:31:07Z

compiler/rustc_codegen_ssa/src/mir/place.rs

                    };
+                    let max = niche.layout.size.unsigned_int_max();
+                    let wrapped = niche_value & max;


Suggested change

let wrapped = niche_value & max;

// We used u128 arithmetic above, so we have to get this back in-bounds.

let wrapped = niche_value & max;

Added a more detailed comment.

RalfJung · 2024-11-30T14:33:38Z

r=me with a comment added (assuming the comment is correct)

the8472 · 2024-11-30T14:49:54Z

I was waffling between making the test a build-pass (just checking codegen no longer ices) or run-pass. I moved to the latter and added some more asserts.

@bors r=RalfJung,estebank rollup

the8472 · 2024-11-30T14:54:01Z

@bors r=RalfJung,estebank rollup

bors · 2024-11-30T14:54:04Z

📌 Commit cca2e96 has been approved by RalfJung,estebank

It is now in the queue for this repository.

the8472 · 2024-11-30T15:02:40Z

I was waffling between...

And then I forgot to commit the changes 😔

@bors r=RalfJung,estebank

bors · 2024-11-30T15:02:42Z

📌 Commit b8d4b75 has been approved by RalfJung,estebank

It is now in the queue for this repository.

RalfJung · 2024-11-30T15:56:21Z

compiler/rustc_codegen_ssa/src/mir/place.rs

                    };
+                    let max = niche.layout.size.unsigned_int_max();
+                    // Niche ranges that wrap around may be represented as out-of-range values
+                    // e.g. niche at 0u8 may end up being u8::MAX + 1.


I don't think that's true? AFAIK our niche_start must always be inbounds of the scalar type they refer to. However, above you are doing u128 arithmetic so even if the niche range endpoints are inbounds, your niche_value is not.

niche_start is inbounds, but niche_start + niche_value might not. And the result was passed as such to the backend, i.e. I have seen 256 passed to llvm for a repr(i8) enum via const_uint_big.

niche_start + niche_value ought to be computed wrapping on the type of the niche field. But the code here takes a shortcut, by doing u128 arithmetic instead. That is why it can go out-of-bounds.

niche_start + niche_value ought to be computed wrapping on the type of the niche field

If that were the case then it would have been buggy for regular enums. Note that I did not change the calculation and the out of range value was passed to the backend before. This worked for integers. What I'm fixing is not that case but that llvm didn't like them for pointers.

fn const_uint_big(&self, t: &'ll Type, u: u128) -> &'ll Value { debug_assert!( self.type_kind(t) == TypeKind::Integer, "only allows integer types in const_uint_big" ); unsafe { let words = [u as u64, (u >> 64) as u64]; llvm::LLVMConstIntOfArbitraryPrecision(t, 2, words.as_ptr()) } }

And the result was passed as such to the backend, i.e. I have seen 256 passed to llvm for a repr(i8) enum via const_uint_big.

If that worked before, that means LLVM has been doing the truncation for integers. It just didn't like doing that for pointers, I guess.

What I don't understand is why you think my comment is incorrect.

We do calculate out of bound values. The backend accepts out of bound values in the common case. So we could just pass them through, as we did before. Ranges that wrap around resulting in out of bounds values is exactly what happens.

The only reason to do the wrapping ourselves is the special case that's currently only reachable via unstable attributes.

This calculates the niche value we tell the backend to set. Not the value we use internally to represent the niche.

The value we tell the backend can go out of bounds due to truncation in the backend.
I could have special-cased this to just do the wrapping outselves for pointers. But feels silly. Since they're equivalent I do it for integers too. But that's not a necessary property of the code.

Would this be better?

The calculated niche_value may end up being out of bounds. This would be fine for integer types since such integer constants do get truncated by the backend to fit the type. But to also cover pointer types we always do it ourselves.

I think your comment is misleading. The reason the value can be out-of-bounds is that it wasn't computed the right way. Or rather, it wasn't yet fully computed. The comment occurs half-way through the computation.

I don't see why we would talk about the backend truncation behavior... it's a bit unclean to rely on that anyway (reasonably a backend could complain if we build an i8 literal of value 256), and we are not relying on it any more with this PR.

RalfJung · 2024-11-30T15:56:49Z

@bors r-

That is a substantially different comment than what I suggested, so we have to figure out which comment is correct here.

RalfJung · 2024-11-30T17:21:40Z

compiler/rustc_codegen_ssa/src/mir/place.rs

@@ -387,13 +388,21 @@ impl<'a, 'tcx, V: CodegenObject> PlaceRef<'tcx, V> {
                    let niche_llty = bx.cx().immediate_backend_type(niche.layout);
                    let niche_value = variant_index.as_u32() - niche_variants.start().as_u32();
                    let niche_value = (niche_value as u128).wrapping_add(niche_start);


Suggested change

let niche_value = (niche_value as u128).wrapping_add(niche_start);

// We are supposed to compute `niche_value.wrapping_add(niche_start)` wrapping

// around the `niche`'s type.

// The easiest way to do that is to do wrapping arithmetic on `u128` and then

// masking off any extra bits that occur because we did the arithmetic with too many bits.

let niche_value = (niche_value as u128).wrapping_add(niche_start);

let niche_value = niche_value & niche.layout.size.unsigned_int_max();

Turns out it can be signed, but the logic should be right even for that case.

M'kay, applied.

RalfJung · 2024-11-30T17:58:32Z

Thanks!

@bors r+

bors · 2024-11-30T17:58:34Z

📌 Commit 26d7b5d has been approved by RalfJung

It is now in the queue for this repository.

…ck, r=RalfJung use stores of the correct size to set discriminants Resolves an old HACK /FIXME. Note that I haven't worked much with codegen so I'm not sure if I'm using the functions correctly and I was surprised seeing out-of-range values being fed into `const_uint_big` but apparently they're wrapped implicitly? By making it explicit we can pass in-range values instead.

Rollup of 6 pull requests Successful merges: - rust-lang#131698 (use stores of the correct size to set discriminants) - rust-lang#133571 (Mark visionOS as supporting `std`) - rust-lang#133655 (Eliminate print_expr_maybe_paren function from pretty printers) - rust-lang#133667 (Remove unused code) - rust-lang#133670 (bump hashbrown version) - rust-lang#133673 (replace hard coded error id with `ErrorKind::DirectoryNotEmpty`) r? `@ghost` `@rustbot` modify labels: rollup

Rollup merge of rust-lang#131698 - the8472:remove-set-discriminant-hack, r=RalfJung use stores of the correct size to set discriminants Resolves an old HACK /FIXME. Note that I haven't worked much with codegen so I'm not sure if I'm using the functions correctly and I was surprised seeing out-of-range values being fed into `const_uint_big` but apparently they're wrapped implicitly? By making it explicit we can pass in-range values instead.

Rollup of 6 pull requests Successful merges: - rust-lang#131698 (use stores of the correct size to set discriminants) - rust-lang#133571 (Mark visionOS as supporting `std`) - rust-lang#133655 (Eliminate print_expr_maybe_paren function from pretty printers) - rust-lang#133667 (Remove unused code) - rust-lang#133670 (bump hashbrown version) - rust-lang#133673 (replace hard coded error id with `ErrorKind::DirectoryNotEmpty`) r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned estebank Oct 14, 2024

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Oct 14, 2024

the8472 force-pushed the remove-set-discriminant-hack branch from 036b79a to e7b2ca9 Compare October 14, 2024 18:09

the8472 mentioned this pull request Oct 17, 2024

Drop the dereferenceable attribute when a reference is passed in an enum #131834

Closed

estebank approved these changes Nov 5, 2024

View reviewed changes

the8472 force-pushed the remove-set-discriminant-hack branch from e7b2ca9 to 1b80892 Compare November 30, 2024 14:13

RalfJung reviewed Nov 30, 2024

View reviewed changes

the8472 force-pushed the remove-set-discriminant-hack branch from 1b80892 to aec5b53 Compare November 30, 2024 14:48

This comment was marked as duplicate.

Sign in to view

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 30, 2024

the8472 force-pushed the remove-set-discriminant-hack branch from aec5b53 to cca2e96 Compare November 30, 2024 14:53

add tests for niches in pointers

97b84e4

the8472 force-pushed the remove-set-discriminant-hack branch from cca2e96 to b8d4b75 Compare November 30, 2024 15:01

RalfJung reviewed Nov 30, 2024

View reviewed changes

bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 30, 2024

RalfJung reviewed Nov 30, 2024

View reviewed changes

use stores of the correct size to set discriminants

26d7b5d

the8472 force-pushed the remove-set-discriminant-hack branch from b8d4b75 to 26d7b5d Compare November 30, 2024 17:33

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 30, 2024

This was referenced Nov 30, 2024

Rollup of 7 pull requests #133682

Closed

Rollup of 6 pull requests #133684

Merged

bors merged commit 3029e09 into rust-lang:master Nov 30, 2024
6 checks passed

rustbot added this to the 1.85.0 milestone Nov 30, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

use stores of the correct size to set discriminants #131698

use stores of the correct size to set discriminants #131698

the8472 commented Oct 14, 2024

rustbot commented Oct 14, 2024

the8472 commented Oct 14, 2024

RalfJung commented Oct 17, 2024

bors commented Nov 4, 2024

estebank left a comment

RalfJung Nov 30, 2024

the8472 Nov 30, 2024

RalfJung commented Nov 30, 2024

the8472 commented Nov 30, 2024 •

edited

Loading

This comment was marked as duplicate.

the8472 commented Nov 30, 2024

bors commented Nov 30, 2024

the8472 commented Nov 30, 2024

bors commented Nov 30, 2024

RalfJung Nov 30, 2024 •

edited

Loading

the8472 Nov 30, 2024 •

edited

Loading

RalfJung Nov 30, 2024 •

edited

Loading

the8472 Nov 30, 2024 •

edited

Loading

RalfJung Nov 30, 2024 •

edited

Loading

the8472 Nov 30, 2024

the8472 Nov 30, 2024

the8472 Nov 30, 2024

RalfJung Nov 30, 2024

RalfJung Nov 30, 2024

RalfJung commented Nov 30, 2024

RalfJung Nov 30, 2024 •

edited

Loading

RalfJung Nov 30, 2024

the8472 Nov 30, 2024

RalfJung commented Nov 30, 2024

bors commented Nov 30, 2024

	let wrapped = niche_value & max;
	// We used u128 arithmetic above, so we have to get this back in-bounds.
	let wrapped = niche_value & max;

-                    let niche_value = (niche_value as u128).wrapping_add(niche_start);
+                    // We are supposed to compute `niche_value.wrapping_add(niche_start)` wrapping
+                    // around the `niche`'s type.
+                    // The easiest way to do that is to do wrapping arithmetic on `u128` and then
+                    // masking off any extra bits that occur because we did the arithmetic with too many bits.
+                    let niche_value = (niche_value as u128).wrapping_add(niche_start);
+                    let niche_value = niche_value & niche.layout.size.unsigned_int_max();

use stores of the correct size to set discriminants #131698

use stores of the correct size to set discriminants #131698

Conversation

the8472 commented Oct 14, 2024

rustbot commented Oct 14, 2024

the8472 commented Oct 14, 2024

RalfJung commented Oct 17, 2024

bors commented Nov 4, 2024

estebank left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Nov 30, 2024

the8472 commented Nov 30, 2024 • edited Loading

This comment was marked as duplicate.

the8472 commented Nov 30, 2024

bors commented Nov 30, 2024

the8472 commented Nov 30, 2024

bors commented Nov 30, 2024

RalfJung Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

the8472 Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

RalfJung Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

the8472 Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

RalfJung Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Nov 30, 2024

RalfJung Nov 30, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RalfJung commented Nov 30, 2024

bors commented Nov 30, 2024

the8472 commented Nov 30, 2024 •

edited

Loading

RalfJung Nov 30, 2024 •

edited

Loading

the8472 Nov 30, 2024 •

edited

Loading

RalfJung Nov 30, 2024 •

edited

Loading

the8472 Nov 30, 2024 •

edited

Loading

RalfJung Nov 30, 2024 •

edited

Loading

RalfJung Nov 30, 2024 •

edited

Loading