Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

CTFE interns allocations multiple times #53552

Closed
oli-obk opened this issue Aug 21, 2018 · 4 comments
Closed

CTFE interns allocations multiple times #53552

oli-obk opened this issue Aug 21, 2018 · 4 comments
Assignees
Labels
A-const-eval Area: constant evaluation (mir interpretation)

Comments

@oli-obk
Copy link
Contributor

oli-obk commented Aug 21, 2018

This is refactoring fallout during the miri merger.

First it is interned in

let alloc = self.tcx.intern_const_alloc(alloc);
and then again in
let alloc = ecx.tcx.intern_const_alloc(alloc);

The second place is wholly unnecessary barring bugs in the first place

@oli-obk oli-obk added the A-const-eval Area: constant evaluation (mir interpretation) label Aug 21, 2018
@oli-obk oli-obk self-assigned this Aug 21, 2018
@RalfJung
Copy link
Member

The interning in memory.rs is also fishy. It determines once, using is_freeze(), whether there is interior mutability, and then recursively uses that information when interning stuff behind references. However, is_freeze() does not say anything about what is behind references: It is guided by the Freeze marker trait, which has

unsafe impl<T: ?Sized> Freeze for *const T {}
unsafe impl<T: ?Sized> Freeze for *mut T {}
unsafe impl<'a, T: ?Sized> Freeze for &'a T {}
unsafe impl<'a, T: ?Sized> Freeze for &'a mut T {}

So really it has to check again after following any pointer indirection.

Of course, that doe snot work either because at that point we do not have type information...

I think right now this is not a problem because this does not compile

static FOO2 : &'static AtomicUsize = &AtomicUsize::new(3);

so the "inner" allocations we arrive when following pointers will never have an UnsafeCell.

But I have a hard time following any of this code, so that analysis may be totally wrong.

@RalfJung
Copy link
Member

Ah and because I don't know there else to dump this: const_to_allocation is a strange beast; it seems to be called for statics only so the logic that copies is unnecessary (and it is non-recursive anyway which seems incomplete).

Likely we want a query for statics that just returns an allocation, or maybe an AllocId instead, done?

@RalfJung
Copy link
Member

@oli-obk I think this got fixed by your big interning refactoring (#58351)?

@oli-obk
Copy link
Contributor Author

oli-obk commented Aug 17, 2019

Yes, the call in memory.rs is gone now

@oli-obk oli-obk closed this as completed Aug 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: constant evaluation (mir interpretation)
Projects
None yet
Development

No branches or pull requests

2 participants