Some cleanup and miri fixes #2439

Skgland · 2024-07-06T02:17:13Z

resolve clippy warnings
run rustfmt
cleanup leftovers from removed num feature
remove rust_beta_channel feature, everything that was gates is stable in the rust-version defined in Cargo.toml
fix various sources of UB, miri now passes or takes longer than I have patients

commit c41aba6 removed the num feature but a few cfgs remained

- removed some unsafe - added some safety comments - add explicit types to transmute calls - reworked UntypedArenaPtr -> TypedArenaPtr conversion might help with mthom#2438, I noticed fewer complains from miri after changing the default impl for `ArenaAllocated::alloc`

- I think this used to overallocate when the alignment was already met

relevant to mthom#2438

- the msrv (i.e. rust-version in Cargo.toml) is high enough that all gated code can now be used on stabe

probably relevant to mthom#2438

pre-genrate constants instaed of driectly genrating the literal for the insert

Skgland · 2024-07-06T10:05:11Z

Marking as Draft as I plan to fix the clippy warnings and want to investigate some of the remaining miri problems

relevant for wide pointers i.e. pointers with metadata

- write directly to formatter, eliminating intermediate String allocations - take Value by reference eliminating clones - remove trim() called on the result of QueryResolution::to_string - we only emit "true", "false", or "[<resolutions>]" neither of which contains trailing or leading withespace, so the calls was effectively a noop

The arcu crate is a more general implementation of the Rcu I implemented in here in scryer. It contains some bug-fixes regarding race-conditions in the Rcu update function, which could cause leaks and uses after free. Source of the problem was the Relaxed load/strore/update of the reference count in side the Arc not being properly ordered with other load/stores.

Skgland · 2024-07-06T16:36:17Z

@bakaq I think this now resolves all the UB detected with miri, except maybe for some hiding in the test that run very long in miri as I didn't have quite the patients to wait that long.

See actions/runner-images#9255

Skgland · 2024-07-06T16:58:03Z

I think this might fix #2391

Skgland · 2024-07-06T17:56:08Z

Ah, I think I introduced a memory leak with my fix for arena and stream.

Previously a Box<TypedAllocSlab<X>> was allocated and when the Arena was dropped X was manually dropped in place and a Box<AllocSlab> constructed from the Box<TypedAllocSlab<X>> was automatically dropped.
The problem with this is that the deallocation of Box<AllocSlab> doesn't match the allocation of Box<TypedAllocSlab<X>> getting detected as UB.

Some streams could also be dropped early, the TypedAllocSlab<X> Payload was dropped in place and the Tag of the AllocSlab was changed to Dropped causing the maual drop in place of X to be skipped, we couldn't even if we wanted as we have lost the type information.

Now we still allocate a Box<TypedAllocSlab<X>> and when the Arena is dropped we reconstruct the Box<TypedAllocSlab<X>> to drop it.
This is a problem when a stream is dropped early as we now drop the stream in place, and tag it as Dropped, but now we have lost the type information needed to reconstruct the Box<TypedAllocSlab<X>> and we never drop the Box.

I am a bit surprised miri's leak detection hasn't caught this, though maybe non of the test that run under miri exercise this edge case.

bakaq · 2024-07-06T18:23:26Z

Wow, very impressive. The test suite passing Miri is a good step in verifying the unsafe code in Scryer. This doesn't mean that there isn't UB, just that the tests don't stumble on any. I'm not sure how much the current test suite covers all the relevant unsafe code interleavings (for example, this possible leak edge case that @Skgland described), but this is a very good start.

I think it's a good idea to make some benchmarks of this just to make sure we haven't lost a bunch of performance for some obscure reason, but it's probably fine.

I was going to point out that, just like Wasm, Miri is an interpreter so it makes sense to run it in just one machine in CI, but it seems you already did it like that.

Skgland · 2024-07-06T18:39:27Z

I think it's a good idea to make some benchmarks of this just to make sure we haven't lost a bunch of performance for some obscure reason, but it's probably fine.

A few changes may eliminate optimization potential, but mostly I would assume no change.
Some of the changes (mostly 6386e70) eliminate intermediate allocations and unnecessary clones, so that might improve performance, though I doubt those are in the hot path.
Fixing the pointer alignment UB requires allocating more memory as it now rounds up to the next multiple of the alignment, so that might reduce performance due to allocating more often.

I was going to point out that, just like Wasm, Miri is an interpreter so it makes sense to run it in just one machine in CI, but it seems you already did it like that.

Miri is only available in nightly so I just added it to the nightly job.

Skgland · 2024-07-06T21:26:57Z

I have removed ArenaHeaderTag::Dropped, wrapping (parts of) the Payload in an Option for those that can be dropped early. Also noticed that I was leaking ArenaAllocated IndexPtr and fixed that.

This fixes all issues with this PR I know of.
The added commits some unsafe by removing manual drop_in_place calls, but add a bunch of unwrap where there are now Option that need to be handled.

Tests and miri still pass (at least locally, still waiting on CI), so the unwraps should hopefully be unreachable due to checking early whether the stream is missing or not.

Skgland · 2024-07-06T22:21:57Z

Looks like some of the miri enables test take a very long time in CI while all run relatively fast locally (under Windows).
I will check miri tomorrow on my Linus system to attempt to find the culprit tests, so that I can disable them for miri and get CI time back down.

Skgland · 2024-07-06T22:23:14Z

Other than that I would consider now this ready again.

Skgland · 2024-07-07T09:23:50Z

Miri now completes in a resonable time with one additional test disabled🎉

Also I have an Idea for how ArenaHeaderTag::Dropped could work, so I will try that to undo a60c669, as the added Options are not nice.

so that the value type passed to `arena_alloc!` can differ from the `ArenaAllocated::Payload` type

- get rid of HeaderOrIdxPtr - add IndexPtrSlab - add UntypedArenaSlab - add to_untyped for converting a typed slab into an unsyped slab - remove TypedArenaPtr::new, as they shouldn't be created outside of this module

Skgland · 2024-07-07T18:17:58Z

Also I have an Idea for how ArenaHeaderTag::Dropped could work, so I will try that to undo a60c669, as the added Options are not nice.

The solution have now took implemented is to add the ArenaHeaderTag in UntypedArenaSlab, so that we don't lose the type information when the tag is changed to Dropped in the ArenaHeader and as such we can correctly deallocate the slab on drop.
On 64-bit System this increases the AllocSlab type by 8-bytes.

Additionally for for the ArenaAllocated types where the Payload can be dropped early the Payload is now wrapped in ManuallyDrop and manually dropped on drop when not already dropped early, so that we don't double drop the Payload when we drop the Box and it has already been dropped.

This is now ready again.

Skgland added 13 commits July 5, 2024 21:29

remove remains of num feature

a935850

commit c41aba6 removed the num feature but a few cfgs remained

adjust alignment calculation

7509cc1

- I think this used to overallocate when the alignment was already met

resolve miri error for pstr_iter_tests in atom_table

b8a3067

relevant to mthom#2438

replace transmut with pointer cast calls

3de0a4e

unify pointer width

821358c

remove rust_beta_channel feature

9125391

- the msrv (i.e. rust-version in Cargo.toml) is high enough that all gated code can now be used on stabe

some test pass miri

cba81b4

fix some more miri errors

6575b1b

probably relevant to mthom#2438

don't use env::current_dir() in miri

52fa518

adjust the generation of the LIBRARIES map

d2d451b

pre-genrate constants instaed of driectly genrating the literal for the insert

update ignore reason

ce40f8f

run rustfmt

467a82f

Skgland marked this pull request as draft July 6, 2024 10:04

Skgland added 15 commits July 6, 2024 13:01

clippy: ignore unused fields on PrologBenchmark struct

ceac9c9

clippy: use clone_from rather than clone

1b16c6c

clippy: allow new without default

1ff995f

clippy: explicit ptr addrs comparision

d2f236d

relevant for wide pointers i.e. pointers with metadata

fix saftey comment

9e7d410

clippy: identity conversion

ed57ef3

clippy: ptr dereference in safe function

8943962

clippy: use type associated constants

722975d

somehow this was never marked as bad

87b4c9d

fix stream.rs UB

fcb4154

add associated Payload type to ArenaAllocated

285f11c

add miri to CI

3379319

make components a comma seperated string instead of a list

8e53d12

Skgland added 3 commits July 6, 2024 17:43

if UB un IndexPtr ArenaAllocated impl

ea041a4

adjust ignore reason

b32498b

fix clippy

943dce5

Skgland marked this pull request as ready for review July 6, 2024 16:30

switch macos to latest as macos-11 has been removed

c414f75

See actions/runner-images#9255

Skgland marked this pull request as draft July 7, 2024 09:23

Skgland and others added 5 commits July 7, 2024 11:30

reduce span of unsafe block

0284a20

replace static Once and two mut statics with one static OnceLock

a2a5058

fix leak of ArenaAllocated IndexPtr

89c1ea4

fix clippy again

496e4e9

ignore pstr_iter_tests test in miri as it takes too long

39bd520

Skgland force-pushed the miri branch from 059f177 to 9130c53 Compare July 7, 2024 11:26

Skgland marked this pull request as ready for review July 7, 2024 11:26

Skgland added 6 commits July 7, 2024 14:18

remove unsafe impl From<UntypedArenaPtr> for CodeIndex

74720d4

add AllocateInArena as a Pivot for arena_alloc!

213ee5c

so that the value type passed to `arena_alloc!` can differ from the `ArenaAllocated::Payload` type

check that we don't attempt to un-drop while evacuating

a0d7904

wrap Payloads that are dropped eraly in ManuallyDrop

1b19ae8

cleanup the slab types

689632b

- get rid of HeaderOrIdxPtr - add IndexPtrSlab - add UntypedArenaSlab - add to_untyped for converting a typed slab into an unsyped slab - remove TypedArenaPtr::new, as they shouldn't be created outside of this module

fix leak of slaps with dropped payload

5d03be4

Skgland force-pushed the miri branch from 9130c53 to 5d03be4 Compare July 7, 2024 17:23

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Some cleanup and miri fixes #2439

Some cleanup and miri fixes #2439

Skgland commented Jul 6, 2024 •

edited

Loading

Skgland commented Jul 6, 2024

Skgland commented Jul 6, 2024

Skgland commented Jul 6, 2024

Skgland commented Jul 6, 2024

bakaq commented Jul 6, 2024

Skgland commented Jul 6, 2024

Skgland commented Jul 6, 2024 •

edited

Loading

Skgland commented Jul 6, 2024 •

edited

Loading

Skgland commented Jul 6, 2024

Skgland commented Jul 7, 2024

Skgland commented Jul 7, 2024 •

edited

Loading

Some cleanup and miri fixes #2439

Are you sure you want to change the base?

Some cleanup and miri fixes #2439

Conversation

Skgland commented Jul 6, 2024 • edited Loading

Skgland commented Jul 6, 2024

Skgland commented Jul 6, 2024

Skgland commented Jul 6, 2024

Skgland commented Jul 6, 2024

bakaq commented Jul 6, 2024

Skgland commented Jul 6, 2024

Skgland commented Jul 6, 2024 • edited Loading

Skgland commented Jul 6, 2024 • edited Loading

Skgland commented Jul 6, 2024

Skgland commented Jul 7, 2024

Skgland commented Jul 7, 2024 • edited Loading

Skgland commented Jul 6, 2024 •

edited

Loading

Skgland commented Jul 6, 2024 •

edited

Loading

Skgland commented Jul 6, 2024 •

edited

Loading

Skgland commented Jul 7, 2024 •

edited

Loading