[PATCH] D89903: [CodeGen] Crash instead of generating broken code with self-capturing __block var

[ille via Phabricator via cfe-commits]

ille created this revision.
ille added reviewers: jfb, rjmccall.
Herald added a project: clang.
ille requested review of this revision.

This applies to situations where a `__block` variable's initializer references a
block that potentially captures the variable itself.

Clang special-cases this because the initializer might call Block_copy and
cause the variable to be moved to the heap while it's still being initialized.
For example, in this snippet:

  int copyBlockAndReturnInt(void (^)(void));
  
  __block int val = copyBlockAndReturnInt(^{
      printf("%p\n", val);
  });

...if `copyBlockAndReturnInt` calls `Block_copy`, `val` will be moved to the
heap.  When `copyBlockAndReturnInt` returns, the generated code needs to store
the value to the new location.

For scalar values like pointers, this is handled by deferring computation of
the variable's address until after the initializer has been evaluated (and just
before actually storing to that address).

But what about structs?  This case would be hard:

  struct Foo { int a, b; };
  int copyBlockAndReturnInt(void (^)(void));
  
  __block struct Foo foo = {42, copyBlockAndReturnInt(^{
      printf("%p\n", &foo);
  })};

To support this, Clang would have to recalculate the location of `foo`
before writing to each struct field, in case the initializer of the
current field moved the struct.  Block_copy would also end up copying a
half-initialized struct, although that's no big deal with C structs.

This C++ case, on the other hand, would be impossible:

  struct Foo {
      void (^block_)(void);
      Foo(void (^block)(void)) : {
          printf("I am at %p\n", this);
          block_ = Block_copy(block);
          printf("I am now at %p\n", this);
      }
  };
  
  __block Foo foo(^{
      printf("%p\n", &foo);
  });

To support *that*, Clang would have to magically move `foo`
mid-construction.  Normally, Clang uses the move or copy constructor
when moving C++ classes to the heap, but C++ classes certainly don't
expect a move constructor to be called in the middle of another
constructor.  memcpy might work in some cases, but it violates C++
classes' expectation that object addresses will remain stable.
Thus there's no real way to make this work, and ideally it would result in
a compile error (or, alternately, a runtime crash upon calling
`Block_copy`).

So how does Clang currently approach this situation?  Well, there's just a
comment:

  case TEK_Aggregate:
    [...]
      // TODO: how can we delay here if D is captured by its initializer?

Clang never calls `drillIntoBlockVariable`, and ends up writing the
variable to the beginning of the byref struct (not the corresponding
field in it), which is never the correct location.  This leaves both the
variable and the byref struct corrupt - even if the block is never
actually copied.

This bug dates back to at least 2012, as I reproduced it on LLVM 3.1.

The current patch is intended to be a very minimal fix, for the sake of
landing sooner.  It simply calls `CGM.ErrorUnsupported`, resulting in an
error followed by an intentional crash, if (and only if) it would
otherwise generate bad code.

Note that I'd like to follow this patch with another one that (at least):

- detects the issue earlier in the pipeline, and emits a proper compile error; and
- makes the "potentially self-capturing" analysis more precise, since it currently has a lot of false positives.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D89903

Files:
  clang/lib/CodeGen/CGDecl.cpp
  clang/test/CodeGenCXX/block-capture-own-init.cpp


Index: clang/test/CodeGenCXX/block-capture-own-init.cpp
===================================================================
--- /dev/null
+++ clang/test/CodeGenCXX/block-capture-own-init.cpp
@@ -0,0 +1,9 @@
+// RUN: not %clang_cc1 -x c++ -std=c++11 -fblocks -emit-llvm %s 2>&1 | FileCheck %s
+
+void test_aggregate_captured_by_own_init() {
+  struct foo { int a[100]; };
+  extern foo get_foo(foo *(^)());
+  // CHECK: error: cannot compile this aggregate initialized with potentially self-capturing block yet
+  __block foo f = get_foo(^{ return &f; });
+}
+
Index: clang/lib/CodeGen/CGDecl.cpp
===================================================================
--- clang/lib/CodeGen/CGDecl.cpp
+++ clang/lib/CodeGen/CGDecl.cpp
@@ -1921,7 +1921,12 @@
         Overlap = AggValueSlot::DoesNotOverlap;
       else if (auto *FD = dyn_cast<FieldDecl>(D))
         Overlap = getOverlapForFieldInit(FD);
-      // TODO: how can we delay here if D is captured by its initializer?
+      if (capturedByInit) {
+        // TODO: how can we delay here if D is captured by its initializer?
+        CGM.ErrorUnsupported(
+            init,
+            "aggregate initialized with potentially self-capturing block");
+      }
       EmitAggExpr(init, AggValueSlot::forLValue(
                             lvalue, *this, AggValueSlot::IsDestructed,
                             AggValueSlot::DoesNotNeedGCBarriers,


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D89903.299763.patch
Type: text/x-patch
Size: 1405 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20201021/a077fe3b/attachment.bin>