Skip to content

Conversation

@quininer
Copy link
Contributor

The PR main intention is to allow enable wasm support in std time (see #48564) with minimal changes via binary patches. but since the now function is inlined, it is not work at the moment. this PR marks it as #[inline(never)] to make it patchable.

image

I have tested the patch and it works.

@rustbot
Copy link
Collaborator

rustbot commented Apr 29, 2025

r? @ChrisDenton

rustbot has assigned @ChrisDenton.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 29, 2025
@bjorn3
Copy link
Member

bjorn3 commented Apr 29, 2025

#[inline(never)] still allows LLVM to modify the ABI in ways that prevent binary patching. For example if it notices that the function never writes to the return area, it can remove the return area pointer on both the caller and callee side (deadargelim). It can also notice that the function never returns and place an unreachable after the call or do a tail call. #[inline(never)] only disables inlining (though even this is not guaranteed), not other interprocedural optimizations.

@ChrisDenton
Copy link
Member

I think a discussion needs to happen before making a PR. If we do want to add hooks (vs. just making a target with time support) then I do not think this PR is the right way to do it for the reasons bjorn3 outlines above.

Please open an issue to discuss the proposal in more depth.

@quininer
Copy link
Contributor Author

@bjorn3 This is a good point, and I was aware of it before open PR. but I don't know how to avoid it in rust.

@ChrisDenton Sorry, I'll open an issue.

@quininer
Copy link
Contributor Author

I don't think this will be merged, so closed

@quininer quininer closed this Apr 29, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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.

4 participants