| Summary: | ARMv8.1 Compare-and-Swap instructions are not supported | ||
|---|---|---|---|
| Product: | [Developer tools] valgrind | Reporter: | ahashmi <assad.hashmi> |
| Component: | vex | Assignee: | Julian Seward <jseward> |
| Status: | RESOLVED FIXED | ||
| Severity: | normal | ||
| Priority: | NOR | ||
| Version First Reported In: | 3.14.0 | ||
| Target Milestone: | --- | ||
| Platform: | Other | ||
| OS: | Linux | ||
| Latest Commit: | Version Fixed In: | ||
| Sentry Crash Report: | |||
| Bug Depends on: | |||
| Bug Blocks: | 414270 | ||
| Attachments: |
Patch implements and tests ARM v8.1 CAS instructions
Patch implements ARM v8.1 CAS instructions Patch tests ARM v8.1 CAS instructions Patch implements ARM v8.1 CAS instructions |
||
|
Description
ahashmi
2019-12-13 13:55:51 UTC
Created attachment 124590 [details]
Patch implements and tests ARM v8.1 CAS instructions
This patch adds support for the following ARM v8.1 instructions:
CAS, CASA, CASAL, CASL
CASB, CASAB, CASALB, CASLB
CASH, CASAH, CASALH, CASLH
(Note this patch does not add support for the pairwise CAS instructions:
CASP, CASPA, CASPAL, CASPL)
Thanks for the patch. I looked at it in some detail. An administrative request: please could you attach the test changes as a separate patch? With it all in one patch, I have to scroll through the entire giant patch to check there aren't some extra implementation bits somewhere in it. I am going to need some context to make sense of this. In particular, why is ARM64in_CAS emitted with a loop, where it did not have a loop before? It is true that a correct usage of CAS in general requires a loop to retry in case of failure, but that loop must exist in the guest code surrounding the incoming CAS instruction. The role of the IR representation and its host-side translation back into arm64 code is merely to attempt the CAS and return a success/failure indication to the guest state. Also, what is the reason for changing the hardwired register numbers? They now don't match the ARM64in_CAS case in getRegUsage_ARM64Instr. Created attachment 126174 [details]
Patch implements ARM v8.1 CAS instructions
Created attachment 126175 [details]
Patch tests ARM v8.1 CAS instructions
Thanks for reviewing in detail. Good point about splitting up the large patch. I've uploaded the implementation and tests as 2 separate patches: arm64_v81_cas_impl.patch arm64_v81_cas_test.patch > I am going to need some context to make sense of this... The loop in this context is not looping for the same reason guest code using a CAS instruction would loop. In this case the cbne w8, loop instruction is checking to see if the previous store exclusive worked. The spec says that store exclusives return 0 in the w register if the store updated memory, 1 if it failed to update. This way we always make sure that memory has been updated before returning from the CAS. Previously it would have returned even if the store failed. > Also, what is the reason for changing the hardwired register numbers? Adding the store check loop means we need to compare the store's status value in a w register and since w8 is the scratch register we use that, leaving w1/x1 untouched after the load. Created attachment 126176 [details]
Patch implements ARM v8.1 CAS instructions
Committed: implementation 2281c8c8675969537dc734237b60157e8a0fe0d6 test cases 3b067535056d7da12d6ca44685e696a99269585e Thank you for the patches, and apologies for the slowness in processing them. |