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/link: if there is a go1.6.3 backport the ppc64le text section size fix #15823 #16356

Closed
laboger opened this issue Jul 13, 2016 · 11 comments

Comments

Projects
None yet
5 participants
@laboger
Copy link
Contributor

commented Jul 13, 2016

Details of the original problem are described in #15823.

Opening this to request that if there is a go1.6.3 then consider the backported version of the fix for #15823 to be included, noting that the go1.6 backported version of the fix would be simpler than the fix for go 1.7.

This is an important fix because it affects the Kubernetes CI but I understand the timing may not work. I just want my request documented and considered in case it can be included.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 13, 2016

To @ianlancetaylor for decision. The text above is too indirect for me to follow easily.

@laboger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 18, 2016

I don't know if your policy is to only backport after something has been merged upstream first. That's what I mean by the timing. I don't know when my upstream fix will go in, and I don't know when you plan to release go 1.6.3.

If you would allow a fix to go into the 1.6 branch before the upstream CL would get approved and merged, I can create a CL for it.

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 19, 2016

When you wrote your comment 9 hours ago, Go 1.6.3 was already out the door.

We try not to mix security updates and bug fix updates, though. Go 1.6.3 was an exception because the macOS time fix was so small would have a large impact to many users.

We could consider something for a hypothetical Go 1.6.4 if it's important. What is the CL to backport, though?

@minux

This comment has been minimized.

Copy link
Member

commented Jul 19, 2016

@laboger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2016

Yes Minux is correct.

I'm just trying to understand the best way to make a fix for this available to Kubernetes. They want an official release for their CI. None of the potential workarounds/alternatives work (gccgo vendoring doesn't work right now; I can't get shared libraries to work on Power for the huge hyperkube application where this failure occurs.)

If there will be a point release for 1.7 in the not too distant future that might be acceptable to them since I believe they want to move to 1.7 anyway. I just thought if I could get something into a 1.6 release sooner since the fix there is a bit simpler, that would be good so that is why I opened this.

If 1.6.4 is possible I can create the CL for it (it is actually a subset of the 1.7 CL and some line numbers might be different.)

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 19, 2016

I recommend ignoring Go 1.6.x. Is there a pending CL for Go 1.7 already? We've basically already past the window for Go 1.7 changes, unless it's very clearly ppc64-only. Maybe.

@laboger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2016

I was told my CL was too risky for 1.7 and it was marked for 1.8 and I understand the concern about including such a fix into 1.7 at this point. The CL is mentioned in the original issue #15823. What options are there other than waiting for Go 1.8? I assume there will be a Go 1.7.1 but how long before that comes out.

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2016

I don't see any good solutions here.

The CL at hand (https://golang.org/cl/23963) changes a lot of general linker code in subtle ways. It's not at all PPC-specific.

A similar CL for 1.6.3 would be simpler. But it seems clearly undesirable to have a working 1.6.3 and a broken 1.7. And it's hard to believe that the CL as it stands would be safe for 1.7.1 either.

I'm open to suggestions.

@laboger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2016

The fix for 1.6 is to split the text sections. Based on the previous comments this part is not considered risky?
The fix for 1.7 is to split the text sections and handle the address offset mappings that were introduced in 1.7 for the case where the text section was split. If the text section was not split, the behavior should not be affected. If you have suggestions to the CL on how to make this less risky and/or more ppc64 specific, then I can change it so it can be considered for 1.7.1.

Another possible fix is to generate multiple .o files, such as go1.o, go2.o, etc. instead of one huge go.o with a single large text section. I don't know if that is feasible or how it would be done.

On a related note, I've been asked why this error doesn't also happen on ARM64 since they have a similar bl instruction. I don't know ARM64 well so I don't know the answer, either they have an alternative way to make the long call or they just haven't hit it yet. Is there someone who would know the answer to this question?

@bradfitz

This comment has been minimized.

Copy link
Member

commented Jul 19, 2016

If you have suggestions to the CL on how to make this less risky and/or more ppc64 specific, then I can change it so it can be considered for 1.7.1.

The barrier to point releases (1.7.x) is extremely high. We only fix security issues and bugs with no workarounds. I don't think we've ever had a case where something known broken in Go 1.n(.0), was fixed in Go 1.n.1. Our goal is not accept a point release change if it has even a plausible chance of regressions.

If there's a less risky fix, it would normally only go into Go 1.7, not a point release.

Sorry, I know this isn't happy news.

Can Docker and/or IBM and/or distros or whoever is doing the building of Docker-on-Power carry their own patch?

@laboger

This comment has been minimized.

Copy link
Contributor Author

commented Jul 19, 2016

We have a patch and have been using it internally, and will make it available externally soon. My understanding was that Kubernetes only wanted official golang versions for their CI builds so we will have to discuss with them whether they will accept a patched version.

This doesn't affect Docker but one of the binaries built during the Kubernetes CI build.

If a fix for go 1.6.4 is not possible then this can be closed. Thanks for the discussion.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
You can’t perform that action at this time.