From 7198d79467b6fcf026e7a29e6e50104d32fe623c Mon Sep 17 00:00:00 2001
From: Keno Fischer <keno@juliacomputing.com>
Date: Tue, 14 Jan 2020 20:36:49 -0500
Subject: [PATCH] Change lowering of gc preserve (#34379)

This fixes #34247 by changing the way gc preserve is lowered.
Instead of lowering it in a macro, lower it in the frontend.
This allows us to use an SSA value directly for the return token
of the gc begin expression. This bypasses the slot-renaming pass
of the compiler, thus preventing the compiler from trying to save
and restore the token. Of course, this kind of code would generally
not be legal (because it uses an SSA value outside of the regular
domination relation), but since this is a julia restriction, not
an LLVM restriction, we can simply exempt gc_begin tokens from this
particular validation. This works fine at the LLVM level also, because
it doesn't have this particular restriction. It also doesn't have
the same correctness problems as doing the same for non-token values,
as the tokens get lowered away by the try/catch lowering before reaching
the LLVM backend.

(cherry picked from commit 07a16d695feeba66db2a44c640675e7974ae870b)
---
 base/compiler/ssair/verify.jl | 16 ++++++++++++----
 base/gcutils.jl               |  8 +-------
 src/julia-syntax.scm          | 19 ++++++++++++++-----
 test/core.jl                  |  9 +++++++++
 4 files changed, 36 insertions(+), 16 deletions(-)

diff --git a/base/compiler/ssair/verify.jl b/base/compiler/ssair/verify.jl
index d8297f9c2cfd1e..7363e70a727254 100644
--- a/base/compiler/ssair/verify.jl
+++ b/base/compiler/ssair/verify.jl
@@ -29,6 +29,7 @@ function check_op(ir::IRCode, domtree::DomTree, @nospecialize(op), use_bb::Int,
             end
         else
             if !dominates(domtree, def_bb, use_bb) && !(bb_unreachable(domtree, def_bb) && bb_unreachable(domtree, use_bb))
+                # At the moment, we allow GC preserve tokens outside the standard domination notion
                 #@Base.show ir
                 @verify_error "Basic Block $def_bb does not dominate block $use_bb (tried to use value $(op.id))"
                 error()
@@ -189,10 +190,17 @@ function verify_ir(ir::IRCode)
                     end
                 end
             end
-            if isa(stmt, Expr) && stmt.head === :(=)
-                if stmt.args[1] isa SSAValue
-                    @verify_error "SSAValue as assignment LHS"
-                    error()
+            if isa(stmt, Expr)
+                if stmt.head === :(=)
+                    if stmt.args[1] isa SSAValue
+                        @verify_error "SSAValue as assignment LHS"
+                        error()
+                    end
+                elseif stmt.head === :gc_preserve_end
+                    # We allow gc_preserve_end tokens to span across try/catch
+                    # blocks, which isn't allowed for regular SSA values, so
+                    # we skip the validation below.
+                    continue
                 end
             end
             for op in userefs(stmt)
diff --git a/base/gcutils.jl b/base/gcutils.jl
index 95de3137deda7f..f9edb5441edb80 100644
--- a/base/gcutils.jl
+++ b/base/gcutils.jl
@@ -90,13 +90,7 @@ macro preserve(args...)
     for x in syms
         isa(x, Symbol) || error("Preserved variable must be a symbol")
     end
-    s, r = gensym(), gensym()
-    esc(quote
-        $s = $(Expr(:gc_preserve_begin, syms...))
-        $r = $(args[end])
-        $(Expr(:gc_preserve_end, s))
-        $r
-    end)
+    esc(Expr(:gc_preserve, args[end], syms...))
 end
 
 """
diff --git a/src/julia-syntax.scm b/src/julia-syntax.scm
index 7ae508c59d601d..5c936908afb068 100644
--- a/src/julia-syntax.scm
+++ b/src/julia-syntax.scm
@@ -2318,7 +2318,18 @@
                       ;; TODO: this is a hack to lower simple comprehensions to loops very
                       ;; early, to greatly reduce the # of functions and load on the compiler
                       (lower-comprehension (cadr e) (cadr (caddr e)) ranges))))
-          `(call (top collect) ,(cadr e) ,(caddr e)))))))
+          `(call (top collect) ,(cadr e) ,(caddr e)))))
+
+    'gc_preserve
+    (lambda (e)
+      (let* ((s (make-ssavalue))
+             (r (gensy)))
+        `(block
+          (= ,s (gc_preserve_begin ,@(cddr e)))
+          (= ,r ,(expand-forms (cadr e)))
+          (gc_preserve_end ,s)
+          ,r)))
+    ))
 
 (define (has-return? e)
   (expr-contains-p return? e (lambda (x) (not (function-def? x)))))
@@ -4009,10 +4020,8 @@ f(x) = yt(x)
              '(null))
 
             ((gc_preserve_begin)
-             (let ((s    (make-ssavalue))
-                   (args (compile-args (cdr e) break-labels linearize-args)))
-               (emit `(= ,s ,(cons (car e) args)))
-               s))
+             (let ((args (compile-args (cdr e) break-labels linearize-args)))
+               (cons (car e) args)))
 
             ;; metadata expressions
             ((line meta inbounds loopinfo gc_preserve_end aliasscope popaliasscope)
diff --git a/test/core.jl b/test/core.jl
index 23ed03072ea5a7..97b66724ed3f34 100644
--- a/test/core.jl
+++ b/test/core.jl
@@ -7164,3 +7164,12 @@ function mre34206(a, n)
     return b[1].offset1
 end
 @test mre34206([44], 1) == 0
+
+# Issue #34247
+function f34247(a)
+    GC.@preserve a try
+    catch
+    end
+    true
+end
+@test f34247("")