Skip to content

Conversation

est31
Copy link
Member

@est31 est31 commented Aug 5, 2023

This teaches the useless_ptr_null_checks lint that some std functions can't ever return null pointers, because they need to point to valid data, get references as input, etc.

This is achieved by introducing an #[rustc_never_returns_null_ptr] attribute and adding it to these std functions (gated behind bootstrap cfg_attr).

Later on, the attribute could maybe be used to tell LLVM that the returned pointer is never null. I don't expect much impact of that though, as the functions are pretty shallow and usually the input data is already never null.

Follow-up of PR #113657

Fixes #114442

And look for it in the useless_ptr_null_checks lint
@rustbot
Copy link
Collaborator

rustbot commented Aug 5, 2023

r? @thomcc

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

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Aug 5, 2023
@est31 est31 force-pushed the extend_useless_ptr_null_checks branch from f0f66b1 to 3d53495 Compare August 5, 2023 03:30
@est31 est31 force-pushed the extend_useless_ptr_null_checks branch 2 times, most recently from fabfc2f to 0071d57 Compare August 5, 2023 12:21
@est31 est31 mentioned this pull request Aug 5, 2023
@est31
Copy link
Member Author

est31 commented Aug 5, 2023

Good point about the function that returns a reference, I suppose I've missed that.

Also good point about the doc vs implementation discrepancy. I've removed the addition of the attribute from this PR and opened #114517 for discussion what to do about the discrepancy. It can then be resolved in a new PR.

est31 added 2 commits August 6, 2023 00:20
Add the attribute to standard library functions that
are guaranteed to never return null pointers, as their
originating data wouldn't allow it.
It was added by rust-lang#113657 for its purposes.
Now it is not used any more, remove it,
as we use the attr now.
@est31 est31 force-pushed the extend_useless_ptr_null_checks branch from 0071d57 to fed728d Compare August 5, 2023 23:39
@rust-log-analyzer

This comment has been minimized.

@est31 est31 force-pushed the extend_useless_ptr_null_checks branch from fed728d to 490eb7c Compare August 6, 2023 00:00
@est31 est31 force-pushed the extend_useless_ptr_null_checks branch from 490eb7c to 4b1bc27 Compare August 6, 2023 17:08
@thomcc
Copy link
Member

thomcc commented Aug 28, 2023

Sorry about the delay here. I think the std side is fine, but I can't review the compiler changes.

r? compiler

@rustbot rustbot assigned jackh726 and unassigned thomcc Aug 28, 2023
@jackh726
Copy link
Member

@bors r+

@bors
Copy link
Collaborator

bors commented Sep 16, 2023

📌 Commit 4b1bc27 has been approved by jackh726

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 16, 2023
@bors
Copy link
Collaborator

bors commented Sep 16, 2023

⌛ Testing commit 4b1bc27 with merge 635c4a5...

@bors
Copy link
Collaborator

bors commented Sep 16, 2023

☀️ Test successful - checks-actions
Approved by: jackh726
Pushing 635c4a5 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 16, 2023
@bors bors merged commit 635c4a5 into rust-lang:master Sep 16, 2023
@rustbot rustbot added this to the 1.74.0 milestone Sep 16, 2023
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (635c4a5): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
3.3% [3.3%, 3.3%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 632.106s -> 632.885s (0.12%)
Artifact size: 318.13 MiB -> 318.13 MiB (0.00%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Extend useless_ptr_null_checks to more functions
8 participants