[go] cmd/compile: generate single deferreturn block
Youlin Feng (Gerrit)
Youlin Feng has uploaded the change for review![]( "Open in Gerrit")
Commit message
cmd/compile: generate single deferreturn block
For the following code, we used to generate two Ret blocks for fn, one
as the function's normal return, the other as the defer's recovery code
path, both containing a call to runtime.deferreturn.
func fn(n int, f func(int)) {
for i := range n {
defer f(i)
}
}
On AMD64, the Ret blocks look like this:
CALL runtime.deferreturn(SB)
ADDQ $0x18, SP
POPQ BP
RET
CALL runtime.deferreturn(SB)
ADDQ $0x18, SP
POPQ BP
RET
This CL omits the second block, uses the normal return block as well as
the recovery code path, thus reducing at least four instructions.
Change diff
diff --git a/src/cmd/compile/internal/ssa/check.go b/src/cmd/compile/internal/ssa/check.go
index ef06bd9..b1067fd 100644
--- a/src/cmd/compile/internal/ssa/check.go
+++ b/src/cmd/compile/internal/ssa/check.go
@@ -85,8 +85,8 @@
f.Fatalf("if block %s has non-bool control value %s", b, b.Controls[0].LongString())
}
case BlockDefer:
- if len(b.Succs) != 2 {
- f.Fatalf("defer block %s len(Succs)==%d, want 2", b, len(b.Succs))
+ if len(b.Succs) != 2 && len(b.Succs) != 1 {
+ f.Fatalf("defer block %s len(Succs)==%d, want 2 or 1", b, len(b.Succs))
}
if b.NumControls() != 1 {
f.Fatalf("defer block %s has no control value", b)
@@ -94,6 +94,22 @@
if !b.Controls[0].Type.IsMemory() {
f.Fatalf("defer block %s has non-memory control value %s", b, b.Controls[0].LongString())
}
+ var deferReturn bool
+ for _, b := range f.Blocks {
+ if b.Kind != BlockRet || len(b.Preds) == 0 {
+ continue
+ }
+ for _, v := range b.Values {
+ if c, ok := v.Aux.(*AuxCall); ok && c.Fn.String() == "runtime.deferreturn" {
+ deferReturn = true
+ break
+ }
+ }
+ break
+ }
+ if !deferReturn {
+ f.Fatalf("defer block %s has no deferreturn", b)
+ }
case BlockFirst:
if len(b.Succs) != 2 {
f.Fatalf("plain/dead block %s len(Succs)==%d, want 2", b, len(b.Succs))
diff --git a/src/cmd/compile/internal/ssa/func.go b/src/cmd/compile/internal/ssa/func.go
index fc8cb3f..1a14eda 100644
--- a/src/cmd/compile/internal/ssa/func.go
+++ b/src/cmd/compile/internal/ssa/func.go
@@ -46,7 +46,7 @@
NoSplit bool // true if function is marked as nosplit. Used by schedule check pass.
dumpFileSeq uint8 // the sequence numbers of dump file. (%s_%02d__%s.dump", funcname, dumpFileSeq, phaseName)
IsPgoHot bool
- DeferReturn *Block // avoid creating more than one deferreturn if there's multiple calls to deferproc-etc.
+ Defer *Block // avoid creating more than one deferreturn if there's multiple calls to deferproc-etc.
// when register allocation is done, maps value ids to locations
RegAlloc []Location
diff --git a/src/cmd/compile/internal/ssagen/ssa.go b/src/cmd/compile/internal/ssagen/ssa.go
index db2ffb5..0529330 100644
--- a/src/cmd/compile/internal/ssagen/ssa.go
+++ b/src/cmd/compile/internal/ssagen/ssa.go
@@ -562,11 +562,20 @@
s.paramsToHeap()
s.stmtList(fn.Body)
+ var r *ssa.Block
// fallthrough to exit
if s.curBlock != nil {
s.pushLine(fn.Endlineno)
- s.exit()
+ r = s.exit()
s.popLine()
+ } else if s.f.Defer != nil {
+ r = s.f.NewBlock(ssa.BlockPlain) // Share a single deferreturn among all defers
+ s.startBlock(r)
+ s.exit()
+ }
+ if r != nil && len(r.Preds) == 0 && s.f.Defer != nil {
+ s.f.Defer.AddEdgeTo(r) // Add recover edge to exit code. This is a fake edge to keep the block live.
+ s.f.Defer.Likely = ssa.BranchLikely
}
for _, b := range s.f.Blocks {
@@ -4927,15 +4936,7 @@
b.SetControl(call)
bNext := s.f.NewBlock(ssa.BlockPlain)
b.AddEdgeTo(bNext)
- r := s.f.DeferReturn // Share a single deferreturn among all defers
- if r == nil {
- r = s.f.NewBlock(ssa.BlockPlain)
- s.startBlock(r)
- s.exit()
- s.f.DeferReturn = r
- }
- b.AddEdgeTo(r) // Add recover edge to exit code. This is a fake edge to keep the block live.
- b.Likely = ssa.BranchLikely
+ s.f.Defer = b
s.startBlock(bNext)
}
Change information
- M src/cmd/compile/internal/ssa/check.go
- M src/cmd/compile/internal/ssa/func.go
- M src/cmd/compile/internal/ssagen/ssa.go
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- David Chase
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- David Chase
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 2 comments![]( "Open in Gerrit")
// block that used as the recovery code path. Since the normal returnthat is used
p.b.ResetControls()I'm not sure I understand how this is supposed to work.
The normal return block might have other stuff in it.
For instance, this program:
```
var g int
func fn(n int, f func(int)) {
for i := range n {
defer f(i)
}
g = 5
}
```
The normal return block has the `g=5` store in it, but we can't have that in the deferreturn path.
Also, if I remember correctly the OpDefer needs to stay around for liveness analysis. (I don't remember what the issue was, so maybe no longer an issue?)
Related details
- David Chase
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- David Chase
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng voted and added 2 comments![]( "Open in Gerrit")
Votes added by Youlin Feng
| Commit-Queue | +1 |
2 comments
// block that used as the recovery code path. Since the normal returnYoulin Fengthat is used
Done
I'm not sure I understand how this is supposed to work.
The normal return block might have other stuff in it.For instance, this program:
```
var g int
func fn(n int, f func(int)) {
for i := range n {
defer f(i)
}
g = 5
}
```
The normal return block has the `g=5` store in it, but we can't have that in the deferreturn path.Also, if I remember correctly the OpDefer needs to stay around for liveness analysis. (I don't remember what the issue was, so maybe no longer an issue?)
The final part of a normal return block and the whole deferreturn block both are generated by the `(*state).exit()`, the assembly code in both blocks that start from the instruction that calls `runtime.deferreturn` should be same. The panic recovery will jump back to the instruction that calls `runtime.deferreturn`, so the code in the normal return block that before this instruction may not matter.
I can find an OpDefer, maybe you mean BlockDefer? I don't known.
Related details
- David Chase
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
p.b.ResetControls()Youlin FengI'm not sure I understand how this is supposed to work.
The normal return block might have other stuff in it.For instance, this program:
```
var g int
func fn(n int, f func(int)) {
for i := range n {
defer f(i)
}
g = 5
}
```
The normal return block has the `g=5` store in it, but we can't have that in the deferreturn path.Also, if I remember correctly the OpDefer needs to stay around for liveness analysis. (I don't remember what the issue was, so maybe no longer an issue?)
The final part of a normal return block and the whole deferreturn block both are generated by the `(*state).exit()`, the assembly code in both blocks that start from the instruction that calls `runtime.deferreturn` should be same. The panic recovery will jump back to the instruction that calls `runtime.deferreturn`, so the code in the normal return block that before this instruction may not matter.
I can find an OpDefer, maybe you mean BlockDefer? I don't known.
Yes, sorry, BlockDefer.
I think maybe it is just used for https://github.com/golang/go/blob/34fec512ce34fb5926aa38e0ccd0083feed94733/src/cmd/compile/internal/ssagen/ssa.go#L4937 . Maybe you would need to add Func.DeferReturn as a reachable block here: https://github.com/golang/go/blob/34fec512ce34fb5926aa38e0ccd0083feed94733/src/cmd/compile/internal/ssa/deadcode.go#L24 ?
But then it won't get deadcoded if the defer statement is determined to be unreachable.
Related details
- David Chase
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Cherry Mui (Gerrit)
Cherry Mui added 1 comment![]( "Open in Gerrit")
cmd/compile/internal/ssa: remove extra deferreturn block in prove passWhy prove pass? This doesn't seem to have anything to do with prove. If we want to do this (which I'm not sure), the deadcode pass seems more reasonable.
Related details
- David Chase
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- David Chase
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng voted and added 2 comments![]( "Open in Gerrit")
Votes added by Youlin Feng
| Commit-Queue | +1 |
2 comments
cmd/compile/internal/ssa: remove extra deferreturn block in prove passWhy prove pass? This doesn't seem to have anything to do with prove. If we want to do this (which I'm not sure), the deadcode pass seems more reasonable.
Yes, I initially tried to do this in the deadcode pass, but it didn't work. In fact, DeferReturn will not be a dead block in most cases, as every BlockDefer maintains an edge to it. What I’m doing in the prove pass is more like proving that the DeferReturn block is useless via a rule: "when there is a live normal return block". This rule cannot be satisfied if the final outcome is a panic or an infinite loop, in which cases the normal return block will be dead. If I remove the DeferReturn block in the deadcode pass using the same rule, and the normal return block is later removed in subsequent passes (for example, the opt pass), a "no deferreturn" runtime error will occur. Thus, moving this logic to the deadcode pass seems to execute it too early—I need to do it a little later, and it seems that the generic deadcode pass may be suitable. I’m not familiar with many of the passes; perhaps there are better alternatives?
p.b.ResetControls()Youlin FengI'm not sure I understand how this is supposed to work.
The normal return block might have other stuff in it.For instance, this program:
```
var g int
func fn(n int, f func(int)) {
for i := range n {
defer f(i)
}
g = 5
}
```
The normal return block has the `g=5` store in it, but we can't have that in the deferreturn path.Also, if I remember correctly the OpDefer needs to stay around for liveness analysis. (I don't remember what the issue was, so maybe no longer an issue?)
Keith RandallThe final part of a normal return block and the whole deferreturn block both are generated by the `(*state).exit()`, the assembly code in both blocks that start from the instruction that calls `runtime.deferreturn` should be same. The panic recovery will jump back to the instruction that calls `runtime.deferreturn`, so the code in the normal return block that before this instruction may not matter.
I can find an OpDefer, maybe you mean BlockDefer? I don't known.
Yes, sorry, BlockDefer.
I think maybe it is just used for https://github.com/golang/go/blob/34fec512ce34fb5926aa38e0ccd0083feed94733/src/cmd/compile/internal/ssagen/ssa.go#L4937 . Maybe you would need to add Func.DeferReturn as a reachable block here: https://github.com/golang/go/blob/34fec512ce34fb5926aa38e0ccd0083feed94733/src/cmd/compile/internal/ssa/deadcode.go#L24 ?
But then it won't get deadcoded if the defer statement is determined to be unreachable.
I tried implementing it in the deadcode pass. How does it look this time?
Related details
- Cherry Mui
- David Chase
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
reachable[f.DeferReturn.ID] = falseI don't understand how this works. If we mark the deferreturn block as unreachable, then what happens to the BlockDefer blocks?
What do we mark as the final deferreturn entry point?
Related details
- Cherry Mui
- David Chase
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng added 1 comment![]( "Open in Gerrit")
reachable[f.DeferReturn.ID] = falseI don't understand how this works. If we mark the deferreturn block as unreachable, then what happens to the BlockDefer blocks?
What do we mark as the final deferreturn entry point?
Because we have two BlockRet blocks both containing the `CALL runtime.deferreturn(SB)`, one is the normal return block of the function, the other one is only used as the recovery destination.
```
BlockDefer
/\
/ \
/ \
/ \
BlockXXX BlockRet (DeferReturn, the recovery path)
|
|
Blocks
|
|
BlockRet (normal return)
```
The linker picks the first deferreturn call it sees, https://github.com/golang/go/blob/go1.25.4/src/cmd/compile/internal/ssagen/ssa.go#L2296. So, when the DeferReturn block is removed, the `recovery()` can use the `CALL runtime.deferreturn(SB)` in the normal return block as its destination.
Is this okay? Did I miss something?
Related details
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
reachable[f.DeferReturn.ID] = falseYoulin FengI don't understand how this works. If we mark the deferreturn block as unreachable, then what happens to the BlockDefer blocks?
What do we mark as the final deferreturn entry point?
Because we have two BlockRet blocks both containing the `CALL runtime.deferreturn(SB)`, one is the normal return block of the function, the other one is only used as the recovery destination.
```
BlockDefer
/\
/ \
/ \
/ \
BlockXXX BlockRet (DeferReturn, the recovery path)
|
|
Blocks
|
|
BlockRet (normal return)
```
The linker picks the first deferreturn call it sees, https://github.com/golang/go/blob/go1.25.4/src/cmd/compile/internal/ssagen/ssa.go#L2296. So, when the DeferReturn block is removed, the `recovery()` can use the `CALL runtime.deferreturn(SB)` in the normal return block as its destination.Is this okay? Did I miss something?
I worry that a specific deadcode pass still has a reachable return block, so we convert BlockDefer to BlockPlain. But then a subsequent deadcode pass might get rid of the return block we were counting on. Then we end up with no deferreturn call anywhere.
(We run deadcode 8 times when compiling.)
Related details
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Youlin Feng
| Commit-Queue | +1 |
1 comment
How about running it in the "late deadcode" pass? This is the last of the eight deadcode passes.
Related details
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng added 1 comment![]( "Open in Gerrit")
reachable[f.DeferReturn.ID] = falseYoulin FengI don't understand how this works. If we mark the deferreturn block as unreachable, then what happens to the BlockDefer blocks?
What do we mark as the final deferreturn entry point?
Keith RandallBecause we have two BlockRet blocks both containing the `CALL runtime.deferreturn(SB)`, one is the normal return block of the function, the other one is only used as the recovery destination.
```
BlockDefer
/\
/ \
/ \
/ \
BlockXXX BlockRet (DeferReturn, the recovery path)
|
|
Blocks
|
|
BlockRet (normal return)
```
The linker picks the first deferreturn call it sees, https://github.com/golang/go/blob/go1.25.4/src/cmd/compile/internal/ssagen/ssa.go#L2296. So, when the DeferReturn block is removed, the `recovery()` can use the `CALL runtime.deferreturn(SB)` in the normal return block as its destination.Is this okay? Did I miss something?
I worry that a specific deadcode pass still has a reachable return block, so we convert BlockDefer to BlockPlain. But then a subsequent deadcode pass might get rid of the return block we were counting on. Then we end up with no deferreturn call anywhere.
(We run deadcode 8 times when compiling.)
How about running it in the "late deadcode" pass? This is the last of the eight deadcode passes.
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
reachable[f.DeferReturn.ID] = falseYoulin FengI don't understand how this works. If we mark the deferreturn block as unreachable, then what happens to the BlockDefer blocks?
What do we mark as the final deferreturn entry point?
Keith RandallBecause we have two BlockRet blocks both containing the `CALL runtime.deferreturn(SB)`, one is the normal return block of the function, the other one is only used as the recovery destination.
```
BlockDefer
/\
/ \
/ \
/ \
BlockXXX BlockRet (DeferReturn, the recovery path)
|
|
Blocks
|
|
BlockRet (normal return)
```
The linker picks the first deferreturn call it sees, https://github.com/golang/go/blob/go1.25.4/src/cmd/compile/internal/ssagen/ssa.go#L2296. So, when the DeferReturn block is removed, the `recovery()` can use the `CALL runtime.deferreturn(SB)` in the normal return block as its destination.Is this okay? Did I miss something?
Youlin FengI worry that a specific deadcode pass still has a reachable return block, so we convert BlockDefer to BlockPlain. But then a subsequent deadcode pass might get rid of the return block we were counting on. Then we end up with no deferreturn call anywhere.
(We run deadcode 8 times when compiling.)
How about running it in the "late deadcode" pass? This is the last of the eight deadcode passes.
I don't want to go down that route. The SSA representation shouldn't have implicit constraints like that (in this case, that would be "can't run deadcode anymore after this phase"). We already have one of those (no deadcode after regalloc), I'd rather not add more.
You would need to make that dependency explicit somehow.
Idea: when a function has defers, have ssagen.(*state).exit use a jump to an existing block if it has run before. This block would be after regular return code has executed (e.g. calling `f` in `return f()`), but would have a deferreturn in it.
Related details
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng added 1 comment![]( "Open in Gerrit")
reachable[f.DeferReturn.ID] = falseYoulin FengI don't understand how this works. If we mark the deferreturn block as unreachable, then what happens to the BlockDefer blocks?
What do we mark as the final deferreturn entry point?
Keith RandallBecause we have two BlockRet blocks both containing the `CALL runtime.deferreturn(SB)`, one is the normal return block of the function, the other one is only used as the recovery destination.
```
BlockDefer
/\
/ \
/ \
/ \
BlockXXX BlockRet (DeferReturn, the recovery path)
|
|
Blocks
|
|
BlockRet (normal return)
```
The linker picks the first deferreturn call it sees, https://github.com/golang/go/blob/go1.25.4/src/cmd/compile/internal/ssagen/ssa.go#L2296. So, when the DeferReturn block is removed, the `recovery()` can use the `CALL runtime.deferreturn(SB)` in the normal return block as its destination.Is this okay? Did I miss something?
Youlin FengI worry that a specific deadcode pass still has a reachable return block, so we convert BlockDefer to BlockPlain. But then a subsequent deadcode pass might get rid of the return block we were counting on. Then we end up with no deferreturn call anywhere.
(We run deadcode 8 times when compiling.)
Keith RandallHow about running it in the "late deadcode" pass? This is the last of the eight deadcode passes.
I don't want to go down that route. The SSA representation shouldn't have implicit constraints like that (in this case, that would be "can't run deadcode anymore after this phase"). We already have one of those (no deadcode after regalloc), I'd rather not add more.
You would need to make that dependency explicit somehow.
Idea: when a function has defers, have ssagen.(*state).exit use a jump to an existing block if it has run before. This block would be after regular return code has executed (e.g. calling `f` in `return f()`), but would have a deferreturn in it.
Initially, I tried to do that but failed. However, after a few days of research, it now seems to work properly. PTAL.
Related details
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
Related details
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 2 comments![]( "Open in Gerrit")
b := s.exit()We set the position of the exit block here. Which would be the deferreturn block, I believe. I don't think we want to do that.
Maybe we need some other mechanism for setting the line number for non-defer returns.
const shareDeferExits = falseWe're currently not sharing the equivalent exit code for open defers. Shouldn't we do that as well? Or does that indicate that sharing is actually erasing line number information that we want?
Consider this program:
```
package main
import "runtime/debug"
func f1(b bool) {
defer g()
if b {
return
}
return
}func f2(b bool) {
for range 1 {
defer g()
}
if b {
return
}
return
}func g() {
debug.PrintStack()
}func main() {
f1(false)
f1(true)
f2(false)
f2(true)
}
```For `f1`, which uses open defers, we get the correct line number for the `return` statement in the stack trace.
For `f2`, which uses regular defers, we get the line number of the closing brace.
I think I'd rather fix the backtraces to have the correct line number. That fix would require us not to share the deferreturn call, right?
I'm still not quite sure I understand what we promise about line numbers. There are I think 3 cases:
1) No panics, just normal running of defers when returning. I think we want the line number of the `return` in that case (or closing brace, if we're running off the end of a function with no return values).
2) Panics, running the defer. We want the line number where the panic occurred (or the call we were at when some child panicked). I think we do that?
3) Panics, a previous defer has recovered, and we're running subsequent defers. Here I think the line number doesn't matter much. Maybe the function closing brace is the right place. Maybe any return is ok? I guess the original panic position would also be ok, if we still knew that.
I think #3 is the line number of the deferreturn call that the runtime jumps to.
#1 is the line number of the deferreturn call at each return instruction. So I would think we can share #3 with any of the #1 deferreturns, but we can't share two different #1 deferreturns.
Just thinking this out loud. Not sure I'm making a whole lot of sense.
Related details
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
Related details
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng added 3 comments![]( "Open in Gerrit")
We set the position of the exit block here. Which would be the deferreturn block, I believe. I don't think we want to do that.
Maybe we need some other mechanism for setting the line number for non-defer returns.
If we share the deferreturn block only with one normal return, maybe it won't be a problem.
I think I understand what you mean.
s.pushLine(s.curfn.Endlineno)I don't known why we set the line number here. I need to remove it to fix the line number in the stack trace.
Related details
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
s.pushLine(s.curfn.Endlineno)I don't known why we set the line number here. I need to remove it to fix the line number in the stack trace.
That code and comment was added in https://go-review.googlesource.com/c/go/+/650795
./all.bash passes for me if I remove this line and the corresponding popLine. So maybe there's no test that checks this line number currently?
Related details
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng added 1 comment![]( "Open in Gerrit")
s.pushLine(s.curfn.Endlineno)Keith RandallI don't known why we set the line number here. I need to remove it to fix the line number in the stack trace.
That code and comment was added in https://go-review.googlesource.com/c/go/+/650795
./all.bash passes for me if I remove this line and the corresponding popLine. So maybe there's no test that checks this line number currently?
It seems so. But how can we make the linker select the deferreturn corresponding to the line number of the function’s closing brace?
Related details
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 1 comment![]( "Open in Gerrit")
s.pushLine(s.curfn.Endlineno)Keith RandallI don't known why we set the line number here. I need to remove it to fix the line number in the stack trace.
Youlin FengThat code and comment was added in https://go-review.googlesource.com/c/go/+/650795
./all.bash passes for me if I remove this line and the corresponding popLine. So maybe there's no test that checks this line number currently?
It seems so. But how can we make the linker select the deferreturn corresponding to the line number of the function’s closing brace?
I don't know. I'm not sure how much the linker understands line numbers.
Related details
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
- TryBots-Pass: LUCI-TryBot-Result+1 by Go LUCI
Related details
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng uploaded new patchset![]( "Open in Gerrit")
Related details
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng voted and added 1 comment![]( "Open in Gerrit")
Votes added by Youlin Feng
| Commit-Queue | +1 |
1 comment
s.pushLine(s.curfn.Endlineno)Keith RandallI don't known why we set the line number here. I need to remove it to fix the line number in the stack trace.
Youlin FengThat code and comment was added in https://go-review.googlesource.com/c/go/+/650795
./all.bash passes for me if I remove this line and the corresponding popLine. So maybe there's no test that checks this line number currently?
Keith RandallIt seems so. But how can we make the linker select the deferreturn corresponding to the line number of the function’s closing brace?
I don't know. I'm not sure how much the linker understands line numbers.
I fixed the line number of `CALL runtime.deferreturn(SB)`, so when we print the stack trace in defer, we can get the correct line number if the function returns normally. PTAL.
Related details
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Youlin Feng (Gerrit)
Youlin Feng voted Commit-Queue+1![]( "Open in Gerrit")
| Commit-Queue | +1 |
Related details
- Keith Randall
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Keith Randall (Gerrit)
Keith Randall added 3 comments![]( "Open in Gerrit")
} else if s.peekPos() == s.curfn.Endlineno {I'd rather pass an argument to `exit` than change what it does based on `peekPos`.
Some kind of enum, representing explicit return, deferreturn, or fall off end, perhaps?
if r == nil {Can this ever happen? peekPos() == s.curfn.Endlineno would only ever happen for the fall-off-end case, and that call would always happen after the calls from the defer sites, which would have set s.f.DeferReturn?
s.exit()This recursive call is strange, and confusing. Why organize it this way?
Related details
- Youlin Feng
Code-Review
No-Unresolved-Comments
Review-Enforcement
TryBots-Pass
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |