[go] cmd/compile: regabi support for DWARF location expressions
Than McIntosh (Gerrit)
Than McIntosh uploaded patch set #2 to this change.
cmd/compile: regabi support for DWARF location expressions
Revise the code that generates DWARF location expressions for input
parameters to get it to work properly with the new register ABI when
optimization is turned off.
The previously implementation assumed stack locations for all
input+output parameters when -N (disable optimization) was in effect.
In the new implementation, a register-resident input parameter is
given a 2-element location list, the first list element pointing to
the ABI register(s) containing the param, and the second element
pointing to the stack home once it has been spilled.
NB, this change fixes a bunch of the Delve pkg/proc unit tests (maybe
about half of the outstanding failures). Still a good number that need
to be investigated, however.
Updates #40724.
Change-Id: I743bbb9af187bcdebeb8e690fdd6db58094ca415
---
M src/cmd/compile/internal/dwarfgen/dwarf.go
M src/cmd/compile/internal/ssa/debug.go
M src/cmd/compile/internal/ssagen/ssa.go
3 files changed, 259 insertions(+), 2 deletions(-)
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: David Chase, Cherry Zhang.
Patch set 2:Run-TryBot +1Trust +1
Cherry Zhang (Gerrit)
Attention is currently required from: Than McIntosh, David Chase.
11 comments:
Patchset:
Looks good overall. Thanks.
File src/cmd/compile/internal/dwarfgen/dwarf.go:
// Invoke createComplexVars to generate dwarf vars for input parameters
// that are register-allocated according to the ABI rules.
decls, vars, selected := createComplexVars(fnsym, fn)Where in the code controls this is only for input parameters? (Sorry, I'm not really familiar with this code)
File src/cmd/compile/internal/ssa/debug.go:
// Pack a value and block ID into an address-sized uint, returning ~0 if they
// don't fit.(Not for this CL) this comment is outdated.
Patch Set #2, Line 1250:
_, ok := regarg[a.ID]; okWe could just check a.Op without using a map.
Patch Set #2, Line 1255:
nonRegNonMemInputsYou probably can check that the other input is OpSP, and v.Aux is a Name of PPARAM.
Patch Set #2, Line 1261:
walk backwardsThis probably will likely to have false positives. What about walking forward? All the spills should be pretty early, so it shouldn't walk very long. We can stop if you see a CALL or something.
Patch Set #2, Line 1313:
func BuildFuncDebugNoOptimized(ctxt *obj.Link, f *Func, loggingEnabled bool, stackOffset func(LocalSlot) int32) *FuncDebug {What about parameters passed on stack?
Patch Set #2, Line 1336:
afterPrologVal := locatePrologEnd(f)Ideally for each parameter we could use the stack location as long as that parameter is spilled. I guess it is fine to change them altogether, as the registers should also be live until at least all of them are spilled.
I'm not sure this. I think this offset is the offset within n, not including n's frame offset. (I could be wrong.)
if list == nil {
pidx++
continue
}Is this possible? list is not nil at line 1362 but is nil here?
Patch Set #2, Line 1394:
DW_OP_call_frame_cfaI'm not sure I understand this. call_frame_cfa sounds like the FP. But the code above also does this, so it is probably okay.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
David Chase (Gerrit)
Attention is currently required from: Than McIntosh.
1 comment:
Patchset:
I did a preliminary test (a quantity test, not a quality test)
with optargorder -- github.com/dr2chase/optargorder -- and
the numbers are superficially worse. The test could be broken,
of course. It is whining about "too many pieces".I'll also try by hand.
The test binary for optargorder, is optargorder.
Ignore the COULD NOT PARSE complaints.```
# drchase-macbookpro ~/work/gocode/src/github.com/dr2chase/optargorder 21-04-27 21:51:37
./optargorder ./old-dbg
WARNING: COULD NOT PARSE (strconv.ryuDigits in /Users/drchase/work/go-debug/src/strconv/ftoaryu.go, err = in:2:1: expected ), found '}' (and 5 more errors))
WARNING: COULD NOT PARSE (strconv.ryuDigits32 in /Users/drchase/work/go-debug/src/strconv/ftoaryu.go, err = in:2:1: expected ), found '}' (and 5 more errors))
WARNING: COULD NOT PARSE (go/scanner.lower in /Users/drchase/work/go-debug/src/go/scanner/scanner.go, err = in:2:1: expected declaration, found '}')
nFunctions,argumentError,tooManyPieces,missingSource,wrongOrder,missingDwarf,duplicated,1-totalErrors/nFunctions
3300,458,31,2,35,7,11,0.835152
# drchase-macbookpro ~/work/gocode/src/github.com/dr2chase/optargorder 21-04-27 21:51:48
./optargorder ./new-dbg
WARNING: COULD NOT PARSE (strconv.ryuDigits in /Users/drchase/work/go-debug/src/strconv/ftoaryu.go, err = in:2:1: expected ), found '}' (and 5 more errors))
WARNING: COULD NOT PARSE (strconv.ryuDigits32 in /Users/drchase/work/go-debug/src/strconv/ftoaryu.go, err = in:2:1: expected ), found '}' (and 5 more errors))
WARNING: COULD NOT PARSE (go/scanner.lower in /Users/drchase/work/go-debug/src/go/scanner/scanner.go, err = in:2:1: expected declaration, found '}')
nFunctions,argumentError,tooManyPieces,missingSource,wrongOrder,missingDwarf,duplicated,1-totalErrors/nFunctions
3300,458,821,2,16,5,11,0.602121
```
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: David Chase.
1 comment:
Patchset:
I did a preliminary test (a quantity test, not a quality test) […]
Thanks, I'll look that over.
Looking at the code, https://github.com/dr2chase/optargorder/blob/master/checkargorder.go#L306, it seems to be assuming that everything is in memory as opposed to registers? Which would definitely not be the case. I'll poke at it a little and see if I can understand the logic.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: David Chase, Cherry Zhang.
8 comments:
File src/cmd/compile/internal/dwarfgen/dwarf.go:
// Invoke createComplexVars to generate dwarf vars for input parameters
// that are register-allocated according to the ABI rules.
decls, vars, selected := createComplexVars(fnsym, fn)
Where in the code controls this is only for input parameters? (Sorry, I'm not really familiar with t […]
The code in createComplexVars is doing everything based on what's been previously computed by the SSA backend (e.g. here https://go.googlesource.com/go/+/5f1df260a91183c605c08af7b00741d2761b84e4/src/cmd/compile/internal/dwarfgen/dwarf.go#320). Since the code in BuildFuncDebugNoOptimized only creates entries for register-allocated in params, this is how that's done.
File src/cmd/compile/internal/ssa/debug.go:
// Pack a value and block ID into an address-sized uint, returning ~0 if they
// don't fit.(Not for this CL) this comment is outdated.
Indeed. I'll fix that up.
Patch Set #2, Line 1250:
_, ok := regarg[a.ID]; okWe could just check a.Op without using a map.
Good suggestion. Done.
Patch Set #2, Line 1255:
nonRegNonMemInputsYou probably can check that the other input is OpSP, and v.Aux is a Name of PPARAM.
Done
Patch Set #2, Line 1313:
func BuildFuncDebugNoOptimized(ctxt *obj.Link, f *Func, loggingEnabled bool, stackOffset func(LocalSlot) int32) *FuncDebug {What about parameters passed on stack?
Those are handled in createABIVars.
Patch Set #2, Line 1336:
afterPrologVal := locatePrologEnd(f)
Ideally for each parameter we could use the stack location as long as that parameter is spilled. […]
Yes, good eye. Nothing gets by you Cherry :-).
I chose to do things in a simple fashion as opposed to picking out the specific spill location for each parameter and generated a customized location for the param based on that (that would be more accurate).
Another consideration is supporting GDB's "set var" command. If you happen to be stopped partway through the spill sequence (let's say the first inparam has been spilled but not yet the second or third) and you do a
(gdb) set var p1=101
GDB will then modify the register that p1 came in, not the stack location, which would of course be incorrect.
My feeling is that it's ok to use this for now and then add more accuracy later if needed.
I'm not sure this. I think this offset is the offset within n, not including n's frame offset. […]
Sorry, this is misleading, see https://go.googlesource.com/go/+/5f1df260a91183c605c08af7b00741d2761b84e4/src/cmd/compile/internal/dwarfgen/dwarf.go#381. The offset in the LocalSlot winds up not being used for anything important, it's the offset in the location list that matters. I'll add a comment.
if list == nil {
pidx++
continue
}Is this possible? list is not nil at line 1362 but is nil here?
I suppose you are right-- if the first one succeeds then the second one should as well. I think I'd like to leave it in just in case the code evolves later on in time somehow.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Alessandro Arzilli (Gerrit)
Attention is currently required from: Than McIntosh, David Chase, Cherry Zhang.
1 comment:
Commit Message:
Patch Set #2, Line 20:
NB, this change fixes a bunch of the Delve pkg/proc unit tests (maybeI also took a look at the state of delve on go 1.17 last week. I have annotated failing tests with the cause and fixed some. You can look at my annotations here:
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: David Chase, Cherry Zhang.
1 comment:
File src/cmd/compile/internal/ssa/debug.go:
Patch Set #2, Line 1377:
ts := inp.Type.Size()This does not look correct. I think I have some more work to do here, stay tuned.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Alessandro Arzilli, David Chase, Cherry Zhang.
1 comment:
Commit Message:
Patch Set #2, Line 20:
NB, this change fixes a bunch of the Delve pkg/proc unit tests (maybe
I also took a look at the state of delve on go 1.17 last week. […]
Thanks. I was going to ask you about the panicCall stuff, but it sounds like you're already looking at that.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Alessandro Arzilli (Gerrit)
Attention is currently required from: Than McIntosh, David Chase, Cherry Zhang.
1 comment:
Commit Message:
Patch Set #2, Line 20:
NB, this change fixes a bunch of the Delve pkg/proc unit tests (maybe
Thanks. […]
If you see delve doing something that looks wrong, let me know. The code for registerized variables should work but it is not as well tested as the code reading from memory.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: David Chase, Cherry Zhang.
2 comments:
Patchset:
Thanks, I'll look that over. […]
Yeah, I don't think the code in checkargorder.go is correct for multi-piece registerized params. Consider a function with a string parameter. The peices we get from with my patch are
[{Size:8 Addr:0 RegNum:8 IsRegister:true} {Size:8 Addr:0 RegNum:9 IsRegister:true}]
The loop at line 322 is not going to touch this since Addr is zero for each piece. So when we return we still have a 2-element slice, meaning that it will trigger the "too many pieces" error.
Let me know if I am misreading the code David, thanks.
File src/cmd/compile/internal/ssa/debug.go:
Patch Set #2, Line 1377:
ts := inp.Type.Size()This does not look correct. I think I have some more work to do here, stay tuned.
OK, I have a fix for this, will upload shortly.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: David Chase, Cherry Zhang.
Than McIntosh uploaded patch set #3 to this change.
cmd/compile: regabi support for DWARF location expressions
Revise the code that generates DWARF location expressions for input
parameters to get it to work properly with the new register ABI when
optimization is turned off.
The previously implementation assumed stack locations for all
input+output parameters when -N (disable optimization) was in effect.
In the new implementation, a register-resident input parameter is
given a 2-element location list, the first list element pointing to
the ABI register(s) containing the param, and the second element
pointing to the stack home once it has been spilled.
NB, this change fixes a bunch of the Delve pkg/proc unit tests (maybe
about half of the outstanding failures). Still a good number that need
to be investigated, however.
Updates #40724.
Change-Id: I743bbb9af187bcdebeb8e690fdd6db58094ca415
---
M src/cmd/compile/internal/dwarfgen/dwarf.go
M src/cmd/compile/internal/ssa/debug.go
M src/cmd/compile/internal/ssagen/ssa.go
3 files changed, 263 insertions(+), 4 deletions(-)
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: David Chase, Cherry Zhang.
Patch set 3:Run-TryBot +1Trust +1
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Zhang (Gerrit)
Attention is currently required from: Than McIntosh, David Chase.
Patch set 3:Code-Review +1
4 comments:
Patchset:
Mostly LGTM. Thanks.
File src/cmd/compile/internal/dwarfgen/dwarf.go:
// Invoke createComplexVars to generate dwarf vars for input parameters
// that are register-allocated according to the ABI rules.
decls, vars, selected := createComplexVars(fnsym, fn)
The code in createComplexVars is doing everything based on what's been previously computed by the SS […]
Okay, thanks. I think I understand how it works now.
File src/cmd/compile/internal/ssa/debug.go:
Patch Set #2, Line 1336:
afterPrologVal := locatePrologEnd(f)Yes, good eye. Nothing gets by you Cherry :-). […]
Yeah, this is okay for now. Setting variable in debugger for a running program is scary. I think I never dared to do that (maybe I did once to bypass a segfault).
A somewhat related thing is how we locate the prolog end. If we walk forward (see also my another comment), it may be easy to locate a spill of a particular arg than to locate the point where all args are spilled (for the latter we probably do need map, or a bitmap or something).
Sorry, this is misleading, see https://go.googlesource. […]
Thanks. Leave it as 0, maybe?
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
File src/cmd/compile/internal/ssa/debug.go:
Patch Set #2, Line 1336:
afterPrologVal := locatePrologEnd(f)
Yeah, this is okay for now. Setting variable in debugger for a running program is scary. […]
This is a fair point. I'll see about revamping it.
Thanks. […]
OK, I suppose this makes sense. Done.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: David Chase, Cherry Zhang.
Than McIntosh uploaded patch set #4 to this change.
cmd/compile: regabi support for DWARF location expressions
Revise the code that generates DWARF location expressions for input
parameters to get it to work properly with the new register ABI when
optimization is turned off.
The previously implementation assumed stack locations for all
input+output parameters when -N (disable optimization) was in effect.
In the new implementation, a register-resident input parameter is
given a 2-element location list, the first list element pointing to
the ABI register(s) containing the param, and the second element
pointing to the stack home once it has been spilled.
NB, this change fixes a bunch of the Delve pkg/proc unit tests (maybe
about half of the outstanding failures). Still a good number that need
to be investigated, however.
Updates #40724.
Change-Id: I743bbb9af187bcdebeb8e690fdd6db58094ca415
---
M src/cmd/compile/internal/dwarfgen/dwarf.go
M src/cmd/compile/internal/ssa/debug.go
M src/cmd/compile/internal/ssagen/ssa.go
3 files changed, 283 insertions(+), 4 deletions(-)
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: David Chase, Cherry Zhang.
Patch set 3:Run-TryBot +1Trust +1
2 comments:
File src/cmd/compile/internal/ssa/debug.go:
Patch Set #2, Line 1261:
walk backwardsThis probably will likely to have false positives. […]
Done
Patch Set #2, Line 1336:
afterPrologVal := locatePrologEnd(f)
This is a fair point. I'll see about revamping it.
OK, latest patch uses a forward walk.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Zhang (Gerrit)
Attention is currently required from: Than McIntosh, David Chase.
Patch set 4:Code-Review +1Trust +1
4 comments:
Patchset:
LGTM. Thanks.
File src/cmd/compile/internal/ssa/debug.go:
Patch Set #4, Line 1264:
regArgs := []ID{}Maybe you could preallocate cap to 32 or so, so it doesn't grow and can stay on stack.
Would it make sense to use BlockEnd.ID ?
// Second entry in the location list will be the stack home
// of the param, once it has been spilled. Emit that now.
list, sizeIdx = setupLocList(ctxt, f, list,
afterPrologVal, BlockEnd.ID)setupLocList uses the entry Block. The stack home should cover the entire function, right?
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Than McIntosh, David Chase.
Than McIntosh uploaded patch set #5 to this change.
cmd/compile: regabi support for DWARF location expressions
Revise the code that generates DWARF location expressions for input
parameters to get it to work properly with the new register ABI when
optimization is turned off.
The previously implementation assumed stack locations for all
input+output parameters when -N (disable optimization) was in effect.
In the new implementation, a register-resident input parameter is
given a 2-element location list, the first list element pointing to
the ABI register(s) containing the param, and the second element
pointing to the stack home once it has been spilled.
NB, this change fixes a bunch of the Delve pkg/proc unit tests (maybe
about half of the outstanding failures). Still a good number that need
to be investigated, however.
Updates #40724.
Change-Id: I743bbb9af187bcdebeb8e690fdd6db58094ca415
---
M src/cmd/compile/internal/dwarfgen/dwarf.go
M src/cmd/compile/internal/ssa/debug.go
M src/cmd/compile/internal/ssagen/ssa.go
3 files changed, 282 insertions(+), 3 deletions(-)
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: David Chase, Cherry Zhang.
Patch set 5:Trust +1
3 comments:
File src/cmd/compile/internal/ssa/debug.go:
Patch Set #4, Line 1264:
regArgs := []ID{}Maybe you could preallocate cap to 32 or so, so it doesn't grow and can stay on stack.
Done
Would it make sense to use BlockEnd. […]
BlockEnd is (unfortunately) somewhat misleadingly named. FWIW I thought the same thing initially, that it meant "end of the current block"... what it really means is "end of the current function".
See https://go.googlesource.com/go/+/1c9e587b90172e7654db897d8c938ffc665e1673/src/cmd/compile/internal/ssagen/ssa.go#6770
// Second entry in the location list will be the stack home
// of the param, once it has been spilled. Emit that now.
list, sizeIdx = setupLocList(ctxt, f, list,
afterPrologVal, BlockEnd.ID)
setupLocList uses the entry Block. […]
See previous remark. BlockEnd seems like it means "end of block", but as implemented it really means "end of function".
I sent a separate CL https://go-review.googlesource.com/c/go/+/314930 to help fix this up, since as things stand it is very confusing and error-prone.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
David Chase (Gerrit)
Attention is currently required from: Than McIntosh, Cherry Zhang.
1 comment:
Patchset:
Thanks, I'll look that over. […]
I tried to back out all the cases where it rejected registers, but may have missed some. This is not quite "I have no idea what I am doing" but it is definitely not something I do well. I "adapted" the test from one of Alessandro's.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Alessandro Arzilli (Gerrit)
Attention is currently required from: Than McIntosh, David Chase, Cherry Zhang.
1 comment:
Patchset:
I tried to back out all the cases where it rejected registers, but may have missed some. […]
That code was originally written with very strong assumptions about the ABI, I think it needs some serious rework to function with the new ABI, orderArgsDwarf is trying to put the DWARF argument list in order by using the address of the arguments on the stack, which doesn't work at all for regabi.
Some knowledge about the order that registers are used in regabi would have to be baked in.
It makes sense that that test gets worse after this change, because the compiler is no longer emitting locations that conform optargorder's worldview.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
David Chase (Gerrit)
Attention is currently required from: Than McIntosh, Cherry Zhang.
Patch set 5:Code-Review +2
1 comment:
Patchset:
This looks good to me, my by-hand debugging (-N -l) is nicely improved.
Would be interesting to see if we could extend the at-entry-point location lists to optimized code, but that is another CL.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Zhang (Gerrit)
Attention is currently required from: Than McIntosh.
Patch set 5:Code-Review +1Trust +1
4 comments:
Patchset:
LGTM. Thanks.
File src/cmd/compile/internal/ssa/debug.go:
Patch Set #4, Line 1264:
regArgs := []ID{}
Done
This seems not addressed in PS5.
BlockEnd is (unfortunately) somewhat misleadingly named. […]
Maybe we can use BlockEnd now?
// Second entry in the location list will be the stack home
// of the param, once it has been spilled. Emit that now.
list, sizeIdx = setupLocList(ctxt, f, list,
afterPrologVal, BlockEnd.ID)
See previous remark. […]
FuncEnd here now?
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Cherry Zhang.
1 comment:
File src/cmd/compile/internal/ssa/debug.go:
Patch Set #4, Line 1264:
regArgs := []ID{}
This seems not addressed in PS5.
Gah, sorry. That got lost while I was rebasing. I'll fix it up.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Cherry Zhang.
Than McIntosh uploaded patch set #6 to this change.
cmd/compile: regabi support for DWARF location expressions
Revise the code that generates DWARF location expressions for input
parameters to get it to work properly with the new register ABI when
optimization is turned off.
The previously implementation assumed stack locations for all
input+output parameters when -N (disable optimization) was in effect.
In the new implementation, a register-resident input parameter is
given a 2-element location list, the first list element pointing to
the ABI register(s) containing the param, and the second element
pointing to the stack home once it has been spilled.
NB, this change fixes a bunch of the Delve pkg/proc unit tests (maybe
about half of the outstanding failures). Still a good number that need
to be investigated, however.
Updates #40724.
Change-Id: I743bbb9af187bcdebeb8e690fdd6db58094ca415
---
M src/cmd/compile/internal/dwarfgen/dwarf.go
M src/cmd/compile/internal/ssa/debug.go
M src/cmd/compile/internal/ssagen/ssa.go
3 files changed, 282 insertions(+), 3 deletions(-)
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Alessandro Arzilli (Gerrit)
Attention is currently required from: Than McIntosh, Cherry Zhang.
Patch set 6:Code-Review +1
Alessandro Arzilli (Gerrit)
Attention is currently required from: Than McIntosh, Cherry Zhang.
Patch set 6:-Code-Review
1 comment:
Patchset:
I've found a case where (I think) the location list that gets computed is wrong.
https://github.com/go-delve/delve/blob/master/_fixtures/goroutinestackprog.go
Compiles to this:
goroutinestackprog.go:7 S 0x455680 493b6610 CMPQ 0x10(R14), SP
goroutinestackprog.go:7 0x455684 7649 JBE 0x4556cf
goroutinestackprog.go:7 SP 0x455686 4883ec18 SUBQ $0x18, SP
goroutinestackprog.go:7 0x45568a 48896c2410 MOVQ BP, 0x10(SP)
goroutinestackprog.go:7 0x45568f 488d6c2410 LEAQ 0x10(SP), BP
goroutinestackprog.go:7 0x455694 4889442420 MOVQ AX, 0x20(SP)
goroutinestackprog.go:7 0x455699 48895c2428 MOVQ BX, 0x28(SP)
goroutinestackprog.go:7 0x45569e 48894c2430 MOVQ CX, 0x30(SP)
...With the arguments having the following locations:
started
0x455680 0x4556a3 DW_OP_reg0
0x4556a3 0x4556f6 DW_OP_call_frame_cfadone
0x455680 0x4556a3 DW_OP_reg3
0x4556a3 0x4556f6 DW_OP_fbreg 0x4i
0x455680 0x4556a3 DW_OP_reg2
0x4556a3 0x4556f6 DW_OP_fbreg 0x8It seems to me that the instructions saving the variables are doing it a 0x8 intervals but the locations go with a step of 0x4, not sure what the cause would be.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Alessandro Arzilli, Cherry Zhang.
Patch set 6:Trust +1
1 comment:
Patchset:
I've found a case where (I think) the location list that gets computed is wrong. […]
Thanks-- I'll take a look.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Alessandro Arzilli, Cherry Zhang.
4 comments:
Patchset:
TryBots beginning. Status page: https://farmer.golang. […]
Done
File src/cmd/compile/internal/ssa/debug.go:
Patch Set #4, Line 1264:
regArgs := []ID{}
Gah, sorry. That got lost while I was rebasing. I'll fix it up.
Done
Maybe we can use BlockEnd now?
Done
// Second entry in the location list will be the stack home
// of the param, once it has been spilled. Emit that now.
list, sizeIdx = setupLocList(ctxt, f, list,
afterPrologVal, BlockEnd.ID)
FuncEnd here now?
Yes, it is now using FuncEnd.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Alessandro Arzilli, Cherry Zhang.
1 comment:
Patchset:
Thanks-- I'll take a look.
OK, this is fixed in the latest patch set. Thanks.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Alessandro Arzilli, Cherry Zhang.
Than McIntosh uploaded patch set #7 to this change.
cmd/compile: regabi support for DWARF location expressions
Revise the code that generates DWARF location expressions for input
parameters to get it to work properly with the new register ABI when
optimization is turned off.
The previously implementation assumed stack locations for all
input+output parameters when -N (disable optimization) was in effect.
In the new implementation, a register-resident input parameter is
given a 2-element location list, the first list element pointing to
the ABI register(s) containing the param, and the second element
pointing to the stack home once it has been spilled.
NB, this change fixes a bunch of the Delve pkg/proc unit tests (maybe
about half of the outstanding failures). Still a good number that need
to be investigated, however.
Updates #40724.
Change-Id: I743bbb9af187bcdebeb8e690fdd6db58094ca415
---
M src/cmd/compile/internal/dwarfgen/dwarf.go
M src/cmd/compile/internal/ssa/debug.go
M src/cmd/compile/internal/ssagen/ssa.go
3 files changed, 282 insertions(+), 3 deletions(-)
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Alessandro Arzilli, Cherry Zhang.
Patch set 7:Run-TryBot +1Trust +1
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Alessandro Arzilli, Cherry Zhang.
1 comment:
Patchset:
TryBots beginning. Status page: https://farmer.golang. […]
Ack
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Alessandro Arzilli (Gerrit)
Attention is currently required from: Than McIntosh, Cherry Zhang.
Patch set 7:Code-Review +1
1 comment:
Patchset:
Thanks. At this point the main issue I find with delve's tests is with return arguments, should I file an issue for that or are you planning to do something already?
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Alessandro Arzilli, Cherry Zhang.
1 comment:
Patchset:
Thanks. […]
I'll take a look. If you can send a couple of the specific delve test failures that would probably be helpful also.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Alessandro Arzilli (Gerrit)
Attention is currently required from: Than McIntosh, Cherry Zhang.
1 comment:
Patchset:
I'll take a look. If you can send a couple of the specific delve test failures that would probably be helpful also.
TestStepOutReturn in proc_test.go for example. We expect:
1. that return arguments are DW_TAG_formal_parameter and have DW_AT_var_param set
2. that their location at the entry point of the function is the location where they will be stored before the function returnsfor the function tested by TestStepOutReturn I see that return arguments appear as local variables. Their location is on the stack and I haven't checked if it actually gets filled correctly.
Delve uses this to print return arguments when stepping out of a function, as well as function call injection (which will need other changes in addition to this).
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Alessandro Arzilli (Gerrit)
Patchset:
> I'll take a look. […]
Also TestTrace in pkg/terminal/command_test.go
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Alessandro Arzilli, Cherry Zhang.
Patch set 8:Trust +1
1 comment:
Patchset:
Also TestTrace in pkg/terminal/command_test. […]
OK, I sent a new CL https://go-review.googlesource.com/c/go/+/315610 to fix this. Thanks for calling it out.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Alessandro Arzilli, Cherry Zhang.
Than McIntosh uploaded patch set #9 to this change.
cmd/compile: regabi support for DWARF location expressions
Revise the code that generates DWARF location expressions for input
parameters to get it to work properly with the new register ABI when
optimization is turned off.
The previously implementation assumed stack locations for all
input+output parameters when -N (disable optimization) was in effect.
In the new implementation, a register-resident input parameter is
given a 2-element location list, the first list element pointing to
the ABI register(s) containing the param, and the second element
pointing to the stack home once it has been spilled.
NB, this change fixes a bunch of the Delve pkg/proc unit tests (maybe
about half of the outstanding failures). Still a good number that need
to be investigated, however.
Updates #40724.
Updates #45720.
Change-Id: I743bbb9af187bcdebeb8e690fdd6db58094ca415
---
M src/cmd/compile/internal/dwarfgen/dwarf.go
M src/cmd/compile/internal/ssa/debug.go
M src/cmd/compile/internal/ssagen/ssa.go
3 files changed, 282 insertions(+), 3 deletions(-)
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Alessandro Arzilli, Cherry Zhang.
Patch set 9:Run-TryBot +1Trust +1
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Cherry Zhang (Gerrit)
Attention is currently required from: Alessandro Arzilli, Than McIntosh.
Patch set 9:Code-Review +2Trust +1
2 comments:
File src/cmd/compile/internal/ssa/debug.go:
if list == nil {
pidx++
continue
}I suppose you are right-- if the first one succeeds then the second one should as well. […]
Ack
File src/cmd/compile/internal/ssa/debug.go:
Done
Seems not done? (as of PS9)
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Alessandro Arzilli.
1 comment:
File src/cmd/compile/internal/ssa/debug.go:
Seems not done? (as of PS9)
OK, fixed.
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Alessandro Arzilli.
Than McIntosh uploaded patch set #10 to this change.
cmd/compile: regabi support for DWARF location expressions
Revise the code that generates DWARF location expressions for input
parameters to get it to work properly with the new register ABI when
optimization is turned off.
The previously implementation assumed stack locations for all
input+output parameters when -N (disable optimization) was in effect.
In the new implementation, a register-resident input parameter is
given a 2-element location list, the first list element pointing to
the ABI register(s) containing the param, and the second element
pointing to the stack home once it has been spilled.
NB, this change fixes a bunch of the Delve pkg/proc unit tests (maybe
about half of the outstanding failures). Still a good number that need
to be investigated, however.
Updates #40724.
Updates #45720.
Change-Id: I743bbb9af187bcdebeb8e690fdd6db58094ca415
---
M src/cmd/compile/internal/dwarfgen/dwarf.go
M src/cmd/compile/internal/ssa/debug.go
M src/cmd/compile/internal/ssagen/ssa.go
3 files changed, 282 insertions(+), 3 deletions(-)
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Alessandro Arzilli.
Patch set 10:Run-TryBot +1Trust +1
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Attention is currently required from: Alessandro Arzilli.
1 comment:
Patchset:
Thanks for the helpful review comments...
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.
Than McIntosh (Gerrit)
Than McIntosh submitted this change.
cmd/compile: regabi support for DWARF location expressions
Revise the code that generates DWARF location expressions for input
parameters to get it to work properly with the new register ABI when
optimization is turned off.
The previously implementation assumed stack locations for all
input+output parameters when -N (disable optimization) was in effect.
In the new implementation, a register-resident input parameter is
given a 2-element location list, the first list element pointing to
the ABI register(s) containing the param, and the second element
pointing to the stack home once it has been spilled.
NB, this change fixes a bunch of the Delve pkg/proc unit tests (maybe
about half of the outstanding failures). Still a good number that need
to be investigated, however.
Updates #40724.
Updates #45720.
Change-Id: I743bbb9af187bcdebeb8e690fdd6db58094ca415
Reviewed-on: https://go-review.googlesource.com/c/go/+/314431
Trust: Than McIntosh <th...@google.com>
Trust: Cherry Zhang <cher...@google.com>
Run-TryBot: Than McIntosh <th...@google.com>
TryBot-Result: Go Bot <go...@golang.org>
Reviewed-by: Cherry Zhang <cher...@google.com>
Reviewed-by: David Chase <drc...@google.com>
---
M src/cmd/compile/internal/dwarfgen/dwarf.go
M src/cmd/compile/internal/ssa/debug.go
M src/cmd/compile/internal/ssagen/ssa.go
3 files changed, 282 insertions(+), 3 deletions(-)
diff --git a/src/cmd/compile/internal/dwarfgen/dwarf.go b/src/cmd/compile/internal/dwarfgen/dwarf.go
index 0754a88..09e1f12 100644
--- a/src/cmd/compile/internal/dwarfgen/dwarf.go
+++ b/src/cmd/compile/internal/dwarfgen/dwarf.go
@@ -138,8 +138,11 @@
var vars []*dwarf.Var
var decls []*ir.Name
var selected ir.NameSet
+
if base.Ctxt.Flag_locationlists && base.Ctxt.Flag_optimize && fn.DebugInfo != nil && complexOK {
decls, vars, selected = createComplexVars(fnsym, fn)
+ } else if fn.ABI == obj.ABIInternal && base.Flag.N != 0 && complexOK {
+ decls, vars, selected = createABIVars(fnsym, fn, apDecls)
} else {
decls, vars, selected = createSimpleVars(fnsym, apDecls)
}
@@ -314,6 +317,37 @@
}
}
+// createABIVars creates DWARF variables for functions in which the
+// register ABI is enabled but optimization is turned off. It uses a
+// hybrid approach in which register-resident input params are
+// captured with location lists, and all other vars use the "simple"
+// strategy.
+func createABIVars(fnsym *obj.LSym, fn *ir.Func, apDecls []*ir.Name) ([]*ir.Name, []*dwarf.Var, ir.NameSet) {
+
+ // Invoke createComplexVars to generate dwarf vars for input parameters
+ // that are register-allocated according to the ABI rules.
+ decls, vars, selected := createComplexVars(fnsym, fn)
+
+ // Now fill in the remainder of the variables: input parameters
+ // that are not register-resident, output parameters, and local
+ // variables.
+ for _, n := range apDecls {
+ if ir.IsAutoTmp(n) {
+ continue
+ }
+ if _, ok := selected[n]; ok {
+ // already handled
+ continue
+ }
+
+ decls = append(decls, n)
+ vars = append(vars, createSimpleVar(fnsym, n))
+ selected.Add(n)
+ }
+
+ return decls, vars, selected
+}
+
// createComplexVars creates recomposed DWARF vars with location lists,
// suitable for describing optimized code.
func createComplexVars(fnsym *obj.LSym, fn *ir.Func) ([]*ir.Name, []*dwarf.Var, ir.NameSet) {
diff --git a/src/cmd/compile/internal/ssa/debug.go b/src/cmd/compile/internal/ssa/debug.go
index 46743f5..4401f56 100644
--- a/src/cmd/compile/internal/ssa/debug.go
+++ b/src/cmd/compile/internal/ssa/debug.go
@@ -5,6 +5,7 @@
package ssa
import (
+ "cmd/compile/internal/abi"
"cmd/compile/internal/ir"
"cmd/internal/dwarf"
"cmd/internal/obj"
@@ -1124,8 +1125,11 @@
listSym.WriteInt(ctxt, listSym.Size, ctxt.Arch.PtrSize, 0)
}
-// Pack a value and block ID into an address-sized uint, returning ~0 if they
-// don't fit.
+// Pack a value and block ID into an address-sized uint, returning encoded
+// value and boolean indicating whether the encoding succeeded. For
+// 32-bit architectures the process may fail for very large procedures
+// (the theory being that it's ok to have degraded debug quality in
+// this case).
func encodeValue(ctxt *obj.Link, b, v ID) (uint64, bool) {
if ctxt.Arch.PtrSize == 8 {
result := uint64(b)<<32 | uint64(uint32(v))
@@ -1192,3 +1196,239 @@
}
}
+
+// setupLocList creates the initial portion of a location list for a
+// user variable. It emits the encoded start/end of the range and a
+// placeholder for the size. Return value is the new list plus the
+// slot in the list holding the size (to be updated later).
+func setupLocList(ctxt *obj.Link, f *Func, list []byte, st, en ID) ([]byte, int) {
+ start, startOK := encodeValue(ctxt, f.Entry.ID, st)
+ end, endOK := encodeValue(ctxt, f.Entry.ID, en)
+ if !startOK || !endOK {
+ // This could happen if someone writes a function that uses
+ // >65K values on a 32-bit platform. Hopefully a degraded debugging
+ // experience is ok in that case.
+ return nil, 0
+ }
+ list = appendPtr(ctxt, list, start)
+ list = appendPtr(ctxt, list, end)
+
+ // Where to write the length of the location description once
+ // we know how big it is.
+ sizeIdx := len(list)
+ list = list[:len(list)+2]
+ return list, sizeIdx
+}
+
+// locatePrologEnd walks the entry block of a function with incoming
+// register arguments and locates the last instruction in the prolog
+// that spills a register arg. It returns the ID of that instruction
+// Example:
+//
+// b1:
+// v3 = ArgIntReg <int> {p1+0} [0] : AX
+// ... more arg regs ..
+// v4 = ArgFloatReg <float32> {f1+0} [0] : X0
+// v52 = MOVQstore <mem> {p1} v2 v3 v1
+// ... more stores ...
+// v68 = MOVSSstore <mem> {f4} v2 v67 v66
+// v38 = MOVQstoreconst <mem> {blob} [val=0,off=0] v2 v32
+//
+// Important: locatePrologEnd is expected to work properly only with
+// optimization turned off (e.g. "-N"). If optimization is enabled
+// we can't be assured of finding all input arguments spilled in the
+// entry block prolog.
+func locatePrologEnd(f *Func) ID {
+
+ // returns true if this instruction looks like it moves an ABI
+ // register to the stack, along with the value being stored.
+ isRegMoveLike := func(v *Value) (bool, ID) {
+ n, ok := v.Aux.(*ir.Name)
+ var r ID
+ if !ok || n.Class != ir.PPARAM {
+ return false, r
+ }
+ regInputs, memInputs, spInputs := 0, 0, 0
+ for _, a := range v.Args {
+ if a.Op == OpArgIntReg || a.Op == OpArgFloatReg {
+ regInputs++
+ r = a.ID
+ } else if a.Type.IsMemory() {
+ memInputs++
+ } else if a.Op == OpSP {
+ spInputs++
+ } else {
+ return false, r
+ }
+ }
+ return v.Type.IsMemory() && memInputs == 1 &&
+ regInputs == 1 && spInputs == 1, r
+ }
+
+ // OpArg*Reg values we've seen so far on our forward walk,
+ // for which we have not yet seen a corresponding spill.
+ regArgs := make([]ID, 0, 32)
+
+ // removeReg tries to remove a value from regArgs, returning true
+ // if found and removed, or false otherwise.
+ removeReg := func(r ID) bool {
+ for i := 0; i < len(regArgs); i++ {
+ if regArgs[i] == r {
+ regArgs = append(regArgs[:i], regArgs[i+1:]...)
+ return true
+ }
+ }
+ return false
+ }
+
+ // Walk forwards through the block. When we see OpArg*Reg, record
+ // the value it produces in the regArgs list. When see a store that uses
+ // the value, remove the entry. When we hit the last store (use)
+ // then we've arrived at the end of the prolog.
+ for k, v := range f.Entry.Values {
+ if v.Op == OpArgIntReg || v.Op == OpArgFloatReg {
+ regArgs = append(regArgs, v.ID)
+ continue
+ }
+ if ok, r := isRegMoveLike(v); ok {
+ if removed := removeReg(r); removed {
+ if len(regArgs) == 0 {
+ // Found our last spill; return the value after
+ // it. Note that it is possible that this spill is
+ // the last instruction in the block. If so, then
+ // return the "end of block" sentinel.
+ if k < len(f.Entry.Values)-1 {
+ return f.Entry.Values[k+1].ID
+ }
+ return BlockEnd.ID
+ }
+ }
+ }
+ if v.Op.IsCall() {
+ // if we hit a call, we've gone too far.
+ return v.ID
+ }
+ }
+ // nothing found
+ return ID(-1)
+}
+
+// isNamedRegParam returns true if the param corresponding to "p"
+// is a named, non-blank input parameter assigned to one or more
+// registers.
+func isNamedRegParam(p abi.ABIParamAssignment) bool {
+ if p.Name == nil {
+ return false
+ }
+ n := p.Name.(*ir.Name)
+ if n.Sym() == nil || n.Sym().IsBlank() {
+ return false
+ }
+ if len(p.Registers) == 0 {
+ return false
+ }
+ return true
+}
+
+// BuildFuncDebugNoOptimized constructs a FuncDebug object with
+// entries corresponding to the register-resident input parameters for
+// the function "f"; it is used when we are compiling without
+// optimization but the register ABI is enabled. For each reg param,
+// it constructs a 2-element location list: the first element holds
+// the input register, and the second element holds the stack location
+// of the param (the assumption being that when optimization is off,
+// each input param reg will be spilled in the prolog.
+func BuildFuncDebugNoOptimized(ctxt *obj.Link, f *Func, loggingEnabled bool, stackOffset func(LocalSlot) int32) *FuncDebug {
+ fd := FuncDebug{}
+
+ pri := f.ABISelf.ABIAnalyzeFuncType(f.Type.FuncType())
+
+ // Look to see if we have any named register-promoted parameters.
+ // If there are none, bail early and let the caller sort things
+ // out for the remainder of the params/locals.
+ numRegParams := 0
+ for _, inp := range pri.InParams() {
+ if isNamedRegParam(inp) {
+ numRegParams++
+ }
+ }
+ if numRegParams == 0 {
+ return &fd
+ }
+
+ // Allocate location lists.
+ fd.LocationLists = make([][]byte, numRegParams)
+
+ // Locate the value corresponding to the last spill of
+ // an input register.
+ afterPrologVal := locatePrologEnd(f)
+ if afterPrologVal == ID(-1) {
+ panic(fmt.Sprintf("internal error: f=%s: can't locate after prolog value", f.Name))
+ }
+
+ // Walk the input params again and process the register-resident elements.
+ pidx := 0
+ for _, inp := range pri.InParams() {
+ if !isNamedRegParam(inp) {
+ // will be sorted out elsewhere
+ continue
+ }
+
+ n := inp.Name.(*ir.Name)
+ sl := LocalSlot{N: n, Type: inp.Type, Off: 0}
+ fd.Vars = append(fd.Vars, n)
+ fd.Slots = append(fd.Slots, sl)
+ slid := len(fd.VarSlots)
+ fd.VarSlots = append(fd.VarSlots, []SlotID{SlotID(slid)})
+
+ // Param is arriving in one or more registers. We need a 2-element
+ // location expression for it. First entry in location list
+ // will correspond to lifetime in input registers.
+ list, sizeIdx := setupLocList(ctxt, f, fd.LocationLists[pidx],
+ BlockStart.ID, afterPrologVal)
+ if list == nil {
+ pidx++
+ continue
+ }
+ rtypes, _ := inp.RegisterTypesAndOffsets()
+ for k, r := range inp.Registers {
+ reg := ObjRegForAbiReg(r, f.Config)
+ dwreg := ctxt.Arch.DWARFRegisters[reg]
+ if dwreg < 32 {
+ list = append(list, dwarf.DW_OP_reg0+byte(dwreg))
+ } else {
+ list = append(list, dwarf.DW_OP_regx)
+ list = dwarf.AppendUleb128(list, uint64(dwreg))
+ }
+ if len(inp.Registers) > 1 {
+ list = append(list, dwarf.DW_OP_piece)
+ ts := rtypes[k].Width
+ list = dwarf.AppendUleb128(list, uint64(ts))
+ }
+ }
+ // fill in length of location expression element
+ ctxt.Arch.ByteOrder.PutUint16(list[sizeIdx:], uint16(len(list)-sizeIdx-2))
+
+ // Second entry in the location list will be the stack home
+ // of the param, once it has been spilled. Emit that now.
+ list, sizeIdx = setupLocList(ctxt, f, list,
+ afterPrologVal, FuncEnd.ID)
+ if list == nil {
+ pidx++
+ continue
+ }
+ soff := stackOffset(sl)
+ if soff == 0 {
+ list = append(list, dwarf.DW_OP_call_frame_cfa)
+ } else {
+ list = append(list, dwarf.DW_OP_fbreg)
+ list = dwarf.AppendSleb128(list, int64(soff))
+ }
+ // fill in size
+ ctxt.Arch.ByteOrder.PutUint16(list[sizeIdx:], uint16(len(list)-sizeIdx-2))
+
+ fd.LocationLists[pidx] = list
+ pidx++
+ }
+ return &fd
+}
diff --git a/src/cmd/compile/internal/ssagen/ssa.go b/src/cmd/compile/internal/ssagen/ssa.go
index fb35d80..5eda8c4 100644
--- a/src/cmd/compile/internal/ssagen/ssa.go
+++ b/src/cmd/compile/internal/ssagen/ssa.go
@@ -6961,7 +6961,12 @@
}
if base.Ctxt.Flag_locationlists {
- debugInfo := ssa.BuildFuncDebug(base.Ctxt, f, base.Debug.LocationLists > 1, StackOffset)
+ var debugInfo *ssa.FuncDebug
+ if e.curfn.ABI == obj.ABIInternal && base.Flag.N != 0 {
+ debugInfo = ssa.BuildFuncDebugNoOptimized(base.Ctxt, f, base.Debug.LocationLists > 1, StackOffset)
+ } else {
+ debugInfo = ssa.BuildFuncDebug(base.Ctxt, f, base.Debug.LocationLists > 1, StackOffset)
+ }
e.curfn.DebugInfo = debugInfo
bstart := s.bstart
idToIdx := make([]int, f.NumBlocks())
To view, visit change 314431. To unsubscribe, or for help writing mail filters, visit settings.