Navigation Menu

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

cmd/asm/internal/asm: add encoding tests for all architectures #18072

Open
robpike opened this issue Nov 28, 2016 · 6 comments
Open

cmd/asm/internal/asm: add encoding tests for all architectures #18072

robpike opened this issue Nov 28, 2016 · 6 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Milestone

Comments

@robpike
Copy link
Contributor

robpike commented Nov 28, 2016

At tip as of Nov 28, 2016: The end-to-end tests in cmd/asm/internal/asm/testdata are much weaker than they used to be. I have vague memories of a change someone (Russ?) made there, but now for most architectures all that is tested is the parse, not the result of the parse. These tests should be verifying that the assembler produces correct output, not just that it accepts valid input.

@robpike robpike added this to the Go1.8 milestone Nov 28, 2016
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Nov 29, 2016
@quentinmit quentinmit modified the milestones: Go1.8Maybe, Go1.8 Nov 29, 2016
@quentinmit
Copy link
Contributor

I don't think this is a release blocker.

@robpike
Copy link
Contributor Author

robpike commented Nov 30, 2016

I disagree mildly. Some time recently the tests were weakened, and they should be restored to the strength they had in prior releases.

@rsc
Copy link
Contributor

rsc commented Dec 13, 2016

I spoke to Rob earlier. He was remembering the fact that there used to be a golden x.out next to each x.s in the testdata directory; the x.out contained the output from 'asm -S x.s'. These proved too annoying to keep in sync, so in https://golang.org/cl/18823 I changed the test to require instead that, by default, each line found in the assembly is reprinted with exactly the same syntax. That is, the result of the parse is tested and must reprint as the original input exactly. When there are multiple equivalent forms, so that the result of parse+reprint may vary, a comment in the testdata file specifies the desired printed form. For example:

arm.s:167:	STREX.S	(R2), R3 // STREX.S (R2), R3, R3

The middle operand is omitted in the input but defaults to the final one, so that's what is printed and what is expected by the test (due to the comment).

In a followup CL, https://golang.org/cl/18844 and https://golang.org/cl/18850, I added support for an optional comment giving the hex instruction encoding too, formerly not tested at all:

amd64enc.s:10560:	XORB $7, AL                             // 3407
amd64enc.s:10561:	XORW $61731, AX                         // 663523f1
amd64enc.s:10562:	XORL $4045620583, AX                    // 35674523f1
amd64enc.s:10563:	XORQ $-249346713, AX                    // 4835674523f1
amd64enc.s:10564:	XORW $61731, (BX)                       // 66813323f1
amd64enc.s:10565:	XORW $61731, (R11)                      // 6641813323f1

The two can be combined if needed, as in // printed syntax // hexbytes. That said, there appear to be no such combinations in the testdata directory that are correct. (There are some in s390x but they look wrong. I will file a separate issue.)

It is true the tests are too weak: there should be hex instruction encoding tests for all architectures, not just amd64 and s390x. But this work is unplanned.

@rsc rsc modified the milestones: Unplanned, Go1.8Maybe Dec 13, 2016
@rsc rsc changed the title cmd/asm/internal/asm: tests are too weak cmd/asm/internal/asm: add encoding tests for all architectures Dec 13, 2016
@rsc rsc removed their assignment Dec 13, 2016
@griesemer
Copy link
Contributor

On a related note, is there a test that verifies the correct pc-line info for assembly output? If not, that would also be nice to have.

@randall77
Copy link
Contributor

@griesemer We have some indirect tests. For example test/nilptr3.go tests (among other things) that line numbers are right, at least up to the nil pointer removal compiler phase.

@benshi001
Copy link
Member

After CL 45996 is merged, I thought I have made a relative full encoding test for ARM32. Some corner cases are not tested, since they use the constant pool, which is hard to calculate their offset by hand.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Development

No branches or pull requests

6 participants