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.

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

i#4504: Implement instr_invert_predicate for AArch64 #4637

Merged
merged 7 commits into from
Jan 4, 2021
Merged

i#4504: Implement instr_invert_predicate for AArch64 #4637

merged 7 commits into from
Jan 4, 2021

Conversation

yury-khrustalev
Copy link
Contributor

@yury-khrustalev yury-khrustalev commented Dec 29, 2020

Add implementation of instr_invert_predicate for AArch64.
This change is required to complete #4500 to allow creation of conditional instructions.

Fixes: #4504

@yury-khrustalev
Copy link
Contributor Author

VS builds failed with "The remote server returned an error: (404) Not Found." error, could you please restart the builds, thank you!

@derekbruening
Copy link
Contributor

VS builds failed with "The remote server returned an error: (404) Not Found." error, could you please restart the builds, thank you!

Please see https://docs.github.com/en/free-pro-team@latest/actions/managing-workflow-runs/re-running-a-workflow

@yury-khrustalev
Copy link
Contributor Author

Please see https://docs.github.com/en/free-pro-team@latest/actions/managing-workflow-runs/re-running-a-workflow

@derekbruening, thank you, I know how to use GitHub actions, but I don't have "Re-run all jobs" button in this repository (probably due to not having permissions or something along these lines).

@yury-khrustalev
Copy link
Contributor Author

Also, it looks like there is a problem with URL used to download Doxygen for windows builds.

@derekbruening
Copy link
Contributor

Also, it looks like there is a problem with URL used to download Doxygen for windows builds.

If the doxygen URL changed, then a re-run won't fix it. If you have the new URL, submit a separate PR to fix it?

@yury-khrustalev
Copy link
Contributor Author

If you have the new URL, submit a separate PR to fix it?

Was there a reason why version 1.8.19 is used? Would it not work with 1.9.0? I simply don't have appropriate devices to test.

@derekbruening
Copy link
Contributor

If you have the new URL, submit a separate PR to fix it?

Was there a reason why version 1.8.19 is used? Would it not work with 1.9.0?

I recall using chocolatey to download the latest w/o having to specify its version but chocolately was flaky: looks like #4000. So we switched to a hardcoded version to solve the choco problems and probably just put in the latest version at the time, which then suffers from breaking when the link disappears. Suggestions for a better way are welcome.

I simply don't have appropriate devices to test.

I think anyone would test it the same way as you: make a PR and see if Windows turns green on the PR. Or wait and someone else will hit it and have to fix it to make progress.

@yury-khrustalev
Copy link
Contributor Author

@derekbruening, could you please review this when you have time. This comes from the discussion in #4500 and is required to rework that change according to what we decided. Thank you.

Copy link
Contributor

@derekbruening derekbruening left a comment

Choose a reason for hiding this comment

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

(I see there's no existing test for the ARM version but it could be nice to add some sanity check to ir_aarch64.c or sthg.)

core/ir/instr.h Outdated Show resolved Hide resolved
@derekbruening
Copy link
Contributor

Fixes: #4504 by adding implementation of instr_invert_predicate for AArch64.

The Fixes has to be on a line by itself. See https://github.com/DynamoRIO/dynamorio/wiki/Code-Reviews#commit-messages where we always put it at the bottom.

@derekbruening derekbruening merged commit 48c8dd8 into DynamoRIO:master Jan 4, 2021
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.

Implement instr_invert_predicate for AArch64
2 participants