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/compile: longtest builder failing on TestNexting/gdb-opt-hist #31786

Closed
randall77 opened this issue May 1, 2019 · 9 comments
Closed

cmd/compile: longtest builder failing on TestNexting/gdb-opt-hist #31786

randall77 opened this issue May 1, 2019 · 9 comments
Labels
FrozenDueToAge Soon This needs to be done soon. (regressions, serious bugs, outages)
Milestone
Go1.13

Comments

@randall77
Copy link
Contributor

After CL 174617, the longtest builder is failing on TestNexting/gdb-opt-hist.
Here's a sample of the log:

--- FAIL: TestNexting (13.35s)
    --- FAIL: TestNexting/gdb-opt-hist (2.98s)
        debug_test.go:245: step/next histories differ, diff=
            --- testdata/hist.gdb-opt.nexts	2019-04-30 21:30:30.000000000 +0000
            +++ /workdir/tmp/debug_test382328161/test-hist.gdb-opt.nexts	2019-04-30 22:37:43.423960976 +0000
            @@ -28,8 +28,6 @@
             i = 1
             81:			hist = ensure(int(i), hist)
             82:			hist[int(i)]++
            -74:		for scanner.Scan() { //gdb-opt=(scanner/A)
            -scanner = (bufio.Scanner *) <A>
             75:			s := scanner.Text()
             76:			i, err := strconv.ParseInt(s, 10, 64)
             77:			if err != nil { //gdb-dbg=(i) //gdb-opt=(err,hist,i)
            @@ -38,8 +36,6 @@
             i = 1
             81:			hist = ensure(int(i), hist)
             82:			hist[int(i)]++
            -74:		for scanner.Scan() { //gdb-opt=(scanner/A)
            -scanner = (bufio.Scanner *) <A>
             75:			s := scanner.Text()
             76:			i, err := strconv.ParseInt(s, 10, 64)
             77:			if err != nil { //gdb-dbg=(i) //gdb-opt=(err,hist,i)
            @@ -48,8 +44,6 @@
             i = 1
             81:			hist = ensure(int(i), hist)
             82:			hist[int(i)]++
            -74:		for scanner.Scan() { //gdb-opt=(scanner/A)
            -scanner = (bufio.Scanner *) <A>
             75:			s := scanner.Text()

When I compile src/cmd/compile/internal/ssa/testdata/hist.go both before and after this change, with both -S and -S=2, I don't see any line number differences. This is the extent of the difference of the -S output:

$ diff old new
1611c1611
< 	0x0020 02 07 01 0e 02 12 01 6e 02 06 01 1c 02 08 01 04  .......n........
---
> 	0x0020 02 07 01 0e 02 12 01 6e 02 02 01 20 02 08 01 04  .......n... ....

This difference is part of the go.isstmt."".test SDWARFMISC size=0 symbol.
So somehow my change messed up the statement marks? Is there a way to display those differences?

@dr2chase , any ideas?
@randall77 randall77 added the Soon This needs to be done soon. (regressions, serious bugs, outages) label May 1, 2019
@randall77 randall77 added this to the Go1.13 milestone May 1, 2019
@bradfitz
Copy link
Contributor

bradfitz commented May 1, 2019

Dup of #31263?

@dr2chase
Copy link
Contributor

dr2chase commented May 1, 2019 •
edited

Working on it...

go test debug_test.go -v -args  -f -n

produces

...
=== RUN   TestNexting/gdb-opt-hist
( /Users/drchase/work/go-quick/bin/go build -o testdata/test-hist.gdb-opt '-gcflags=all= -l' -ldflags=-compressdwarf=false testdata/hist.go )
( TERM=dumb gdb -nx -iex 'add-auto-load-safe-path /Users/drchase/work/go-quick/src/runtime' -ex 'set startup-with-shell off' testdata/test-hist.gdb-opt )
tbreak main.test
run
# Tag for above is gdb-opt
...

Redo good and bad compilations prepending GOSSAFUNC=test to both of them.

Also compare dwarfdump -debug-line for both, search forward for testdata:
"good"

..................     74      0      2   0             0  is_stmt
..................     82      0      2   0             0  is_stmt
..................     82      0      2   0             0 
..................     81      0      2   0             0 
..................     74      0      2   0             0 
..................     81      0      2   0             0 
..................     74      0      2   0             0  is_stmt
..................     74      0      2   0             0

"bad"

..................     74      0      2   0             0  is_stmt
(missing)
..................     82      0      2   0             0 
..................     81      0      2   0             0 
..................     74      0      2   0             0 
..................     81      0      2   0             0 
..................     74      0      2   0             0  is_stmt
..................     74      0      2   0             0

It works okay with delve, I am still searching through the ssa output to see where the difference appears. It's in blocks 17 and 19, v264 and v275.

edit: The problem is in the original assignment of statement marks (numberlines).

@dr2chase
Copy link
Contributor

dr2chase commented May 3, 2019

I have a CL coming that doesn't fix this, but it at least stops us from relying on dumb luck to make gdb happy. I have not yet figured out what would make gdb happy; a naive reading of the marked lines here (the loop in question, after coming CL) shows more than enough is_stmt=1 lines.

Screen Shot 2019-05-02 at 8 03 27 PM

@gopherbot
Copy link

Change https://golang.org/cl/174948 mentions this issue: cmd/compile: make numberlines line mismatch check ignore columns

gopherbot pushed a commit that referenced this issue May 6, 2019
@dr2chase
cmd/compile: make numberlines line mismatch check ignore columns
d199369 
This does not repair #31786, and in fact also unfixes the revert
of CL 174617.  We were just getting lucky when it looked like
it was working.  And unfortunately for the bug, there does not
appear to be any particular problems with the line numbers;
if anything they're a couple of extras, i.e., stepping might
repeat, rather than skip.  Delve works fine either way.

Updates #31786.

Change-Id: I5c2fdc2a0265bb99773b3a85492a3db557dffee4
Reviewed-on: https://go-review.googlesource.com/c/go/+/174948
Run-TryBot: David Chase <drchase@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@bcmills
Copy link
Contributor

bcmills commented May 8, 2019

If the fix isn't going to land today, please add a skip so that the longtest builder will be able to proceed past TestNexting again. (The build dashboard is still unpleasantly red and I don't want to mask any more regressions.)

@dr2chase
Copy link
Contributor

dr2chase commented May 8, 2019

How do you feel about me changing it to use Delve instead?

@bradfitz
Copy link
Contributor

bradfitz commented May 8, 2019

@dr2chase, you're asking the wrong people. We don't know what this is testing. We just know it's red & harmful to people trying to get a useful regression signal out of this builder, and making people get used to broken builds.

@dr2chase
Copy link
Contributor

dr2chase commented May 8, 2019

Should have a fix (switch to delve, tested for 100+ repetitions on Linux and Mac) later today.
Tests running now will I had off to a talk.

@gopherbot
Copy link

Change https://golang.org/cl/176058 mentions this issue: cmd/compile: test delve instead of gdb in ssa/debug_test.go

@golang golang locked and limited conversation to collaborators May 8, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Soon This needs to be done soon. (regressions, serious bugs, outages)
Projects
None yet
Milestone
Go1.13
Development

No branches or pull requests
5 participants
@bradfitz @dr2chase @bcmills @randall77 @gopherbot