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

x/debug: prune and reset x/debug/* #25503

Closed
heschi opened this issue May 22, 2018 · 9 comments
Closed

x/debug: prune and reset x/debug/* #25503

heschi opened this issue May 22, 2018 · 9 comments

Comments

@heschi
Copy link
Contributor

heschi commented May 22, 2018

As we work with the Delve team and develop the viewcore command ourselves, there's an opportunity to share code. x/debug seems like the obvious place to do it, but it's currently in a pretty poor state, with lots of undermaintained packages. I would like to delete many of them and make backward-incompatible changes to some of the others. The packages are in three categories, each of which needs separate treatment, so I'll discuss them one by one.

Once this is done, I'll probably file a new proposal for a few new dwarf subpackages, but that's for later.

Packages with versions in the stdlib

dwarf, elf, gosym, and macho are related to the similarly-named packages in the stdlib. They have not been kept in sync with their stdlib counterparts, so they may not work any more. Also, at least in the case of dwarf, incompatibilities have been introduced -- it now produces Go specific types like DotDotDot instead of DWARF standards. Porting a program between the two isn't easy, and it's not clear whether a new feature or fix should be applied to the stdlib, x/debug/, or both.

According to godoc, gosym is completely unused. dwarf, elf, and macho are used only by other packages in x/debug (see next section) and old versions of Delve. We can't rule out other uses, of course, but the impact of changing these packages is probably very low.

elf, gosym, and macho are small packages, with limited functionality.
Action: delete them and accept necessary changes to the stdlib.

The DWARF spec is far more complicated than ELF, and it is still evolving. We probably need a place to accept changes outside the stdlib, much like the relationship between syscall and x/sys/*.
Action: reset dwarf to the stdlib version, keep the stdlib version frozen, and accept changes to the x/debug version while maintain API compatibility with the stdlib version.

Packages implementing the x/debug debugger

The primary purpose of x/debug was at some point to implement a debugger for Go. It has been almost entirely replaced by Delve. The one user I know of is the Google Cloud Debugger, who have agreed to vendor it.

Most of the packages in x/debug are in this set: x/debug itself, arch, cmd/debugproxy, local, remote, server, and server/protocol.

Action: delete these packages.

Packages implementing cmd/viewcore

The viewcore command is in cmd/viewcore, core, and gocore. I mention them only for completeness; they are fresh, use stdlib packages, and need no modification.

cc @aclements @randall77 @hyangah @dr2chase @aarzilli @derekparker (sorry if I forgot anybody)

@gopherbot gopherbot added this to the Proposal milestone May 22, 2018
@aarzilli
Copy link
Contributor

reset dwarf to the stdlib version

Do you want to do this because you want to have debug_frame and debug_loc stuff as methods of a x/debug/dwarf.Data object?

Because besides that IMHO debug/dwarf is mostly fine. The only thing that's weak is the dwarf.Type stuff: compare for example the attributes of DW_TAG_structure_type (DWARF standard v4, page 204) with the fields of dwarf.StructType (not to mention the extended attributes that go emits).

I think as it is dwarf.Type isn't useful for anything but implementing very basic tools.

The good thing is that it can all be reimplemented outside of debug/dwarf like delve does in pkg/dwarf/godwarf (which is derived from x/debug/dwarf).

@heschi
Copy link
Contributor Author

heschi commented May 23, 2018

I don't have a detailed plan for what to do with dwarf. Yeah, it seems like direct API support for _frame and _loc would be good. I'm also concerned about what will happen if/when we need to handle DWARF5, which could be tricky, especially if we want to try to provide one consistent view of (say) both .debug_loc and .debug_loclist. I think we're much better off doing that sort of evolution outside of the stdlib, especially because the stdlib is covered by the compatibility promise.

Basically, unless you think debug/dwarf is so stable that we're never going to want to make nontrivial changes, I think we should plan to have and actively develop x/debug/dwarf.

@aarzilli
Copy link
Contributor

Alright.

@rsc
Copy link
Contributor

rsc commented Jun 4, 2018

I'm not sure we should delete any existing code until we have a version tag for it. And backwards-incompatible changes (deleting or replacing packages) should be avoided until we have a way to flag them.

It might be fine to create a v2 subdirectory at the top of the repo and develop something there.

On the one hand, it looks like the only public users of this repo at all are cloud.google.com/go and at least some versions of delve. Maybe as long as you coordinate with them, it would be fine to make changes. But on the other hand we don't know who else is using it in non-public code, and it's a dangerous precedent to set to start deleting stuff from x repos right now, before we have tagged versions.

Especially in the absence of a clear plan for a replacement, my suggestion is still a v2 top-level directory, and maybe a strongly worded v2/README.md that you are going to make backwards-incompatible changes there for a while.

@hyangah
Copy link
Contributor

hyangah commented Jun 4, 2018

Is there any official API deprecation mechanism that we can utilize to communicate with hidden users? (E.g. build time warning about use of deprecated APIs or packages)

Even when we decide to "experiment" in v2, we still want to make it clear that the current version is "unsupported" and its use is "strongly discouraged".

For cloud.google.com/go, we discussed with google cloud team so they would vendor the x/debug/*. Given that we can't delete the package from x/debug for the hypothetical users now, it's better not to proceed because that will leave us with one extra variant of the packages.

Delve stopped using x/debug long time ago.

@hyangah
Copy link
Contributor

hyangah commented Jun 5, 2018

@rsc Given the current state, I think the current x/debug should be already considered as 'experimental' or 'pre-release' version. So, resetting or introducing breaking changes should be expected.

Also, isn't it better to garden and fix the repo before tagging or including it in 1.11-tagged x repos?

@rsc
Copy link
Contributor

rsc commented Jun 6, 2018

OK then I guess delete away.

@andybons
Copy link
Member

Spoke with Russ and marking this as accepted.

@andybons andybons changed the title proposal: prune and reset x/debug/* x/debug: prune and reset x/debug/* Jun 12, 2018
@hyangah
Copy link
Contributor

hyangah commented Jul 9, 2018

https://go-review.googlesource.com/121017 for cleanup was submitted.
https://code-review.googlesource.com/29071 moved code for the google cloud debugger to google cloud debugger.

debug sub-repository is now included in build.golang.org subrepo list. (by https://go-review.googlesource.com/115057 and an administrative operation to re-initialize the subrepo list)

Closing this issue.

@hyangah hyangah closed this as completed Jul 9, 2018
@golang golang locked and limited conversation to collaborators Jul 9, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

6 participants