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/compile: all.bash failures with -dwarflocationlists hard-wired on #22694

Closed
thanm opened this issue Nov 13, 2017 · 7 comments

Comments

Projects
None yet
4 participants
@thanm
Copy link
Member

commented Nov 13, 2017

What version of Go are you using (go version)?

tip:
go version devel +0cee4b7b78 Mon Nov 13 18:23:38 2017 +0000 linux/amd64

Does this issue reproduce with the latest release?

Yes

What operating system and processor architecture are you using (go env)?

GOARCH="amd64"
GOHOSTARCH="amd64"
GOHOSTOS="linux"
GOOS="linux"

What did you do?

If I edit cmd/compile/internal/gc/main.go to change the default for -dwarflocationlsts from false to true, I get a number of failures in all.bash.

What did you expect to see?

passing all.bash

What did you see instead?

Specifically of interest, a couple of the SSA tests fail. Example:

--- FAIL: TestGenFlowGraph (0.52s)
	ssa_test.go:82: Failed: exit status 2:
		Out: 
		Stderr: # command-line-arguments
		/tmp/ssa_fg_tmp1041688254/run.go:93:10: internal compiler error: phi after non-phi @ b5: v14

and others. My understanding is that we want to eventually make -dwarflocationlists the default, in which case it seems as though these failures should be looked at.

@heschik heschik added the Debugging label Nov 13, 2017

@bradfitz bradfitz changed the title all.bash failures with -dwarflocationlists hard-wired on cmd/compile: all.bash failures with -dwarflocationlists hard-wired on Nov 13, 2017

@thanm

This comment has been minimized.

Copy link
Member Author

commented Nov 14, 2017

I did a little archeology on this. The block that the checker is complaining about is:

  b11: <- b14 b10
    v48 = RegKill <void> : R9 (next[int64])
    v82 = Phi <int64> v41 v56 : R9 (next[int64])
    v47 = RegKill <void> : DX (x[int64])
    v50 = SARQconst <int64> [1] v83 : DX (x[int64])
    Plain -> b9

which clearly violates the "no non-phi after phi rule", but in fact this specific testpoint wasn't added until October, meaning that the locations lists work predates the test (the location lists code was written back in June(ish), then the dev.debug branch was merged into master in August).

I'm not familiar enough with the SSA code to know what the right fix is. One thing to do would just be to relax the checking rules to ignore RegKills, since it is pretty clear that they don't correspond to any sort of real value definition.

David, maybe you could weigh in on this. @dr2chase

@heschik

This comment has been minimized.

Copy link
Contributor

commented Nov 14, 2017

Thanks. This is working as I intended at the time, and the test could probably be updated to ignore the RegKill ops. Phis put values in registers like anything else, and if one clobbers something that was already in the register before that, there should be a RegKill before it. That was a change in the invariants of the SSA form, but it didn't cause much trouble.

In this particular case, since the set of names that's being set on the registers by the Phis match the ones being removed by the RegKills, they're pointless. But I'm pretty sure there are cases where the set of names differ, and then they are necessary.

@gopherbot

This comment has been minimized.

Copy link

commented Nov 14, 2017

Change https://golang.org/cl/77610 mentions this issue: cmd/compile: ignore RegKill ops for non-phi after phi check

@thanm

This comment has been minimized.

Copy link
Member Author

commented Nov 15, 2017

Beyond the SSA sanity checker (which should be fixed with 77610) there are also a couple of GDB test failures:

--- FAIL: TestGdbPython (2.16s)
	runtime-gdb_test.go:55: gdb version 7.9
	runtime-gdb_test.go:210: print strvar failed: $2 = <optimized out>
--- FAIL: TestGdbPythonCgo (2.54s)
	runtime-gdb_test.go:55: gdb version 7.9
	runtime-gdb_test.go:210: print strvar failed: $2 = <optimized out>

It looks like these two above will go away once my DWARF inlining patch is submitted. After that, there are a set of failures of the following form:

--- FAIL: TestAssembly (2.78s)
    --- FAIL: TestAssembly/platform (0.00s)
        --- FAIL: TestAssembly/platform/linux/mips (0.12s)
        	asm_test.go:213: error running cmd: exit status 2
        		stdout:
        		stderr:
        		# math/bits
        		compile: location lists requested but register mapping not available on mips

These are due to this line in cmd/compile/internal/gc/main.go:

	if Ctxt.Flag_locationlists && len(Ctxt.Arch.DWARFRegisters) == 0 {
		log.Fatalf("location lists requested but register mapping not available on %v", Ctxt.Arch.Name)
	}

meaning someone needs to create fill in the DWARF registers for the various non-x86 archs.

@heschik

This comment has been minimized.

Copy link
Contributor

commented Nov 15, 2017

Huh. Interesting that the inlining patch would fix the GDB test. There must be some strange stuff going on with the name table.

gopherbot pushed a commit that referenced this issue Nov 21, 2017

cmd/compile: ignore RegKill ops for non-phi after phi check
Relax the 'phi after non-phi' SSA sanity check to allow
RegKill ops interspersed with phi ops in a block. This fixes
a sanity check failure when -dwarflocationlists is enabled.

Updates #22694.

Change-Id: Iaae604ab6f1a8b150664dd120003727a6fb2f698
Reviewed-on: https://go-review.googlesource.com/77610
Run-TryBot: Than McIntosh <thanm@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: David Chase <drchase@google.com>
@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2018

The tests mentioned above all seem to pass when run with -gcflags=-dwarflocationlists. Is this still a problem?

@ianlancetaylor ianlancetaylor added this to the Go1.11 milestone Mar 29, 2018

@thanm

This comment has been minimized.

Copy link
Member Author

commented Mar 29, 2018

Thanks Ian-- yes, I agree this can be closed out.

@golang golang locked and limited conversation to collaborators Mar 29, 2019

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.