Skip to content

Commit

Permalink
fixes regression #22909; don't optimize result init if statements can…
Browse files Browse the repository at this point in the history
… raise which corrupts the compiler (#23271)

fixes #22909
required by #23267

```nim
proc foo: string =
  assert false
  result = ""
```

In the function `foo`, `assert false` raises an exception, which can
cause `result` to be uninitialized if the default result initialization
is optimized out
  • Loading branch information
ringabout authored Feb 1, 2024
1 parent 519d976 commit 7d97210
Show file tree
Hide file tree
Showing 2 changed files with 31 additions and 10 deletions.
28 changes: 19 additions & 9 deletions compiler/cgen.nim
Original file line number Diff line number Diff line change
Expand Up @@ -1033,7 +1033,7 @@ proc easyResultAsgn(n: PNode): PNode =
type
InitResultEnum = enum Unknown, InitSkippable, InitRequired

proc allPathsAsgnResult(n: PNode): InitResultEnum =
proc allPathsAsgnResult(p: BProc; n: PNode): InitResultEnum =
# Exceptions coming from calls don't have not be considered here:
#
# proc bar(): string = raise newException(...)
Expand All @@ -1048,7 +1048,7 @@ proc allPathsAsgnResult(n: PNode): InitResultEnum =
# echo "a was not written to"
#
template allPathsInBranch(it) =
let a = allPathsAsgnResult(it)
let a = allPathsAsgnResult(p, it)
case a
of InitRequired: return InitRequired
of InitSkippable: discard
Expand All @@ -1060,14 +1060,16 @@ proc allPathsAsgnResult(n: PNode): InitResultEnum =
case n.kind
of nkStmtList, nkStmtListExpr:
for it in n:
result = allPathsAsgnResult(it)
result = allPathsAsgnResult(p, it)
if result != Unknown: return result
of nkAsgn, nkFastAsgn, nkSinkAsgn:
if n[0].kind == nkSym and n[0].sym.kind == skResult:
if not containsResult(n[1]): result = InitSkippable
else: result = InitRequired
elif containsResult(n):
result = InitRequired
else:
result = allPathsAsgnResult(p, n[1])
of nkReturnStmt:
if n.len > 0:
if n[0].kind == nkEmpty and result != InitSkippable:
Expand All @@ -1076,7 +1078,7 @@ proc allPathsAsgnResult(n: PNode): InitResultEnum =
# initialized. This avoids cases like #9286 where this heuristic lead to
# wrong code being generated.
result = InitRequired
else: result = allPathsAsgnResult(n[0])
else: result = allPathsAsgnResult(p, n[0])
of nkIfStmt, nkIfExpr:
var exhaustive = false
result = InitSkippable
Expand All @@ -1102,9 +1104,9 @@ proc allPathsAsgnResult(n: PNode): InitResultEnum =
of nkWhileStmt:
# some dubious code can assign the result in the 'while'
# condition and that would be fine. Everything else isn't:
result = allPathsAsgnResult(n[0])
result = allPathsAsgnResult(p, n[0])
if result == Unknown:
result = allPathsAsgnResult(n[1])
result = allPathsAsgnResult(p, n[1])
# we cannot assume that the 'while' loop is really executed at least once:
if result == InitSkippable: result = Unknown
of harmless:
Expand All @@ -1129,9 +1131,17 @@ proc allPathsAsgnResult(n: PNode): InitResultEnum =
allPathsInBranch(n[0])
for i in 1..<n.len:
if n[i].kind == nkFinally:
result = allPathsAsgnResult(n[i].lastSon)
result = allPathsAsgnResult(p, n[i].lastSon)
else:
allPathsInBranch(n[i].lastSon)
of nkCallKinds:
if canRaiseDisp(p, n[0]):
result = InitRequired
else:
for i in 0..<n.safeLen:
allPathsInBranch(n[i])
of nkRaiseStmt:
result = InitRequired
else:
for i in 0..<n.safeLen:
allPathsInBranch(n[i])
Expand Down Expand Up @@ -1188,7 +1198,7 @@ proc genProcAux*(m: BModule, prc: PSym) =
assignLocalVar(p, resNode)
assert(res.loc.r != "")
if p.config.selectedGC in {gcArc, gcAtomicArc, gcOrc} and
allPathsAsgnResult(procBody) == InitSkippable:
allPathsAsgnResult(p, procBody) == InitSkippable:
# In an ideal world the codegen could rely on injectdestructors doing its job properly
# and then the analysis step would not be required.
discard "result init optimized out"
Expand All @@ -1208,7 +1218,7 @@ proc genProcAux*(m: BModule, prc: PSym) =
# global is either 'nil' or points to valid memory and so the RC operation
# succeeds without touching not-initialized memory.
if sfNoInit in prc.flags: discard
elif allPathsAsgnResult(procBody) == InitSkippable: discard
elif allPathsAsgnResult(p, procBody) == InitSkippable: discard
else:
resetLoc(p, res.loc)
if skipTypes(res.typ, abstractInst).kind == tyArray:
Expand Down
13 changes: 12 additions & 1 deletion tests/arc/tarc_macro.nim
Original file line number Diff line number Diff line change
Expand Up @@ -43,4 +43,15 @@ macro bar2() =
doAssert &%&%y == 1 # unary operator => need to escape
doAssert y &% y == 2 # binary operator => no need to escape
doAssert y == 3
bar2()
bar2()

block:
macro foo(a: openArray[string] = []): string =
echo a # Segfault doesn't happen if this is removed
newLit ""

proc bar(a: static[openArray[string]] = []) =
const tmp = foo(a)

# bug #22909
doAssert not compiles(bar())

0 comments on commit 7d97210

Please sign in to comment.