Free42 state file switching, export, and import

[Thomas Okken]

I'm working on state file switching, export, and import in Free42. It's currently working in the Mac and Windows versions. I put test versions up here: https://thomasokken.com/free42/download/test/

These have a new platform-independent state file format.
The GTK (Linux), iOS, and Android versions will follow over the course of the next few weeks.

Comments welcome. Enjoy!

UPDATE (8/25): Release 2.5 is ready, and I removed the directory with the test builds from my web site, so the link shown above no longer works. The Android, Windows, MacOS, and Linux versions are available for download in the usual place. The Google Play Store and iOS App Store haven't approved 2.5 yet as I write this, but I expect it to show up there in the next day or two.

[Vincent Weber]

Thanks Thomas ! You've finally done it ! Can't wait for the Android version
Cheers

[SammysHP]

Nice! Thank you, Thomas.

I can test it once there is a Linux or Android version.

[Vincent Weber]

Btw, could the state file format be compatible / same as emu42 ? Could be useful..

[Thomas Okken]

I'm sure that a person who knows the HP-42S through and through could take a memory dump from that machine -- or an Emu42 state file, which I assume is basically the same thing -- and translate it to a Free42 state file. However, I am not that person. I only know the 42S from a user's point of view; I've never looked at the internals or ROM disassembly etc.

[Vincent Weber]

(08-15-2019 03:49 PM)Thomas Okken Wrote:  I'm sure that a person who knows the HP-42S through and through could take a memory dump from that machine -- or an Emu42 state file, which I assume is basically the same thing -- and translate it to a Free42 state file. However, I am not that person. I only know the 42S from a user's point of view; I've never looked at the internals or ROM disassembly etc.

Fair enough Thomas, I overlooked that ! Never mind then, .raw files compatibility is enough...

[Thomas Okken]

Finished & uploaded the Linux version.

The GTK build now works in FreeBSD and Solaris as well; I also uploaded experimental binaries for those platforms.
(FreeBSD 12 and Solaris 11.3, both on x86_64.)

[SammysHP]

Hi Thomas,

Wow, that are many changes to the source! That must have been a lot of time that you invested in such a "small" feature and everything for free and open source! Thank you!

Here are a few remarks after compiling Free42 under Arch Linux:

1. Arch Linux requires the X11 library as well (LIBS += -lX11). Probably all Linux distributions require it.
2. At least under Arch Linux -Wno-narrowing is necessary. As you've added it to the FreeBSD and SunOS builds, can you please add it to the normal Linux build as well? I think there are no downsides.
3. Speaking of compile options: Adding -Wno-missing-braces -Wno-unused-but-set-variable -Wno-unused-variable removes hundreds of warnings.
4. The buffer for the path generation in gtk/shell_main.cc is too small:
   
   Code:
   

   shell_main.cc:384:57: warning: '%s' directive output may be truncated writing up to 255 bytes into a region of size between 0 and 255 [-Wformat-truncation=]
  384 |         snprintf(core_state_file_name, FILENAMELEN, "%s/%s.f42", free42dirname, state.coreName);
      |                                                         ^~                      ~~~~~~~~~~~~~~
shell_main.cc:384:17: note: 'snprintf' output between 6 and 516 bytes into a destination of size 256
  384 |         snprintf(core_state_file_name, FILENAMELEN, "%s/%s.f42", free42dirname, state.coreName);
      |         ~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~​~~~~~~~~~~~~
   You are trying to put two strings with a length of up to 255 characters in a buffer of only 255 characters. There are several places with this issue.
5. There are some misleading-indentation warnings like
   
   Code:
   

   core_keydown.cc: In function 'void keydown_normal_mode(int, int)':
core_keydown.cc:1821:18: warning: this 'if' clause does not guard... [-Wmisleading-indentation]
 1821 |             else if (prgms[current_prgm].text[pc] != CMD_END)
      |                  ^~
core_keydown.cc:1823:17: note: ...this statement, but the latter is misleadingly indented as if it were guarded by the 'if'
 1823 |                 prgm_highlight_row = 1;
      |                 ^~~~~~~~~~~~~~~~~~

[Thomas Okken]

Hi Sammy,

Thank you for your feedback!

The platform-independent state files did require a lot of code changes, but that was a feature I've wanted for a long time, and doing it the way I ended up doing wasn't that bad, it took about a day and a half in the end. And it gave me an opportunity to simplify the core/shell interface a bit. I'm pretty happy with the result.

The state file management dialog is a lot more work, because that's all shell code, so it has to be done five times. Three done, two to go! I'll work on the Android version today, that should be a bit less work that the previous three. For the iOS version, I'll probably be able to reuse a lot of the code I wrote for the MacOS version, but that's going to have to wait, there's no way I'll be able to finish both this weekend.

My comments on your comments follow below.

Thomas

(08-18-2019 09:43 AM)SammysHP Wrote:  1. Arch Linux requires the X11 library as well (LIBS += -lX11). Probably all Linux distributions require it.

The X11 library is always required, but what varies is whether you have to tell the linker about it or not. I do my Linux builds in Ubuntu 12.04, and -lX11 is not needed there. I'll have to check what happens when I do add it there; I don't remember whether I left it off out of misguided command-line minimalism or because of a warning.

(08-18-2019 09:43 AM)SammysHP Wrote:  2. At least under Arch Linux -Wno-narrowing is necessary. As you've added it to the FreeBSD and SunOS builds, can you please add it to the normal Linux build as well? I think there are no downsides.

The downsides are error messages from compilers that don't recognize -Wno-narrowing.

(08-18-2019 09:43 AM)SammysHP Wrote:  3. Speaking of compile options: Adding -Wno-missing-braces -Wno-unused-but-set-variable -Wno-unused-variable removes hundreds of warnings.

Argh. In Ubuntu 12.04, there are zero warnings (there are some in the Intel library but none in Free42 itself). Newer compilers warn about more things, and some of those warnings are idiotic in my opinion (like the ones about adding parentheses in expressions like a && b || c && d) and others are about things that used to be common, even unavoidable, in C programming, and for which better alternatives sometimes exist in newer versions of the C and C++ standards. Unfortunately, bringing the source code up to date with those new standards is a big task, especially considering all the regression testing that would be required, so I just end up adding -Wno-annoying-warning-du-jour options in an endless game of whac-a-mole. And what makes this especially annoying is that every revision of gcc/llvm/clang on every distribution and every OS has its own set of warnings, and you can't just turn them all off, because turning off a warning that a compiler doesn't recognize, is an error.
See also: item 2, above.

(08-18-2019 09:43 AM)SammysHP Wrote:  4. The buffer for the path generation in gtk/shell_main.cc is too small [...]
You are trying to put two strings with a length of up to 255 characters in a buffer of only 255 characters. There are several places with this issue.

I don't think it's an issue in practice, because who actually uses directories nested that deep and filenames that long that you end up with absolute paths of more than 255 characters? My main reason for using snprintf is that in the unlikely event of such extremely long paths occurring, the code would at least fail gracefully rather than crash.

Still, it's not exactly elegant, and a few days ago, I went through the Windows version and eliminated all fixed-size strings wherever I could, replacing them with std::string. It turned out to require a ridiculous number of code changes, and to re-test everything because those fixed-length strings are everywhere, and then I would have to repeat that whole miserable exercise in the GTK, MacOS, and iOS versions as well. (The Android shell is Java so no fixed-length strings there, praise be to James Gosling and the 1990s Sun Microsystems managers who recognized the value of his work!) So in the end I backed out those changes. Maybe I'll re-apply them in some indeterminate future, when I'm sufficiently bored and when I feel up to the additional challenge of making the Windows version fully Unicode-compatible at the same time.

Sorry about the rant! TL;DR It's not something to worry about, but the compiler is too stupid to know that, and I'm unable to deal with all warnings myself; that task falls on downstream package maintainers. See also items 2 and 3...

(08-18-2019 09:43 AM)SammysHP Wrote:  5. There are some misleading-indentation warnings

Misleading indentation is misleading! I'll take a look and fix the source where I find them, at least where I agree with the warnings. The indentation in the example you gave is indeed wrong.

[ijabbott]

(08-18-2019 11:37 AM)Thomas Okken Wrote:  

(08-18-2019 09:43 AM)SammysHP Wrote:  1. Arch Linux requires the X11 library as well (LIBS += -lX11). Probably all Linux distributions require it.

The X11 library is always required, but what varies is whether you have to tell the linker about it or not. I do my Linux builds in Ubuntu 12.04, and -lX11 is not needed there. I'll have to check what happens when I do add it there; I don't remember whether I left it off out of misguided command-line minimalism or because of a warning.

Since Ubuntu 12.04 went EOL over 2 years ago, have you considered building it on something more modern? At least to pick up the problems reported by later versions of gcc.

Quote:

(08-18-2019 09:43 AM)SammysHP Wrote:  2. At least under Arch Linux -Wno-narrowing is necessary. As you've added it to the FreeBSD and SunOS builds, can you please add it to the normal Linux build as well? I think there are no downsides.

The downsides are error messages from compilers that don't recognize -Wno-narrowing.

The other option is to fix the warnings.

[Thomas Okken]

(08-18-2019 11:53 AM)ijabbott Wrote:  

(08-18-2019 11:37 AM)Thomas Okken Wrote:  The X11 library is always required, but what varies is whether you have to tell the linker about it or not. I do my Linux builds in Ubuntu 12.04, and -lX11 is not needed there. I'll have to check what happens when I do add it there; I don't remember whether I left it off out of misguided command-line minimalism or because of a warning.

Since Ubuntu 12.04 went EOL over 2 years ago, have you considered building it on something more modern? At least to pick up the problems reported by later versions of gcc.

The advantage of building on an old distribution is that the binaries I create work just about everywhere. If I were to build on a newer distro, that would probably cause incompatibilities with older ones.

(08-18-2019 11:53 AM)ijabbott Wrote:  

(08-18-2019 11:37 AM)Thomas Okken Wrote:  The downsides are error messages from compilers that don't recognize -Wno-narrowing.

The other option is to fix the warnings.

I think you missed the part of how much work that would entail. I can think of many more enjoyable and/or productive ways to spend my time!

[SammysHP]

You are right, Thomas. Each new version of gcc brings new features and checks for common issues that were discovered in the meantime or just considered helpful. Sometimes changes aren't even fully backwards compatible without additional options. Additionally every distribution might define a different set of default options, which makes it even more complicated. I work(ed) with all major versions from 5 to 9 and have seen many unexpected things.

Regarding the warning: An unknown -Wno-* option should not produce an error.

https://gcc.gnu.org/wiki/FAQ#wnowarning Wrote:Since GCC 4.4, and as explained in the GCC manual: when an unrecognized warning option is requested (-Wunknown-warning), GCC emits a diagnostic stating that the option is not recognized. However, if the -Wno- form is used, the behavior is slightly different: no diagnostic is produced for -Wno-unknown-warning unless other diagnostics are being produced. This allows the use of new -Wno- options with old compilers, but if something goes wrong, the compiler warns that an unrecognized option is present.

On the other hand I have no problems with patching your makefile as a package maintainer. But it causes additional work for me and people who want to compile Free42 directly from source.

(08-18-2019 11:37 AM)Thomas Okken Wrote:  

(08-18-2019 09:43 AM)SammysHP Wrote:  4. The buffer for the path generation in gtk/shell_main.cc is too small [...]
You are trying to put two strings with a length of up to 255 characters in a buffer of only 255 characters. There are several places with this issue.

I don't think it's an issue in practice, because who actually uses directories nested that deep and filenames that long that you end up with absolute paths of more than 255 characters? My main reason for using snprintf is that in the unlikely event of such extremely long paths occurring, the code would at least fail gracefully rather than crash.

Here I like to disagree. First, "who actually…" caused a lot of trouble in computer history. Second, you know that this part of the source does not work with valid input in some circumstances, but you ignore it. You don't even check the return value of snprintf(). The code silently fails and (hopefully) an error is produced at different place (when the program tries to read the file). Such long paths are unlikely, but allowed.
In the end this is not a big issue that can cause all kinds of problems (except that loading of the file might fail sometimes), it is just not clean code which could have been avoided.

[Thomas Okken]

(08-18-2019 12:45 PM)SammysHP Wrote:  Regarding the warning: An unknown -Wno-* option should not produce an error.

https://gcc.gnu.org/wiki/FAQ#wnowarning Wrote:Since GCC 4.4, and as explained in the GCC manual: when an unrecognized warning option is requested (-Wunknown-warning), GCC emits a diagnostic stating that the option is not recognized. However, if the -Wno- form is used, the behavior is slightly different: no diagnostic is produced for -Wno-unknown-warning unless other diagnostics are being produced. This allows the use of new -Wno- options with old compilers, but if something goes wrong, the compiler warns that an unrecognized option is present.

Sweet! I did not know that. Even my Ubuntu 12.04 build VM has a sufficiently recent gcc for that (4.6.3 -- whew).
It doesn't mind the redundant -lX11, either. OK, I'll clean up those two things in the Makefile (and fix the indentation where appropriate).

(08-18-2019 12:45 PM)SammysHP Wrote:  

(08-18-2019 11:37 AM)Thomas Okken Wrote:  I don't think it's an issue in practice, because who actually uses directories nested that deep and filenames that long that you end up with absolute paths of more than 255 characters? My main reason for using snprintf is that in the unlikely event of such extremely long paths occurring, the code would at least fail gracefully rather than crash.

Here I like to disagree. First, "who actually…" caused a lot of trouble in computer history.

Sure, but consider the specific situation at hand. Free42 is not a backup daemon that traverses entire filesystems and thus would absolutely have to be able to handle long paths. Free42 is an interactive program, and all the files it deals with are in $HOME/.free42 or in whatever directory the user specifically chooses. Honestly, who is going to exceed 255 characters that way?

(08-18-2019 12:45 PM)SammysHP Wrote:  Second, you know that this part of the source does not work with valid input in some circumstances, but you ignore it. You don't even check the return value of snprintf(). The code silently fails and (hopefully) an error is produced at different place (when the program tries to read the file). Such long paths are unlikely, but allowed.
In the end this is not a big issue that can cause all kinds of problems (except that loading of the file might fail sometimes), it is just not clean code which could have been avoided.

As I said before, the rationale for using snprintf is to avoid crashes, not to make sure extremely long paths actually work. The Windows version doesn't even use snprintf, because it's not provided in Visual C++, at least not the versions I've used. The better is always the enemy of the good, and I'd rather have a program that can be made to malfunction or even crash when trying to use it in extremely remote corners of the filesystem, than not have the program at all.

[Thomas Okken]

OK, I made those changes to gtk/Makefile, and fixed the indentation warnings in core_display.cc and core_keydown.cc.

Code:

/Users/thomas/xcode/free42/gtk $ svn diff -r 2932 -x -w
Index: Makefile
===================================================================
--- Makefile    (revision 2932)
+++ Makefile    (working copy)
@@ -20,6 +20,11 @@
      -Wno-parentheses \
      -Wno-write-strings \
      -Wno-sign-compare \
+     -Wno-narrowing \
+     -Wno-constant-conversion \
+     -Wno-sometimes-uninitialized \
+     -Wno-format-truncation \
+     -Wno-unknown-warning-option \
      -g \
      -I/usr/X11R6/include \
      $(shell pkg-config --cflags gtk+-2.0) \
@@ -36,26 +41,14 @@
      -D_WCHAR_T_DEFINED
 
 LDFLAGS = -L/usr/X11R6/lib
-LIBS = gcc111libbid.a $(shell pkg-config --libs gtk+-2.0) -lXmu
+LIBS = gcc111libbid.a $(shell pkg-config --libs gtk+-2.0) -lXmu -lX11
 
-ifeq "$(shell uname -s)" "Linux"
-LDFLAGS += -Wl,--hash-style=both
-LIBS += -lpthread
-endif
-
 ifeq "$(shell uname -s)" "FreeBSD"
-CFLAGS += -Wno-narrowing -Wno-constant-conversion -Wno-sometimes-uninitialized
-LIBS += -lX11
 ifdef AUDIO_ALSA
 LIBS += -ldl
 endif
 endif
 
-ifeq "$(shell uname -s)" "SunOS"
-CFLAGS += -Wno-narrowing -Wno-misleading-indentation
-LIBS += -lX11
-endif
-
 ifneq "$(findstring 6162,$(shell echo ab | od -x))" ""
 CFLAGS += -DF42_BIG_ENDIAN -DBID_BIG_ENDIAN
 endif
/Users/thomas/xcode/free42/gtk $

I also had to add -Wno-unknown-warning-option, because clang does still complain about unknown -Wno-foo options. But at least you can tell it not to!

And I removed the -Wl,--hash-style=both from the Linux build. That was a holdover from when I was still using Fedora 6, and I doubt anyone still needs that.

I'll post an updated tarball shortly. No sense in waiting for the new Android build; it is hot and humid here today, and I have no energy, so while I may tinker with the state file management window for Android a bit, there is no way that work is getting completed this weekend.

[Thomas Okken]

The Android version is progressing nicely. I almost finished it last night, but then I got stuck at figuring out how to set things up so that Free42 will load state files attached to emails. Sending state files as attachments is working already, but as happens sometimes with Android, when it comes to setting up the filters that allow the OS to decide which apps to offer for opening a document, the seemingly simple API turns out to be surprisingly difficult and frustrating to use.

I'm sure I'll figure it out, but if anyone here has already slain this particular kind of dragon before, I could use a hint or two.

When it works, the way Android and iOS apps share data is quite nice, and far more suited to the app-centric mobile experience than the desktop- and file-oriented approach of traditional desktop OSes. But working with this approach is new to me. Once the Android version is done, I'll be curious to see what doing this is like in the iOS environment. That OS has given me its share of frustration in the past as well.

[Thomas Okken]

The test builds had a nasty bug that would cause programs to become corrupted. I fixed it and uploaded new builds for all platforms.

If you weren't using the test builds, you're OK, because the production builds don't have this bug. If you were or are using any of the test builds, you should either go back to using production builds, or upgrade to the very latest test builds, and reload all your programs from a known-good source. That last part is important because the corruption may not be immediately apparent.

N.B. Some of you may have noticed an Android build among the test builds; I hadn't advertised its presence yet because there is still work to be done there. It works great in the Android Emulator, but on my real phone (Moto G5), loading *.f42 (state) files from email attachments and web servers is still not working, for as yet undetermined reasons. Apart from that flaw, it's no worse than the others.

[Thomas Okken]

To elaborate a bit about the bug in the earlier test builds...

These new versions have an architecture-independent state file format, in order to allow sharing state files between Free42 instances even if they're running on different platforms. In order to achieve platform independence, I had to stop saving data structures by simply dumping their in-memory representations to the state file, since the details of those in-memory representations depend on data sizes (32- or 64-bit integers), endianness (not as much of an issue as it used to be, but the world isn't 100% little-endian yet), and alignment (partly governed by hardware requirements, partly the result of compiler implementation choices).

The most complex data structures used by Free42 are programs, and in order to write those in an architecture-independent way, the obvious choice was to use the same "raw" format that is already used for importing and exporting user-code programs, and which is basically the HP-42S native format.

One side effect of this change is that the number of I/O operations involved in loading and saving programs from and to the state file increases dramatically, because the state file loader reads programs a byte at a time. Even though there is some buffering, the read operations were slowing state file loading down by a lot on Android, since on that platform, the reads were performed by Java code, while the core of course is native code, and Java/native calls are very CPU-intensive.

One side effect of all these changes is that the core has to make many more I/O calls while reading and writing state, specifically, lots of small ones instead of a relatively small number of large ones, and on Android, that caused a significant slowdown. The reason is that the actual I/O calls used to be performed in the shell, while everything else involving the state happened in the core, and since the Android version has a Java shell, that meant lots of CPU-intensive JNI calls.

In order to prevent the I/O-related slowdown, I moved all the state file read and write operations into the core, eliminating the shell_read_* and shell_write_* functions in the core/shell interface.

This core/shell API change worked fine in terms of speed, but unfortunately, I missed three places in the code where bytes read from the raw stream are "pushed back," causing the push-backs to fail silently, with the result that in certain cases, it's as if a byte is dropped from the program.

Two of those three cases are obscure, and guaranteed never to affect Free42 state files. The first handles the zero byte that terminates numbers in HP-42S programs; in HP-41 raw files, that byte may not be there, while in HP-42S files, it always is. When the program reader finds a non-zero byte after a number, it pushes it back so it can be treated as the first byte of the next instruction.

The second case handles ALPHA GTO or XEQ with a zero-length label, GTO "" or XEQ "", encoded as 1D F0 or 1E F0, respectively. When the program loader sees that F0 byte, it discards the 1D or 1E byte, and treats the F0 as the start of a new instruction. I'm not sure why I implemented it that way, but it was probably to match the behavior of the real 42S.

Neither of these two cases are a problem when reading a state file, because Free42 will never generate the problematic byte sequences in the first place.

The third one is different, though: it is guaranteed to cause problems. This is the case of ALPHA strings.

When reading an ALPHA string, the program loader has to examine the string's second byte, i.e. the first byte of the actual text, to see if its most significant bit is set. (This is how the HP-42S encodes instructions that aren't HP-41C functions but that do have arguments, so they can't be encoded using the XROM mechanism.) When the program loader finds that the second byte does not have its high bit set, it pushes it back so that it can then continue handling the entire string just like any other string; this code is shared between handling plain strings and functions with ALPHA arguments.

The failing pushback in this third case causes the first character of any ALPHA string to be lost, and, and here is the really serious problem, it causes the first byte following the string to be gobbled up by the string, turning every string into an unintentional byte grabber, seriously corrupting any affected program in the process.

Yikes.

Fortunately, I became aware of the problem before the new state file logic was released on my main page, but even the small number of people who downloaded the test versions in order to check out the new state file switching/import/export logic should not have been bitten by this bug; the new state file logic is several weeks old at this point, and I thought it was bug-free by now. My apologies to everyone who was affected, and many thanks to Vincent Weber for sending the bug report that caused me to identify the problem and enabled me to fix it.

[Vincent Weber]

Most welcome Thomas, I am more than happy to help a little the testing of this wonderful app !

Cheers