-
Notifications
You must be signed in to change notification settings - Fork 17.9k
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
Go 1.12.1 dsymutil segmentation fault on MacOS Sierra #30908
Comments
/cc @randall77 |
I've done some more work narrowing this down, and it seems that Go 1.12.1 works with the revision immediately before replicatedhq/ship@7c3cd3b but not with ones after/including that. Unfortunately, that's an absolutely massive commit... And it DOES compile with Go 1.11.6 -
|
Can you isolate the binary that is generated and on which |
I can reproduce. Additional commands I needed (in case anyone else needs to reproduce):
There's at least a bug in dsymutil. It probably shouldn't segfault, no matter the input we give it. |
Ok, I can catch the program in the act.
Despite crashing, it does generate an output directory @thanm sent me a dsymutil built from source. It succeeds, but with warnings:
I've packed up the offending file in case anyone else wants to have a look. |
Thanks for the stand-alone testcase. I will poke at it for a bit and see what I can figure out. |
Confirmed on this minimal example on Homebrew's Sierra machines. Running Mojave's dsymutil on the same example does not crash. |
Wed Mar 20 13:28:20 EDT 2019I ran dsymutil in the debugger and it does indeed look as though it is encountering bad DWARF. Offending compiliation unit is here:
It appears that the routine 'chartutil.ReadValues' is being inlined into 'chartutil.CoalesceValues'. The inlined routine subprogram DIE being generated looks like
The second formal parameter abstract_origin offset (0xd5892a) is bad -- it looks as though it's coming out somewhere in between the formal parameter DIEs in the subprogram it's referring to. So almost certainly some sort of compiler problem here. I'll grab the bug if that is ok and see about trying to come up with a compiler test case. |
Ok, thanks. |
Hmm, that's odd. When I download and compile the offending package by itself, the inline in question doesn't happen: ./values.go:134:6: cannot inline ReadValues: function too complex: cost 81 exceeds budget 80 Is there perhaps something in the build procedure that is using a higher inlining level? I am sure the bug is still worth fixing, but it would help to know if this is a case where non-standard compile flags are being used. |
Hmm - the
|
OK, sorry for the noise about compile flags -- it looks as though the inline cost is being computed slightly differently when I compile on Mac. I still haven't quite figured out what is tickling this bug yet (don't see the problem when I build the chartutil test binary). |
Hey, it's always good to check! After all, the proportion of people who are doing weird things with compiler flags is probably much higher among those who are encountering compiler bugs 😉 |
Change https://golang.org/cl/168410 mentions this issue: |
Change https://golang.org/cl/168818 mentions this issue: |
For posterity, here's what's happening with this bug. The function causing the issue is here:
This is small enough to be an inlining candidate; it gets inlined in its home package and in other packages that import that package. During the home package compile, 'vals' is promoted to the heap, and in the process the dcl node for the param updated, converting its class from PPARAMOUT to PAUTOHEAP. This happens at esc.go:2175. This change in class confuses the DWARF generation code, specifically createDwarfVars. The code there winds up treating PAUTOHEAP the same as PAUTO, hence 'vals' is reclassified as a local and not an output parameter. This has a cascading effect and means that the abstract subprogram DIE generated for chartutil.ReadValues is slightly different in its home package relative to what gets emitted in other packages that inline it. The fix is fairly straightforward in createDwarfVars(); I also wrote a separate patch to add some checking to the linker to try to detect these cases early and on Linux (at the moment it seems that we only see problems on Macos and with specific more strict versions of dsymutil). |
Introduce a new linker command line option "-strictdups", which enables sanity checking of "ok to duplicate" symbols, especially DWARF info symbols. Acceptable values are 0 (no checking) 1 (issue warnings) and 2 (issue a fatal error checks fail). Currently if we read a DWARF symbol (such as "go.info.PKG.FUNCTION") from one object file, and then encounter the same symbol later on while reading another object file, we simply discard the second one and move on with the link, since the two should in theory be identical. If as a result of a compiler bug we wind up with symbols that are not identical, this tends to (silently) result in incorrect DWARF generation, which may or may not be discovered depending on who is consuming the DWARF and what's being done with it. When this option is turned on, at the point where a duplicate symbol is detected in the object file reader, we check to make sure that the length/contents of the symbol are the same as the previously read symbol, and print a descriptive warning (or error) if not. For the time being this can be used for one-off testing to find problems; at some point it would be nice if we can enable it by default. Updates #30908. Change-Id: I64c4e07c326b4572db674ff17c93307e2eec607c Reviewed-on: https://go-review.googlesource.com/c/go/+/168410 Run-TryBot: Than McIntosh <thanm@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Keith Randall <khr@golang.org>
Change https://golang.org/cl/168957 mentions this issue: |
Thanks, @thanm. Nominating this for backport since it prevents Homebrew from packaging any Go software that happens to trigger this invalid DWARF generation. @gopherbot please consider this for backport to 1.12; it's a serious toolchain bug. |
Backport issue(s) opened: #31028 (for 1.12). Remember to create the cherry-pick CL(s) as soon as the patch is submitted to master, according to https://golang.org/wiki/MinorReleases. |
@benesch thanks for the heads-up. Backport seems reasonable. I will discuss with the other compiler/runtime folks. |
New test case designed to mimic the code in issue 30908, which features duplicate but non-indentical DWARF abstract subprogram DIEs. Updates #30908. Change-Id: Iacb4b53e6a988e46c801cdac236cef883c553f8f Reviewed-on: https://go-review.googlesource.com/c/go/+/168957 Run-TryBot: Than McIntosh <thanm@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org> Reviewed-by: Cherry Zhang <cherryyz@google.com> Reviewed-by: David Chase <drchase@google.com>
Change https://golang.org/cl/169121 mentions this issue: |
Change https://golang.org/cl/169161 mentions this issue: |
Change https://golang.org/cl/169417 mentions this issue: |
… DWARF inline gen When generating DWARF inline info records, the post-SSA code looks through the original "pre-inline" dcl list for the function so as to handle situations where formal params are promoted or optimized away. This code was not properly handling the case where an output parameter was promoted to the heap -- in this case the param node is converted in place from class PPARAMOUT to class PAUTOHEAP. This caused inconsistencies later on, since the variable entry in the abstract subprogram DIE wound up as a local and not an output parameter. Updates #30908. Fixes #31028. Change-Id: Ia70b89f0cf7f9b16246d95df17ad6e307228b8c7 Reviewed-on: https://go-review.googlesource.com/c/go/+/168818 Reviewed-by: Cherry Zhang <cherryyz@google.com> (cherry picked from commit 68a98d5) Reviewed-on: https://go-review.googlesource.com/c/go/+/169417 Run-TryBot: Cherry Zhang <cherryyz@google.com> TryBot-Result: Gobot Gobot <gobot@golang.org>
Update the issue 30908 test to work with the no-opt builder (this requires a corresponding change in the linker as well). As part of this change, 'rundir' tests are now linked without passing "-w" to the linker. Updates #30908. Fixes #31034. Change-Id: Ic776e1607075c295e409e1c8230aaf55a79a6323 Reviewed-on: https://go-review.googlesource.com/c/go/+/169161 Reviewed-by: Ian Lance Taylor <iant@golang.org>
What version of Go are you using (
go version
)?Does this issue reproduce with the latest release?
Yes
What operating system and processor architecture are you using (
go env
)?go env
OutputWhat did you do?
Run
make VERSION=0.36.0 build-minimal
with release 0.36.0 of replicatedhq/ship on MacOS Sierra. (Running on High Sierra or Mojave seems to be fine)What did you expect to see?
Compilation finishes.
What did you see instead?
/usr/local/Cellar/go/1.12/libexec/pkg/tool/darwin_amd64/link: running dsymutil failed: signal: segmentation fault
The text was updated successfully, but these errors were encountered: