Skip to content
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

i#1569 trace support for AArch64, part 1: cbr invertion for AArch64 #5042

Merged
merged 5 commits into from
Aug 11, 2021
Merged

i#1569 trace support for AArch64, part 1: cbr invertion for AArch64 #5042

merged 5 commits into from
Aug 11, 2021

Conversation

Vincent-lau
Copy link
Collaborator

This patch added implementation for instr_invert_cbr() on AArch64 as well as
changing the implementation of instr_set_predicate by clearing the predicate
bits before setting them.

Issue: #1569

@Vincent-lau
i#1569 trace support for AArch64, part 1: cbr invertion for AArch64
6055566 
This patch added implementation for instr_invert_cbr() on AArch64 as well as
changing the implementation of instr_set_predicate by clearing the predicate
bits before setting them.

Issue: #1569
@Vincent-lau
Copy link
Collaborator Author

First commit of attempting to add trace support for AArch64. Many of the work here is based on the patch #2442 which is work done Kevin @egrimley, but the branch i1569-trace seems to be a bit outdated so I started working on a new branch. Let me know if it is necessary to work on the old branch.

@AssadHashmi
Copy link
Contributor

add to whitelist

@Vincent-lau
Copy link
Collaborator Author

add to whitelist

Sorry, what is the whitelist, and what do I need to add?

@AssadHashmi
Copy link
Contributor

add to whitelist

Sorry, what is the whitelist, and what do I need to add?

That comment is special in that it's a command directed at the Jenkins test server used for AArch64 testing.
It tells Jenkins that you are a new contributor and to allow your patch to build and run the test suite.
I only have to do it once for a contributor's first PR. From now on all your pre-commits tests will trigger automatically.

@AssadHashmi
Copy link
Contributor

If you haven't already had a look, please see https://dynamorio.org/page_code_reviews.html#sec_commit_messages for our commit message format.

@Vincent-lau
i#2974 trace support for AArch64, part 2: implementation of the main …
849b812 
…part of support

This patch incorporated changes from patch #2442, and fixed some corner cases
where the assumption was incorrect and caused the program to crash.

Trace support is not yet enabled by default, but can be enabled with "-enable_traces".

A large part of this work was based on work done by Kevin Zhou. Please see branch
i1569-trace and patch #2442 for the original version.

Issues: #1569, #2974
@derekbruening
Copy link
Contributor

i#1569 trace support for AArch64, part 1: cbr invertion for AArch64

Spelling: s/invertion/inversion/

@derekbruening
Copy link
Contributor

i#2974 trace support for AArch64, part 2: implementation of the main …

Please merge part 1 by itself and post part 2 in a separate PR.

@derekbruening
Copy link
Contributor

Be sure to see https://dynamorio.org/page_code_reviews.html#autotoc_md117 on Merging into Master and Cleaning Up the Final Commit Message

derekbruening
derekbruening requested changes Aug 11, 2021
View reviewed changes
Copy link
Contributor

@derekbruening derekbruening left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just re-iterating as an official review request to revert part 2 here as it should be in a separate PR.

@Vincent-lau
i#2974 trace support for AArch64, part 3: removed non-compatible code…
85f0590 
… with arm

Removed the use of movz which is only available on AArch64.

Issues: #1569, #2974
@Vincent-lau
Revert "trace support for AArch64, part 2 and 3"
dd2d210 
This reverts commit 85f0590 and commit
849b812 as they are going to be submitted
as separate pull requests.

Issues: #1569, #2947
@Vincent-lau
Copy link
Collaborator Author

I don't think I can do the merge (the squash and merge button is greyed out). Is it because I don't have merge privileges or something I have missed some step before merging?

@derekbruening
Copy link
Contributor

I don't think I can do the merge (the squash and merge button is greyed out). Is it because I don't have merge privileges or something I have missed some step before merging?

See the message "This branch is out-of-date with the base branch". Press the "Update branch" button.

@Vincent-lau Vincent-lau merged commit b38ed40 into master Aug 11, 2021
@Vincent-lau Vincent-lau deleted the i2974-trace branch August 11, 2021 19:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AssadHashmi AssadHashmi AssadHashmi approved these changes

@derekbruening derekbruening derekbruening approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@Vincent-lau @AssadHashmi @derekbruening