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

proposal: x/debug: make the core and gocore packages public #57447

Open
4 tasks
mknyszek opened this issue Dec 22, 2022 · 13 comments
Open
4 tasks

proposal: x/debug: make the core and gocore packages public #57447

mknyszek opened this issue Dec 22, 2022 · 13 comments
Assignees
Labels
Milestone

Comments

@mknyszek
Copy link
Contributor

mknyszek commented Dec 22, 2022

Authors: @mknyszek @prattmic @aclements

Purpose

The purpose of this proposal is to help fill in an important gap in Go debugging by providing a foundation for Go heap analysis. The primary use-cases for heap analysis (vs. heap profiles) are to pinpoint memory leak issues and debug out-of-memory errors by providing an in-depth look at the memory graph. Other use-cases exist as well, mostly related to the large amount of detail available in typical heap analysis software.

Go currently has two heap analysis options: analyzing core files for Go processes (via golang.org/x/debug/cmd/viewcore) or analyzing heap dumps (produced by runtime/debug.WriteHeapDump). As of today, both of these options are not easily available to users. The former has bit-rotted, while the latter lacks working tooling (and so may not actually work either).

Some companies using Go have expressed interest in building their own heap analysis tooling, while members of the Go community have contributed time and effort to updating viewcore. The utility of such functionality is lauded in the Java world, especially when it's available for servers over a connection (e.g. net/http/pprof).

Plan

We propose the following plan for improving the state of Go heap analysis, with the core API change here being to make the golang.org/x/debug/internal/core and golang.org/x/debug/internal/gocore packages public.

Before that can happen, however, we also need to fix them and prevent them from breaking again by:

  • Updating gocore and core to work with Go at tip.
  • Ensure that gocore and core are well-tested, and tested at tip.
    • Running the full test suite for a core dump created from a Go binary built at tip.

After that, I propose making gocore and core externally-visible APIs, allowing Go developers to build on top of it with some fairly minor changes to the API that I would like to hash out. To identify the full scope of the API changes and get feedback on the design, we plan to first fork the API into x/exp before moving them back to x/debug. To let viewcore take advantage of improvements in x/exp, and as a data point in trialing the API, x/debug will temporarily depend on x/exp in the process.

Once the API moves to its final home in x/debug, I would like to lastly propose

  • Setting up the golang.org/x/debug builder to run against the main Go repository.

in order to identify future breakage of these APIs as early as possible in the development of the Go toolchain.

These small steps would bring back heap analysis to the Go ecosystem and will ensure it continues to work.

Since one goal of this package is to support third-party tooling, we need a compatibility story. Simultaneously, these packages rely on and poke at Go internals that are subject to change with each release. Exposing these internals to the Go 1 compatibility promise would likely be too large of maintenance burden to carry forever across all Go versions. As a compromise, I would like to propose the following policy, which will be posted in the package's documentation:

  • We guarantee compile-time compatibility: upgrading x/debug versions will never break code at compile-time.
  • We guarantee support for analyzing core files created from Go programs built using one of the last two major Go releases.
  • Support for core files created by Go programs built with older Go versions will be best-effort.
    • In the interest of user-friendliness we will make an effort to making it keep older Go versions working, but if package maintenance becomes untenable we reserve the right to remove support.
    • If a core file is too old to work with an x/debug version, it will be reported by the gocore package.

viewcore, though minimalistic, provides a proof-of-concept for the gocore library that we hope will motivate Go developers to build fancier tooling for heap analysis. Documentation will be critical to making this tooling more widely known ("how to debug a memory leak with viewcore").

Potential future work

I want to also mention some ideas for future work to gauge interest, even though they're not relevant to the proposal directly.

  1. Stub out runtime/debug.WriteHeapDump to reduce eliminate some technical debt.
  2. Provide a way to construct a core file at runtime and expose it through net/http (perhaps via WriteHeapDump).
  3. Reduce dependence on DWARF information for type-casting regions.

The goal of (1) and (2) is to replace WriteHeapDump with a way to produce a process snapshot that works with existing tooling (i.e. gocore and viewcore). Constructing our own core file isn't terribly difficult, provides us with a stable binary format specification so we don't have to go through the trouble of defining our own, and is a format already automatically generated by Linux, so we have fewer formats to support.

The goal of (3) would be to improve the debugging experience with core dumps from stripped binaries. This might not need any additional API, but should be considered up-front.

@jordanlewis
Copy link

This would be an incredible help to the ecosystem! viewcore is an absolutely key tool for developers of CockroachDB, and it's been challenging to be able to contribute to viewcore because of its position as an internal package.

I created some basic viewcore documentation for CockroachDB developers, in case this prior art is useful at all. It also contains a pointer to the branch I've been maintaining with some extra tools that allow dumping the viewcore output into something that can be consumed by pprof to create a very primitive object graph viewer.

https://cockroachlabs.atlassian.net/wiki/spaces/CRDB/pages/1931018299/Using+viewcore+to+analyze+core+dumps

@gopherbot
Copy link

Change https://go.dev/cl/468916 mentions this issue: cmd/coordinator: add linux-amd64 x/debug trybots to the main repo

@mknyszek
Copy link
Contributor Author

I think the main thing missing here is actually what the final API itself will look like. After an offline discussion today with @aclements and @prattmic, I think we've decided the right next step is to fork these x/debug-internal libraries into x/exp where we can iterate on them freely. The API is far too large to try to summarize for a proposal and have everyone comment on it. Meanwhile, x/exp will allow us to put it out there in a way that will allow for experimentation and trial.

The reason we chose x/exp over x/debug is because x/exp has clear expectations with respect to backwards compatibility (i.e. there is none). Meanwhile, x/debug contains only one exported package with zero imports in the Go ecosystem and a handful in the main Go repository (for testing). Although x/debug has a disclaimer on it with respect to compatibility, it is less clearly experimental in its import path and long-term we do want it to maintain backwards compatibility, at least at compile time. By experimenting directly in x/debug, we risk tarnishing its reputation, so-to-speak.

To keep viewcore working as a tool, I think our plan is to temporarily have x/debug import x/exp, so it can take advantage of improvements we make.

Frankly, I don't expect the API to change all that much, but simultaneously it's hard to get a sense of the API in totality without first using it and iterating on it.

@mknyszek
Copy link
Contributor Author

I updated the proposal to reflect these changes, and also updated the proposal's compatibility story based on our current thinking.

@gopherbot
Copy link

Change https://go.dev/cl/474541 mentions this issue: debug: add module

@gopherbot
Copy link

Change https://go.dev/cl/474543 mentions this issue: debug/gobinary: add initial package sketch

@gopherbot
Copy link

Change https://go.dev/cl/474542 mentions this issue: debug/gobinary: add package

@gopherbot
Copy link

Change https://go.dev/cl/506558 mentions this issue: internal/core: refactor loading for maintainability

@gopherbot
Copy link

Change https://go.dev/cl/506757 mentions this issue: internal/gocore: refactor loading for maintainability

gopherbot pushed a commit to golang/debug that referenced this issue Jun 30, 2023
Loading a new core.Process in core.Core is a rather long, complex
process. Currently, this function creates a stub Process with very few
fields set, and then subsequent method calls initially the remaining
fields.

The problem with this approach is that the inputs/requirements and
outputs of these initialization methods is poorly documented. These
methods (e.g., readCore) usually have some prerequisite fields in Process
that must be set on entry, and some fields that they set prior to
return.

Neither of these are well documented, and many are far from obvious. For
example, Process.mainExecName is initialized in readCore -> readNote ->
readNTFile -> openMappedFile.

This is made more of a mess by the fact that Process tries to do most
initialization with a single pass, resulting in fields getting
initialized in unintuitive locations (such as mainExecName).

These combine to make refactoring to support new functionality
difficult, as changing ordering breaks implicit dependencies between
methods.

This CL addresses these issues by eliminating the iterative
initialization of Process. Instead, Process is only instantiated once
all fields are ready. During loading, all work is done by free
functions/types, where inputs and outputs are explicit via function
arguments/returns.

This may inadvertently improve golang/go#44757, as it makes main binary
lookup more explicit.

This CL intends to keep behavior the same, but there are a few changes:

* If the entry point is unknown, We no longer apply the heuristic of
  assuming that first executable mapping is the main binary. This could
  be added back, but it is probably better to ensure that the entry
  point is available on all architectures.

* Failure to ELF parse a file for symbols isn't a hard error (as the
  mapped file might not even be an ELF).

For golang/go#57447.

Change-Id: I7c3712ccb99ceddddc6adbae57d477c25ff4e5ba
Reviewed-on: https://go-review.googlesource.com/c/debug/+/506558
Reviewed-by: Michael Knyszek <mknyszek@google.com>
Run-TryBot: Michael Pratt <mpratt@google.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
@superajun-wsj
Copy link
Contributor

@mknyszek @jordanlewis is there any update now about viewcore in golang.org/x/debug or golang.org/x/exp, please? do we plan to add one tool to parse the dump file from debug.WriteHeapDump()?

@mknyszek
Copy link
Contributor Author

Hey @superajun-wsj, unfortunately the focus on diagnostics lately has been on execution tracing. We still plan to come back to this in the near future and there's been a little bit of progress in June by @prattmic, but this is still a little ways off.

Thanks for your interest, and stay tuned.

@gopherbot
Copy link

Change https://go.dev/cl/547536 mentions this issue: internal/gocore: handle unsafe.pointer case for hchan.buf in chan

@bboreham
Copy link
Contributor

bboreham commented Jan 3, 2024

Hi, thought you might like to know I got viewcore to work with binaries compiled by Go 1.20 and 1.21.

It's somewhat empirical, definitely not complete, and I haven't tried to make it compatible with multiple versions, but my branch it's at golang/debug@master...bboreham:go-debug:go-1-20

gopherbot pushed a commit to golang/debug that referenced this issue Jan 11, 2024
In the hchan struct, buf field is an unsefa.Pointer, and its actual type is
an array of length hchan.dataqsiz and type hchan.elemtype.
Correct its type so that the object it points to can continue to be analyzed.

For golang/go#57447.

Change-Id: I47969a9a8e2870bb9573587bac5416fbea9db9ac
Reviewed-on: https://go-review.googlesource.com/c/debug/+/547536
Reviewed-by: Keith Randall <khr@golang.org>
LUCI-TryBot-Result: Go LUCI <golang-scoped@luci-project-accounts.iam.gserviceaccount.com>
Auto-Submit: Keith Randall <khr@golang.org>
Reviewed-by: Keith Randall <khr@google.com>
Reviewed-by: Dmitri Shuralyov <dmitshur@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

5 participants