Skip to content

Allow to track multiple alloc-ids, call-ids and pointer tags #2075

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

Merged
merged 2 commits into from
Apr 21, 2022

Conversation

BennoLossin
Copy link
Contributor

Closes #2073.

@bors
Copy link
Contributor

bors commented Apr 20, 2022

☔ The latest upstream changes (presumably #2071) made this pull request unmergeable. Please resolve the merge conflicts.

@RalfJung
Copy link
Member

Thanks for the PR! I'll try to review it soon, but I am also flying overseas today and still need to pack my stuff so things might get delayed a bit.

In case of a conflict, please do a rebase -- we'd like to not have merge commits inside PRs.

@BennoLossin
Copy link
Contributor Author

No problem, i havent made that many changes, so resolving any conflicts is not too difficult.

Sorry, i did not know that you are using rebases in this repo, i read about it in the rustc dev guide, but the contribution guidelines did not mention it and i am not used to using rebase. Should i do something about the merge commits (like try to rebase and resolve again)?

@RalfJung
Copy link
Member

Yeah we follow the same procedures as rustc, generally speaking.

Rebasing will require you to resolve the conflicts. So if you follow the dev guide you should be good. :)

@BennoLossin
Copy link
Contributor Author

So should i leave the current commits or delete them and then resolve the conflicts using rebase?

@RalfJung
Copy link
Member

Once you are done there should be a single commit with your changes.

@BennoLossin
Copy link
Contributor Author

Ah now i think i get it, i did not create a branch in my fork, does anything break if i branch of master and then delete (through force push) the current commits to master and then rebase my local branch onto master again?

@RalfJung
Copy link
Member

Eh, I don't quite know what you mean. You can do all the rebasing locally until your local state is just one commit on top of the master branch from the main repo, and then force-push that to the master branch of your fork.
(But it is also generally a good idea to use dedicated branches for PRs, not master.)

@RalfJung
Copy link
Member

This looks solid, I just have some minor nits. :) Thanks a lot!

- Changed arg parsing to handle comma seperated list of `u64`'s.
- Changed type and field names of config, executor and global state
  to hold a set of tracked ids.
- Adjusted Readme:
    - explained list format
    - arguments do not overwrite, instead append
    - no effect on duplication
- Created a parsing function for comma separated lists
- Added error printing to alloc_id parsing
@RalfJung
Copy link
Member

Nice. :) I just did a last tweak that I did not notice the first time around.

@bors r+

@bors
Copy link
Contributor

bors commented Apr 21, 2022

📌 Commit b472ef5 has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Apr 21, 2022

⌛ Testing commit b472ef5 with merge 3ac7ca4...

@bors
Copy link
Contributor

bors commented Apr 21, 2022

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 3ac7ca4 to master...

@bors bors merged commit 3ac7ca4 into rust-lang:master Apr 21, 2022
@saethlin
Copy link
Member

This PR produced a ~8% performance regression because it degrades inlining, particularly of Stack::check_protector. We should do some strategic outlining of all the error/diagnostic code.

@RalfJung
Copy link
Member

Ouch, good catch.

Can you open an issue for that?

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.

Allow passing more than one pointer tag to -Zmiri-track-pointer-tag=<tag>
4 participants