gocore: explicitly load symbols from executable ELF file This fixes partial typing fails when the core in question is complete (meaning: no section has Filesz != Memsz). Such cores can be triggered on Linux by writing (see core(5)): echo 0x3f > /proc/self/coredump_filter In this case, none of the mem mappings are backed by the executable file on disk. This meant that the previous readSymbols implementation only reads the core file (which it ignored anyway). Instead of looping over the mappings, read the symbols explicitly from the executable. I added a test which fails before my change. I had to refactor the testing code a little bit to make it easier to pass an environment variable. Unfortunately, changing /proc/self/coredump_filter means writing to a file. Pulling in a dependency on package os makes the dominator tests panic. For now I've disabled these tests: we're not sure it worked well anyway (it must have crashed for non-trivial programs). There may need to be more improvements in general typing/walking before re-enabling it. In principle, the dominator failure could be partially avoided by using build tags and not running the dominator test when coredump_filter doesn't need to be manipulated. But given what I wrote before, I don't see why we should bother. The history of this is interesting: - https://go.dev/cl/137375 is the last good change to this part of the code. It reads symbols from all loaded executables. Its CL description mentions that this is required for PIE and mixed (e.g.: Go/C++ binaries). Yet the PIE and (internal) mixed binary tests continue to pass with this change: - I found that PIE support didn't work and added it in https://go.dev/cl/618977 (with tests). Perhaps this is a reference to PIE support that was somehow removed in-between. - viewcore does not explicitly support mixed binaries, in the sense that it does not (attempt) to understand C objects. The PIE tests - https://go.dev/cl/506558 introduced the full core bug by replacing the read from all executable files with an iteration of the mappings. Change-Id: I2538cd863da72a9ebfc9415b32a97bf962479b61 Reviewed-on: https://go-review.googlesource.com/c/debug/+/637415 LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com> Auto-Submit: Michael Knyszek <mknyszek@google.com> Reviewed-by: Michael Knyszek <mknyszek@google.com>
diff --git a/internal/core/process.go b/internal/core/process.go index 92bf9b3..d7c000c 100644 --- a/internal/core/process.go +++ b/internal/core/process.go
@@ -273,7 +273,10 @@ return nil, fmt.Errorf("error reading args: %v", err) } - syms, symErr := readSymbols(&mem, coreFile, staticBase) + syms, symErr := readSymbols(staticBase, exeElf) + if symErr != nil { + symErr = fmt.Errorf("%v: from file %s", symErr, exeFile.Name()) + } dwarf, dwarfErr := exeElf.DWARF() if dwarfErr != nil { @@ -735,42 +738,29 @@ return threads } -func readSymbols(mem *splicedMemory, coreFile *os.File, staticBase uint64) (map[string]Address, error) { - seen := map[*os.File]struct{}{ - // Don't bother trying to read symbols from the core itself. - coreFile: struct{}{}, - } - +// readSymbols loads all symbols from the SHT_SYMTAB section of the executable +// file. +// +// TODO(aktau): Should we read symbols from the files underlying all available +// executable mappings? This used to be done (see e.g.: +// https://go.dev/cl/137375) but currently viewcore supports PIE and mixed +// binaries without needing to read multiple files. +// +// NOTE: The core file itself does not contain a symbols section (SHT_SYMTAB), +// so we don't read from it. +func readSymbols(staticBase uint64, exeElf *elf.File) (map[string]Address, error) { allSyms := make(map[string]Address) - var symErr error - // Read symbols from all available files. - for _, m := range mem.mappings { - if m.f == nil { - continue - } - if _, ok := seen[m.f]; ok { - continue - } - seen[m.f] = struct{}{} - - e, err := elf.NewFile(m.f) - if err != nil { - symErr = fmt.Errorf("can't read symbols from %s: %v", m.f.Name(), err) - continue - } - - syms, err := e.Symbols() - if err != nil { - symErr = fmt.Errorf("can't read symbols from %s: %v", m.f.Name(), err) - continue - } - for _, s := range syms { - allSyms[s.Name] = Address(s.Value).Add(int64(staticBase)) - } + syms, err := exeElf.Symbols() + if err != nil { + return allSyms, fmt.Errorf("can't read symbols from main executable: %v", err) } - return allSyms, symErr + for _, s := range syms { + allSyms[s.Name] = Address(s.Value).Add(int64(staticBase)) + } + + return allSyms, nil } func (p *Process) Warnings() []string {
diff --git a/internal/gocore/gocore_test.go b/internal/gocore/gocore_test.go index 02fdece..ac0ace5 100644 --- a/internal/gocore/gocore_test.go +++ b/internal/gocore/gocore_test.go
@@ -8,6 +8,7 @@ import ( "bytes" + "cmp" "errors" "fmt" "os" @@ -37,7 +38,7 @@ // loadExampleGenerated generates a core from a binary built with // runtime.GOROOT(). -func loadExampleGenerated(t *testing.T, buildFlags ...string) *Process { +func loadExampleGenerated(t *testing.T, buildFlags, env []string) *Process { t.Helper() testenv.MustHaveGoBuild(t) switch runtime.GOOS { @@ -56,7 +57,7 @@ } dir := t.TempDir() - file, output, err := generateCore(dir, buildFlags...) + file, output, err := generateCore(dir, buildFlags, env) t.Logf("crasher output: %s", output) if err != nil { t.Fatalf("generateCore() got err %v want nil", err) @@ -137,15 +138,6 @@ return nil } -// runCrasher spawns exe via [doRunCrasher] with wd as working directory. -// GOTRACEBACK=crash is set. -func runCrasher(exe, wd string) (pid int, output []byte, err error) { - cmd := exec.Command(exe) - cmd.Env = append(os.Environ(), "GOMAXPROCS=2", "GOTRACEBACK=crash") - cmd.Dir = wd - return doRunCrasher(cmd) -} - // doRunCrasher spawns the supplied cmd, propagating parent state (see // [exec.Cmd.Run]), and returns an error if the process failed to start or did // *NOT* crash. @@ -166,7 +158,7 @@ return cmd.Process.Pid, b.Bytes(), nil } -func generateCore(dir string, buildFlags ...string) (string, []byte, error) { +func generateCore(dir string, buildFlags, env []string) (string, []byte, error) { goTool, err := testenv.GoTool() if err != nil { return "", nil, fmt.Errorf("cannot find go tool: %w", err) @@ -190,7 +182,11 @@ return "", nil, fmt.Errorf("error building crasher: %w\n%s", err, string(b)) } - _, b, err = runCrasher("./test.exe", dir) + cmd = exec.Command("./test.exe") + cmd.Env = append(os.Environ(), "GOMAXPROCS=2", "GOTRACEBACK=crash") + cmd.Env = append(cmd.Env, env...) + cmd.Dir = dir + _, b, err = doRunCrasher(cmd) if err != nil { return "", b, err } @@ -226,20 +222,45 @@ t.Errorf("stat[%q].Size == 0, want >0", heapName) } - lt := runLT(p) - if !checkDominator(t, lt) { - t.Errorf("sanityCheckDominator(...) = false, want true") + // TODO(aktau): Adding package os to the test binary causes the dominator test + // to panic. We suspect the dominator code may not be working right even if it + // doesn't crash. This needs a fixup and dedicated tests at a later time. + if false { + lt := runLT(p) + if !checkDominator(t, lt) { + t.Errorf("sanityCheckDominator(...) = false, want true") + } } } +type parameters struct { + buildFlags []string + env []string +} + +func (p parameters) String() string { + var parts []string + if len(p.buildFlags) != 0 { + parts = append(parts, "gcflags="+strings.Join(p.buildFlags, ",")) + } + if len(p.env) != 0 { + parts = append(parts, "env="+strings.Join(p.env, ",")) + } + return cmp.Or(strings.Join(parts, "%"), "DEFAULT") +} + +// Variations in build and execution environments common to different tests. +var variations = [...]parameters{ + {}, // Default. + {buildFlags: []string{"-buildmode=pie"}}, + {buildFlags: []string{"-buildmode=pie"}, env: []string{"GO_DEBUG_TEST_COREDUMP_FILTER=0x3f"}}, +} + func TestVersions(t *testing.T) { t.Run("goroot", func(t *testing.T) { - for _, buildFlags := range [][]string{ - nil, - {"-buildmode=pie"}, - } { - t.Run(strings.Join(buildFlags, ","), func(t *testing.T) { - p := loadExampleGenerated(t, buildFlags...) + for _, test := range variations { + t.Run(test.String(), func(t *testing.T) { + p := loadExampleGenerated(t, test.buildFlags, test.env) checkProcess(t, p) }) } @@ -248,14 +269,11 @@ func TestObjects(t *testing.T) { t.Run("goroot", func(t *testing.T) { - for _, buildFlags := range [][]string{ - nil, - {"-buildmode=pie"}, - } { - t.Run(strings.Join(buildFlags, ","), func(t *testing.T) { + for _, test := range variations { + t.Run(test.String(), func(t *testing.T) { const largeObjectThreshold = 32768 - p := loadExampleGenerated(t, buildFlags...) + p := loadExampleGenerated(t, test.buildFlags, test.env) // Statistics to check. n := 0
diff --git a/internal/gocore/testdata/coretest/test.go b/internal/gocore/testdata/coretest/test.go index bd35566..d0e8443 100644 --- a/internal/gocore/testdata/coretest/test.go +++ b/internal/gocore/testdata/coretest/test.go
@@ -1,6 +1,7 @@ package main import ( + "os" "runtime" ) @@ -130,6 +131,17 @@ var a anyNode +func init() { + if coredumpFilter := os.Getenv("GO_DEBUG_TEST_COREDUMP_FILTER"); coredumpFilter != "" { + if err := os.WriteFile("/proc/self/coredump_filter", []byte(coredumpFilter), 0600); err != nil { + os.Stderr.WriteString("crash: unable to set coredump_filter: ") + os.Stderr.WriteString(err.Error()) + os.Stderr.WriteString("\n") + os.Exit(0) // Don't crash (which is an error for the called). + } + } +} + func main() { globalAnyTree.root = makeAnyTree(5) globalTypeSafeTree.root = makeTypeSafeTree(5)