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

Jump to bottom

test/codegen: wrong test cases not detected #25452

Closed
benshi001 opened this issue May 18, 2018 · 8 comments
Closed

test/codegen: wrong test cases not detected #25452

benshi001 opened this issue May 18, 2018 · 8 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Milestone
Unplanned

Comments

@benshi001
Copy link
Member

benshi001 commented May 18, 2018 •
edited

The following test case in memcombine.go is wrong, the MOVH is never generated, and still two MOVB remain there. But the codegen test framework does not detect that failure.

func zero_byte_2_idx(b []byte, idx int) {
	// arm64: `MOVH\sZR,\s\(R[0-9]+\)\(R[0-9]+<<1\)`,-`MOVB`
	b[(idx<<1)+0] = 0
	b[(idx<<1)+1] = 0
}

The failure can only be detected in the below form

func zero_byte_2_idx(b []byte, idx0 int) {
	b[(idx0<<1)+0], b[(idx0<<1)+1] = 0, 0 // arm64:`MOVH\sZR,\s\(R[0-9]+\)\(R[0-9]+<<1\)`,-`MOVB`
}
@benshi001 benshi001 added this to the Go1.11 milestone May 18, 2018
@benshi001 benshi001 self-assigned this May 18, 2018
@benshi001 benshi001 added the NeedsFix The path to resolution is known, but the work has not been done. label May 18, 2018
@gopherbot
Copy link

Change https://golang.org/cl/113736 mentions this issue: test/codegen: fix issues in test cases

@benshi001
Copy link
Member Author

cc @cherrymui @williamweixiao

@benshi001
Copy link
Member Author

cc @randall77 @rasky

@benshi001 benshi001 changed the title test/codegen: wrong test combined store cases test/codegen: wrong test cases not detected May 18, 2018
@ALTree
Copy link
Member

ALTree commented May 18, 2018 •
edited 

// arm64: `MOVH\sZR,\s\(R[0-9]+\)\(R[0-9]+<<1\)`,-`MOVB`

The test is ineffective because there's a space between the : and the start of the quoted regex. This is not allowed. Remove the whitespace and the test will fail.

codegen/memcombine.go:300: linux/arm64/: opcode not found: "^MOVH\\sZR,\\s\\(R[0-9]+\\)\\(R[0-9]+<<1\\)"
codegen/memcombine.go:300: linux/arm64/: wrong opcode found: "^MOVB"

@josharian
Copy link
Contributor

Can we trim whitespace? I can very easily see myself making that mistake—and am wondering now whether I already have.

@rasky
Copy link
Member

rasky commented May 19, 2018

Technically, it should be a syntax error, but since parsing is made with a regexp, the regexp simply doesn't match and there's no obvious way to tell a comment line in English from a command line with a syntax error -- both don't contain a regexp match. Stripping whitespace is a workaround for this specific bug, but any other syntax error can possibly cause the same problem.

I've sent a CL to strip whitespaces.

@gopherbot
Copy link

Change https://golang.org/cl/113835 mentions this issue: test: relax whitespaces matching in codegen tests

@benshi001 benshi001 assigned rasky and unassigned benshi001 May 21, 2018
@benshi001
Copy link
Member Author

Thank you. @rasky

@ianlancetaylor ianlancetaylor modified the milestones: Go1.11, Unplanned Jun 29, 2018
@golang golang locked and limited conversation to collaborators Sep 2, 2019
@rsc rsc unassigned rasky Jun 23, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done.
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
6 participants
@josharian @rasky @ianlancetaylor @ALTree @gopherbot @benshi001