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

[Vassil Vassilev]

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 
>> <mailto: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
>>>     <mailto: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 <mailto:cfe-commits at cs.uiuc.edu>
>>>         http://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/20140803/79ad3806/attachment.html>