-
Notifications
You must be signed in to change notification settings - Fork 34
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
File -> Save Logo Session creates incorrectly named files. #39
Comments
Confirmed. |
From a bit of digging about in the source, I also notice that it appears to be the intention that if Logo is closed with unsaved changes, then the user should be prompted with an opportunity to save (or abandon the changes). This doesn't happen. Perhaps this is another bug. |
I believe I have worked out what is going on here... There is a global variable save_name, which is a pointer to a type called NODE. save_name is set to NIL (a pointer with the value zero, so effectively the same as NULL) when Logo starts. When File->Save Logo Session is activated (LogoFrame::OnSave in wxTerminal.cpp), if save_name is NIL, then LogoFrame::OnSaveAs(...) is invoked, which opens a box asking the user to select/enter a file name. Next, the file name string pointer obtained by an instance of wxFileDialog is passed to doSave(...). In doSave(...) in wxMain.cpp, the supplied file name is copied** into the global variable nameBuffer. Then the string pointer passed to doSave(...) (importantly NOT a pointer to nameBuffer) is given to make_static_strnode(...), which creates a new NODE structure and returns a pointer to it. This new NODE contains a pointer to the file name string that was passed to doSave(...). The NODE then is passed to lsave(...). (I deliberately leave out the detail of the call to cons(...) and the new NODE actually being a pair of NODEs.) (**If I'm not mistaken, by a loop that fails to copy the terminating /0, but this probably isn't noticed in simple tests if the compiler initialises nameBuffer with /0s.) lsave(...) in files.c saves the current Logo definitions to a file with the name pointed to by the NODE. It then sets save_name to point to this NODE. The problem is that the NODE pointed to by save_name now contains a string pointer that was returned by an instance of wxFileDialog. Presumably the pointed to storage becomes invalid/available to be reused by wxWidgets when the wxFileDialog instance goes out of scope, which it does when LogoFrame::OnSaveAs(...) returns. If File->Save Logo Session or the built-in Logo procedure SAVE is invoked when save_name is not NIL, then lsave(...) gets called with the NODE pointed to by save_name (File->Save Logo Session) or uses save_name if no name was specified (SAVE). The solution would seem to be to have doSave(...) use make_static_strnode(...) to create a NODE the embeds a pointer to nameBuffer instead. So far as I can tell, the sole purpose of nameBuffer is to retain the file name used in the most recent save or load operation, but only when the menu load and save operations are used. It is not used by the built-in Logo SAVE and LOAD procedures. All this also leads me to be curious about the approach, if any, UCB Logo takes to memory allocation and deallocation. For example, if the Logo instruction SAVE "mylogo.lg is used, what makes sure the NODE and file name buffer it points to remain valid for at least as long as it is pointed to by save_name? lsave(...) stores a pointer to this NODE in save_name for future save operations that do not specify a file name. Also, what takes care of freeing the NODE and file name buffer resources when they are no longer needed, for example if the menu Save As facility is used. I attach a patch (to the GitHub Master branch) that seems to correct the saving problem. It struck me as a bit silly to be using loops all over the place to copy strings and so the patch also adds a new file containing the function zstrncpy(...), which is a slightly better behaved version of the C library strncpy. I'm not sure you'll want to accept the modifications exactly as made by this patch, but it should work to demonstrate appropriate changes. I fear my editor made some minor formatting changes too, so the patch possibly contains some deletions and additions that aren't real ones. You might also want to go further and completely remove the commented out (by me) references to nameBufferSize and perhaps remove the unnecessary passing of length to doSave(...) and doLoad(...). I note that when a file is loaded (by File->Load Logo Session or the Logo LOAD procedure), save_name is set to retain a reference to this file name. This means that a subsequent save operation overwrites that file. This is probably intended and isn't altogether unreasonable, but I'm not absolutely sure it's the most desirable behaviour. For example, I believe it's possible to use multiple load operations to pull in definitions from several files. It would probably then be undesirable to have a save overwrite the last file that was loaded with all of the definitions in the Logo session. It would be easy to cause that to happen by accident. If this behaviour isn't wanted, then I believe it could be changed by having lload(...) set save_name to NIL, rather than car(arg) (see files.c, line 514). |
Thank you for this brilliant detective work! There's a garbage collector (which is meant to be a generational GC but was written by me and is therefore horrible) that runs when allocating a new pair fails. We malloc a big chunk of memory all at once to serve as free pairs. When that runs out, the garbage collector is called. It does a mark-sweep, starting with a long list of variables that hold pairs during various computations, plus the oblist (where the user's procedures and variables are found), plus the C stack, taking everything that might be the address of a pair as an address to mark. (That's not as bad as it sounds, because the address has to be in a block of pair-space.) And then, you know, things point to other things and they get marked too. The code is in mem.c and the entry point is gc(). |
Hi,
Nice work. UCB Logo already have a “safe” strncpy it is called strnzcpy() in file logodata.c.
The code could be changed in wxMain.cpp to
doLoad()
...
nameBufferSize = length < NAME_BUFFER_SIZE? length : NAME_BUFFER_SIZE - 1;
strnzcpy(nameBuffer, name, nameBufferSize);
(void)lload(cons(make_static_strnode(nameBuffer),NIL));
…
doSave()
...
nameBufferSize = length < NAME_BUFFER_SIZE? length : NAME_BUFFER_SIZE - 1;
strnzcpy(nameBuffer, name, nameBufferSize);
(void)lsave(cons(make_static_strnode(nameBuffer),NIL));
…
Regards,
pahihu
… On 2020. Jun 19., at 3:54, zigggit ***@***.***> wrote:
I believe I have worked out what is going on here...
There is a global variable save_name, which is a pointer to a type called NODE. save_name is set to NIL (a pointer with the value zero, so effectively the same as NULL) when Logo starts.
When File->Save Logo Session is activated (LogoFrame::OnSave in wxTerminal.cpp), if save_name is NIL, then LogoFrame::OnSaveAs(...) is invoked, which opens a box asking the user to select/enter a file name. Next, the file name string pointer obtained by an instance of wxFileDialog is passed to doSave(...).
In doSave(...) in wxMain.cpp, the supplied file name is copied** into the global variable nameBuffer. Then the string pointer passed to doSave(...) (importantly NOT a pointer to nameBuffer) is given to make_static_strnode(...), which creates a new NODE structure and returns a pointer to it. This new NODE contains a pointer to the file name string that was passed to doSave(...). The NODE then is passed to lsave(...).
(I deliberately leave out the detail of the call to cons(...) and the new NODE actually being a pair of NODEs.)
(**If I'm not mistaken, by a loop that fails to copy the terminating /0, but this probably isn't noticed in simple tests if the compiler initialises nameBuffer with /0s.)
lsave(...) in files.c saves the current Logo definitions to a file with the name pointed to by the NODE. It then sets save_name to point to this NODE.
The problem is that the NODE pointed to by save_name now contains a string pointer that was returned by an instance of wxFileDialog. Presumably the pointed to storage becomes invalid/available to be reused by wxWidgets when the wxFileDialog instance goes out of scope, which it does when LogoFrame::OnSaveAs(...) returns.
If File->Save Logo Session or the built-in Logo procedure SAVE is invoked when save_name is not NIL, then lsave(...) gets called with the NODE pointed to by save_name (File->Save Logo Session) or uses save_name if no name was specified (SAVE).
The solution would seem to be to have doSave(...) use make_static_strnode(...) to create a NODE the embeds a pointer to nameBuffer instead. So far as I can tell, the sole purpose of nameBuffer is to retain the file name used in the most recent save or load operation, but only when the menu load and save operations are used. It is not used by the built-in Logo SAVE and LOAD procedures.
All this also leads me to be curious about the approach, if any, UCB Logo takes to memory allocation. and deallocation. For example, if the Logo instruction SAVE "mylogo.lg is used, what makes sure the NODE and file name buffer it points to remain valid for at least as long as it is pointed to by save_name? lsave(...) stores a pointer to this NODE in save_name for future save operations that do not specify a file name. Also, what takes care of freeing the NODE and file buffer resources when they are no longer needed, for example if the menu Save As facility is used.
I attach a patch (to the GitHub Master branch) that seems to correct the saving problem. It struck me as a bit silly to be using loops all over the place to copy strings and so the patch also adds a new file containing the function zstrncpy(...), which is a slightly better behaved version of the C library strncpy. I'm not sure you'll want to accept the modifications exactly as made by this patch, but it should work to demonstrate appropriate changes. I fear my editor made some minor formatting changes too, so the patch possibly contains some deletions and additions that aren't real ones. You might also want to go further and completely remove the commented out (by me) references to nameBufferSize and perhaps remove the unnecessary passing of length to doSave(...) and doLoad(...).
I note that when a file is loaded (by File->Load Logo Session or the Logo LOAD procedure), save_name is set to retain a reference to this file name. This means that a subsequent save operation overwrites that file. This is probably intended and isn't altogether unreasonable, but I'm not absolutely sure it's the most desirable behaviour. For example, I believe it's possible to use multiple load operations to pull in definitions from several files. It would probably then be undesirable to have a save overwrite the last file that was loaded with all of the definitions in the Logo session. It would be easy to cause that to happen by accident. If this behaviour isn't wanted, then I believe it could be changed by having lload(...) set save_name to NIL, rather than car(arg) (see files.c, line 514).
file-save-logo-session-patch.txt <https://github.com/jrincayc/ucblogo-code/files/4802030/file-save-logo-session-patch.txt>
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub <#39 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADTEB2Y7PAE233GACUW4XELRXLAMJANCNFSM4NLHYIFQ>.
|
I checked and pahihu's suggested fix does fix this bug. I created a merge request based off of it. |
Is nameBufferSize necessary? So far as I can see it gets set in doLoad(...) and doSave(...), but not used anywhere else. If that is true, at the very least it doesn't need to be global, but probably isn't needed at all. If strnzcpy works in the same way as the one I supplied, then it can just be given NAME_BUFFER_SIZE as its third argument. I assumed the string created by wxFileDialog is terminated with \0. If it isn't, then some other changes are probably going to be needed. |
It is useful to know about the copying functions in logodata.c. Looking at strnzcpy(), I wonder whether it is intended to operate in exactly the same way as strncpy(), but make sure the destination string is always \0 terminated, as strncpy() alternatives usually are. The strncpy() man page says: The strncpy() function is similar, except that at most n bytes of src Therefore, when using strncpy(), it would be usual to pass n as the number of bytes in the destination buffer and so long as the source buffer (containing its string and terminating \0) is no longer than n, all will be well. If strnzcpy() is meant to work in the same way, then it should contain s1[n-1] = '\0'; rather than s1[n] = '\0';, otherwise strnzcpy() always wrongly writes a \0 into the byte immediately following the destination buffer. Alternatively, UCB Logo programmers are aware of this and always call strnzcpy() with n one less than the size of the destination buffer, perhaps using the convention that n is the maximum expected number of characters in the string (not including the \0). |
Further experimentation seems to confirm that nameBufferSize and the passing of length to doSave() and doLoad() are unnecessary. Perhaps it would be better to remove these things unless they are there to support some planned future change. The attached patch does this and cursory testing suggests it corrects the saving bug. The current operation of strnzcpy() is assumed. |
While thinking about things that aren't needed. I've also been wondering about firstloadsave in wxTerminal.cpp. It is initialised to 1 and set to 0 following a save or load operation. It doesn't seem to be referenced anywhere else and so achieves nothing. Could/should this also be removed? |
Hi,
There are globals which aren’t used more than once or they are remnants
from other features which are planned or already solved in a different way.
Say, if nameBuffer[] contains a string, then nameBufferSize contains its length.
Take the code as given. It is not finished, polished in any way.
You are great at spotting bug ridden behavior, rough edges.
Let’s continue with bug hunting.
pahihu
… On 2020. Jun 20., at 2:55, zigggit ***@***.***> wrote:
While thinking about things that aren't needed. I've also been wondering about firstloadsave in wxTerminal.cpp. It is initialised to 1 and set to 0 following a save or load operation. It doesn't seem to be referenced anywhere else and so achieves nothing. Could/should this also be removed?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#39 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADTEB27HUE6X3KVDIY5CTD3RXQCHZANCNFSM4NLHYIFQ>.
|
Absolutely, however unused bits cause clutter and and waste time in chasing irrelevant clues when trying to find the source of bugs. It would seem sensible to get rid of them when making related changes, so long as they are of the remnant type and not there to support some soon to be added new feature. Otherwise the mess just builds up and up. |
Yeah, it's the product of a sequence of Berkeley undergrads, none especially expert on programming language development, and me, even less so. The only thing we had going for us, really, was the Explicit Control Evaluator in Chapter 5 of SICP. But even so, Logo is a messier language than Scheme, so there are layers of kludges on top of the beautiful core of the evaluator. On the other hand, it filled a gap in the world, so on balance I feel okay about it -- except with respect to you guys, toward whom I feel embarrassed. (No need for return strokes.) |
I didn’t sayd that as criticism. It's a great piece of work.
I'm working with code every day which is much more convoluted ;)
pahihu
… On 2020. Jun 20., at 10:35, Brian Harvey ***@***.***> wrote:
Take the code as given. It is not finished, polished in any way.
Yeah, it's the product of a sequence of Berkeley undergrads, none especially expert on programming language development, and me, even less so. The only thing we had going for us, really, was the Explicit Control Evaluator in Chapter 5 of SICP. But even so, Logo is a messier language than Scheme, so there are layers of kludges on top of the beautiful core of the evaluator.
On the other hand, it filled a gap in the world, so on balance I feel okay about it -- except with respect to you guys, toward whom I feel embarrassed. (No need for return strokes.)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub <#39 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ADTEB22G24CL5WYX7SQBKALRXRYEXANCNFSM4NLHYIFQ>.
|
Closes #39 Also removes unneeded global nameBufferSize.
I created this as a merge request #42 Since it fixes the issue, and removing an unneeded global variable, I'll merge it in a couple days unless there are new suggestions for changes. |
I merged @zigggit's patch. Thanks for finding and fixing this. |
I can reliably reproduce this problem with UCB Logo 6.1 running on Linux by doing the following:
Start Logo and define one or more things in it.
Use File -> Save Logo Session or File -> Save As (it doesn't matter which) to save the definitions to a file. This works as expected.
If you like, change/add some definitions in Logo or don't, it doesn't make any difference.
Use File -> Save Logo Session.
A new file containing the Logo definitions is created. The file previously created is not updated. The new file has a short name consisting of seemingly random characters. For example, Logo has just created a file called ' 9[K:?' for me. Occasionally no new file is created and neither is the previous file updated; perhaps this is because of an attempt to create a file with an invalid name.
I should think that use of Save Logo Session after the first one is either using an uninitialised buffer for the file name or the buffer that holds the file name following the first use is getting overwritten by something.
Sticking to using File -> Save As every time for saving avoids the problem.
Update: From some further brief experimentation, it seems this problem only happens if the first save is initiated from the File menu. If SAVE "filename is used from within Logo, then subsequent saves using SAVE or File -> Save Logo Session seem to work correctly.
The text was updated successfully, but these errors were encountered: