[Pseudo-ObjC] Render @autoreleasepool blocks - #8525
Conversation
4c9e11b to
47e0fd0
Compare
bdash
left a comment
There was a problem hiding this comment.
Thanks for the PR! The @autoreleasepool rendering is a nice addition. Factoring the handling of HLIL_BLOCK so it can be overridden by Obj-C rendering seems like generally the right shape.
The autorelease region matching needs some work, though.
The fundamental issue is that TryEmitBlockRegion only ever looks at the immediate children of a single block. Nothing requires that a pool's pops are all siblings of the push. An autorelease pool containing a conditional return, or a conditional break or continue when the pool is inside a loop, will result in multiple pops for a single push, with at least one at a deeper level. Those nested pops are missed by the current approach, so they'll survive into the output despite referencing a variable that is no longer visible:
while (true)
{
@autoreleasepool
{
...
if (cond)
{
// What is `context` that this refers to?
_objc_autoreleasePoolPop(context);
break;
}
}
...
}I think the shape you want is to pair pops with pushes using the pool handle rather than by position, and then elide any pop of a handle whose region is currently open, wherever it appears inside that region.
| } | ||
|
|
||
| const auto& statement = exprs[i]; | ||
| EmitStandardBlockStatement(instr, statement, isFirst, isLast, needSeparator, tokens, settings); |
There was a problem hiding this comment.
The code within EmitStandardBlockStatement assumes that isLast means the statement is the final statement in the block. This pairs it with the block.exprIndex == root check to detect and suppress a trailing void return. After this refactoring, isLast now only means it's the final statement in the span that was passed to EmitBlockStatements as exprs. PseudoObjCFunction::TryEmitBlockRegion passes a subspan of the block's instructions, which violates that assumption.
I'm not entirely sure how this would manifest in practice, but it makes the code harder to reason about.
| const HighLevelILInstruction& statement, | ||
| bool isFirst, | ||
| bool isLast, | ||
| bool& needsSeparator, |
There was a problem hiding this comment.
This being an output parameter is easy to miss.
|
This snippet, compiled with #import <Foundation/Foundation.h>
int main(int argc, char** argv)
{
for (id obj in NSProcessInfo.processInfo.environment)
{
@autoreleasepool {
if (!obj)
break;
NSLog(@"%@", obj);
}
}
} |
|
Appreciate the review, Mark! I'll investigate these tomorrow as soon as I can. |
47e0fd0 to
e2698d1
Compare
|
I made |
9a8a910 to
4f93515
Compare
Translate autorelease pool runtime function calls to
@autoreleasepool {...}blocks.Original Objective-C snippet:
Pseudo-C snippet:
Equivalent Pseudo-Objective-C: