Threaded Mode | Linear Mode

Claudio L. · 05-07-2018, 11:09 PM

(05-07-2018 08:55 PM)3298 Wrote: Taking back the lead doesn't get much easier than a fast-forward merge, so with the tip of his fork merged into the new quasi-official branch, (and perhaps a pull request or message to notify him,) everyone should be happy.

Done. I followed your exact instructions on a clean directory. Created the commits for your patches and did a pull request from the new "official" repo.
I would call Egan's fork the quasi-official, since he maintained the project for many years, and pulled quite a few of my patches in the past. Eddie's repository should become the one-and-only-true-official repository (no quasi-official, he's the original author).
I wish there was a way to move the new offical repository to the top of the tree, I bet very few people would realize that one of those repositories lost in the middle is the original author.

Claudio L. · 05-08-2018, 02:57 AM

A couple of things I noticed after applying all the patches:
* Trying to mount the SD card on newRPL showed "Unsupported CMD8 and CMD55". These are SD commands for SDHC cards. I need to double check on other computers. I could swear I implemented those on x49gp at some point.
* The menu sometimes shows up with green text (green as in the same background of the calculator screen). I think the text is being drawn as transparent?
* There were 3 errors (actually warnings) at block-vvfat.c (or vvfat-block.c, whatever) which I just silenced in the makefile (it was an Unused result value from a few functions), not sure if it's the most elegant solution.

Perhaps you would like to investigate also the ones I couldn't solve myself. In all of them, I'm using Qt Creator as IDE/debugger (but the issues happen with all debuggers in general), works great except:
* When debugging, somehow breakpoints are not set/removed properly. If the code is running, it won't pause when you insert a breakpoint (it should pause, let the debugger set the breakpoint, then continue), so you need to manually pause the code before setting any breakpoints.
* An easy one: when the debugger stops at a breakpoint, doing single step should run the instruction regardless of the presence of the breakpoint (perhaps remove it temporarily, execute, then re-apply the breakpoint). Right now it won't single-step, you need to manually disable the breakpoint or it will be triggered every time.
* When single-stepping through the code, there has to be a way to avoid stepping through IRQs. Right now as soon as an IRQ is triggered, you start single-stepping through its code, making it impossible to debug the main application.

3298 · 05-08-2018, 11:19 AM

(05-07-2018 11:09 PM)Claudio L. Wrote: Done. I followed your exact instructions on a clean directory. Created the commits for your patches and did a pull request from the new "official" repo.

You squashed the patches into a single commit... that wasn't quite what I had in mind, but oh well. To be fair, my instructions weren't precise on that topic.

(05-07-2018 11:09 PM)Claudio L. Wrote: I would call Egan's fork the quasi-official, since he maintained the project for many years, and pulled quite a few of my patches in the past. Eddie's repository should become the one-and-only-true-official repository (no quasi-official, he's the original author).
I wish there was a way to move the new offical repository to the top of the tree, I bet very few people would realize that one of those repositories lost in the middle is the original author.

The "quasi-official" term was not meant to describe Egan's repository, but the one you subsequently turned yours into. I agree that drowning the official repository is less than ideal, but we don't have the power to tweak GitHub's user interface, so we can only hope that soon it floats to the top again by merging yours and putting some additional commit in.

(05-08-2018 02:57 AM)Claudio L. Wrote: A couple of things I noticed after applying all the patches:
* Trying to mount the SD card on newRPL showed "Unsupported CMD8 and CMD55". These are SD commands for SDHC cards. I need to double check on other computers. I could swear I implemented those on x49gp at some point.

I haven't actually tried running NewRPL yet. Your simulator more or less fills the role of providing a platform for fresh NewRPL users to try it out, so getting x49gp to run it is not urgent. On the other hand, I think x49gp could still be a decent development platform in the future (I'm hoping there will be some way to run native applications in NewRPL, roughly similar to HPGCC3?) thanks being an actual emulator, so improved compatibility would be awesome.

(05-08-2018 02:57 AM)Claudio L. Wrote: * The menu sometimes shows up with green text (green as in the same background of the calculator screen). I think the text is being drawn as transparent?

I did notice that the menu text sometimes changes color, but I didn't notice that it's the screen's color. I don't think I missed a note in the API docs about having to set the color for the menu, but even if there's a framework bug, it should get at least a workaround. I'll look into it. I'm pretty sure it's not transparent, because that would mean it would've been partially black when opening it such that it covers the display frame of the 50g. Perhaps it's wrongly picking up something from the color switching for the grayscale stuff.

(05-08-2018 02:57 AM)Claudio L. Wrote: * There were 3 errors (actually warnings) at block-vvfat.c (or vvfat-block.c, whatever) which I just silenced in the makefile (it was an Unused result value from a few functions), not sure if it's the most elegant solution.

That's weird. It simply worked for me with no warnings. Are they new or did they occur in older versions too (i.e. before the -Werror from the official fork turned them into errors)? If they are new, I'd guess they start to appear with the patch suppressing the debug log. And since I don't see them, which functions are they from? I will happily make fixes, but it's kind of hard to fix something one can't reproduce.
I'm running GCC 7.3.1, by the way. Maybe it's a difference between our compilers; a difference in #ifdef branches taken based on our systems could also be there, but I just searched through the file and only found #ifdefs for DEBUG, DBG (don't ask me about the difference between these), DEBUG_SECTORS (I commented out that #define DEBUG line, and there already was #undef DEBUG_SECTORS below it), QEMU_OLD (which shouldn't be set because it persuades the Makefile to try using QEMU 0.9.0 instead of 0.12.50; according to my experience that will fail horribly), __MINGW32__ and _WIN32 (shouldn't be set on supported systems because they clearly aren't Windows), S_IWGRP and S_IWOTH (only defines them as 0 if missing), and a single constant #if 0 (which cannot be different between our systems), so I think that's unlikely.

(05-08-2018 02:57 AM)Claudio L. Wrote: Perhaps you would like to investigate also the ones I couldn't solve myself. In all of them, I'm using Qt Creator as IDE/debugger (but the issues happen with all debuggers in general), works great except:
* When debugging, somehow breakpoints are not set/removed properly. If the code is running, it won't pause when you insert a breakpoint (it should pause, let the debugger set the breakpoint, then continue), so you need to manually pause the code before setting any breakpoints.
* An easy one: when the debugger stops at a breakpoint, doing single step should run the instruction regardless of the presence of the breakpoint (perhaps remove it temporarily, execute, then re-apply the breakpoint). Right now it won't single-step, you need to manually disable the breakpoint or it will be triggered every time.
* When single-stepping through the code, there has to be a way to avoid stepping through IRQs. Right now as soon as an IRQ is triggered, you start single-stepping through its code, making it impossible to debug the main application.

This is also not happening for me. Mind you, I use bare-bones GDB in a terminal instead of an IDE.
With GDB in a terminal, I've only been able to type commands when the debugged process is halted. That may be why I haven't had the chance to trigger the first issue, but I'm not even 100% sure it's an actual issue with x49gp. The IDE likely uses GDB inside, so it might simply be unable to send the commands (that would mean it's an IDE limitation outside x49gp's control). I think it would be interesting to see what happens when combining the IDE and a remote GDB connection to an instance of the regular gdbserver program; if that setup exhibits the same behavior, then we can't do much about it.
I thought I fixed the step / continue from breakpoint issue. That added "continue;" line in timer.c in the "misc fixes" patch makes it skip the code responsible for raising timer interrupts between the call to the GDB stub and the call to QEMU; these timer interrupts were what prevented me from continuing from breakpoints (by stepping / continuing into a timer interrupt, going through that until it returns to user code at the location of the breakpoint, and then immediately raising a trap exception). If the timers are skipped, the single step / continue can move away from the breakpoint position before the check for trap exceptions kicks in.
With that fix in place, I was able to single-step and continue from active breakpoints in my HPGCC3 program just fine, so I'm curious why it's still broken for you.

Claudio L. · 05-08-2018, 03:02 PM

(05-08-2018 11:19 AM)3298 Wrote: You squashed the patches into a single commit... that wasn't quite what I had in mind, but oh well. To be fair, my instructions weren't precise on that topic.

Well, it was my intent at first, but then I noticed the changes were numbered 1 to 24 and the patches 1 to 23. Then I wasn't sure which patch went with which description (offset-by-1) and decided to go for the single commit. In my defense... I did copy/paste your entire changelog descriptions on the commit, and gave you full credit for the changes.

(05-08-2018 11:19 AM)3298 Wrote: The "quasi-official" term was not meant to describe Egan's repository, but the one you subsequently turned yours into. I agree that drowning the official repository is less than ideal, but we don't have the power to tweak GitHub's user interface, so we can only hope that soon it floats to the top again by merging yours and putting some additional commit in.

Oh, I get it now. Mine is not official, the only reason I had to create it is to be able to do a pull request to Egan a few years ago. I don't like the fact that I have no way to tell people "stay away from my forks, it's not official, doesn't really track the latest changes, and most likely won't even compile".

(05-08-2018 11:19 AM)3298 Wrote: I haven't actually tried running NewRPL yet. Your simulator more or less fills the role of providing a platform for fresh NewRPL users to try it out, so getting x49gp to run it is not urgent.

True, not a major priority.

(05-08-2018 11:19 AM)3298 Wrote: On the other hand, I think x49gp could still be a decent development platform in the future (I'm hoping there will be some way to run native applications in NewRPL, roughly similar to HPGCC3?) thanks being an actual emulator, so improved compatibility would be awesome.

That idea is still in flux, I have yet to come up with a decent ABI so an independent program can call newRPL API. The exported jump table used in hpgcc3 is one way to do it, but a pain to maintain so I'm still looking for a better way.

(05-08-2018 11:19 AM)3298 Wrote: That's weird. It simply worked for me with no warnings. Are they new or did they occur in older versions too (i.e. before the -Werror from the official fork turned them into errors)?

I never paid much attention to warnings, but I think perhaps the vvfat feature wasn't enabled before?
I just rebuilt my old directory, and I see lots of warnings but that file isn't even mentioned, so it's not being built.

(05-08-2018 11:19 AM)3298 Wrote: If they are new, I'd guess they start to appear with the patch suppressing the debug log. And since I don't see them, which functions are they from? I will happily make fixes, but it's kind of hard to fix something one can't reproduce.

Should be easy to reproduce. The first one was line 2360, there's a call to ftruncate() which returns an integer number that's never used and that's generating an "unused result" warning:

Code:

block-vvfat.c: In function ‘commit_one_file’:

block-vvfat.c:2360:5: error: ignoring return value of ‘ftruncate’, declared with attribute warn_unused_result [-Werror=unused-result]

     ftruncate(fd, size);

In my view, there's nothing wrong with ignoring the result, so silencing the warning is fine.

(05-08-2018 11:19 AM)3298 Wrote: With GDB in a terminal, I've only been able to type commands when the debugged process is halted.

I think whenever you set the breakpoint and the code is running, it sends the 'pause' command over the line, sets the breakpoint, then resume execution.
When you manually press the 'pause' button it works well, you set the breakpoint, then resume manually. When the IDE does it, it fails somehow about 3 out of 4 times, a few times the breakpoint is set correctly. These issues happen with both Eclipse and Qt Creator, both using remote gdb backend. I think it's a timing issue, the IDE doesn't wait enough for the 'pause' to work before the next command is sent or something.

(05-08-2018 11:19 AM)3298 Wrote: I thought I fixed the step / continue from breakpoint issue. That added "continue;" line in timer.c in the "misc fixes" patch makes it skip the code responsible for raising timer interrupts between the call to the GDB stub and the call to QEMU; these timer interrupts were what prevented me from continuing from breakpoints (by stepping / continuing into a timer interrupt, going through that until it returns to user code at the location of the breakpoint, and then immediately raising a trap exception). If the timers are skipped, the single step / continue can move away from the breakpoint position before the check for trap exceptions kicks in.
With that fix in place, I was able to single-step and continue from active breakpoints in my HPGCC3 program just fine, so I'm curious why it's still broken for you.

Now that you mention, I tried it again and it works better than before, you can step as long as the line doesn't have a function call (which by chance is what I tried yesterday). Trying to step over a line with a function call stops at the IRQ handler with a timer interrupt.
That's a big improvement, still not quite there but a big usability improvement.

3298 · 05-08-2018, 06:17 PM

(05-08-2018 03:02 PM)Claudio L. Wrote: Well, it was my intent at first, but then I noticed the changes were numbered 1 to 24 and the patches 1 to 23. Then I wasn't sure which patch went with which description (offset-by-1) and decided to go for the single commit. In my defense... I did copy/paste your entire changelog descriptions on the commit, and gave you full credit for the changes.

The instructions are actually numbered up to 25. Tongue

The difference between patches and instructions comes from the first two instructions (clone repositories, grab patches) which don't have associated patches. Oh, and just nitpicking: after doing the merges there are only 21 patches left, not 23 like your commit message states. Wink

What worries me most about the squashing is that it makes it harder for others to understand and follow the sequence, especially with you apparently forgetting the link back here.

(05-08-2018 03:02 PM)Claudio L. Wrote: That idea is still in flux, I have yet to come up with a decent ABI so an independent program can call newRPL API. The exported jump table used in hpgcc3 is one way to do it, but a pain to maintain so I'm still looking for a better way.

Let's not get too off-topic here, but I'll say this: take your time. On a scale from "we need it YESTERDAY" to "who cares?", it's definitely closer to the latter, as the primary usecases would be hardware access (for whatever reason) and high-performance applications. Both of these are niche cases, especially while NewRPL itself is still somewhat experimental. It's just something that would be nice to have ... eventually.

(05-08-2018 03:02 PM)Claudio L. Wrote: I never paid much attention to warnings, but I think perhaps the vvfat feature wasn't enabled before?
I just rebuilt my old directory, and I see lots of warnings but that file isn't even mentioned, so it's not being built.

I'm pretty sure it has always been there and enabled.

(05-08-2018 03:02 PM)Claudio L. Wrote: Should be easy to reproduce. The first one was line 2360, there's a call to ftruncate() which returns an integer number that's never used and that's generating an "unused result" warning:

Code:

block-vvfat.c: In function ‘commit_one_file’: block-vvfat.c:2360:5: error: ignoring return value of ‘ftruncate’, declared with attribute warn_unused_result [-Werror=unused-result] ftruncate(fd, size);
In my view, there's nothing wrong with ignoring the result, so silencing the warning is fine.

Ah-ha, system headers with function declarations explicitly tagged to warn about unused results (using attributes). My system headers apparently don't do that sort of thing. I'll look into how I get them to behave that way.
For ftruncate in particular, there is a QEMU commit less than two weeks after the version we have in x49gp fixing it (it explicitly says so). Maybe we could do a slight QEMU upgrade until we run into a roadblock. We probably wouldn't get very far (as I mentioned, I've been banging my head against current QEMU for a while), but a few fixes like that one wouldn't hurt.

(05-08-2018 03:02 PM)Claudio L. Wrote: I think whenever you set the breakpoint and the code is running, it sends the 'pause' command over the line, sets the breakpoint, then resume execution.
When you manually press the 'pause' button it works well, you set the breakpoint, then resume manually. When the IDE does it, it fails somehow about 3 out of 4 times, a few times the breakpoint is set correctly. These issues happen with both Eclipse and Qt Creator, both using remote gdb backend. I think it's a timing issue, the IDE doesn't wait enough for the 'pause' to work before the next command is sent or something.

Hmm, you might be on to something there. I thought the commands would simply be queued (e.g. in the TCP socket), but that is a lead at least. On the other hand, how does it miss the breakpoint command without missing the continue command as well?
(after a cursory source check) The breakpoint command sends a response back (failure/success). GDB probably delays sending the continue command until after that's done, and then the timing may be generous enough to always let the stub catch the continue command. But the break command also prompts a response, and I couldn't find obvious holes in the buffering scheme...

(05-08-2018 03:02 PM)Claudio L. Wrote: Now that you mention, I tried it again and it works better than before, you can step as long as the line doesn't have a function call (which by chance is what I tried yesterday). Trying to step over a line with a function call stops at the IRQ handler with a timer interrupt.
That's a big improvement, still not quite there but a big usability improvement.

Another thing for me to check out. I thought I'd stepped into and over functions with this fix, but maybe not. I was very annoyed at the constant need to disable the breakpoint, continue, and enable the breakpoint again, and a paragraph here is not enough to tell you how happy finding that cause made me. The fix looks so simple, but it was the single biggest issue I had with the GDB interface, and finding the fix took some effort and a awkward feeling from running two different debuggers (arm-none-eabi and regular x86) at the same time for a single task. Now I can simply tap Enter to go through one iteration of a loop (with the breakpoint somewhere in the loop and a continue as the last GDB command) where I had to type six commands before: disable the breakpoint, continue, then enable it again, and that times two because I need at least one enabled breakpoint in the loop whenever it's running - to keep it from running for more than one iteration. And don't get me started on single-stepping, it always stepped into a timer interrupt for obvious reasons.

Okay, looks like I have some investigating and coding to do. Results will be posted here, of course.

3298 · 05-13-2018, 10:27 PM

Good news, I found and fixed the menu text color bug. It was actually an old issue from the grayscale patch, where it replaced an even older issue. The root cause was that the dot-matrix pixels were drawn using a GdkGC from the current theme. It seems that this GdkGC is the one usually used for standard texts.
Before the grayscale patch, the pixels were drawn with this GdkGC as-is if a pixel was on, skipping it otherwise. This caused pixels to be drawn in funny colors when using certain themes (white for me since I switched to a dark GTK+ theme a few years ago). I actually saw this issue in action while checking out some other forks a while back, but I didn't track it down because I deemed the forks uninteresting.
When I wrote the grayscale patch, I didn't know enough about GDK to spot the bug even though I was touching the relevant line. My replacement code for drawing a pixel (always setting the GdkGC's color depending on the pixel's effective color value to one of 16 precalculated colors) inadvertently fixed the issue described above before I even saw it for the first time. Back then x49gp didn't have any standard themed UI components, so the issue with overwriting part of the theme was hidden. Until the menu came, that is.
The fix is straight-forward: make the pixel drawing loop use its own GdkGC so changing the color doesn't affect anything else. I looked at the glyph drawing functions in ui.c to get an idea of how to do it. The diff of my resulting local commit is attached; the commit message is:

Code:

Fix the text color in the popup menu

The actual bug was in the grayscale patch already, which overwrote a color in the default style when drawing a pixel.

Until the popup menu arrived there were no widgets using the default style, allowing the bug to stay unnoticed for so long.

As a user of a dark theme I didn't pay much attention to the issue before you reported it; the bright green-ish color of the screen wasn't significantly different from the white text color the menu should have had, so it was just as readable. I suspected something funky going on inside GDK itself and forgot about it. Now that I know where this color is coming from, I can see how it can interfere with the usability of x49gp, especially when using a bright theme.

In other news, I managed to replicate your issue with stepping over function calls. It happens quite rarely for me, which doesn't exactly make debugging this any easier, so it's still not tracked down. The issue about setting breakpoints while running hasn't been touched yet, and I didn't have the courage to get back into the QEMU upgrade mess either. I also witnessed some minor UI corruption yesterday, and there are some inconsequential typos I made in the patch series. That's the todo list for the near future, though perhaps not in the exact order.

Claudio L. · 05-14-2018, 01:24 AM

Great, bug by bug it will get cleaned up eventually.
Thanks again, I'll do the patching tomorrow if I have time (I noticed Eddie hasn't pulled in my request yet on github, he must be busy).

