Hi Yaron,
On 08/04/2014 10:31 AM, Yaron Keren wrote:
Hi Vassil,
If you decide to keep the last code posted as a cling patch, it could
do with 'I' only instead of 'I' and 'current', and when MI is the
first node the code should set MIChainHead but not set its Next.
Thanks for pointing out, will do.
To the point, ReleaseMacroInfo just releases the SmallVector Tokens
memory if it wasn't small.
It did not modify anything else. You could still removeMacro without
ReleaseMacroInfo.
Thanks for explaining. My code looks like this:
void Preprocessor::removeMacro(IdentifierInfo *II, const MacroDirective
*MD) {
assert(II && MD);
assert(!MD->getPrevious() && "Already attached to a MacroDirective
history.");
//Release the MacroInfo allocated space so it can be reused.
MacroInfo* MI = MD->getMacroInfo();
if (MI) {
ReleaseMacroInfo(MI);
}
Macros.erase(II);
}
IIUC I need to check if the small vector isSmall and if not then do a
ReleaseMacro, or even this is redundant?
There's lots of places in clang where memory is allocated and not
released until destruction for performance.
The whole AST for starters...
It would be nice to early release the Tokens but In this context it
would hardly move the needle.
I agree. So I need to somehow implement it.
cling memory use should going up every iteration due to this startegy, no?
Yes, it grows. The context I want things removed is support of 'code
unloading'. Say:
[cling] #include "MyFile.h"
[cling] MyClass m; m.do();
// Figure out that do is not what I want. I edit the file and do:
[cling] #include "MyFile.h" // It would undo everything up to #include
"MyFile.h" (inclusively). I want the memory to be reduced also. This is
why I need to delete the macros and not only undef them. (The same holds
for the AST)
[cling] MyClass m; m.do(); // Here do and MyClass may have completely
different implementation.
Vassil
Yaron
2014-08-04 10:47 GMT+03:00 Vassil Vassilev
<[email protected] <mailto:[email protected]>>:
Hi Richard,
Thanks for the fix!
Unfortunately it doesn't help for cling case. I implement a
removeMacro routine using ReleaseMacroInfo. ReleaseMacroInfo
allowed me to implement efficiently the removal of a macro instead
of dragging a long def undef chains, for example.
IIUC it allowed some memory reduction in some cases for clang,
too. Is there any chance to keep the ReleaseMacroInfo upstream?
Vassil
On 08/04/2014 01:50 AM, Richard Smith wrote:
Fixed in a much more simple way in r214675. Thanks for reporting!
On Sun, Aug 3, 2014 at 11:52 AM, Vassil Vassilev
<[email protected] <mailto:[email protected]>> 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
<[email protected] <mailto:[email protected]>>:
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
<[email protected] <mailto:[email protected]>>:
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
[email protected]
<mailto:[email protected]>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected] <mailto:[email protected]>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected] <mailto:[email protected]>
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
