Plugin support (part 2) #1414

aarzilli · 2018-11-16T09:05:03Z

proc: support debugging plugins

This change splits the BinaryInfo object into a slice of Image objects
containing information about the base executable and each loaded shared
library (note: go plugins are shared libraries).

Delve backens are supposed to call BinaryInfo.AddImage whenever they
detect that a new shared library has been loaded.

Member fields of BinaryInfo that are used to speed up access to dwarf
(Functions, packageVars, consts, etc...) remain part of BinaryInfo and
are updated to reference the correct image object. This simplifies this
change.

This approach has a few shortcomings:

1. Multiple shared libraries can define functions or globals with the
same name and we have no way to disambiguate between them.

2. We don't have a way to handle library unloading.

Both of those affect C shared libraries much more than they affect go
plugins. Go plugins can't be unloaded at all and a lot of name
collisions are prevented by import paths.

There's only one problem that is concerning: if two plugins both import
the same package they will end up with multiple definition for the same
function.
For example if two plugins use fmt.Printf the final in-memory image
(and therefore our BinaryInfo object) will end up with two copies of
fmt.Printf at different memory addresses. If a user types
break fmt.Printf
a breakpoint should be created at *both* locations.
Allowing this is a relatively complex change that should be done in a
different PR than this.

For this reason I consider this approach an acceptable and sustainable
stopgap.

Updates #865

derekparker

Got through about half the review, will do another pass tomorrow or Monday.

derekparker · 2019-05-02T16:57:02Z

pkg/proc/arch.go

@@ -302,5 +304,10 @@ func (a *AMD64) GoroutineToDwarfRegisters(g *G) op.DwarfRegisters {
 	dregs[amd64DwarfIPRegNum] = op.DwarfRegisterFromUint64(g.PC)
 	dregs[amd64DwarfSPRegNum] = op.DwarfRegisterFromUint64(g.SP)
 	dregs[amd64DwarfBPRegNum] = op.DwarfRegisterFromUint64(g.BP)
-	return op.DwarfRegisters{StaticBase: g.variable.bi.staticBase, Regs: dregs, ByteOrder: binary.LittleEndian, PCRegNum: amd64DwarfIPRegNum, SPRegNum: amd64DwarfSPRegNum, BPRegNum: amd64DwarfBPRegNum}
+
+	bi := g.variable.bi


We should create a helper for this to clean it up like some of the others that have been created. This should overall be more efficient than simply using the PC to find the compilation unit and thus the image because we can binary search function entries and we're doing a linear search of compilation unit PC ranges, correct?

Added the helper function.

This should overall be more efficient than simply using the PC to find the compilation unit and thus the image because we can binary search function entries and we're doing a linear search of compilation unit PC ranges, correct?

I think so. Ideally we'd just do a binary search on images but I haven't figured out how to find the end address.

I agree but I think this is alright for now. When (and if) we ever run into a situation where a linear search through loaded shared objects becomes a problem we can tackle it.

derekparker · 2019-05-02T17:01:07Z

pkg/proc/bininfo.go

@@ -324,16 +326,22 @@ func (bi *BinaryInfo) LoadBinaryInfo(path string, entryPoint uint64, debugInfoDi
 		bi.lastModified = fi.ModTime()
 	}

+	bi.debugInfoDirectories = debugInfoDirs


nit: nothing wrong with this particular piece of code, but the comment above is incorrect and out of date, we no longer require a sync.WaitGroup to be passed in.

derekparker · 2019-05-03T22:07:52Z

pkg/proc/bininfo.go

-	if !strings.HasPrefix(path, "/") {
-		return
+// hasImage returns true if the shared object has already been added.
+func (bi *BinaryInfo) hasImage(path string, addr uint64) bool {


This is only used once, let's inline it, or if you prefer to have it as a function for stylistic reasons, I'd prefer it as a function instead of a method. It doesn't really need bi, just the list of images.

Inlined is good.

derekparker · 2019-05-04T00:15:00Z

pkg/proc/bininfo.go

+				errs = append(errs, fmt.Errorf("closing shared object %q (split dwarf): %v", image.Path, err))
+			}
+		}
+		if image.closer != nil {


nit: A refactor could possibly be to embed the closer in image so we can just call image.Close().

derekparker · 2019-05-04T00:20:51Z

pkg/proc/bininfo.go

 }

-// LoadError returns any internal load error.
+// LoadError returns any internal error incurred while loading the last shared object.


Why return just the last error specifically?

IIRC it was the easiest way to do it at the time, because it was called from the backends. Given that now it's only called in proc.PostInitializationSetup, I've replaced it there with a loop through all the images.

derekparker · 2019-05-04T00:22:33Z

pkg/proc/bininfo.go

@@ -474,22 +566,26 @@ func (c *nilCloser) Close() error { return nil }
 // LoadFromData creates a new BinaryInfo object using the specified data.
 // This is used for debugging BinaryInfo, you should use LoadBinary instead.
 func (bi *BinaryInfo) LoadFromData(dwdata *dwarf.Data, debugFrameBytes, debugLineBytes, debugLocBytes []byte) {


We should rename this to something like LoadImageFromData and update the comment above to reflect the intent of the new changes.

derekparker · 2019-05-04T00:26:05Z

pkg/proc/bininfo.go

+		// - addr is entryPoint therefore staticBase needs to be calculated by
+		//   subtracting it from the entry point specified in the executable file
+		// - memory address of the .dynamic section needs to be recorded in
+		//   BinaryInfo so that we can find loadded libraries.


nit: s/loadded/loaded/

derekparker · 2019-05-04T00:27:12Z

pkg/proc/bininfo.go

+	if image.index == 0 {
+		// adding executable file:
+		// - addr is entryPoint therefore staticBase needs to be calculated by
+		//   subtracting it from the entry point specified in the executable file


nit: technically you're subtracting the entry point from addr.

derekparker · 2019-05-04T00:29:49Z

pkg/proc/bininfo.go

+		//   BinaryInfo so that we can find loadded libraries.
+		if addr != 0 {
+			image.StaticBase = addr - elfFile.Entry
+		} else {


nit: else if

This change splits the BinaryInfo object into a slice of Image objects containing information about the base executable and each loaded shared library (note: go plugins are shared libraries). Delve backens are supposed to call BinaryInfo.AddImage whenever they detect that a new shared library has been loaded. Member fields of BinaryInfo that are used to speed up access to dwarf (Functions, packageVars, consts, etc...) remain part of BinaryInfo and are updated to reference the correct image object. This simplifies this change. This approach has a few shortcomings: 1. Multiple shared libraries can define functions or globals with the same name and we have no way to disambiguate between them. 2. We don't have a way to handle library unloading. Both of those affect C shared libraries much more than they affect go plugins. Go plugins can't be unloaded at all and a lot of name collisions are prevented by import paths. There's only one problem that is concerning: if two plugins both import the same package they will end up with multiple definition for the same function. For example if two plugins use fmt.Printf the final in-memory image (and therefore our BinaryInfo object) will end up with two copies of fmt.Printf at different memory addresses. If a user types break fmt.Printf a breakpoint should be created at *both* locations. Allowing this is a relatively complex change that should be done in a different PR than this. For this reason I consider this approach an acceptable and sustainable stopgap. Updates go-delve#865

aarzilli force-pushed the listlib branch from ff9c894 to 23f6c44 Compare November 16, 2018 09:18

aarzilli changed the title ~~Plugin support (part 2)~~ Plugin support (part 2) [WIP] Nov 16, 2018

aarzilli force-pushed the listlib branch 2 times, most recently from 7b5a93c to 38ac8b8 Compare November 21, 2018 08:12

aarzilli changed the title ~~Plugin support (part 2) [WIP]~~ Plugin support (part 2) Nov 21, 2018

aarzilli force-pushed the listlib branch 3 times, most recently from c2a34d0 to 959a839 Compare November 23, 2018 15:10

aarzilli force-pushed the listlib branch from 959a839 to e09230e Compare December 4, 2018 09:00

aarzilli force-pushed the listlib branch from e09230e to b8801e6 Compare January 5, 2019 10:44

aarzilli force-pushed the listlib branch from b8801e6 to 786b07a Compare March 20, 2019 17:44

aarzilli force-pushed the listlib branch 2 times, most recently from 5d481c7 to 897ec55 Compare March 29, 2019 11:35

aarzilli force-pushed the listlib branch from 897ec55 to c2e68fd Compare May 3, 2019 08:29

derekparker requested changes May 4, 2019

View reviewed changes

aarzilli force-pushed the listlib branch from c2e68fd to e338c99 Compare May 8, 2019 14:42

derekparker approved these changes May 8, 2019

View reviewed changes

derekparker merged commit f3b149b into go-delve:master May 8, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Plugin support (part 2) #1414

Plugin support (part 2) #1414

aarzilli commented Nov 16, 2018 •

edited

Loading

derekparker left a comment

derekparker May 2, 2019

aarzilli May 8, 2019

derekparker May 8, 2019

derekparker May 2, 2019

derekparker May 3, 2019

aarzilli May 8, 2019

derekparker May 4, 2019

derekparker May 4, 2019

aarzilli May 8, 2019

derekparker May 4, 2019

derekparker May 4, 2019

derekparker May 4, 2019

derekparker May 4, 2019

Plugin support (part 2) #1414

Plugin support (part 2) #1414

Conversation

aarzilli commented Nov 16, 2018 • edited Loading

derekparker left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aarzilli commented Nov 16, 2018 •

edited

Loading