Skip to content

Add whitelist of safe intrinsics #6307

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 4 commits into from
Oct 21, 2020
Merged

Add whitelist of safe intrinsics #6307

merged 4 commits into from
Oct 21, 2020

Conversation

frazar
Copy link
Contributor

@frazar frazar commented Oct 21, 2020

This PR should fix #5996, where intrinsic operations where all marked as unsafe.

I'm rather new to this codebase, so I might be doing something very wrong. Please forgive me!

In particular, I'm not sure how to "check that we are in extern rust-intrinsics" as mentioned in this comment.

@lnicola
Copy link
Member

lnicola commented Oct 21, 2020 •
edited
Loading

Awesome, thanks for the PR :-). In other places we clone the repository and read the files from there, but this feels like such a huge wart TBH...

lnicola
lnicola reviewed Oct 21, 2020
View reviewed changes
lnicola
lnicola reviewed Oct 21, 2020
View reviewed changes
@lnicola
Copy link
Member

lnicola commented Oct 21, 2020

Regarding the type of the extern block, the AST looks like this:

  [email protected]
    [email protected]
      [email protected] "extern"
      [email protected] " "
      [email protected] "\"C\""

@lnicola
Copy link
Member

lnicola commented Oct 21, 2020 •
edited
Loading

I think you can get the ABI node using extern_block.abi(), but there's no child for the type, only for the extern token. @matklad, we should probably add it to the AST? We're missing the accessor for it.

matklad
matklad reviewed Oct 21, 2020
View reviewed changes
@bors
Copy link
Contributor

bors bot commented Oct 21, 2020

✌️ frazar can now approve this pull request. To approve and merge a pull request, simply reply with bors r+. More detailed instructions are available here.

@matklad
Copy link
Member

matklad commented Oct 21, 2020

@lnicola good question! Turns out this needs a bit of design, I've put together #6308

@frazar
Copy link
Contributor Author

frazar commented Oct 21, 2020

Thank you for your feedback and comments. I think I addressed all points, except for the issue about checking the ABI name, which I guess is blocked on #6308.

Also, I'm not sure why CI failed for macos-latest.. It might be unrelated.

Let me know what should I do next!

@lnicola
Copy link
Member

lnicola commented Oct 21, 2020

bors r=frazar

@bors
Copy link
Contributor

bors bot commented Oct 21, 2020

Build succeeded:

@bors bors bot merged commit 9eb6cbb into rust-lang:master Oct 21, 2020
@frazar
Copy link
Contributor Author

frazar commented Oct 21, 2020

Great! As a final note, would you mind adding the hacktoberfest-accepted label to this PR? Thanks!

@lnicola
Copy link
Member

lnicola commented Oct 21, 2020

Sure, done.

@frazar frazar deleted the fix-5996 branch October 23, 2020 07:16
@frazar frazar mentioned this pull request Oct 23, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lnicola lnicola lnicola left review comments

@matklad matklad matklad left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Incorrect "operation requires unsafe block"
3 participants
@frazar @lnicola @matklad