Skip to content

Add core::ptr::slice_len #68169

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

Closed
wants to merge 1 commit into from
Closed

Add core::ptr::slice_len #68169

wants to merge 1 commit into from

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Jan 13, 2020

Uses a placeholder tracking issue currently. I'll file a tracking issue and update the commit if this is OK'd.

If I'm reading the various issue threads about raw pointer metadata correctly, the validity invariant of pointer metadata is at max that of the metadata type. For slice pointers, the metadata is typed at usize. This means that this function should be sound over all *const [_] pointers.

This is doable stably for all but the null-ptr case by casting to *const [()] and making that a reference &[()], as [()] has an alignment of 1 and a size of 0 no matter its length, so is valid at any location (IIUC). This just exposes a standard way of doing it without having to know to erase the type and that works for potentially-null pointers as well.

cc @rust-lang/wg-unsafe-code-guidelines @RalfJung rust-lang/unsafe-code-guidelines#158 (comment)

@rust-highfive
Copy link
Contributor

r? @shepmaster

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

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 13, 2020
@CAD97
Copy link
Contributor Author

CAD97 commented Jan 13, 2020

(CI (tidy) will fail because I marked the placeholder issue number with a TODO, though this does unfortunately block further CI tests)

@rust-highfive
Copy link
Contributor

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2020-01-13T03:17:12.8763203Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-01-13T03:17:12.8774507Z ##[command]git config gc.auto 0
2020-01-13T03:17:12.8777196Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-01-13T03:17:12.8779458Z ##[command]git config --get-all http.proxy
2020-01-13T03:17:12.8782645Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/68169/merge:refs/remotes/pull/68169/merge
---
2020-01-13T03:22:57.1715261Z    Compiling serde_json v1.0.40
2020-01-13T03:22:58.8398233Z    Compiling tidy v0.1.0 (/checkout/src/tools/tidy)
2020-01-13T03:23:08.9108943Z     Finished release [optimized] target(s) in 1m 23s
2020-01-13T03:23:08.9205240Z tidy check
2020-01-13T03:23:09.1727195Z tidy error: /checkout/src/libcore/ptr/mod.rs:301: TODO is deprecated; use FIXME
2020-01-13T03:23:11.8038528Z some tidy checks failed
2020-01-13T03:23:11.8039222Z Found 486 error codes
2020-01-13T03:23:11.8039480Z Found 0 error codes with no tests
2020-01-13T03:23:11.8039668Z Done!
2020-01-13T03:23:11.8039668Z Done!
2020-01-13T03:23:11.8044455Z 
2020-01-13T03:23:11.8044723Z 
2020-01-13T03:23:11.8045954Z command did not execute successfully: "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0-tools-bin/tidy" "/checkout/src" "/checkout/obj/build/x86_64-unknown-linux-gnu/stage0/bin/cargo" "--no-vendor"
2020-01-13T03:23:11.8046142Z 
2020-01-13T03:23:11.8046169Z 
2020-01-13T03:23:11.8056054Z failed to run: /checkout/obj/build/bootstrap/debug/bootstrap test src/tools/tidy
2020-01-13T03:23:11.8056379Z Build completed unsuccessfully in 0:01:35
2020-01-13T03:23:11.8056379Z Build completed unsuccessfully in 0:01:35
2020-01-13T03:23:11.8111790Z == clock drift check ==
2020-01-13T03:23:11.8123379Z   local time: Mon Jan 13 03:23:11 UTC 2020
2020-01-13T03:23:12.1006636Z   network time: Mon, 13 Jan 2020 03:23:12 GMT
2020-01-13T03:23:12.1006957Z == end clock drift check ==
2020-01-13T03:23:12.8221054Z 
2020-01-13T03:23:12.8310705Z ##[error]Bash exited with code '1'.
2020-01-13T03:23:12.8340570Z ##[section]Starting: Checkout
2020-01-13T03:23:12.8342301Z ==============================================================================
2020-01-13T03:23:12.8342383Z Task         : Get sources
2020-01-13T03:23:12.8342438Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

@RalfJung
Copy link
Member

[()] has an alignment of 1 and a size of 0 no matter its length, so is valid at any location (IIUC)

No, it's not. The pointer still must not be dangling, even if the size is 0. IOW, it must not point to an allocation that has been freed, or beyond the bounds of an existing one.

(CI (tidy) will fail because I marked the placeholder issue number with a TODO, though this does unfortunately block further CI tests)

Just use issue number 0, for now.

@RalfJung
Copy link
Member

More generally speaking, this should probably be part of a family of methods on raw slices. Besides len, most immediately coming to my mind are (unchecked) methods to index into the slice.

@CAD97
Copy link
Contributor Author

CAD97 commented Jan 14, 2020

I'm unsure how to go about adding the lang item required to add methods to *const [_]/*mut [_]; if someone gives me a pointer how to, I'm willing to pivot this PR to adding that whole family of potential methods.

I took a glance at the typeck inherent impls part of #[lang = "const_ptr"], and that simple overview suggests that because for<T> *const [T] overlaps with for<T> *const T, it's not just as simple as adding another inherent impl lang item.

It'd be "nice" for these to be methods on [T] that take self: *const Self, but I don't know the status of raw pointer receivers (plus that would require the len method to have a different name, which I fail to come up with a decent option for, unless somehow making the existing <[T]>::len take self: *const Self wouldn't be breaking... this would require weird adjustments to the autoref rules IIUC.)

@nikomatsakis
Copy link
Contributor

@CAD97 Hmm. We ought to be able to manage inherent methods on *const [], I suppose, since we can manage things like

impl MyType<u32> { }

and the like. Glancing over the code it seems like most of the logic is just there to detect the correct usage of the const_ptr lang-item more than anything!

Note that in some cases we have already the option of having "two" def-ids, for distinct crates:

ty::Slice(slice_item) if slice_item == self.tcx.types.u8 => {
self.check_primitive_impl(
def_id,
lang_items.slice_u8_impl(),
lang_items.slice_u8_alloc_impl(),
"slice_u8",
"[u8]",
item.span,
);
}

we could probably just add a second lang item like const_ptr but const_ptr_slice or something and then include it in that call instead of None here:

It wouldn't be perfect in some sense, since the lang item const_ptr_slice could be used on any *const impl but...I'm not sure how much that matters. If we wanted to check more deeply, we could also do that, by matching on the ty: _ type here:

ty::RawPtr(ty::TypeAndMut { ty: _, mutbl: hir::Mutability::Not }) => {

@JohnTitor JohnTitor 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 26, 2020
@wirelessringo
Copy link

Ping from triage. @CAD97 any updates on this? Thanks.

@CAD97
Copy link
Contributor Author

CAD97 commented Jan 31, 2020

IIUC the status is that we want to attach len, as well as indexing methods (probably) directly to *const [_] and *mut [_] as inherent methods rather than free fn in the ptr module.

Unfortunately, I don't have the bandwidth right now to go about adding the lang item required to add these inherent impls, so I'll close this for now.

@CAD97 CAD97 closed this Jan 31, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants