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

cmd/internal/obj/x86: invalid zoffset for ymmxmm0f38[1] #21286

Closed
quasilyte opened this issue Aug 3, 2017 · 6 comments
Closed

cmd/internal/obj/x86: invalid zoffset for ymmxmm0f38[1] #21286

quasilyte opened this issue Aug 3, 2017 · 6 comments
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Milestone
Go1.10

Comments

@quasilyte
Copy link
Contributor

quasilyte commented Aug 3, 2017 •
edited

Problem:
I think there is a bug inside cmd/internal/obj/x86/asm6.go.

Suspicious lines:

var ymmxmm0f38 = []ytab{
	{Ymm, Ynone, Ymr, Zlitm_r, 3},
	{Yxm, Ynone, Yxr, Zlitm_r, 5}, // <-- Here
}

link to sources

The zoffset field of 2-nd element of ymmxmm0f38 seems incorrect.

Here is optab item for additional context:

//                                                      Pe byte (may be important)
//                                                      |
APHADDD, ymmxmm0f38, Px, [23]uint8{0x0F, 0x38, 0x02, 0, 0x66, 0x0F, 0x38, 0x02, 0}
//                                 |                 |  |                       |
//                                 |                 |  4 bytes+Z
//                                 3 bytes+Z

link to sources

Potential solution:
5 should be replaced with 4 (If I get it right).

Severity:
This bug never triggers because zoffset does not matter
for last ytab entry.
Affects only code clarity and correctness, I guess.
@ALTree ALTree added this to the Go1.10 milestone Aug 3, 2017
@ALTree ALTree added the NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one. label Aug 3, 2017
@quasilyte
Copy link
Contributor Author

quasilyte commented Aug 4, 2017 •
edited

It may have something to do with 0x66 byte which is actually Pe byte.
Updated description above to reflect that.

Code seems overly complex, identifier names are unhelpful and inconsistent.

@ALTree
Copy link
Member

ALTree commented Aug 4, 2017

Code seems overly complex, identifier names are unhelpful and inconsistent.

IIRC it was mechanically translated from c code. Definitely not the most pleasant part of the toolchain to work on.

@quasilyte
Copy link
Contributor Author

quasilyte commented Aug 4, 2017 •
edited

Ok, it really needs "investigation", because simple expectations does not work.

// works
var ymmxmm0f38 = []ytab{
	{Ymm, Ynone, Ymr, Zlitm_r, 3}, // Does not count trailing "0"
	{Yxm, Ynone, Yxr, Zlitm_r, 5}, // Does count trailing "0"
}
{0x0F, 0x38, 0x02, 0, 0x66, 0x0F, 0x38, 0x02, 0}

// => maybe "0" is not counted?
// - does not work
var ymmxmm0f38 = []ytab{
	{Yxm, Ynone, Yxr, Zlitm_r, 4}, // Switched 2 lines; does not count trailing "0"
	{Ymm, Ynone, Ymr, Zlitm_r, 3},
}
{0x66, 0x0F, 0x38, 0x02, 0, 0x0F, 0x38, 0x02, 0} // Swapped zero-terminating byte sequences

// also does not work
var ymmxmm0f38 = []ytab{
	{Ymm, Ynone, Ymr, Zlitm_r, 4}, // Set zoffset to 4
	{Yxm, Ynone, Yxr, Zlitm_r, 5},
}
{0x0F, 0x38, 0x02, 0, 0x66, 0x0F, 0x38, 0x02, 0}

The observation is that "0" must be counted for 0x66, 0x0F, 0x38, 0x02, 0,
but not for 0x0F, 0x38, 0x02, 0. Both ytab have same zcase.
The only difference left is operands and op .

There is also odd line like xo := obj.Bool2int(o.op[0] == 0x0f) in doasm.
z += int(yt.zoffset) + xo.
First byte of {Ymm, Ynone, Ymr, Zlitm_r, 3} is 0x0f, so maybe it is the reason.

@quasilyte
Copy link
Contributor Author

quasilyte commented Aug 4, 2017 •
edited

My guess is:
"You always count all op bytes for Zlitm_r (and other 0-terminated zcases?), except 0x0f for op[0]".
If that is true, this issue can be closed because there is no bug there.

@randall77
Copy link
Contributor

Even if there is no bug, it would be really nice to condense what you learned down to some comments in the code.

@quasilyte
Copy link
Contributor Author

There is no bug.
The way ytab tables are reused are somewhat error-prone and sometimes look like invalid, but this is another issue.

@golang golang locked and limited conversation to collaborators Sep 15, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsInvestigation Someone must examine and confirm this is a valid issue and not a duplicate of an existing one.
Projects
None yet
Milestone
Go1.10
Development

No branches or pull requests
4 participants
@ALTree @quasilyte @randall77 @gopherbot