r183597 - Debug info: An if condition now creates a lexical scope of its own.

[John McCall]

On Jul 9, 2013, at 10:04 AM, Eric Christopher <echristo at gmail.com> wrote:
> On Tue, Jul 9, 2013 at 2:33 AM, John McCall <rjmccall at apple.com> wrote:
>> On Jun 17, 2013, at 2:32 PM, Eric Christopher <echristo at gmail.com> wrote:
>>> On Mon, Jun 17, 2013 at 2:29 PM, Robinson, Paul
>>> <Paul_Robinson at playstation.sony.com> wrote:
>>>>> From: cfe-commits-bounces at cs.uiuc.edu [mailto:cfe-commits-
>>>>> bounces at cs.uiuc.edu] On Behalf Of Eric Christopher
>>>>> Sent: Monday, June 10, 2013 4:05 PM
>>>>> To: Adrian Prantl
>>>>> Cc: Nadav Rotem; cfe-commits at cs.uiuc.edu
>>>>> Subject: Re: r183597 - Debug info: An if condition now creates a lexical
>>>>> scope of its own.
>>>>> 
>>>>> On Mon, Jun 10, 2013 at 3:56 PM, Adrian Prantl <aprantl at apple.com>
>>>>> wrote:
>>>>>> (CC'ing John because he understands the intricacies of LexicalScope
>>>>> better than I do)
>>>>>> 
>>>>>> On the first glimpse LexicalScope appears to be a subclass of
>>>>> RunCleanupsScope that additionally emits a (DebugInfo-)LexicalScope. But
>>>>> looking at the destructors it appears that they have slightly different
>>>>> semantics: ~LexicalScope runs ForceCleanup and ~RunCleanupsScope
>>>>> apparently doesn't.
>>>>>> 
>>>>>> I'm wary that switching to Lexicalscope in
>>>>> CodeGenFunction::EmitIfStmt() might lead to tricky ARC or EH-related
>>>>> problems because of that.
>>>>>> 
>>>>>> Does anyone have an opinion on that?
>>>>> 
>>>>> That bit was added here:
>>>>> 
>>>>> commit 495cfa46300979642acde8d93a1f21c9291dac98
>>>>> Author: Nadav Rotem <nrotem at apple.com>
>>>>> Date:   Sat Mar 23 06:43:35 2013 +0000
>>>>> 
>>>>>   Make clang to mark static stack allocations with lifetime markers
>>>>> to enable a more aggressive stack coloring.
>>>>>   Patch by John McCall with help by Shuxin Yang.
>>>>>   rdar://13115369
>>>>> 
>>>>> 
>>>>> and oddly not to RunCleanupsScope.
>>>>> 
>>>>> Nadav?
>>>>> 
>>>>> -eric
>>>> 
>>>> Looks like ~LexicalScope() just wants RunCleanupsScope::ForceCleanup()
>>>> to happen before rescopeLabels().  All the right stuff happens in the right
>>>> order if you change the RunCleanupsScope instance to LexicalScope.
>>>> (RunCleanupsScope::ForceCleanup() does pretty much exactly the same thing
>>>> as ~RunCleanupsScope() so it works out.  It could be done in a more obvious
>>>> way, but functionally they're equivalent.)
>>>> 
>>> 
>>> Agreed. I was just curious why Nadav changed one, but not the other.
>> 
>> You're asking, why does only LexicalScope manage the set of labels to
>> rescope?  Because RunCleanupsScope is specifically just supposed to
>> manage the cleanup stack; it's not meant to do every possible thing that
>> you might want to do with a language-level lexical scope.
>> 
> 
> *nod* That's fair. In this case we have a cleanup scope and a debug
> lexical scope (though not enshrined as such).

Ah, sure.  In that case, there's no harm from rescoping labels.  If you see
any generation difference, it's more likely to be a reordering of the cleanups
around jump emission or something similar.

>> Specifically, rescoping labels isn't necessary for an arbitrary bundle of
>> cleanups (like a full expression) because you can't typically jump into
>> such things, and permitting that would involve more than just rescoping
>> some labels.
> 
> I think this makes sense, but an example would probably illuminate.

You can't jump into an expression because there's stuff gettin' evaluated there.

>> You really don't need a full LexicalScope here; all of the child
>> expressions and statements are already being appropriately scoped.
>> You should just make your own RAII class.
> 
> Oddly enough I did. It was called "LexicalScope" for things that
> needed a debug lexical scope along with the assorted cleanups. It
> seems to have been appropriated though :)

Well, I didn't realize the code in this case was so closely aligned with
an actual cleanups scope.

John.