Skip to content

Comments

Bounds-check Array access for the debug runtime#2675

Merged
mergify[bot] merged 1 commit intomasterfrom
gabor/bounds-check
Jul 26, 2021
Merged

Bounds-check Array access for the debug runtime#2675
mergify[bot] merged 1 commit intomasterfrom
gabor/bounds-check

Conversation

@ggreif
Copy link
Contributor

@ggreif ggreif commented Jul 26, 2021

This adds debug assertions that no access outside of Array's length is attempted.

@ggreif ggreif requested a review from osa1 July 26, 2021 09:02
@@ -226,11 +226,13 @@ impl Array {
}

pub unsafe fn get(self: *mut Self, idx: u32) -> SkewedPtr {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@osa1 I wonder if *const Self would be appropriate here...

Copy link
Contributor

@osa1 osa1 Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ggreif these methods currently all have *mut Self receiver because until recently it wasn't possible to call a *const Self method on a *mut value, see rust-lang/rust#80258. Without the fix for that issue it's quite noisy to use these methods as we need casts x as *const _ everywhere.

I fixed that issue in rust-lang/rust#82436 so we just need to update rustc.

Copy link
Contributor Author

@ggreif ggreif Jul 26, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not many people seem to use raw pointers in Rust... 🤔

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

MMTk is also implemented in Rust, I'd expect it to use raw pointers in the GC code. Would be interesting to look at how they use raw pointers.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we can prune this list then?

// TODO (osa): Some of these are stabilized, we need to update rustc
#![feature(
    arbitrary_self_types,
    panic_info_message,
    assoc_char_funcs,
    core_intrinsics,
    ptr_offset_from
)]

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah most of those should also be gone with an update.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dfinity-ci
Copy link

This PR does not affect the produced WebAssembly code.

@ggreif ggreif changed the title Bounds-check array access for the debug runtime Bounds-check Array access for the debug runtime Jul 26, 2021
@ggreif ggreif added the automerge-squash When ready, merge (using squash) label Jul 26, 2021
@mergify mergify bot merged commit e3d21a5 into master Jul 26, 2021
@mergify mergify bot deleted the gabor/bounds-check branch July 26, 2021 10:28
@mergify mergify bot removed the automerge-squash When ready, merge (using squash) label Jul 26, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants