fix some issues around ZST handling #115277

RalfJung · 2023-08-27T16:19:35Z

This fixes two bugs:

We used to entirely skip enum variants like B([u16; 0], !), even failing to properly align the enum! Honoring the alignment of uninhabited variants is important for the same reason that we must reserve space for their fields -- see here for why.
~~We uses to reject repr(transparent) on struct MyType([u16; 0]), which is weird because a one-field struct should always be allowed to be transparent around that field.~~ (moved to separate PR)

I also found two places in the layout code that did something special for ZST without explaining why, and removing those special cases doesn't seem to have any effect except for reordering some zero-sized fields which shouldn't be an issue... maybe PR CI will explain why those cases were needed, or maybe they became obsolete at some point.

rustbot · 2023-08-27T16:19:41Z

r? @davidtwco

(rustbot has picked a reviewer for you, use r? to override)

rustbot · 2023-08-27T16:19:43Z

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

RalfJung · 2023-08-27T16:20:11Z

compiler/rustc_abi/src/layout.rs

@@ -357,10 +359,8 @@ pub trait LayoutCalculator {
                // It'll fit, but we need to make some adjustments.
                match layout.fields {
                    FieldsShape::Arbitrary { ref mut offsets, .. } => {
-                        for (j, offset) in offsets.iter_enumerated_mut() {
-                            if !variants[i][j].0.is_zst() {


This is one of these places special-casing ZST that I removed

This got added in d7a750b by @mikebenfield. But their tests still pass after removing the if. So... maybe it just was never needed?

RalfJung · 2023-08-27T16:20:23Z

compiler/rustc_abi/src/layout.rs

@@ -954,9 +957,6 @@ fn univariant(
                        };

                        (
-                            // Place ZSTs first to avoid "interesting offsets", especially with only one
-                            // or two non-ZST fields. This helps Scalar/ScalarPair layouts.
-                            !f.0.is_zst(),


This is the other place special-casing ZST that I removed

This was present since #45225 by @eddyb. Maybe at the time Scalar/ScalarPair computation still worked differently and the "interesting" offsets were an issue? It doesn't seem to be an issue any more today... 🤷

This was likely obviated by #73453

rustbot · 2023-08-27T16:34:08Z

Some changes occurred in diagnostic error codes

cc @GuillaumeGomez

compiler/rustc_error_codes/src/error_codes.rs

GuillaumeGomez

The error code change looks good to me. 👍

RalfJung · 2023-08-27T18:12:31Z

@GuillaumeGomez it seems to be running the doctest even for the removed error code? That doesn't make sense, does it? I'll mark it as ignore but I wouldn't think I should have to.

rustbot · 2023-08-28T07:13:09Z

Some changes occurred in compiler/rustc_codegen_gcc

cc @antoyo

Some changes occurred in compiler/rustc_codegen_cranelift

cc @bjorn3

RalfJung · 2023-08-28T07:14:27Z

compiler/rustc_codegen_llvm/src/type_of.rs

@@ -311,7 +311,7 @@ impl<'tcx> LayoutLlvmExt<'tcx> for TyAndLayout<'tcx> {
            }
            // only wide pointer boxes are handled as pointers
            // thin pointer boxes with scalar allocators are handled by the general logic below
-            ty::Adt(def, args) if def.is_box() && cx.layout_of(args.type_at(1)).is_zst() => {
+            ty::Adt(def, args) if def.is_box() && cx.layout_of(args.type_at(1)).is_1zst() => {


I think this is also a bugfix -- a Box with an allocator that is ZST but highly aligned (more aligned than a pointer) cannot just be treated like a raw pointer.

davidtwco

This looks good to me, one small typo then r=me.

compiler/rustc_abi/src/lib.rs

RalfJung · 2023-08-28T08:51:22Z

@bors r=davidtwco,GuillaumeGomez

bors · 2023-08-28T08:51:24Z

📌 Commit 610c65f1681b166c68b36bc50543a196f6b103e2 has been approved by davidtwco,GuillaumeGomez

It is now in the queue for this repository.

…cient alignment

RalfJung · 2023-08-29T07:00:30Z

@bors r=davidtwco,GuillaumeGomez

bors · 2023-08-29T07:00:32Z

📌 Commit 5835c8a9510c60eb47b2a21cb3da39405d7e0c3d has been approved by davidtwco,GuillaumeGomez

It is now in the queue for this repository.

RalfJung · 2023-08-29T07:06:26Z

@bors r=davidtwco
(There are no longer any diagnostic changes here)

bors · 2023-08-29T07:06:28Z

📌 Commit 82a168b6f0b6f253e3df80d3df407e8727d6fec0 has been approved by davidtwco

It is now in the queue for this repository.

RalfJung · 2023-08-29T07:21:40Z

@bors r=davidtwco

bors · 2023-08-29T07:21:42Z

📌 Commit b2ebf1c has been approved by davidtwco

It is now in the queue for this repository.

bors · 2023-08-29T10:22:38Z

⌛ Testing commit b2ebf1c with merge 0b84f18...

bors · 2023-08-29T12:04:28Z

☀️ Test successful - checks-actions
Approved by: davidtwco
Pushing 0b84f18 to master...

rust-timer · 2023-08-29T17:32:39Z

Finished benchmarking commit (0b84f18): comparison URL.

Overall result: ✅ improvements - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-0.8%	[-0.8%, -0.8%]	2
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.4%	[0.4%, 0.4%]	1
Regressions ❌ (secondary)	2.4%	[2.4%, 2.4%]	1
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.4%	[-2.4%, -2.4%]	1
All ❌✅ (primary)	0.4%	[0.4%, 0.4%]	1

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	-	-	0
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-2.8%	[-2.8%, -2.8%]	1
All ❌✅ (primary)	-	-	0

Binary size

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	0.0%	[0.0%, 0.0%]	5
Regressions ❌ (secondary)	0.0%	[0.0%, 0.0%]	4
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	0.0%	[0.0%, 0.0%]	5

Bootstrap: 631.441s -> 630.165s (-0.20%)
Artifact size: 316.24 MiB -> 316.28 MiB (0.01%)

some more is_zst that should be is_1zst Follow-up to rust-lang#115277

rustbot assigned davidtwco Aug 27, 2023

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 Aug 27, 2023

RalfJung commented Aug 27, 2023

View reviewed changes

This comment has been minimized.

Sign in to view

RalfJung mentioned this pull request Aug 27, 2023

statically guarantee that current error codes are documented #108482

Merged

RalfJung commented Aug 27, 2023

View reviewed changes

compiler/rustc_error_codes/src/error_codes.rs Outdated Show resolved Hide resolved

This comment was marked as resolved.

Sign in to view

RalfJung force-pushed the is_1zst branch from 3708970 to 882c005 Compare August 27, 2023 16:41

This comment has been minimized.

Sign in to view

RalfJung force-pushed the is_1zst branch from 882c005 to b8905a3 Compare August 27, 2023 17:07

This comment has been minimized.

Sign in to view

RalfJung force-pushed the is_1zst branch from b8905a3 to 748af28 Compare August 27, 2023 17:44

This comment has been minimized.

Sign in to view

GuillaumeGomez approved these changes Aug 27, 2023

View reviewed changes

RalfJung force-pushed the is_1zst branch from 748af28 to d179d72 Compare August 27, 2023 18:12

This comment has been minimized.

Sign in to view

RalfJung force-pushed the is_1zst branch 2 times, most recently from ccf77c3 to aaf8833 Compare August 28, 2023 07:13

RalfJung commented Aug 28, 2023

View reviewed changes

davidtwco approved these changes Aug 28, 2023

View reviewed changes

compiler/rustc_abi/src/lib.rs Outdated Show resolved Hide resolved

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 28, 2023

RalfJung added 2 commits August 29, 2023 08:58

rustc_abi: audit uses of is_zst; fix a case of giving an enum insuffi…

0da9409

…cient alignment

there seems to be no reason to treat ZST specially in these cases

bf91321

RalfJung force-pushed the is_1zst branch from 25f4848 to 5835c8a Compare August 29, 2023 06:59

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 Aug 29, 2023

const_eval and codegen: audit uses of is_zst

b2ebf1c

RalfJung force-pushed the is_1zst branch from 5835c8a to 82a168b Compare August 29, 2023 07:03

RalfJung mentioned this pull request Aug 29, 2023

repr(transparent): it's fine if the one non-1-ZST field is a ZST #115334

Merged

This comment has been minimized.

Sign in to view

RalfJung force-pushed the is_1zst branch from 82a168b to b2ebf1c Compare August 29, 2023 07:21

bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 29, 2023

bors merged commit 0b84f18 into rust-lang:master Aug 29, 2023
12 checks passed

rustbot added this to the 1.74.0 milestone Aug 29, 2023

RalfJung deleted the is_1zst branch August 29, 2023 12:05

RalfJung mentioned this pull request Aug 29, 2023

some more is_zst that should be is_1zst #115340

Merged

matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 29, 2023

Rollup merge of rust-lang#115340 - RalfJung:more_is_1zst, r=oli-obk

a644f37

some more is_zst that should be is_1zst Follow-up to rust-lang#115277

dtolnay mentioned this pull request Aug 30, 2023

Fix zero sizing of Iter<T> dtolnay/inventory#63

Merged

RalfJung mentioned this pull request Sep 9, 2023

fix homogeneous_aggregate not ignoring some ZST #115708

Merged

RalfJung mentioned this pull request Nov 27, 2023

ICE in sparc64 fn_abi_of_instance #115399

Open

matthiaskrgr mentioned this pull request Mar 27, 2024

univariant: field #2 comes after unsized field #123134

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix some issues around ZST handling #115277

fix some issues around ZST handling #115277

RalfJung commented Aug 27, 2023 •

edited

rustbot commented Aug 27, 2023

rustbot commented Aug 27, 2023

RalfJung Aug 27, 2023

RalfJung Aug 27, 2023

RalfJung Aug 27, 2023

RalfJung Aug 27, 2023

erikdesjardins Aug 28, 2023

This comment has been minimized.

rustbot commented Aug 27, 2023

This comment was marked as resolved.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

GuillaumeGomez left a comment

RalfJung commented Aug 27, 2023

This comment has been minimized.

rustbot commented Aug 28, 2023

RalfJung Aug 28, 2023

davidtwco left a comment

RalfJung commented Aug 28, 2023

bors commented Aug 28, 2023

RalfJung commented Aug 29, 2023

bors commented Aug 29, 2023

RalfJung commented Aug 29, 2023

bors commented Aug 29, 2023

This comment has been minimized.

RalfJung commented Aug 29, 2023

bors commented Aug 29, 2023

bors commented Aug 29, 2023

bors commented Aug 29, 2023

rust-timer commented Aug 29, 2023

fix some issues around ZST handling #115277

fix some issues around ZST handling #115277

Conversation

RalfJung commented Aug 27, 2023 • edited

rustbot commented Aug 27, 2023

rustbot commented Aug 27, 2023

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

This comment has been minimized.

rustbot commented Aug 27, 2023

This comment was marked as resolved.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

GuillaumeGomez left a comment

Choose a reason for hiding this comment

RalfJung commented Aug 27, 2023

This comment has been minimized.

rustbot commented Aug 28, 2023

Choose a reason for hiding this comment

davidtwco left a comment

Choose a reason for hiding this comment

RalfJung commented Aug 28, 2023

bors commented Aug 28, 2023

RalfJung commented Aug 29, 2023

bors commented Aug 29, 2023

RalfJung commented Aug 29, 2023

bors commented Aug 29, 2023

This comment has been minimized.

RalfJung commented Aug 29, 2023

bors commented Aug 29, 2023

bors commented Aug 29, 2023

bors commented Aug 29, 2023

rust-timer commented Aug 29, 2023

Overall result: ✅ improvements - no action needed

RalfJung commented Aug 27, 2023 •

edited