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

x/arch/arm/armasm: second source register is calculated incorrectly #19100

Open
williamweixiao opened this issue Feb 15, 2017 · 6 comments
Open

x/arch/arm/armasm: second source register is calculated incorrectly #19100

williamweixiao opened this issue Feb 15, 2017 · 6 comments
Labels
Milestone

Comments

@williamweixiao
Copy link
Member

@williamweixiao williamweixiao commented Feb 15, 2017

Please answer these questions before submitting your issue. Thanks!

What version of Go are you using (go version)?

golang.org/x/arch/arm

What operating system and processor architecture are you using (go env)?

Ubuntu 16.04.1 LTS and ARM64

What did you do?

Disassemble following two instructions
937facb1
d6530d61

What did you expect to see?

STREXD.LT [R12], R4, R3, R7
LDRD.VS [SP, -R6], R6, R5

What did you see instead?

STREXD.LT [R12], R3, R3, R7
LDRD.VS [SP, -R6], R5, R5

@gopherbot
Copy link

@gopherbot gopherbot commented Feb 15, 2017

CL https://golang.org/cl/37032 mentions this issue.

@williamweixiao
Copy link
Member Author

@williamweixiao williamweixiao commented Feb 15, 2017

it's hard to expose the bug by test with gnu syntax which just print out the first source register. so i add test with go assembly syntax which print both source registers.

@rsc
Copy link
Contributor

@rsc rsc commented Feb 15, 2017

I don't believe those are valid instructions. The ARM manual I am looking at says that when Rt2 is not explicitly encoded in those instructions, Rt must be even and not R14. So +1 and |1 are the same. I think I used |1 specifically because it wouldn't look like +1 if someone did accidentally write one of those instructions. Printing the same register again is at least a hint that something is wrong, instead of silently pretending everything is OK.

Do you know something I don't? Do those instructions now accept odd Rt values as well?

@williamweixiao
Copy link
Member Author

@williamweixiao williamweixiao commented Feb 16, 2017

Yes, you are right according to ARMv7 manual.
The latest ARMv8 (i mean AArch32) manual somehow release the even-numbered constraint as below:

If Rt<0> == '1', then one of the following behaviors must occur:
• The instruction is UNDEFINED.
• The instruction executes as NOP.
• The instruction executes with the additional decode: Rt<0> = '0'.
• The instruction executes with the additional decode: t2 = t.
• The instruction executes as described, with no change to its behavior and no additional side effects.

Considering that both |1 and +1 are valid for different implementations. I suggest emitting UNDEF or INVALID for more explicit hint.

@williamweixiao
Copy link
Member Author

@williamweixiao williamweixiao commented Feb 16, 2017

I just test on Cortex-A57 (Huawei) and find that the implementation choose:

• The instruction executes with the additional decode: Rt<0> = '0'

@bradfitz bradfitz added this to the Go1.9 milestone Mar 21, 2017
@bradfitz bradfitz modified the milestones: Go1.10, Go1.9 Jun 14, 2017
@rsc
Copy link
Contributor

@rsc rsc commented Jun 26, 2017

It's fine to change this to some kind of undefined. Please don't change it to +1 or ^1 or something else silently different.

@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Dec 13, 2017
@gopherbot gopherbot modified the milestones: Go1.11, Unplanned May 23, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants