[PATCH] Inconsistency in Preprocessor::ReleaseMacroInfo(MacroInfo *MI)

[Yaron Keren]

Thanks Richard!



2014-08-04 2:50 GMT+03:00 Richard Smith <richard at metafoo.co.uk>:

> Fixed in a much more simple way in r214675. Thanks for reporting!



On Sun, Aug 3, 2014 at 11:52 AM, Vassil Vassilev <vvasilev at cern.ch> wrote:

>  I will try just one more time and then shut up :)
>
>
> diff --git a/lib/Lex/PPDirectives.cpp b/lib/Lex/PPDirectives.cpp
> index 5f38387..000ea7a 100644
> --- a/lib/Lex/PPDirectives.cpp
> +++ b/lib/Lex/PPDirectives.cpp
> @@ -94,6 +94,19 @@
> Preprocessor::AllocateVisibilityMacroDirective(SourceLocation Loc,
>  /// error in the macro definition.
>  void Preprocessor::ReleaseMacroInfo(MacroInfo *MI) {
>    // Don't try to reuse the storage; this only happens on error paths.
> +
> +  // If this is on the macro info chain, avoid double deletion on
> teardown.
> +  MacroInfoChain *current = MIChainHead;
> +  while (MacroInfoChain *I = current) {
> +    if (&(I->MI) == MI) {
> +      I->Next = (I->Next) ? I->Next->Next : 0;
> +      if (I == MIChainHead)
> +        MIChainHead = I->Next;
>
> +      break;
> +    }
> +    current = I->Next;
> +  }
> +
>    MI->~MacroInfo();
>  }
>
>
> On 03/08/14 20:47, Vassil Vassilev wrote:
>
> Hi,
>   Sorry overlooked, thanks for pointing it out!
>   I hope this is what we want.
> Vassil
>
> diff --git a/lib/Lex/PPDirectives.cpp b/lib/Lex/PPDirectives.cpp
> index 5f38387..000ea7a 100644
> --- a/lib/Lex/PPDirectives.cpp
> +++ b/lib/Lex/PPDirectives.cpp
> @@ -94,6 +94,19 @@
> Preprocessor::AllocateVisibilityMacroDirective(SourceLocation Loc,
>  /// error in the macro definition.
>  void Preprocessor::ReleaseMacroInfo(MacroInfo *MI) {
>    // Don't try to reuse the storage; this only happens on error paths.
> +
> +  // If this is on the macro info chain, avoid double deletion on
> teardown.
> +  MacroInfoChain *current = MIChainHead;
> +  while (MacroInfoChain *I = current) {
> +    if (&(I->MI) == MI) {
> +      I->Next = (I->Next) ? I->Next->Next : 0;
> +      if (I == MIChainHead)
> +        MIChainHead = I;
> +      break;
> +    }
> +    current = I->Next;
> +  }
> +
>    MI->~MacroInfo();
>  }
>
> On 03/08/14 20:28, Yaron Keren wrote:
>
>  Hi,
>
>  MIChainHead is a pointer to the head of a linked list of MacroInfoChain
> nodes, each containing a MacroInfo and MacroInfoChain*.
>
>  Why does the while loop modify MIChainHead on every iteration?
> MIChainHead should be modified only if it points to the node containing
> the removed MacroInfo MI. In all other cases it should not change.
>
>  As it is now, the loop will always terminate with MIChainHead == nullptr.
>
>  Yaron
>
>
>
>  2014-08-03 21:10 GMT+03:00 Vassil Vassilev <vvasilev at cern.ch>:
>
>>  Hi Yaron,
>>   Yes I meant double destruction.
>> Vassil
>>
>> On 03/08/14 20:08, Yaron Keren wrote:
>>
>>  Hi Vassil,
>>
>>  Do you mean double destruction (not deletion) of MacroInfo first time
>> in ReleaseMacroInfo and the second time in ~Preprocessor via
>>  ~MacroInfoChain?
>>
>>    while (MacroInfoChain *I = MIChainHead) {
>>     MIChainHead = I->Next;
>>     I->~MacroInfoChain();
>>   }
>>
>>  or something else?
>>
>>  Yaron
>>
>>
>>
>>  2014-08-02 23:05 GMT+03:00 Vassil Vassilev <vvasilev at cern.ch>:
>>
>>> Hi,
>>>   In cases where ReleaseMacroInfo gets called and it doesn't cleanup the
>>> Preprocessor's MIChainHead can lead to double deletion. I am sending the
>>> patch that fixes the problem for me.
>>> Vassil
>>>
>>>
>>> diff --git a/lib/Lex/PPDirectives.cpp b/lib/Lex/PPDirectives.cpp
>>> index 5f38387..1a9b5eb 100644
>>> --- a/lib/Lex/PPDirectives.cpp
>>> +++ b/lib/Lex/PPDirectives.cpp
>>> @@ -94,6 +94,14 @@
>>> Preprocessor::AllocateVisibilityMacroDirective(SourceLocation Loc,
>>>  /// error in the macro definition.
>>>  void Preprocessor::ReleaseMacroInfo(MacroInfo *MI) {
>>>    // Don't try to reuse the storage; this only happens on error paths.
>>> +
>>> +  // If this is on the macro info chain, avoid double deletion on
>>> teardown.
>>> +  while (MacroInfoChain *I = MIChainHead) {
>>> +    if (&(I->MI) == MI)
>>> +      I->Next = (I->Next) ? I->Next->Next : 0;
>>> +    MIChainHead = I->Next;
>>> +  }
>>> +
>>>    MI->~MacroInfo();
>>>  }
>>>
>>> _______________________________________________
>>> cfe-commits mailing list
>>> cfe-commits at cs.uiuc.edu
>>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>>>
>>
>>
>>
>
>
>
> _______________________________________________
> cfe-commits mailing listcfe-commits at cs.uiuc.eduhttp://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140804/936eb5d6/attachment.html>