3298 · 05-15-2018, 10:22 AM

(05-14-2018 01:24 AM)Claudio L. Wrote: (I noticed Eddie hasn't pulled in my request yet on github, he must be busy).

I'm not surprised. He gave me some details about the reason he's busy, but I think I should respect his privacy and not post them in a public forum. Suffice to say it's something that normally takes weeks.

I believe I found the reason for the single-step problems. I disabled x49gp's timers when starting from a out of the debugger code using that continue statement, which does take care of most things. However, there seems to be another source of interrupts somewhere inside the QEMU code. There is a commit somewhere between the 0.9.0 and 0.12.50 versions shipped with x49gp that is supposed to improve that, but the gdbstub.c file used by x49gp is still based on the one from 0.9.0, so it doesn't have the fix. It was copied out of the QEMU directory (and probably modified), and during the upgrade to 0.12.50 it was only lightly touched to make it work again; the changes in QEMU's own copy of the file between the versions were not applied. That means more work on the QEMU upgrade front should fix it. I think by now nobody cares about 0.9.0 anymore, so we can just get rid of that entirely, which would allow gdbstub.c to receive a proper upgrade.

3298 · 08-23-2018, 05:44 PM

3 months - I got sidetracked, obviously ...
But I do have a few fixes for x49gp in store, and I want to get them out there. Originally I wanted to push them out together with a proper QEMU update, but it looks like I've failed completing that for the second time, so I'll have to settle for the duct tape.

Let's get started with the easy stuff.

Code:

Fix some silly but fortunately inconsequential mistakes in recently added comments and similarly recently changed Makefile recipes

Well, "recently" relative to the average commit time spacing in x49gp. Wink

And don't ask how I managed to fail at dragging my finger across the top row of my keyboard to type "QUERTY" and "QUERTZ" instead of "QWERTY" and "QWERTZ".
The Makefile part was caused by me having a target for $(TARGET).man.gz - seeing many such compressed man-pages in my system directories, I tried to mirror them and install a compressed version of the man-page. I later saw that other projects don't do that; it's the package manager compressing them for me. As a result, I deleted the rule for compression out of the Makefile, did a find-and-replace from ".gz" to "" to update the install targets - and forgot about the distclean targets (which were affected by the find-and-replace operation too). No big deal though, removing something twice doesn't hurt anyone when failures due to missing files are ignored anyway, so it all worked just fine. Removing the repetition merely makes the Makefile cleaner.

Code:

Remove the option to build with QEMU 0.9.0, it was very broken anyway

0.12.50 is not much better, but at least it's ancient instead of stone-age, and it compiles without jumping through ridiculous hoops.

Warning: big patch for this one due to a large number (and large total size) of deleted text files.

Code:

Update gdbstub to the appropriate QEMU 0.12.50 revision

It was a heavily modified copy from 0.9.0 before, with slight changes to make it work with 0.12.50 when that update came to x49gp.

Now it's still heavily modified, but at least it has the fixes that were applied to the original between these QEMU revisions, including a fix for the rare remaining cases of single-step walking into an interrupt.

This is one of the two main reasons I didn't want to hold these patches back any longer (see the post just before this one). Claudio, your debugger fix is finally here. Smile

Code:

Fix some warnings in block-*.c which caused compilation failures via -Werror

These are basically (back-)ports of the fixes standard QEMU received in commits 2dedf83e, 3e1a8134, 1e37d059, just without the qemu_write_full part in the second one because we don't have that.

When I can't do a proper upgrade, the duct-tape that is a (back-)port of the crucial fixes will have to do. This is the other reason for getting this out; shortly after the last post a compiler / system header update caused a compilation error via a warning and -Werror on my setup. (Something about stringop and truncation, I don't remember the exact message.) For some weird reason it vanishes when compiled with _FORTIFY_SOURCE, but that makes the warnings Claudio got appear instead (they are all in the block-*.c files). I've chosen to add _FORTIFY_SOURCE to the Makefile and to (back-)port the QEMU fixes that get the other warnings out of the way.
You might wonder why I wrote "(back-)port" instead of "backport". One of the three QEMU commits mentioned in my commit message is actually older than the version 0.12.50 used in x49gp, which makes this technically not a backport - but similar to the gdbstub.* files the block-*.c files have been copied out of 0.9.0 for heavy modification and were lightly touched up in the 0.12.50 update, so this fix was not present in the modified files. Updating the gdbstub part is done now, but the block drivers seem to be more stubborn - in the gdbstub update I was able to dodge the references to stuff that's stripped out of our QEMU 0.12.50 version by picking bits for system-level and user-level emulation as needed (just like it was originally done in the 0.9.0 version), but it looks like the block drivers won't let me get away so easily. And I believe getting every QEMU part in x49gp to the same version first is the best path towards a QEMU update, sooo ... that's the spot where I failed this time.

If someone else wants to look into this QEMU update mess (or if I manage to forget this info and need to look it up somewhere), you'll probably want to know what revision this version 0.12.50 actually refers to. It's somewhere between git commits 576c2cdc (from Jan 15, 2010) and baee019f (from Jan 19, 2010), both inclusive. The files touched inbetween are all stripped out, so getting more precise than that is both impossible and pointless. The old 0.9.0 version used to be shipped with x49gp as a ZIP archive extracted and patched at build time, until I ditched it as part of the second patch in this post (with Git being a distributed version control system the history suffices as a backup); it's also simply the release_0_9_0 tag, as far as I know.

The patches are attached, as usual.

ijabbott · 08-23-2018, 07:15 PM

(08-23-2018 05:44 PM)3298 Wrote: This is the other reason for getting this out; shortly after the last post a compiler / system header update caused a compilation error via a warning and -Werror on my setup. (Something about stringop and truncation, I don't remember the exact message.)

I lot of projects started getting that when GCC 8 became the default compiler on their systems. ISTR it is due to calling strncpy with a length that is the same length as the destination buffer. Something that many would argue is a reasonable thing to do if you know what you are doing! (The destination buffer may end up not null-terminated.)

Claudio L. · 08-24-2018, 02:36 AM

(08-23-2018 05:44 PM)3298 Wrote: 3 months - I got sidetracked, obviously ...
But I do have a few fixes for x49gp in store, and I want to get them out there.

Thanks for your continued work, slow is perfectly fine.

(08-23-2018 05:44 PM)3298 Wrote: Claudio, your debugger fix is finally here.

Too bad I can't test it until the weekend, this is truly helpful. I'll report back after testing!

3298 · 08-26-2018, 02:22 PM

Another fix incoming, but it's a small one that applies just to the build system.

Code:

Avoid clobbering CFLAGS, LDFLAGS, LDLIBS variables inside the main Makefile

If they are already defined as environment variables, they apparently get passed down with all changes to QEMU's configure script

and thereby to QEMU's Makefile, which chokes on it.

(Discovered while trying to package x49gp for the Arch Linux distro with the 'makepkg' utility, which supplies default flags for compilers that way.)

My computer is running Arch Linux (or rather something based on it), so I figured I might attempt to package up x49gp for installation, since that's fairly easy to do on this distro (and the supported way for system-wide installation of programs, because it lets the package manager track all files in the system directories). However, one trait of the packaging tool made the x49gp build process throw a fit: there are a bunch of definitions for standard Makefile variables in a config file for the tool, which it sets as environment variables, so stuff gets compiled with certain options by default (e.g. CFLAGS=-O2).
Today I figured out what's happening: Apparently setting an environment variable of that name causes the matching variable definitions inside the Makefile to affect that environment variable, which subsequently gets passed down to qemu/qemu-git/configure-small, where they get recorded (including the additions from the main Makefile) for use in QEMU's sub-Makefile. And when the sub-Makefile is subsequently used to compile the QEMU part of x49gp, it goes boom.
My workaround is to avoid writing to variables with those standard names, using similarly named ones (simply prefixed with "X49GP_") instead. The standard variables still get used in the recipes in addition to the custom-named ones, to allow picking up whatever options come in via environment variables. That seems to work just fine, my package got built without the manual hacks I had to use for that job till yesterday.

As a small bonus, I'm attaching the PKGBUILD file (which tells makepkg how to build the package) in case there are other Arch users who happen to be interested in x49gp.
A few notes:
- I wrote "GPL" into the license slot. x49gp has not had a proper license yet, but because QEMU is GPL code, and we're including a modified version of QEMU in x49gp (thus making x49gp a derivative work of QEMU), so it's implicitly GPL. We're probably not conforming to GPL by the letter (no explicit notice about it, let alone a copy of the license text - should probably fix this), but at least we've done so in spirit by distributing it as source. Though to be honest, until recently the reliance on parts of the build system for certain semi-regular tasks (fresh config file, SD card) just made that the most convenient option.
- It's currently pointing to https://github.com/chwdt/x49gp as the source location. That's currently supposed to be the canonical source, but the maintainer still hasn't incorporated my patch series, so until he does, this will fail horribly (at the time I write this, the version over there does not even support being installed). For the time being, I suggest changing the line that reads "source=(git+https://github.com/chwdt/x49gp)" to point to your local repository (e.g. "source=(git+file:///home/you/x49gp)") or maybe Claudio's GitHub repository, who has so far been pretty quick at publishing my patches (thanks!).
- If you plan to submit this PKGBUILD to the AUR (that's the Arch User Repository, not the Advanced Users Reference which the abbreviation normally refers to around here), it may be a good idea to wait until 1. the license thing is properly sorted out, 2. the patch above is in a public repository so it can actually compile out of the box (this will probably happen before the license thing), and 3. that repository has a tag on the master branch, so later commits don't get treated as earlier versions just because their commit hash happens to be numerically smaller than the previous one. I've hacked the version retrieval function in the PKGBUILD to not fail in tag-less situations (like what we have now) so it doesn't bail out, but there is a reason 'git describe' treats tag-less branches as errors by default.
- The filename should be PKGBUILD without any extension, it's just named PKGBUILD.txt to convince the forum software to allow uploading it. Same issue as with the diffs, so you know the drill.
- Don't worry about the dummy version number listed inside, it's automatically updated anytime you run makepkg on it. makepkg is even smart enough to check for updates via git without you having to change the version number manually, and when there's something newer than what it finds next to the PKGBUILD, it'll pull and build, complete with a version number update. An AUR submission should have a proper version number in it, though (just use whatever is the current version when submitting, that appears to be common practice among the VCS-based packages).
- It showcases how I intended the INSTALL_PREFIX mechanism to work with packaging tools: Compile ('make all') with INSTALL_PREFIX pointing to where it ultimately gets installed (because INSTALL_PREFIX is compiled into the binary to allow it to find its installed resources), then install ('make install') with it pointing into the fakeroot that will get processed by the packaging tool. That should be enough to work with pretty much any packaging tool out there.

(08-23-2018 07:15 PM)ijabbott Wrote: I lot of projects started getting that when GCC 8 became the default compiler on their systems. ISTR it is due to calling strncpy with a length that is the same length as the destination buffer. Something that many would argue is a reasonable thing to do if you know what you are doing! (The destination buffer may end up not null-terminated.)

Yes, strncpy was involved, so that's definitely the cause. But I don't get how a definition that usually makes new warnings and errors appear can make this one disappear. Anyway, it works now, and we can worry about fixing it if/when it breaks.
Null termination is indeed not an issue in this case, because the warning location was deep inside the Virtual VFAT driver - the code there was filling part of the FAT structures where things work with fixed maximum length (and space padding when names are shorter) instead of null termination.

pier4r · 08-26-2018, 06:31 PM

thumbs up! Slowly but still on track is what builds quality over time. Like posts of many users here. For example gerald H solutions in sysrpl (about him, gerald h are you still between us?)

Claudio L. · 08-27-2018, 01:39 PM

I had a little trouble with patch 28 not wanting to work but I finally fixed it manually and got it all working. I have not been able to test the debugging part, mainly because the only way to test for me is with newRPL, and haven't figured out how to flash it with this version of x49gp.
For the first time I removed my old x49gp version and moved on to this one, and noticed that:
a) Keyboard emulation is somehow not letting me use the + and - press while doing reset from the menu to recall the boot loader. Also it's very hard to do On-A-C or On-A-F. Anyway, I figured the only way to access the bootloader was to delete ~/.x49gp/flash. Perhaps there should be an option in the menu to do a reset while simulating + and - for 1 second or so?
b) Because of a) the only way to update firmware (after you install newRPL) is to mount a directory as an SD card, with a proper update.scp file and firmware, while at the same time deleting the flash image. This worked a couple of times and failed horribly about 80% of the times. Got an error message about invalid file name with some chinese symbols all the time, so it might be an ASCII vs. UTF8 problem.
c) The text in the menu comes up green when using grayscale mode. It's correctly black when using B&W mode. I think we need to save/restore the state of the gdk context while doing a screen update.

On second thought, we could also have an option in the menu to "manufacture" a flash image from a firmware file without using the bootloader. This functionality was in the original Makefile, it would copy the boot loader, patch the serial number and write a firmware image into the flash file. Much of this is already done when the flash file is deleted and recreated, all that's needed is to ask the user to select a firmware file. Ideally, a command line option to do this would be handy too, so the IDE can load custom firmware when starting the debugger.

3298 · 08-27-2018, 08:20 PM

(08-27-2018 01:39 PM)Claudio L. Wrote: I had a little trouble with patch 28 not wanting to work but I finally fixed it manually and got it all working.

I'm guessing that means you encountered additional compilation errors ... if so, could you post them so I can get them fixed? Because it's all working fine without so much as a note from the compiler over here.

(08-27-2018 01:39 PM)Claudio L. Wrote: a) Keyboard emulation is somehow not letting me use the + and - press while doing reset from the menu to recall the boot loader. Also it's very hard to do On-A-C or On-A-F. Anyway, I figured the only way to access the bootloader was to delete ~/.x49gp/flash. Perhaps there should be an option in the menu to do a reset while simulating + and - for 1 second or so?

Yes, pressing + and - quickly enough after clicking in the menu is nearly impossible. But there's a solution: Use F12 to reset instead of the menu. That way the menu won't eat your + and - keypresses, so they can be held down before punching F12. The F12 keybind is not mine, it comes from chwdt's fork which my patches go on top of.
Alternatively, right-click on + and - before opening the menu. As long as you don't left-click on any keys or press / release any hardware keys mapped to virtual keys, that should keep them down even across menu actions. That mechanism is not mine either (it's ancient), though one of the initial big batch of patches did tweak something about it.

