Fix HashStable implementation on AllocId #93472

compiler-errors · 2022-01-30T02:22:02Z

(take 2:)

This PR removes the HashStable implementation from AllocId, and fixes the HashStable implementation on ConstValue, ConstAlloc, and Allocation so that their Eq and HashStable implementations agree (as required by HashStable).

Fixes #93470

r? @oli-obk

rust-highfive · 2022-01-30T02:22:05Z

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

compiler-errors · 2022-01-30T02:37:35Z

Ok this definitely doesn't work :^) Of course I should've checked the incremental tests before putting this up, lol.

compiler-errors · 2022-01-30T02:37:43Z

@rustbot author

oli-obk · 2022-01-30T08:34:51Z

The easy solution here is to remove the HashStable impl entirely and instead add a manual impl for ConstAlloc and ConstValue and other types that have an alloc id and require HashStable. Alternatively create a convenience wrapper type around AllodID with manual partialeq and stablehash impls and use it in those places.

compiler-errors · 2022-01-31T05:59:33Z

The easy solution here is to remove the HashStable impl entirely and instead add a manual impl for ConstAlloc and ConstValue and other types that have an alloc id and require HashStable.

@oli-obk, I took an initial stab at that suggestion of yours. In order to implement HashStable correctly on those types mentioned above, I had to reimplement Eq (and friends) to agree with the HashStable implementation (that compares the allocation behind the AllocId). However, this isn't a complete solution.. evident by the fact that I'm having trouble building stage1 std due to some const-eval issues:

error[E0080]: it is undefined behavior to use this value                                                                                                                                                                 
   --> /home/michael/.cargo/registry/src/github.com-1ecc6299db9ec823/getopts-0.2.21/src/lib.rs:927:1                                                                                                                     
    |                                                                                                                                                                                                                    
