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.

Sign up for GitHub

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

runtime: move tests for runtime-gdb.py to a separate package and do not run them during all.bash #39204

Open
bcmills opened this issue May 21, 2020 · 9 comments
Open

runtime: move tests for runtime-gdb.py to a separate package and do not run them during all.bash #39204

bcmills opened this issue May 21, 2020 · 9 comments
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Debugging NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Testing An issue that has been verified to require only test changes, not just a test failure.
Milestone
Backlog

Comments

@bcmills
Copy link
Contributor

bcmills commented May 21, 2020 •
edited

The tests in runtime-gdb_test.go, for the GDB bindings in runtime-gdb.py, have historically been very flaky. They are currently skipped on a large fraction of platforms due to:

If we had an incoming change to add a test with this many skips today, I suspect that we would turn it down: it tests what is supposed to be cross-platform code, but only checks the actual behavior of that code for a narrow subset of users — because that behavior does not work reliably across platforms.

But it doesn't work reliably on the remaining tested platforms either: despite all of those skips, the tests are still flaky today:

#39021 in particular describes a regression during the 1.15 cycle that is interfering with the SlowBots and TryBots on pending CLs, will likely interfere with release testing as well, and so far has resisted attempts at bisection because the test (and its failure mode) is highly nondeterministic.

If we consider the runtime “incorrect” if it does not work with gdb, then we should take the time to fix and maintain the Go bindings properly and portably — not just Skip their tests! — in the same way that we fix and maintain other features of the runtime across platforms. Otherwise, it is not appropriate to run the tests for those bindings as part of the tests for the runtime package, which is a dependency of most Go binaries (and is therefore likely to have its tests run in users' CI systems).

Most of the other Go project tests for integration with third-party tools are in the misc subdirectory. I propose that we move the gdb integration tests there.

I further propose that we should not run these tests as part of all.bash, run.bash, or on the Go project builders until such time as they can be made reliable, with only one Skip based on a check for a gdb executable at a sufficiently recent version.
@gopherbot gopherbot added this to the Proposal milestone May 21, 2020
@ianlancetaylor
Copy link
Contributor

I don't think this has to be a proposal, in that I don't think it has to go through the proposal committee. This does not affect any user visible API. I think the runtime package maintainers can decide.

@bcmills bcmills changed the title proposal: runtime: move tests for runtime-gdb.py to misc/ and do not run them during all.bash runtime: move tests for runtime-gdb.py to misc/ and do not run them during all.bash May 21, 2020
@bcmills bcmills modified the milestones: Proposal, Go1.15 May 21, 2020
@bcmills bcmills added NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Debugging Testing An issue that has been verified to require only test changes, not just a test failure. and removed Proposal labels May 21, 2020
@mundaym
Copy link
Member

mundaym commented May 25, 2020

The tests in runtime-gdb_test.go, for the GDB bindings in runtime-gdb.py ...

TestGdbBacktrace doesn't actually load the python script. It just gets GDB to execute a Go program, sets a breakpoint and triggers a backtrace. It is one of the few places where we test that the debug information in a Go executable can be consumed by a debugger like GDB.

I'm not sure what the solution to the flakiness is but I think having basic GDB support is valuable and should be tested on platforms where GDB is available. In particular I think we should continue to run the tests on the builders wherever possible - even if it is just a subset of platforms.

@bcmills
Copy link
Contributor Author

bcmills commented May 26, 2020

@mundaym

I'm not sure what the solution to the flakiness is but I think having basic GDB support is valuable and should be tested on platforms where GDB is available.

Note that we already do not test basic GDB support on most platforms for which GDB is available, due to the bugs mentioned in the first section.¹ If we agree that that support is valuable, then we should remove those skips and fix the behavior on those platforms. If we don't fix the tests when they are broken, what's the point in running them?

Either way, I still don't think these tests belong with the runtime package: they are testing integration with a third-party tool (gdb), not the Go-observable behavior of the runtime package.

¹

go/src/runtime/runtime-gdb_test.go

Lines 22 to 50 in 5c802c1

func checkGdbEnvironment(t *testing.T) {
testenv.MustHaveGoBuild(t)
switch runtime.GOOS {
case "darwin":
t.Skip("gdb does not work on darwin")
case "netbsd":
t.Skip("gdb does not work with threads on NetBSD; see https://golang.org/issue/22893 and https://gnats.netbsd.org/52548")
case "windows":
t.Skip("gdb tests fail on Windows: https://golang.org/issue/22687")
case "linux":
if runtime.GOARCH == "ppc64" {
t.Skip("skipping gdb tests on linux/ppc64; see https://golang.org/issue/17366")
}
if runtime.GOARCH == "mips" {
t.Skip("skipping gdb tests on linux/mips; see https://golang.org/issue/25939")
}
case "freebsd":
t.Skip("skipping gdb tests on FreeBSD; see https://golang.org/issue/29508")
case "aix":
if testing.Short() {
t.Skip("skipping gdb tests on AIX; see https://golang.org/issue/35710")
}
case "plan9":
t.Skip("there is no gdb on Plan 9")
}
if final := os.Getenv("GOROOT_FINAL"); final != "" && runtime.GOROOT() != final {
t.Skip("gdb test can fail with GOROOT_FINAL pending")
}
}

@ianlancetaylor
Copy link
Contributor

This didn't happen for 1.15 (and the gdb tests seem to have gotten a bit more reliable, too). Changing milestone to 1.16.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.15, Go1.16 Jun 26, 2020
@bcmills bcmills mentioned this issue Dec 8, 2020
Closed
@aclements aclements modified the milestones: Go1.16, Go1.17 Dec 8, 2020
@mengzhuo
Copy link
Contributor

mengzhuo commented Dec 9, 2020

FYI, maybe related to
https://sourceware.org/bugzilla/show_bug.cgi?id=20301

@bcmills bcmills mentioned this issue Jan 21, 2021
Open
@changkun
Copy link
Member

I just encountered one on tip:

##### GOMAXPROCS=2 runtime -cpu=1,2,4 -quick
--- FAIL: TestGdbPythonCgo (2.21s)
    runtime-gdb_test.go:76: gdb version 9.2
    runtime-gdb_test.go:269: gdb output:
        Loading Go Runtime support.
        Loading Go Runtime support.
        Breakpoint 1 at 0x49a289: file /tmp/go-build247077086/main.go, line 27.
        [Thread debugging using libthread_db enabled]
        Using host libthread_db library "/lib/x86_64-linux-gnu/libthread_db.so.1".
        hi
        
        Thread 1 "a.exe" hit Breakpoint 1, main.main () at /tmp/go-build247077086/main.go:27
        27              gslice = slicevar
        BEGIN info goroutines
        * 1 running  syscall.Syscall
          2 waiting  runtime.gopark
          3 waiting  runtime.gopark
          4 waiting  runtime.gopark
          18 waiting  runtime.gopark
        END
        BEGIN print mapvar
        $1 = map[string]string = {["abc"] = "def", ["ghi"] = "jkl"}
        END
        BEGIN print slicemap
        $2 = map[string][]string = {["e"] = []string = {"f", "g", "h"}, ["a"] = []string = {"b", "c", "d"}}
        END
        BEGIN print strvar
        $3 = "abc"
        END
        BEGIN print chanint
        $4 = chan int = {99, 11}
        END
        BEGIN print chanstr
        $5 = chan string = {"spongepants", "squarebob"}
        END
        BEGIN info locals
        mapvar = map[string]string = {["abc"] = "def", ["ghi"] = "jkl"}
        chanstr = chan string = {"spongepants", "squarebob"}
        chanint = chan int = {99, 11}
        strvar = "abc"
        slicemap = map[string][]string = {["e"] = []string = {"f", "g", "h"}, ["a"] = []string = {"b", "c", "d"}}
        ptrvar = <optimized out>
        slicevar = []string
        END
        BEGIN goroutine 1 bt
        #0  main.main () at /tmp/go-build247077086/main.go:27
        END
        BEGIN goroutine all bt
        #0  main.main () at /tmp/go-build247077086/main.go:27
        No such goroutine:  17
        #0  runtime.gopark (unlockf={void (runtime.g *, void *, bool *)} 0xc00003afb0, lock=0x554130 <runtime.forcegc>, reason=17 '\021', traceEv=20 '\024', traceskip=1) at /home/changkun/dev/godev/go-gerrit/src/runtime/proc.go:337
        #1  0x00000000004385e5 in runtime.goparkunlock (lock=<optimized out>, reason=<optimized out>, traceEv=<optimized out>, traceskip=<optimized out>) at /home/changkun/dev/godev/go-gerrit/src/runtime/proc.go:342
        #2  runtime.forcegchelper () at /home/changkun/dev/godev/go-gerrit/src/runtime/proc.go:276
        #3  0x0000000000464e61 in runtime.goexit () at /home/changkun/dev/godev/go-gerrit/src/runtime/asm_amd64.s:1445
        #4  0x0000000000000000 in ?? ()
        #0  runtime.gopark (unlockf={void (runtime.g *, void *, bool *)} 0xc00003b7a8, lock=0x554280 <runtime.sweep>, reason=12 '\f', traceEv=20 '\024', traceskip=1) at /home/changkun/dev/godev/go-gerrit/src/runtime/proc.go:337
        #1  0x0000000000423e88 in runtime.goparkunlock (lock=<optimized out>, reason=<optimized out>, traceEv=<optimized out>, traceskip=<optimized out>) at /home/changkun/dev/godev/go-gerrit/src/runtime/proc.go:342
        #2  runtime.bgsweep (c=chan int) at /home/changkun/dev/godev/go-gerrit/src/runtime/mgcsweep.go:163
        #3  0x0000000000464e61 in runtime.goexit () at /home/changkun/dev/godev/go-gerrit/src/runtime/asm_amd64.s:1445
        #4  0x000000c00001c070 in ?? ()
        #5  0x0000000000000000 in ?? ()
        #0  runtime.gopark (unlockf={void (runtime.g *, void *, bool *)} 0xc00003bf78, lock=0x554320 <runtime.scavenge>, reason=13 '\r', traceEv=20 '\024', traceskip=1) at /home/changkun/dev/godev/go-gerrit/src/runtime/proc.go:337
        #1  0x0000000000421d36 in runtime.goparkunlock (lock=<optimized out>, reason=<optimized out>, traceEv=<optimized out>, traceskip=<optimized out>) at /home/changkun/dev/godev/go-gerrit/src/runtime/proc.go:342
        #2  runtime.bgscavenge (c=chan int) at /home/changkun/dev/godev/go-gerrit/src/runtime/mgcscavenge.go:265
        #3  0x0000000000464e61 in runtime.goexit () at /home/changkun/dev/godev/go-gerrit/src/runtime/asm_amd64.s:1445
        #4  0x000000c00001c070 in ?? ()
        #5  0x0000000000000000 in ?? ()
        #0  runtime.gopark (unlockf={void (runtime.g *, void *, bool *)} 0xc00003a740, lock=0x581530 <runtime.finlock>, reason=16 '\020', traceEv=20 '\024', traceskip=1) at /home/changkun/dev/godev/go-gerrit/src/runtime/proc.go:337
        #1  0x0000000000418c66 in runtime.goparkunlock (lock=<optimized out>, reason=<optimized out>, traceEv=<optimized out>, traceskip=<optimized out>) at /home/changkun/dev/godev/go-gerrit/src/runtime/proc.go:342
        #2  runtime.runfinq () at /home/changkun/dev/godev/go-gerrit/src/runtime/mfinal.go:177
        #3  0x0000000000464e61 in runtime.goexit () at /home/changkun/dev/godev/go-gerrit/src/runtime/asm_amd64.s:1445
        #4  0x0000000000000000 in ?? ()
        END
        No breakpoint at main.go:15.
        Breakpoint 2 at 0x49a41f: file /tmp/go-build247077086/main.go, line 30.
    runtime-gdb_test.go:271: gdb exited with error: signal: aborted (core dumped)
FAIL
FAIL    runtime 12.139s
FAIL
2021/03/19 08:47:46 Failed: exit status 1
go tool dist: FAILED

@ianlancetaylor
Copy link
Contributor

Moving to Backlog.

@ianlancetaylor ianlancetaylor modified the milestones: Go1.17, Backlog Apr 28, 2021
@agarciamontoro agarciamontoro mentioned this issue Oct 8, 2021
Closed
@bcmills bcmills mentioned this issue Mar 17, 2022
Closed
Open
@bcmills bcmills added the compiler/runtime Issues related to the Go compiler and/or runtime. label May 30, 2023
@bcmills
Copy link
Contributor Author

bcmills commented Jun 1, 2023 •
edited

@golang/runtime, can you please take another look at this in triage? I keep hitting #58698 in TryBots.

I expect that external Go users may hit these test flakes in their own projects when running go test all (because every go project depends on the runtime package), and go test all ought to be a reasonable thing for them to do. Given how flaky the GDB tests are, I would really rather not have them standing in the way of Go users improving their testing practices.

@gopherbot
Copy link

Change https://go.dev/cl/499975 mentions this issue: runtime: update skips for TestGdbBacktrace

gopherbot pushed a commit that referenced this issue Jun 1, 2023
@prattmic @gopherbot
runtime: update skips for TestGdbBacktrace
d045976 
One issue simply has a reworded message, probably from a new version of
GDB. Another is a new issue.

Fixes #60553.
Fixes #58698.
Updates #39204.

Change-Id: I8389aa981fab5421f57ee761bfb5e1dd237709ed
Reviewed-on: https://go-review.googlesource.com/c/go/+/499975
Reviewed-by: Bryan Mills <bcmills@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
Auto-Submit: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Austin Clements <austin@google.com>
@bcmills bcmills changed the title runtime: move tests for runtime-gdb.py to misc/ and do not run them during all.bash runtime: move tests for runtime-gdb.py to a separate package and do not run them during all.bash Jul 21, 2023
@bcmills bcmills mentioned this issue Jan 2, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
compiler/runtime Issues related to the Go compiler and/or runtime. Debugging NeedsDecision Feedback is required from experts, contributors, and/or the community before a change can be made. Testing An issue that has been verified to require only test changes, not just a test failure.
Projects
Go Compiler / Runtime
Status: Todo
Milestone
Backlog
Development

No branches or pull requests
7 participants
@mengzhuo @aclements @ianlancetaylor @bcmills @changkun @gopherbot @mundaym