Bugs item #1894726, was opened at 2008-02-16 00:44
Message generated for change (Comment added) made by ajbj
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=111005&aid=1894726&group_id=11005
Please note that this message will contain a full copy of the comment thread,
including the initial issue submission, for this request,
not just the latest update.
Category: Program
Group: None
Status: Open
Resolution: Fixed
Priority: 5
Private: No
Submitted By: Bert Wesarg (lebert)
Assigned to: Nobody/Anonymous (nobody)
Summary: fix memoy leak in PromoteToGlobal()
Initial Comment:
InstallSymbol() allocates a new Symbol and put this to the GlobalSymList the
Symbol passed to PromoteToGlobal() is lost.
This patch adds the Symbol directly to the GlobalSymList, without calling
InstallSymbol().
The case in which the Symbol is already in the LocalSymbolList is not changed,
because I don't know if this can a cause SIGSEGV but it should also be
corrected (ie freeing sym).
----------------------------------------------------------------------
Comment By: Tony Balinski (ajbj)
Date: 2008-03-31 09:03
Message:
Logged In: YES
user_id=618141
Originator: NO
Yes this is right. A very cut down version of the
NEDIT_require_macro_file() functionality!
I must apologise though: I think I must have mucked up my patched version
using your checked-in code. I've reworked it again and, like you, never hit
one of the fprintf()s you added. Since I had removed the patched version, I
cannot say what I did wrong though!
Sorry for the lost bandwidth. I would still change your code though, not
to change sym->type if sym was found using LookupSymbol(), even if this
should never happen. In fact, after removing the symbol from the local
list, we should perhaps just do an assert() that the same name isn't still
available with LookupSymbol() before adding it to the global list. (I for
one always compile NEdit with -g). I think that would be enough.
Tony
----------------------------------------------------------------------
Comment By: Bert Wesarg (lebert)
Date: 2008-03-29 08:52
Message:
Logged In: YES
user_id=122956
Originator: YES
Ok, my previously test case has little todo with the problem, but should
be handled as a separate bug (and I think this limitation is mostly
known).
I currently use the following test case, and can't get tony's described
problem:
-----autoload.nm-----
$loaded = $empty_array
define requires {
file = $1
if (file in $loaded) {
return
}
$loaded[file] = 1
load_macro_file(file)
t_print("loaded: " file "\n")
}
-----autoload.nm-----
-----test.nm-----
define test {
return "hello"
}
-----test.nm-----
-----test.rc-----
nedit.macroCommands: \
test:::: {\n\
requires("test.nm")\n\
a = test()\n\
b = test()\n\
t_print(a ":" b "\\n")\n\
}
-----test.rc-----
I then import the test.rc and run the 'test' macro menu entry several
times.
Tony, does this test case model your usage?
----------------------------------------------------------------------
Comment By: Bert Wesarg (lebert)
Date: 2008-03-28 14:23
Message:
Logged In: YES
user_id=122956
Originator: YES
File Added: symbol-debug.patch
----------------------------------------------------------------------
Comment By: Bert Wesarg (lebert)
Date: 2008-03-28 14:21
Message:
Logged In: YES
user_id=122956
Originator: YES
Ohh, I think this is stranger than i thought. I currently try this macro
file, to reproduce your behaviuor:
-----test.nm-----
a = test()
b = test()
define test {
return "hello"
}
c = test()
t_print(a ":" b ":" c "\n")
-----test.nm-----
and with the debugging code in the attached patch, I get this:
parse.y:559:LookupSymbol: a [notfound]
parse.y:562:InstallSymbol: a [local]
parse.y:559:LookupSymbol: test [notfound]
parse.y:562:InstallSymbol: test [local]
parse.y:320:PromoteToGlobal: test [local]
interpret.c:806:LookupSymbol: test [notfound]
parse.y:559:LookupSymbol: b [notfound]
parse.y:562:InstallSymbol: b [local]
parse.y:559:LookupSymbol: test
parse.y:320:PromoteToGlobal: test
interpret.c:717:InstallSymbol: string #52
macro.c:889:LookupSymbol: test
parse.y:559:LookupSymbol: c [notfound]
parse.y:562:InstallSymbol: c [local]
parse.y:559:LookupSymbol: test
parse.y:320:PromoteToGlobal: test
parse.y:559:LookupSymbol: t_print
parse.y:559:LookupSymbol: a [notfound]
parse.y:562:InstallSymbol: a [local]
interpret.c:717:InstallSymbol: string #53
parse.y:559:LookupSymbol: b [notfound]
parse.y:562:InstallSymbol: b [local]
parse.y:559:LookupSymbol: c [local]
parse.y:243:PromoteToGlobal: t_print
plus the 'test is not a function'-error and a 'a is not defined'-error.
note, I don't get any of the two fprintf's from PromoteToGlobal.
I dig further and reopen this bug.
----------------------------------------------------------------------
Comment By: Tony Balinski (ajbj)
Date: 2008-03-28 10:36
Message:
Logged In: YES
user_id=618141
Originator: NO
I've tried using the currently checked in code and it breaks my macro file
loading. It also never deletes an unneeded symbol so could present a memory
leak, although in practice this should never happen (see below).
I use my NEDIT_require_macro_file() macros for just-in-time macro loading.
It calls load_macro_file() for its parameter if it hasn't already seen that
file name. Now the rest of the macro code after the
NEDIT_require_macro_file() call contains calls to the functions defined in
the file to be loaded. Since these are unknown when the macro code is
compiled, their identifiers end up being promoted from local symbols to
global symbols. If this happens more than once, I get the "duplicate
symbol" messages. What's worse is that the symbol type is reset each time
to GLOBAL_SYM, so the interpreter doesn't know what kind of symbol
(function) it's dealing with! I get "<function> is not a function or
subroutine" dialogs.
As for the "memory leak", the parser creates a local symbol if it can't
find one
for the identifier found. This needs promoting to global (just to change
its context) when the parser decides it is in fact a function. If a
situation existed whereby a symbol with the same identifier were already
present in the global list, the local symbol would be redundant and should
be deleted. However, since, if the global symbol already existed we
wouldn't have needed to create the local symbol in the first place, so this
shouldn't happen. (So your reflection about the LookupSymbol() being
redundant is true, overall.)
All this means, by the way, that you cannot have a local symbol for a
variable with the same name as a function name, unless the function is
compiled after the code containing the variable! This might mean that a
declaration keyword ("local" or "var" spring to mind) might be good to tie
a symbol down to local name scope, making it unpromotable.
----------------------------------------------------------------------
Comment By: Bert Wesarg (lebert)
Date: 2008-03-27 18:21
Message:
Logged In: YES
user_id=122956
Originator: YES
> So in all I think you're being overly cautious here.
I thought that was clear from my comments in the code. I really don't
expect that any of the two fprintf's ever trigger.
Actually, I now have the opinion, that even the LockupSymbol in the
PromoteToGlobal function will never return non-NULL. So this could be
removed.
----------------------------------------------------------------------
Comment By: Tony Balinski (ajbj)
Date: 2008-03-27 07:42
Message:
Logged In: YES
user_id=618141
Originator: NO
It does what the old function did (which must have been there ever since
the macro language was invented!), but without the leak you discovered. As
for safety, I don't think this is much of a concern. The equivalence of the
LOCAL_SYM flag and the local symbol list (which gets attached to the
compiled function) is enshrined in the InstallSymbol() function, which is
always used one way or another to register a symbol to either the global or
local context (with the SINGLE exception of my proposed change). As for
assumptions, you test whether a symbol marked LOCAL_SYM is in the global
list: this should really be done as an assertion since it should never
happen and represents a mistake in code. The other causes complaints when a
new function name is found before it is defined. This is a legitimate
occurrence and causes complaints to occur with mutually recursive functions
and "include file"-style function loading. So in all I think you're being
overly cautious here.
----------------------------------------------------------------------
Comment By: Bert Wesarg (lebert)
Date: 2008-03-26 10:21
Message:
Logged In: YES
user_id=122956
Originator: YES
This is the optimistic version.
You assume that PromoteToGlobal() has the only reference to the symbol
passed. This is currently correct. If you assume this, than you can simply
free the symbol and return the symbol from the GlobalSymList, in case the
symbol was found there, which should not happen currently. The next
assumption you made is, that a LOCAL_SYM can never be in the GlobalSymList.
This is currently true, hopefully.
I have tried to address both assumptions in the commit and put in
fprintf's. Also, as I stated in the comment, I think these are parser
errors, which should error out the parsing process.
----------------------------------------------------------------------
Comment By: Tony Balinski (ajbj)
Date: 2008-03-26 01:55
Message:
Logged In: YES
user_id=618141
Originator: NO
After consideration I don't agree with the committed solution.
All that needs to be done is: if the symbol being promoted is on the local
list, take it off that one, change its type to GLOBAL_SYM, then add it to
the global list if it isn't already there, or delete it if it is.
See http://www.nedit.org/pipermail/develop/2008-March/014355.html and
http://www.nedit.org/pipermail/develop/2008-March/014365.html (my analysis
was correct and Bert's response true).
The solution I propose is:
/*
** Promote a symbol from local to global, removing it from the local
symbol
** list.
**
** The parser assumes that all non-global symbols are for local variables
** (unless already known) until it sees a following parenthesis
(supposedly
** starting an argument list). When this happens, PromoteToGlobal() is
called
** so that the symbol is placed on the global symbol list, since this is
where
** all function symbols should be held. Since, when this occurs, there is
as
** yet no function defined, we say the symbol is still that of a variable
(now
** global). This will be overwritten correctly when a definition of a
function
** with that symbol is read.
*/
Symbol *PromoteToGlobal(Symbol *sym)
{
Symbol *s, *global_sym;
static DataValue noValue = {NO_TAG, {0}};
if (sym->type != LOCAL_SYM)
return sym;
/* Remove sym from the local symbol list */
if (sym == LocalSymList)
LocalSymList = sym->next;
else {
for (s = LocalSymList; s != NULL; s = s->next) {
if (s->next == sym) {
s->next = sym->next;
break;
}
}
}
sym->next = NULL;
global_sym = LookupSymbol(sym->name);
if (global_sym != NULL) {
/* get rid of sym's instance - the name is already there */
freeSymbolTable(sym);
}
else {
/* put sym on the global list */
sym->next = GlobalSymList;
sym->type = GLOBAL_SYM;
GlobalSymList = global_sym = sym;
}
return global_sym;
}
----------------------------------------------------------------------
Comment By: Bert Wesarg (lebert)
Date: 2008-03-09 17:40
Message:
Logged In: YES
user_id=122956
Originator: YES
committed with plenty of new comments ;-)
----------------------------------------------------------------------
Comment By: Bert Wesarg (lebert)
Date: 2008-03-01 02:07
Message:
Logged In: YES
user_id=122956
Originator: YES
I will commit this fix by the next week. There is only the problem with
the extra LookupSymbol() call. I suspect that this would be a greater
error, if this call ever succeeded, because that would mean that a
LOCAL_SYM was not in the LocalSymList, or there was previously a non
LOCAL_SYM in the GlobalSymList. I would like to add a fprintf instead of
the return, and see if this case is ever triggered (see the new patch).
File Added: fix-mem-leak-in-PromoteToGlobal.patch
----------------------------------------------------------------------
Comment By: Nobody/Anonymous (nobody)
Date: 2008-02-16 23:10
Message:
Logged In: NO
should be updated to CVS immediately
----------------------------------------------------------------------
You can respond by visiting:
https://sourceforge.net/tracker/?func=detail&atid=111005&aid=1894726&group_id=11005
--
NEdit Develop mailing list - [email protected]
http://www.nedit.org/mailman/listinfo/develop
