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

gccgo: MemmoveAtomicity fails on ppc64x since it uses C's memmove #41428

Open
laboger opened this issue Sep 16, 2020 · 8 comments
Open

gccgo: MemmoveAtomicity fails on ppc64x since it uses C's memmove #41428

laboger opened this issue Sep 16, 2020 · 8 comments
Milestone

Comments

@laboger
Copy link
Contributor

@laboger laboger commented Sep 16, 2020

In the testing of gccgo runtime package on ppc64le and ppc64 we are seeing this error:

[New Thread 0x7fffbeb8eff0 (LWP 166699)]
--- FAIL: TestMemmoveAtomicity (0.01s)
    --- FAIL: TestMemmoveAtomicity/24-backward (0.00s)
        memmove_test.go:267: got partially updated pointer 0xc000000000 at dst[0], want either nil or 0xc00028a088
    --- FAIL: TestMemmoveAtomicity/32-backward (0.00s)
        memmove_test.go:267: got partially updated pointer 0xc000000000 at dst[0], want either nil or 0xc00028a088
    --- FAIL: TestMemmoveAtomicity/48-backward (0.00s)
        memmove_test.go:267: got partially updated pointer 0xc000000000 at dst[0], want either nil or 0xc00028a088
    --- FAIL: TestMemmoveAtomicity/24-forward (0.00s)
        memmove_test.go:267: got partially updated pointer 0x28a088 at dst[2], want either nil or 0xc00028a088
FAIL

This has been happening since the addition of the MemmoveAtomicity test, but has been hidden because of a segv in other runtime tests which prevented all the runtime tests from running. For some reason this now shows up intermittently in the libgo.log.

According to this CL https://go-review.googlesource.com/c/go/+/213418/ it is understood that while Go's memmove copies pointers atomically, but C's memmove does not have that guarantee, and gccgo is using libc's memmove at least on ppc64le/ppc64.

@gopherbot gopherbot added this to the Gccgo milestone Sep 16, 2020
@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Sep 16, 2020

That's unfortunate. It is somewhat surprising that C memmove breaks word boundary, even the thing we move is a multiple of word size and well aligned...

@laboger
Copy link
Contributor Author

@laboger laboger commented Sep 18, 2020

I'm not really involved with what goes into glibc for each ISA and distro. Sometimes for a distro they want to chose a version that will work on multiple ISAs, and different ISAs might have different alignment requirements so that all affects its implementation. However the main point is that libc's memmove has no guarantee of atomicity of pointer copies so when it is implemented that is not taken into consideration. Having said that, I looked at the Go language definition and I don't see where it mentions that pointers are always aligned either. So couldn't there be a pointer within a struct that is unaligned because of the size of the previous structure members? I'm looking at the Go Programming Language document, at the very end where it says "size and alignment guarantees".

@randall77
Copy link
Contributor

@randall77 randall77 commented Sep 18, 2020

We don't provide any alignment guarantees on pointers to the user.
We do require it internally, though. Pointers must be naturally aligned. Struct layout will insert padding if the field before a pointer does not end at pointer alignment. (And a struct containing a pointer must be pointer-aligned as well.)

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Sep 18, 2020

It is deeply baked in the runtime that pointers are aligned. If it is not, everything will go wrong. The compiler ensures pointers are aligned.

@laboger
Copy link
Contributor Author

@laboger laboger commented Oct 13, 2020

@ianlancetaylor Would it be possible to add (power) asm implementations of memove and memclr to gccgo that are similar to those implementations in golang?

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 13, 2020

Sure, it's possible. We have one piece of assembler code already in runtime/go-context.S. Of course it will be necessary to change the frontend to call those functions where appropriate, which may mean losing some compiler optimizations.

That said, I haven't looked into this issue, and I'm somewhat flabbergasted that the C library memmove function doesn't do the right thing. As far as I can see the natural way to write memmove on PPC64 would copy a pointer at a time. Any idea why that doesn't happen?

@cherrymui
Copy link
Contributor

@cherrymui cherrymui commented Oct 13, 2020

I'm somewhat flabbergasted that the C library memmove function doesn't do the right thing. As far as I can see the natural way to write memmove on PPC64 would copy a pointer at a time.

This is my impression as well. Kind of surprising.

Of course it will be necessary to change the frontend to call those functions where appropriate, which may mean losing some compiler optimizations.

Maybe it is possible to play some symbol shadowing trick? In the frontend we still lower to __builtin_memmove and when the backend decides to generate an actual memmove call, our symbol shadows the libc symbol? Not sure if it would work. Also not sure about all build modes... (Kind of ugly anyway.)

@laboger
Copy link
Contributor Author

@laboger laboger commented Oct 19, 2020

That said, I haven't looked into this issue, and I'm somewhat flabbergasted that the C library memmove function doesn't do the right thing. As far as I can see the natural way to write memmove on PPC64 would copy a pointer at a time. Any idea why that doesn't happen?

There is no requirement that the memmove in libc ensure pointer atomicity, so they wouldn't have tested for it. On previous ISAs the alignment requirements varied, so that moving 8-bytes required certain alignment either for performance or correctness. In the memmove for the distro they wanted an implementation that would work for several instruction sets. In some cases doing 2 4-byte moves was as good as an 8-byte move if they didn't have to check for alignment.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
6 participants
You can’t perform that action at this time.