Flashing newRPL:

(08-27-2018 01:39 PM)Claudio L. Wrote: This worked a couple of times and failed horribly about 80% of the times. Got an error message about invalid file name with some chinese symbols all the time, so it might be an ASCII vs. UTF8 problem.

That's weird. I didn't experience anything like that so far. I did flash x49gp a few times - not with newRPL, but as far as x49gp and the bootloader are concerned, the difference to the stock firmware should be just data and a filename. I noticed that you made your newRPL filenames fit into 8.3 limits with only alphanumeric characters, so that filename should be fine. The only things that might cause trouble are the names of other files in the same directory. The obvious thing to try would be using a directory with just update.scp and newrplfw.bin in it. Unless you did that already...
I also deliberately left the Makefile targets for generating SD images in, just in case someone needs to know how to generate a usable image. That plus the appropriate SD mounting option should work too; it bypasses the whole VVFAT driver and performs direct access to the image file, so it's probably more robust than the VVFAT driver. Especially when taking the age of the VVFAT driver version we have into account (from QEMU 0.9.0); there may be bugs in it.
... I know, going through an SD image is significantly more work. I'm not suggesting it as a permanent solution, but as an option to test once. Due to the simplicity of the code behind that, it'll probably work; if it doesn't, we'd have to bust out the debugger to find out if there's a bug hiding in that simple code or if it's the bootloader acting up (I hope not, because that would be really painful to work around).

(08-27-2018 01:39 PM)Claudio L. Wrote: c) The text in the menu comes up green when using grayscale mode. It's correctly black when using B&W mode. I think we need to save/restore the state of the gdk context while doing a screen update.

WHAT? That was supposed to be fixed with patch 24, the one from end of May... I'm confused.

(08-27-2018 01:39 PM)Claudio L. Wrote: On second thought, we could also have an option in the menu to "manufacture" a flash image from a firmware file without using the bootloader. This functionality was in the original Makefile, it would copy the boot loader, patch the serial number and write a firmware image into the flash file. Much of this is already done when the flash file is deleted and recreated, all that's needed is to ask the user to select a firmware file. Ideally, a command line option to do this would be handy too, so the IDE can load custom firmware when starting the debugger.

Correct, I took that section out of the Makefile. I initially planned to put equivalent C code into flash.c - but then I found out that the bootloader is perfectly fine without firmware. Overwriting the serial number is not necessary either, because the ones from the Makefile were already baked into the bootcode versions at the appropriate place; it was a no-op unless one overwrote the bootcodes shipped with x49gp. However, the Makefile had another step, which was writing certain values to specific places (at regular intervals, but not throughout the entire flash). I wasn't so sure that this would be appropriate for firmwares other than the stock one (I explicitly want to support custom firmware), so I figured it was best to hand the job of finding out what needs this treatment over to the bootloader and/or the firmware, which ought to know their own needs better than me.
I see the point about automated re-flashing though. Maybe have the flag build an SD image in memory and inject some keypresses? Resetting the calculator should be done anyway after switching the firmware, because not doing so amounts to an unsafe hot-patch.

Claudio L. · (This post was last modified: 08-27-2018 09:08 PM by Claudio L..)

(08-27-2018 08:20 PM)3298 Wrote: But there's a solution: Use F12 to reset instead of the menu. That way the menu won't eat your + and - keypresses, so they can be held down before punching F12. The F12 keybind is not mine, it comes from chwdt's fork which my patches go on top of.
Alternatively, right-click on + and - before opening the menu. As long as you don't left-click on any keys or press / release any hardware keys mapped to virtual keys, that should keep them down even across menu actions. That mechanism is not mine either (it's ancient), though one of the initial big batch of patches did tweak something about it.

Ohhhhh... so right-click on a key leaves it pressed? I had no idea, that's an interesting feature.

(08-27-2018 08:20 PM)3298 Wrote: I did flash x49gp a few times - not with newRPL, but as far as x49gp and the bootloader are concerned, the difference to the stock firmware should be just data and a filename. I noticed that you made your newRPL filenames fit into 8.3 limits with only alphanumeric characters, so that filename should be fine. The only things that might cause trouble are the names of other files in the same directory. The obvious thing to try would be using a directory with just update.scp and newrplfw.bin in it. Unless you did that already...

Yes, I did. Created an empty dir, put just the firmware newrplfw.bin and update.scp, and still the same error. The error comes up in a GUI pop-up window, it's not the emulated firmware complaining about the FAT image, but I think the VVFAT emulation layer complaining about perhaps the path of the directory I passed. The firmware never even detected the virtual SD card.

(08-27-2018 08:20 PM)3298 Wrote:
(08-27-2018 01:39 PM)Claudio L. Wrote: c) The text in the menu comes up green when using grayscale mode. It's correctly black when using B&W mode. I think we need to save/restore the state of the gdk context while doing a screen update.
WHAT? That was supposed to be fixed with patch 24, the one from end of May... I'm confused.

Me too, and when you use the stock firmware it's all black, but under newRPL it does come out green on mine, it's the latest from my github + your new patches.

(08-27-2018 08:20 PM)3298 Wrote: I initially planned to put equivalent C code into flash.c - but then I found out that the bootloader is perfectly fine without firmware. Overwriting the serial number is not necessary either, because the ones from the Makefile were already baked into the bootcode versions at the appropriate place; it was a no-op unless one overwrote the bootcodes shipped with x49gp. However, the Makefile had another step, which was writing certain values to specific places (at regular intervals, but not throughout the entire flash). I wasn't so sure that this would be appropriate for firmwares other than the stock one (I explicitly want to support custom firmware), so I figured it was best to hand the job of finding out what needs this treatment over to the bootloader and/or the firmware, which ought to know their own needs better than me.
I see the point about automated re-flashing though. Maybe have the flag build an SD image in memory and inject some keypresses? Resetting the calculator should be done anyway after switching the firmware, because not doing so amounts to an unsafe hot-patch.

The other bytes that were written were to initialize the user flash banks for the stock firmware. That's easy to do with the command PINIT, so you are right, there's no need to do that.
If the serial number doesn't need touching, all you have to do is have the user select a firmware file, open it and load it at offset 0x4000 of the brand new flash, that's it. That's all the bootloader does.
PS: When I say offset 0x4000 is in bytes, from C (as opposed to an offset in nibbles from the emulated side).

3298 · 08-27-2018, 10:39 PM

(08-27-2018 09:07 PM)Claudio L. Wrote: Yes, I did. Created an empty dir, put just the firmware newrplfw.bin and update.scp, and still the same error. The error comes up in a GUI pop-up window, it's not the emulated firmware complaining about the FAT image, but I think the VVFAT emulation layer complaining about perhaps the path of the directory I passed. The firmware never even detected the virtual SD card.

Well, that's at least something to track that down. Although that pop-up window bit puzzles me even more, because I only programmed two places where dialogs are opened, and neither of them is a message window (one is the directory version of the file chooser, the other is the file version; GTK does not support both at the same time, otherwise I'd have used just one). It might be GTK itself complaining about ... something, whatever it is. I'll do some tests with non-ASCII paths tomorrow.

(08-27-2018 09:07 PM)Claudio L. Wrote: Me too, and when you use the stock firmware it's all black, but under newRPL it does come out green on mine, it's the latest from my github + your new patches.

At the risk of sounding annoying, does "new patches" actually include the one from May or just the bunch from the last few days? Because the one from May didn't end up in your GitHub repository yet, so you might just have skipped it unintentionally. If that's not it, I'm out of ideas for now.

(08-27-2018 09:07 PM)Claudio L. Wrote: The other bytes that were written were to initialize the user flash banks for the stock firmware. That's easy to do with the command PINIT, so you are right, there's no need to do that.

Oh, so that's what they were for? I'll need to check the user banks in the stock firmware then, to see if I need to do something about them. In an ideal situation, PINIT shouldn't be necessary; if it is, I have to put a note into the manual or, better yet, reimplement the flash sector marking. Custom firmware are forced to deal with remnants of the user's flash banks on real calculators anyway, so I'm less concerned about breaking them with this now.

(08-27-2018 09:07 PM)Claudio L. Wrote: If the serial number doesn't need touching, all you have to do is have the user select a firmware file, open it and load it at offset 0x4000 of the brand new flash, that's it. That's all the bootloader does.
PS: When I say offset 0x4000 is in bytes, from C (as opposed to an offset in nibbles from the emulated side).

Yeah, that's kind of obvious. A file chooser window is pretty straightforward (I can steal code from the SD file chooser windows in a pinch), and after that it's just a 'read' call with the firmware location (just behind the bootcode, which I already know is 16KiB long) in the mmap'ed flash file as buffer. The fancy command-line parameter parser I wrote makes adding new flags pretty much a drop-in affair, so the IDE automation isn't that much more complicated either. I can even make that flag trigger the interactive selection when no filename is passed on the command line.

Claudio L. · (This post was last modified: 08-28-2018 02:58 AM by Claudio L..)

(08-27-2018 10:39 PM)3298 Wrote:
(08-27-2018 09:07 PM)Claudio L. Wrote: Me too, and when you use the stock firmware it's all black, but under newRPL it does come out green on mine, it's the latest from my github + your new patches.
At the risk of sounding annoying, does "new patches" actually include the one from May or just the bunch from the last few days? Because the one from May didn't end up in your GitHub repository yet, so you might just have skipped it unintentionally. If that's not it, I'm out of ideas for now.

I tought everything ended up in my gihtub fork, if not then that explains it. I'll fix it by reapplying old patches too.

(08-27-2018 10:39 PM)3298 Wrote:
(08-27-2018 09:07 PM)Claudio L. Wrote: If the serial number doesn't need touching, all you have to do is have the user select a firmware file, open it and load it at offset 0x4000 of the brand new flash, that's it. That's all the bootloader does.
PS: When I say offset 0x4000 is in bytes, from C (as opposed to an offset in nibbles from the emulated side).
Yeah, that's kind of obvious. A file chooser window is pretty straightforward (I can steal code from the SD file chooser windows in a pinch), and after that it's just a 'read' call with the firmware location (just behind the bootcode, which I already know is 16KiB long) in the mmap'ed flash file as buffer. The fancy command-line parameter parser I wrote makes adding new flags pretty much a drop-in affair, so the IDE automation isn't that much more complicated either. I can even make that flag trigger the interactive selection when no filename is passed on the command line.

Sounds perfect. I did some tests with 'dd' to create the flash file manually and it's very straightforward but there's something else I didn't catch before:
1) Dump the new firmware at 0x4000 as we discussed.
2) Change the 16-byte string at 0x4000 from "KINPOUPDATEIMAGE" to "Kinposhcopyright" (case-sensitive!). This is a marker the bootloader uses to tell if there's a valid firmware installed or not.

Also, there should be perhaps another option to wipe out with 0xff all the way to the end of flash. This is not needed for newRPL but stock firmware needs a clean flash after the binary, so it can initialize those banks.

PS: Can we add another command-line option '-r' to reset the CPU when x49gp starts? When you manually change the firmware you must trigger a reset, and if the change in firmware happens outside of x49gp, we need to tell x49gp that we need a reset. Otherwise it will try to resume execution with a completely different ROM, it's a sure crash (resolved by Reset from the menu, I know, but still, the idea is that when used as a debugger we should be able to start x49gp from a script to a well-known state).

3298 · (This post was last modified: 08-28-2018 11:12 AM by 3298.)

(08-28-2018 02:51 AM)Claudio L. Wrote: I tought everything ended up in my gihtub fork, if not then that explains it. I'll fix it by reapplying old patches too.

The patch 24 from May is the oldest one that didn't make it into your fork yet (Edit: the part you published on GitHub, at least). But looking at it again, I spotted what caused your troubles with patch 28: You inserted a patch adding -Wno-unused-result, which conflicted with the addition of -D_FORTIFY_SOURCE=1 in the same line. The unused results shouldn't happen anymore (as of patch 28), so you could probably revert that patch without causing problems.

(08-28-2018 02:51 AM)Claudio L. Wrote: Sounds perfect. I did some tests with 'dd' to create the flash file manually and it's very straightforward but there's something else I didn't catch before:
1) Dump the new firmware at 0x4000 as we discussed.
2) Change the 16-byte string at 0x4000 from "KINPOUPDATEIMAGE" to "Kinposhcopyright" (case-sensitive!). This is a marker the bootloader uses to tell if there's a valid firmware installed or not.

Right, I forgot about that step. It's in the old Makefile though, so I can just use that as a guide. VCS history for the win.

(08-28-2018 02:51 AM)Claudio L. Wrote: Also, there should be perhaps another option to wipe out with 0xff all the way to the end of flash. This is not needed for newRPL but stock firmware needs a clean flash after the binary, so it can initialize those banks.

Good point. I wanted to leave the flash intact and only overwrite what's necessary (to avoid overwriting the user banks when someone just switches between different versions of the same firmware), but another flag to force full reinitialization (as if the flash image was deleted) is not a terrible idea.

(08-28-2018 02:51 AM)Claudio L. Wrote: PS: Can we add another command-line option '-r' to reset the CPU when x49gp starts? When you manually change the firmware you must trigger a reset, and if the change in firmware happens outside of x49gp, we need to tell x49gp that we need a reset. Otherwise it will try to resume execution with a completely different ROM, it's a sure crash (resolved by Reset from the menu, I know, but still, the idea is that when used as a debugger we should be able to start x49gp from a script to a well-known state).

I was just going to make the flash reinitialization code reboot automatically, but for debugging startup code it may indeed be beneficial having access to such a flag, even when not reinitializing the flash.

Claudio L. · 08-29-2018, 02:17 AM

Done. I applied all your patches and pushed to my github, including the missing one (24) from May. It all works well now.
I was getting and error with the FORTIFY_SOURCE being redefined, so I removed from the Makefile, I don't know if it can affect other platforms (I only tested on Linux).

I also tested it as a debugger, and it worked much better than previously. Now it's fast and responsive, I can inspect any kind of variables, etc.
The only issue is that stepping over one line sometimes jumps many lines, as in goes overboard. For example I single-stepped one line within a loop, and by the time it stopped it had finished the loop and advanced 5 or 6 lines past the end of the loop. But at least now there's no errors when setting and removing breakpoints, and it actually stops at breakpoints (seems trivial but not always worked before).