927 | fn is_arg(arg: &str) -> bool {                                                                                                                                                                                     
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed at .<deref>.<enum-variant(Some)>.0.<deref>: encountered (potentially part of) a pointer, but expected plain (non-pointer) bytes                                
    |                                                                                                                                                                                                                    
    = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rustc repository if you believe it should not be considered undefined behavior.
    = note: the raw bytes of the constant (size: 8, align: 8) {                                                                                                                                                          
                ╾──────alloc1253──────╼                         │ ╾──────╼                                                                                                                                               
            }                                                                                                                                                                                                            
                                                                                                                                                                                                                         
For more information about this error, try `rustc --explain E0080`.

I'm assuming some const eval code depends on that compare-alloc-by-id behavior, but I can't track it down. 🤷

compiler-errors · 2022-01-31T06:03:18Z

compiler/rustc_middle/src/ty/relate.rs

@@ -634,7 +634,8 @@ fn check_const_value_eq<'tcx, R: TypeRelation<'tcx>>(
            ConstValue::Scalar(Scalar::Ptr(a_val, _a_size)),
            ConstValue::Scalar(Scalar::Ptr(b_val, _b_size)),
        ) => {
-            a_val == b_val
+            // FIXME(compiler-errors): This is highly suspicious
+            a_val.provenance.0 == b_val.provenance.0


this is to preserve the old "equate AllocId by value" behavior.

edit: actually I don't need this change anymore, I'll revert it when I clean up this PR.

compiler-errors · 2022-01-31T06:50:38Z

Alright, this definitely has to do with some code relying on relocations in Allocation being compared by id.

--

As I learn more about this code, I think I was silly to think Allocation should have the HashStable/Eq behavior I implemented here. I wonder if I could leave Allocation's Eq and Hash implementations alone (and remove its HashStable impl as well), and instead implement a "correct" HashStable/Eq on GlobalAlloc instead.

compiler-errors · 2022-01-31T08:51:24Z

Well this is a rough set of working changes. I can clean it up tomorrow...

oli-obk · 2022-01-31T09:23:49Z

Ah yes, sorry. I completely forgot about allocations themselves. Remove their HashStable impl and make it a free/inherent function. Then you can just keep calling it from ConstAlloc's impl

oli-obk · 2022-01-31T09:24:23Z

Oh, my browser didn't refresh. Ignore the previous comment. Reading the impl now

oli-obk · 2022-01-31T09:46:05Z

Looks like most of these manual impls will be gone when we get valtrees... so I am somewhat inclined to roll with the general idea.

I was considering just swapping out the AllocId type (Scalar and Allocation are generic over it) so we could keep all derives but the one on whatever we use instead of AllocId.

RalfJung · 2022-01-31T14:09:09Z

I am a bit worried about considering allocations equal when their contents are equal (but their ID is not) -- this might make sense for some allocations, but certainly not for all of them (e.g., static). And moreover, the fact that the type system depends on this equality at all is a bug that we need to fix anyway (by using valtrees). So is it really worth introducing subtle and risky changes that won't fix the actual problem?

oli-obk · 2022-01-31T14:47:16Z

I am a bit worried about considering allocations equal when their contents are equal (but their ID is not)

we're def not going to do that :) I'm more than worried about something like this. Hash/Eq should just be plain derives on Allocation and AllocId, but HashStable should not be implemented for either of them, and the surrounding types must handle that manually.

compiler-errors · 2022-01-31T17:54:30Z

I was considering just swapping out the AllocId type (Scalar and Allocation are generic over it) so we could keep all derives but the one on whatever we use instead of AllocId.

@oli-obk: yeah, I considered that too, especially because Tag is generic all over the place, but (at least if I understand it correctly) then there's a deep-copy you need to perform when converting between AllocId and "AllocId that hashes/equates according to its contents"..

oli-obk · 2022-02-01T09:21:02Z

Yea, I agree it would kill perf, so let's keep doing what you were doing

b-naber · 2022-02-01T15:14:08Z

compiler/rustc_middle/src/ty/impls_ty.rs

+                        let (ptr, offset) = ptr.into_parts();
+                        tcx.get_global_alloc(ptr).hash_stable(hcx, hasher);
+                        offset.hash_stable(hcx, hasher);
+                        size.hash_stable(hcx, hasher);


Is it really necessary to hash size? This is the pointer size of the target, so identical for all pointers. Why would we need to hash this too?

Good point, but I'd rather implement a correct implementation of HashStable than an optimized one first. I'll leave a FIXME and get back to it.

compiler-errors · 2022-02-01T23:36:07Z

@rustbot ready

compiler-errors · 2022-02-02T00:29:14Z

-                ╾─allocN─╼ ╾─allocN─╼                         │ ╾──╼╾──╼
+                ╾─allocN──╼ ╾─allocN─╼                         │ ╾──╼╾──╼

Literally don't know what's going on with these tests. I don't even understand what these diagrams are trying to convey 😵‍💫.

I guess something is hashing one of these types that I fixed in the example and using them for diagnostics?

oli-obk · 2022-02-02T10:26:22Z

😄 these are normalized to hide the actual id, but that doesn't account for IDs with different number of digits.

Also you need to bless tests with --target i686_unknown_linux_gnu or whatever the appropriate 32 bit target is

compiler-errors · 2022-02-03T05:17:56Z

I'll try to take a stab at fixing these CI failures in a few days.

@rustbot author

RalfJung · 2022-02-04T17:05:32Z

we're def not going to do that :)

Good. :) In that case I guess I don't understand what actually is being done here.

Hash/Eq should just be plain derives on Allocation and AllocId, but HashStable should not be implemented for either of them, and the surrounding types must handle that manually.

Should we have Hash/Eq at all on Allocation? Seems a bit odd to be comparing allocations, TBH.

compiler-errors · 2022-02-06T07:17:42Z

Well looks like CI is happy once I blessed the 32-bit tests, so

@rustbot ready

oli-obk · 2022-02-06T11:18:20Z

Is this PR still necessary with the realization in https://rust-lang.zulipchat.com/#narrow/stream/260443-project-const-generics/topic/valtrees/near/270830218 that we don't need to put alloc IDs into query keys?

compiler-errors · 2022-02-06T14:19:22Z

we don't need to put alloc IDs into query keys?

my understanding is that this PR mostly addresses hashing the structs that are returned by various queries, not the ones used as keys

oli-obk · 2022-02-06T14:42:52Z

Right, but most of these will become obsolete with valtrees.

The ones that don't, should actually be using alloc IDs by their identity, as otherwise static items may get the deduplication, too, and that is bad. We have existing bugs around this for mutable references in static mut (and probably interior mut ones in plain statics)

compiler-errors · 2022-02-06T14:57:16Z

I am not opposed to closing this PR in favor of waiting for valtrees to be the proper fix of representing stable const values returned by queries. Lemme know if that's what you mean and I can close this out.

oli-obk · 2022-02-06T15:47:20Z

I do. Closing then, thanks for trying it out!

rust-highfive assigned oli-obk Jan 30, 2022

rustbot added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jan 30, 2022

rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 30, 2022

rustbot 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 Jan 30, 2022

compiler-errors force-pushed the alloc-id-hash branch from b522ee1 to d778a62 Compare January 31, 2022 05:53

This comment has been minimized.

Sign in to view

compiler-errors commented Jan 31, 2022

View reviewed changes

This comment has been minimized.

Sign in to view

b-naber reviewed Feb 1, 2022

View reviewed changes

compiler-errors added 2 commits February 1, 2022 14:49

make vtable_allocation return an Allocation, not an AllocId

aff004b

implement operators respecting HashStable

0e0251e

compiler-errors force-pushed the alloc-id-hash branch from 8ac75ce to 0e1b142 Compare February 1, 2022 23:28

bless tests, add fixme comment

50a22d9

compiler-errors force-pushed the alloc-id-hash branch from 0e1b142 to 50a22d9 Compare February 1, 2022 23:36

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 1, 2022

This comment has been minimized.

Sign in to view

rustbot 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 Feb 3, 2022

bless 32-bit tests

054df2e

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 6, 2022

oli-obk closed this Feb 6, 2022

compiler-errors deleted the alloc-id-hash branch April 7, 2022 04:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix HashStable implementation on AllocId #93472

Fix HashStable implementation on AllocId #93472

compiler-errors commented Jan 30, 2022 •

edited

Loading

rust-highfive commented Jan 30, 2022

compiler-errors commented Jan 30, 2022 •

edited

Loading

compiler-errors commented Jan 30, 2022

oli-obk commented Jan 30, 2022

compiler-errors commented Jan 31, 2022 •

edited

Loading

This comment has been minimized.

compiler-errors Jan 31, 2022 •

edited

Loading

compiler-errors commented Jan 31, 2022 •

edited

Loading

compiler-errors commented Jan 31, 2022

oli-obk commented Jan 31, 2022

oli-obk commented Jan 31, 2022

This comment has been minimized.

oli-obk commented Jan 31, 2022

RalfJung commented Jan 31, 2022

oli-obk commented Jan 31, 2022

compiler-errors commented Jan 31, 2022

oli-obk commented Feb 1, 2022

b-naber Feb 1, 2022 •

edited

Loading

compiler-errors Feb 1, 2022

compiler-errors commented Feb 1, 2022

This comment has been minimized.

compiler-errors commented Feb 2, 2022

oli-obk commented Feb 2, 2022

compiler-errors commented Feb 3, 2022

RalfJung commented Feb 4, 2022

compiler-errors commented Feb 6, 2022

oli-obk commented Feb 6, 2022

compiler-errors commented Feb 6, 2022

oli-obk commented Feb 6, 2022

compiler-errors commented Feb 6, 2022

oli-obk commented Feb 6, 2022

Fix HashStable implementation on AllocId #93472

Fix HashStable implementation on AllocId #93472

Conversation

compiler-errors commented Jan 30, 2022 • edited Loading

rust-highfive commented Jan 30, 2022

compiler-errors commented Jan 30, 2022 • edited Loading

compiler-errors commented Jan 30, 2022

oli-obk commented Jan 30, 2022

compiler-errors commented Jan 31, 2022 • edited Loading

This comment has been minimized.

compiler-errors Jan 31, 2022 • edited Loading

Choose a reason for hiding this comment

compiler-errors commented Jan 31, 2022 • edited Loading

compiler-errors commented Jan 31, 2022

oli-obk commented Jan 31, 2022

oli-obk commented Jan 31, 2022

This comment has been minimized.

oli-obk commented Jan 31, 2022

RalfJung commented Jan 31, 2022

oli-obk commented Jan 31, 2022

compiler-errors commented Jan 31, 2022

oli-obk commented Feb 1, 2022

b-naber Feb 1, 2022 • edited Loading

Choose a reason for hiding this comment

compiler-errors Feb 1, 2022

Choose a reason for hiding this comment

compiler-errors commented Feb 1, 2022

This comment has been minimized.

compiler-errors commented Feb 2, 2022

oli-obk commented Feb 2, 2022

compiler-errors commented Feb 3, 2022

RalfJung commented Feb 4, 2022

compiler-errors commented Feb 6, 2022

oli-obk commented Feb 6, 2022

compiler-errors commented Feb 6, 2022

oli-obk commented Feb 6, 2022

compiler-errors commented Feb 6, 2022

oli-obk commented Feb 6, 2022

compiler-errors commented Jan 30, 2022 •

edited

Loading

compiler-errors commented Jan 30, 2022 •

edited

Loading

compiler-errors commented Jan 31, 2022 •

edited

Loading

compiler-errors Jan 31, 2022 •

edited

Loading

compiler-errors commented Jan 31, 2022 •

edited

Loading

b-naber Feb 1, 2022 •

edited

Loading