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

[ille via Phabricator via cfe-commits]

ille added a comment.

One downside of that approach is performance.  It's somewhat idiosyncratic, but I work on codebases that use blocks heavily in performance-sensitive paths.  Typically, the blocks are never copied (they don't escape) and are expected to be inlined.  Any implicit heap allocations added by the compiler would have an undesirable performance cost, and would likely also hinder inlining.  For those codebases, I would much prefer to be prompted to refactor the code to avoid the self-capture.  I suppose this could be implemented as a warning, perhaps off by default.

Another, possibly more serious issue is allocation failure.  `Block_copy` returns null rather than aborting on allocation failure.  This is from the Darwin userland implementation:

  // Its a stack block.  Make a copy.
  if (!isGC) {
      struct Block_layout *result = malloc(aBlock->descriptor->size);
      if (!result) return NULL;

xnu also has a `Block_copy` implementation that can fail.

If Clang implicitly called `Block_copy` and it failed, it would have to either hand the user a null block, which would be unexpected, or abort the process.  For most codebases, aborting the process would be fine, but there are codebases that attempt to recover cleanly from all allocation failures, and some of those may happen to use blocks.  xnu is one example; I don't know of any others, but they might exist.

I suppose those codebases could also enable the hypothetical warning, but it seems like a footgun, at least if the warning is off by default.

Incidentally, I checked how this behaves under ARC.  ARC has to deal with the same situation, of course, but Objective-C generally aborts on allocation failure, so having failed block copies do the same would be more acceptable and is the behavior I'd expect.  However, `objc_retainBlock`, the function ARC generates calls to, simply forwards directly to `Block_copy`; it doesn't check for a null return value, nor does the ARC-generated code do so itself.  So the user just gets a null block, which seems like a bug.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D89903/new/

https://reviews.llvm.org/D89903