Adjust the alignment when passing a niche as a pointer #131739

dianqk · 2024-10-15T14:50:47Z

When passing a niche as a pointer, we have to consider drop align and dereferenceable. The issue about dereferenceable is #131834.

Take https://rust.godbolt.org/z/rbxqKedG6 as an example: we pass E::B with a value of 32769 as a parameter, and it's clear that the alignment of this as an address is not 8.

#![feature(rustc_attrs)]

#[rustc_layout_scalar_valid_range_start(1)]
#[rustc_layout_scalar_valid_range_end(0x7fff)]
pub struct RestrictedAddress(&'static i64);

enum E {
    A,
    B,
    C(RestrictedAddress),
}

#[no_mangle]
fn foo(a: E)  {}

Instead of dropping the align we could probably also shorten it, but we don't have a clear example to show it makes sense for optimization.

r? @the8472 or @saethlin

the8472 · 2024-10-15T15:30:51Z

Interesting. Perhaps another option would be keeping the align and teaching the niche logic about bit masks. Or emit an error when rustc_layout_scalar_valid_range_start is used on pointer types with an alignment > 1.

And what about the dereferenceable(8) on godbolt?

saethlin · 2024-10-16T00:09:50Z

I've been trying to get my head around this. The zero niche is special, because if we do use the forbidden value for another enum variant, that enum variant's bit pattern will still be aligned. Zero is still aligned.

But there are other values that are still aligned. I think handling the general case would actually make it easier to understand this code. So what I'd like to see is instead of special-casing zero, check if:

The niche range only has 1 value in it
That single value is aligned

Can you implement that?

dianqk · 2024-10-16T01:34:03Z

Interesting. Perhaps another option would be keeping the align and teaching the niche logic about bit masks.

I’m not sure, it might be worth a try. Introducing additional instructions could make things worse.

Or emit an error when rustc_layout_scalar_valid_range_start is used on pointer types with an alignment > 1.

Maybe not. This seems more like the address space restrictions we manually annotate.

And what about the dereferenceable(8) on godbolt?

It's ok with me. According to the semantic of dereferenceable(), this is only valid for load instructions.

A pointer that is dereferenceable can be loaded from speculatively without a risk of trapping.

dianqk · 2024-10-16T01:37:41Z

But there are other values that are still aligned. I think handling the general case would actually make it easier to understand this code. So what I'd like to see is instead of special-casing zero, check if:

The niche range only has 1 value in it

That single value is aligned

Can you implement that?

~~I’m not sure if this makes sense, unless we can specify the discriminant while also applying niche optimization?~~

r? saethlin

the8472 · 2024-10-16T08:13:36Z

It's ok with me. According to the semantic of dereferenceable(), this is only valid for load instructions.

A pointer that is dereferenceable can be loaded from speculatively without a risk of trapping.

My understanding is that this allows LLVM to insert spurious loads. Which would then violate the "risk of trapping" clause if we used those for niches instead of real allocations.
For Option we get dereferenceable_or_null(8) instead.

dianqk · 2024-10-16T08:32:24Z

It's ok with me. According to the semantic of dereferenceable(), this is only valid for load instructions.

A pointer that is dereferenceable can be loaded from speculatively without a risk of trapping.

My understanding is that this allows LLVM to insert spurious loads. Which would then violate the "risk of trapping" clause if we used those for niches instead of real allocations. For Option we get dereferenceable_or_null(8) instead.

Ah, I have checked again. We should drop dereferenceable(). This allows LLVM to hoist loads: https://alive2.llvm.org/ce/z/JPrWGp.

tests/codegen/enum/enum-ptr-align-niche.rs

dianqk · 2024-10-17T13:13:41Z

But there are other values that are still aligned. I think handling the general case would actually make it easier to understand this code. So what I'd like to see is instead of special-casing zero, check if:
* The niche range only has 1 value in it

* That single value is aligned
Can you implement that?

I have expanded it to more general.

dianqk · 2024-10-17T13:23:23Z

It's ok with me. According to the semantic of dereferenceable(), this is only valid for load instructions.

A pointer that is dereferenceable can be loaded from speculatively without a risk of trapping.

My understanding is that this allows LLVM to insert spurious loads. Which would then violate the "risk of trapping" clause if we used those for niches instead of real allocations. For Option we get dereferenceable_or_null(8) instead.

Ah, I have checked again. We should drop dereferenceable(). This allows LLVM to hoist loads: https://alive2.llvm.org/ce/z/JPrWGp.

I have created an issue #131834 for that.

compiler/rustc_middle/src/ty/layout.rs

saethlin · 2024-10-23T21:50:14Z

compiler/rustc_abi/src/lib.rs

@@ -1489,6 +1489,29 @@ pub enum Variants<FieldIdx: Idx, VariantIdx: Idx> {
    },
 }

