File -> Save Logo Session creates incorrectly named files. about ucblogo-code HOT 16 CLOSED

Confirmed.

from ucblogo-code.

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.

from ucblogo-code.

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).

file-save-logo-session-patch.txt

from ucblogo-code.

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().

from ucblogo-code.

from ucblogo-code.

I checked and pahihu's suggested fix does fix this bug. I created a merge request based off of it.
@brianharvey or @pahihu are welcome to merge #41 if they agree with it.
Thanks @zigggit for finding where the problem was.

from ucblogo-code.

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.

from ucblogo-code.

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
are copied. Warning: If there is no null byte among the first n bytes
of src, the string placed in dest will not be null-terminated.

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).

from ucblogo-code.

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.

file-save-logo-session-patch2.txt

from ucblogo-code.

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?

from ucblogo-code.

from ucblogo-code.

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.

from ucblogo-code.

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.)

from ucblogo-code.

from ucblogo-code.

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.

file-save-logo-session-patch2.txt

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.

from ucblogo-code.

I merged @zigggit's patch. Thanks for finding and fixing this.

from ucblogo-code.