+impl<FieldIdx: Idx, VariantIdx: Idx> Variants<FieldIdx, VariantIdx> {
+    // Returns all used niche values.


What is a used niche value? I think a niche by definition is a value that must not be used, so that combination of words is confusing. Is this an iterator over the discriminants?

Yes, it iterates the discriminant and returns the corresponding niche.

But this is returning a single value per variant, and niches are at least a range. And this is based on the discriminant start, so I'm pretty sure this just returns discriminants.

Then we fold these using restrict_for_offset? The values we're passing to that function aren't offsets. Does that function just so happen to implement the arithmetic that we need?

I think I'm going to try to work through what this does with pencil and paper, because I feel like the math works out in your implementation, but I still don't think I can see how.

IIUC, a variant corresponds to a niche. I convert to a discriminant(variant_idx) to a niche by (d - niche_variants.start).wrapping_add(niche_start), so this returns an iterator for all niches.

Then we pass a niche as a pointer, the max alignment of a niche is max_for_offset(niche). For example:

the max alignment of 8 is 8

the max alignment of 24(0b11000) is 8

the max alignment of 26(0b11010) is 2

In order for the alignment to be legal, we must choose the min alignment and restrict_for_offset is I want.

dianqk · 2024-11-03T04:02:24Z

Maybe ping?

RalfJung · 2024-11-08T06:21:05Z

Is this PR related to #131834 ? There is no link in either direction so I have to assume they are completely unrelated, but they look very related. Please always add links to related issues / PRs, that's a crucial part of being able to follow what happens in rustc development, and making sure the right people notice the discussions relevant to them.

EDIT: Ah, it got linked in the middle of this thread. That's very easy to miss.

RalfJung · 2024-11-08T06:22:25Z

compiler/rustc_middle/src/ty/layout.rs

+                                align.restrict_for_offset(Size::from_bytes(niche))
+                            })
+                        });
+                        (Some(this.for_variant(cx, untagged_variant)), niche_align)


This logic is still not right, we need something like #132745 or else there will be wrong dereferenceable flags.

The point of this function is to compute the pointee info, but with a type like MultipleAlign8 in your example, there is no pointee. More precisely, we are computing "pointee or null" info; null is the one and only special case that is allowed here.

RalfJung · 2024-11-08T06:28:15Z

tests/codegen/enum/enum-ptr-align-niche.rs

+fn call_multiple_niches_align_8() {
+    // CHECK: call void @multiple_niches_align_8(ptr {{.*}}align 8 {{.*}}(i64 32768 to ptr)
+    multiple_niches_align_8(MultipleAlign8::Niche_32768);
+    // CHECK: call void @multiple_niches_align_8(ptr {{.*}}align 8 {{.*}}(i64 32776 to ptr)


Specifically, among the attributes hidden by the {{.*}} is dereferenceable, and that is unsound.

We currently don't have a way to represent a pointer that is aligned but maybe non-dereferenceable. However, I am not convinced that is something worth having -- it will be extremely rare; IMO we should have evidence of real-world benefit for such a construction before we spend the effort of implementing and maintaining that in the compiler.

Correction -- we do have a way to represent a non-dereferenceable pointer, and it is by setting size to 0. This is what we already do for Box.

But the rest of my point still stands.

RalfJung · 2024-11-08T06:45:28Z

compiler/rustc_abi/src/lib.rs

+                    let niche = ((variant_idx.index() - niche_variants.start().index()) as u128)
+                        .wrapping_add(niche_start);


This logic is wrong; it needs to take into account the size of the field that holds the niche, and wrap around accordingly.

Since I only want to calculate alignment, I think the actual value of niche is not necessary. I will add some comment.

This function is called niches and offers itself as a general-purpose function. If it's only correct for alignment, then it should be a private helper function inside the alignment logic.

dianqk · 2024-11-08T06:53:49Z

Is this PR related to #131834 ? There is no link in either direction so I have to assume they are completely unrelated, but they look very related. Please always add links to related issues / PRs, that's a crucial part of being able to follow what happens in rustc development, and making sure the right people notice the discussions relevant to them.

EDIT: Ah, it got linked in the middle of this thread. That's very easy to miss.

This is a different issue. I will create a separate one.
Sorry for causing you confusion and making you leave so many comments here. :p

RalfJung · 2024-11-08T07:36:07Z

Take https://rust.godbolt.org/z/rbxqKedG6 as an example: we pass E::B with a value of 32769 as a parameter, and it's clear that the alignment of this as an address is not 8.

This frames the PR as a bugfix. But it doesn't fully fix the bug, we are still computing a wrong PointeeInfo for these types. So if the goal is a bugfix then IMO #132745 is the better approach.

The PR description does not motivate why we should keep the align attribute on contrived examples such as the ones added in the codegen tests here.

the8472 · 2024-11-08T13:13:12Z

I think with my change Option<Option<&u32>> could keep the aligned attribute. I'm not adding an alignment gap at the beginning of the range, so the next niche would be at isize::MAX+1, which is still aligned.

RalfJung · 2024-11-08T13:47:02Z

Why is isize::MAX+1 a niche for &u32? That doesn't sound right.

I've seen some revisions of your change, but I don't know which one you are referring to here.

the8472 · 2024-11-08T14:45:15Z

Valid on platforms where userspace addresses always have the top bit zeroed. That's one of the things I am experimenting with.
Though with recent discussions about TBI and LAM that appears to be on less solid ground than I thought.

RalfJung · 2024-11-08T14:57:33Z

Ah, that one. Yeah that's at least somewhat fishy.

Once we are reasonably sure we'll have types that could benefit from align without dereferenceable, that is a good motivation for some extra complexity in the compiler, sure. But I don't think we are there yet.

bors · 2024-11-09T15:34:17Z

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

dianqk · 2024-11-11T01:47:12Z

Is this PR related to #131834 ? There is no link in either direction so I have to assume they are completely unrelated, but they look very related. Please always add links to related issues / PRs, that's a crucial part of being able to follow what happens in rustc development, and making sure the right people notice the discussions relevant to them.
EDIT: Ah, it got linked in the middle of this thread. That's very easy to miss.

This is a different issue. I will create a separate one. Sorry for causing you confusion and making you leave so many comments here. :p

I have updated the PR's description.

dianqk · 2024-11-11T01:49:41Z

I'm closing this because the align is useful when using on a load/store instruction that we still retain after #132745.
I don't's think that it is also useful for other instructions in the real world, but we can re-open it when we need.

rustbot assigned the8472 Oct 15, 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 15, 2024

dianqk force-pushed the niche-ptr-align branch 2 times, most recently from cdf9a48 to d910764 Compare October 15, 2024 14:53

rustbot assigned saethlin and unassigned the8472 Oct 16, 2024

the8472 reviewed Oct 17, 2024

View reviewed changes

tests/codegen/enum/enum-ptr-align-niche.rs Outdated Show resolved Hide resolved

dianqk force-pushed the niche-ptr-align branch from d910764 to 759f333 Compare October 17, 2024 13:10

dianqk changed the title ~~Set the pointer alignment to 1 whenever any niche is non-zero~~ Adjust the alignment when passing the niche as a pointer Oct 17, 2024

dianqk force-pushed the niche-ptr-align branch from 759f333 to 6238a57 Compare October 17, 2024 13:44

saethlin reviewed Oct 23, 2024

View reviewed changes

compiler/rustc_middle/src/ty/layout.rs Outdated Show resolved Hide resolved

dianqk force-pushed the niche-ptr-align branch from 6238a57 to 7bc5a39 Compare October 23, 2024 14:02

dianqk changed the title ~~Adjust the alignment when passing the niche as a pointer~~ Adjust the alignment when passing a niche as a pointer Oct 23, 2024

saethlin reviewed Oct 23, 2024

View reviewed changes

Adjust the alignment when passing a niche as a pointer

fef7d19

dianqk force-pushed the niche-ptr-align branch from 7bc5a39 to fef7d19 Compare October 23, 2024 23:53

dianqk mentioned this pull request Nov 8, 2024

pointee_info_at: fix logic for recursing into enums #132745

Merged

RalfJung reviewed Nov 8, 2024

View reviewed changes

RalfJung mentioned this pull request Nov 8, 2024

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

Closed

RalfJung reviewed Nov 8, 2024

View reviewed changes

dianqk closed this Nov 11, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adjust the alignment when passing a niche as a pointer #131739

Adjust the alignment when passing a niche as a pointer #131739

dianqk commented Oct 15, 2024 •

edited

Loading

the8472 commented Oct 15, 2024

saethlin commented Oct 16, 2024

dianqk commented Oct 16, 2024

dianqk commented Oct 16, 2024 •

edited

Loading

the8472 commented Oct 16, 2024

dianqk commented Oct 16, 2024

dianqk commented Oct 17, 2024

dianqk commented Oct 17, 2024

saethlin Oct 23, 2024 •

edited

Loading

dianqk Oct 23, 2024

saethlin Oct 24, 2024 •

edited

Loading

dianqk Oct 24, 2024

dianqk commented Nov 3, 2024

RalfJung commented Nov 8, 2024 •

edited

Loading

RalfJung Nov 8, 2024 •

edited

Loading

RalfJung Nov 8, 2024

RalfJung Nov 8, 2024

RalfJung Nov 8, 2024

dianqk Nov 8, 2024

RalfJung Nov 8, 2024

dianqk commented Nov 8, 2024 •

edited

Loading

RalfJung commented Nov 8, 2024 •

edited

Loading

the8472 commented Nov 8, 2024

RalfJung commented Nov 8, 2024

the8472 commented Nov 8, 2024

RalfJung commented Nov 8, 2024

bors commented Nov 9, 2024

dianqk commented Nov 11, 2024

dianqk commented Nov 11, 2024

		let niche = ((variant_idx.index() - niche_variants.start().index()) as u128)
		.wrapping_add(niche_start);

Adjust the alignment when passing a niche as a pointer #131739

Adjust the alignment when passing a niche as a pointer #131739

Conversation

dianqk commented Oct 15, 2024 • edited Loading

the8472 commented Oct 15, 2024

saethlin commented Oct 16, 2024

dianqk commented Oct 16, 2024

dianqk commented Oct 16, 2024 • edited Loading

the8472 commented Oct 16, 2024

dianqk commented Oct 16, 2024

dianqk commented Oct 17, 2024

dianqk commented Oct 17, 2024

saethlin Oct 23, 2024 • edited Loading

Choose a reason for hiding this comment

dianqk Oct 23, 2024

Choose a reason for hiding this comment

saethlin Oct 24, 2024 • edited Loading

Choose a reason for hiding this comment

dianqk Oct 24, 2024

Choose a reason for hiding this comment

dianqk commented Nov 3, 2024

RalfJung commented Nov 8, 2024 • edited Loading

RalfJung Nov 8, 2024 • edited Loading

Choose a reason for hiding this comment

RalfJung Nov 8, 2024

Choose a reason for hiding this comment

RalfJung Nov 8, 2024

Choose a reason for hiding this comment

RalfJung Nov 8, 2024

Choose a reason for hiding this comment

dianqk Nov 8, 2024

Choose a reason for hiding this comment

RalfJung Nov 8, 2024

Choose a reason for hiding this comment

dianqk commented Nov 8, 2024 • edited Loading

RalfJung commented Nov 8, 2024 • edited Loading

the8472 commented Nov 8, 2024

RalfJung commented Nov 8, 2024

the8472 commented Nov 8, 2024

RalfJung commented Nov 8, 2024

bors commented Nov 9, 2024

dianqk commented Nov 11, 2024

dianqk commented Nov 11, 2024

dianqk commented Oct 15, 2024 •

edited

Loading

dianqk commented Oct 16, 2024 •

edited

Loading

saethlin Oct 23, 2024 •

edited

Loading

saethlin Oct 24, 2024 •

edited

Loading

RalfJung commented Nov 8, 2024 •

edited

Loading

RalfJung Nov 8, 2024 •

edited

Loading

dianqk commented Nov 8, 2024 •

edited

Loading

RalfJung commented Nov 8, 2024 •

edited

Loading