Paint crashes when using Brush tool

Nice work! I really appreciate your help, thanks Rick.

First, it’s probably the LDR two instructions earlier that is choking

Ah, that explains a lot. I didn’t know it does that.

perhaps knowing the values of the registers may help?

Do you have any backtrace, to know where in Paint it is failing? (more specifically than “the brush tool”)? Do you have the value of any registers at the point of failure?

As mentioned previously, I don’t remember ever personally seeing Paint crash in SharedCLibrary, so I’m relying on whatever dumps other people may post in this thread.

I have had various other crashes but to date the only repeatable ones have turned out (AFAICR) to be things I’d done wrong (I have recently been getting Illegal Window Handle apparently relating to a RISC_OSLib dbox I’ve added but perversely the errors went away as soon as I added more debug output; grrr).

If it helps, I created a simple backtrace handler that dumps its output to file or screen (depending if file handle given).

Yeah, that could be exceedingly useful, thank you.

I’m only tinkering around the edges of debugging these crashes at the moment in between working on the other features. There’s a build option XTRACE in Paint that I leave off most of the time (because it spams megs and megs of output to the logs) but that does a lot of extra checks on the integrity of the memory (de)allocations so that might shed some more light on any problems as well. At some point, maybe at the end when I stop changing the rest of the code, I can hopefully pay full attention to figuring it out.

but perversely the errors went away as soon as I added more debug output; grrr

Exactly what I saw with Manga (with the added benefit of when it failed, it was a complete machine freeze!).

You probably have something that’s writing beyond its buffer/allocation, and is trampling on something else.

I’ll describe more later this evening, when I’m back home.

Something like

*MemoryI PC -24 +40

In a taskwindow

Plus

*Showregs

After a abort.
--

Also the utility Prog

Addr

Back pedals from an abort address to a Function name.

Written by Justin Fletcher 0.12

[Updated]

Any chance for updates to Addr Justin/Ian :-)

*MemoryI PC -24 +40

Unfortunately it is getting stuck in ROM. It would be better to try to understand how it ended up there.

This is probably what you want: https://www.riscosopen.org/wiki/documentation/show/Debugger%20Annotated%20Exception%20Dumps

Oh, and ensure things are built with function names embedded. There’s no excuse not to, these days.

but perversely the errors went away as soon as I added more debug output; grrr

Exactly what I saw with Manga (with the added benefit of when it failed, it was a complete machine freeze!).

And when I undid the debugging changes, the errors still didn’t come back.

You probably have something that’s writing beyond its buffer/allocation, and is trampling on something else.

Probably. The hard part is spotting what it is and that’s assuming it’s even something in Paint’s code and not the OS or libraries.

*MemoryI PC -24 +40

Regarding the new “Invalid window handle” error, I think “*where” just returned &00000000.

Also the utility Prog

Addr

Back pedals from an abort address to a Function name.

Written by Justin Fletcher 0.12

Was that bundled with RISC OS 4?

No – but the author has been on this website under another name – Ian?

Addr

I have found a 32bit Addr v0.12 on an Iyonix backup where presumably it worked. It doesn’t work now on OS5.27, RPCEmu to ensure a low vector environment.

*addr 8f00
Area Free pool (inaccessible) has start within Zero page

It looks more useful on OS4.02.

Address &8f00
 Application space, at &8000 offset &f00 (size &1bf8000)
*

I even found its help!!!

Addr	0.12 (06 May 2003)
*Addr is a debugging tool, used to describe an address in memory. It is intended for developers to locate problems and isolate faults.
Syntax: *Addr <address> | -A <filename> | -a | -e | -m | -h

Switches:
  -A Absolute file to search for address in
  -a Display address of last abort
  -e Display list of known entities (entry points, known data points)
  -m Display entire known memory
  -h Display help

Some notes from the author.

As promised: the fun, frolics, and freaky headaches of a one byte overrun in reading a string.

https://heyrick.eu/blog/index.php?diary=20200908
(plus, you get to see Anna ¹ handling a ladder…both ways!)

¹ For those who don’t read my blog, Anna is a kitten.
Jeez, you didn’t think I’d be able to land an actual female human did you?!?!? ☺

Interestingly to read about Gerf’s involvement with DDT.

Perhaps he could explain what can and cannot be done with the Prog.

DDT seems to written in ‘C’ what’s the latest version number?

The version I have seems to query zero page.

I believe 1.98 is the latest.

I have had various other crashes but to date the only repeatable ones have turned out (AFAICR) to be things I’d done wrong (I have recently been getting Illegal Window Handle apparently relating to a RISC_OSLib dbox I’ve added but perversely the errors went away as soon as I added more debug output; grrr).

Turns out that particular Illegal Window Handle was my fault as well. I wasn’t checking whether the tool window had been closed before trying to disable the opacity slider.

I’m hesitant to say this, but we may, just maybe, be on the verge of a breakthrough with this brush bug!

ROOL very kindly saved me some crash dumps from when Paint crashed using the brush tool while they were working with some screenshots.

Luckily, this time with their help we were able to narrow down the location of the crash to near the start of psprite_drop_translation(). This function is responsible for deleting translation tables both for the sprite and the brush tool.

void psprite_drop_translation (main_ttab **tb)

{ ftracef0 ("psprite_drop_translation\n");

  if (*tb != NULL)
  { if (--(*tb)->refcount == 0)
    { main_ttab **prev;

      for (prev = &tbchain; *prev != NULL; prev = &(*prev)->link)
        if (*prev == *tb)
        { ftracef1 ("ttab block @ 0x%X dies\n", *tb);
          *prev = (*tb)->link;
          psprite_ttab_delete (*tb);
          break;
        }
    }
    /* Always nullify the pointer passed in, even if refcount hasn't *
     * reached 0, because this particular reference is now invalid   *
     * and should no longer be used.                                 */
    *tb = NULL;
  }
}

I believe the crash is occurring on the line --(*tb)->refcount because it’s trying to read the supplied translation table’s reference count from memory, decrement it, and then store the updated value. It’s falling over when the value in tb is pointing to an address before the start of application memory, hence the aborts when it tries to read or sometimes write there.

There are two translation tables that could be relevant here, sprite->transtab and sprite->toolspace[2]. To cut to the chase, sprite->toolspace[2] is I think the likely culprit. But what is it, I hear you ask?

Well, the toolspace is an array of integers that the code for each of Paint’s tools uses as a sort of scratch space. Significantly, this means the values in the toolspace have different meanings depending on which tool is selected. In the case of the brush tool, toolspace[2] is supposed to hold a pointer to a translation table for brush plotting.

Annoyingly I have still never, ever been able to reproduce this crash on my systems, but my current best guess by inspecting the code is that psprite_drop_translation() is being called with an invalid pointer in toolspace[2] and I strongly suspect it’s because sprite->toolspace[2] was never initialised with the translation table pointer.

Ordinarily, when the brush tool’s selected, toolspace[2] is supposed to be assigned a translation table pointer in the function Set_Brush_Translation(). However, there’s a condition in there with the comment “amended to do nothing if a sprite has no windows open”. That’s a perfectly good optimisation because building translation tables can be slow. The bit that I think is, shall we say, inappropriate, if I’m being kind, inexcusable, if I’m not – at least on modern machines – is that when there are no windows open on the sprite, it could at least clear sprite->toolspace[2] to zero, but it doesn’t. So it gets left with whatever was in there before, often a coordinate used by an earlier tool which can obviously cause fireworks if psprite_drop_translation() thinks that’s a pointer to dereference!

The way the code is supposed to work is that if a sprite has no windows open, its toolspace is left alone. As soon as you open a window onto that sprite, the toolspace is then supposed to be initialised appropriately for the current tool. The fact that each sprite has its own toolspace whereas the current tool itself is global for all of Paint makes things more complicated, especially if you’re switching between multiple sprites and using multiple tools. It means there are likely some edge cases that can trigger the crash.

I’m thinking this sort of thing might cause a crash sometimes:

1. Select the camera tool and maybe use it a bit.
2. Create (or load) a new sprite called “one”.
3. Close all sprite windows of “one” but keep its sprite file open.
4. Edit a different sprite with the brush tool (doesn’t matter if you create it, load it or already had it open).
5. Now attempt to delete sprite “one”.

Deleting the sprite may cause a crash because even though the brush tool is selected, the sprite never got the translation table pointer assigned in its toolspace, because it had no sprite windows open.

Here’s why the deletion of a sprite in those circumstances can cause a crash, in the code for psprite_delete():

  /* free any brush translation table */
  if (toolwindow_current_tool == &tools_brushpaint)
    psprite_drop_translation ((main_ttab **) &sprite->toolspace [2]);

That code doesn’t check whether “sprite” has any windows open. If it doesn’t, toolspace[2] will be invalid.

Now obviously the above steps describe more of a “crash on sprite deletion” rather than a “crash on brush selection” and psprite_delete() isn’t called by the brush code but it illustrates the dangers of leaving toolspace[2] uninitialised, so perhaps there’s another set of circumstances where it can lead to a crash (You know, I have to wonder whether sometimes by chance psprite_drop_translation() is supplied values that let it merrily overwrite a random location of memory that doesn’t immediately crash Paint but destabilises things so the crash will happen soon afterwards).

There’s another situation where toolspace[2] is failed to be initialised for a sprite when the brush tool’s selected, and that’s if the current brush sprite pointer is null. That may have happened in older versions of Paint when the brush sprite got deleted (thanks to Will Ling for highlighting a bug involving this) but I added code that should mitigate that by swapping to the Circle brush.

My plan of action is to make sure that toolspace[2] is guaranteed to either be the brush translation table pointer or null whenever the brush tool is in use. We’ll see if that makes the crash go away. If it doesn’t, it would suggest it’s either a different translation table causing the crash, or something’s corrupting the toolspace memory of the sprites, or something else completely unforeseen (an OS bug? Faulty RAM? The list is endless).

That’s enough for now, methinks.

It’s falling over when the value in tb is pointing to an address before the start of application memory

[…]

Annoyingly I have still never, ever been able to reproduce this crash on my systems

What sort of RISC OS are you using? High vector or low?

If high vector, do you have the compatibility page enabled?

In other words, does this work in BASIC:

PRINT !0

If it works, and you have a high vector build, then:

SYS "OS_Memory", 20, 0

will turn it off so zero page addresses cause a crash.

Together with either ZeroPain or the Debugger annotated dump, it can help you see what’s been going on.

It’s OS_Memory 20, 1 to turn the compatibility page back on again.

if (*tb != NULL)
{ if (--(*tb)->refcount == 0

Of course, that code is a particular sort of awful. I mean, it’s saying “if tb isn’t a null pointer than let’s pre-decrement it and then go look at an array element of something else”. WTF?
<shudder>
It’s code like that that gives C a bad rep.

Of course, that code is a particular sort of awful. I mean, it’s saying “if tb isn’t a null pointer than let’s pre-decrement it and then go look at an array element of something else”.

Is it??? I’ll go and revise operator precedence. I can tell you what the programmer intended it to do. They intended it to pre-decrement the refcount integer variable and then test if it’s 0. If, as you say, it’s decrementing the “*tb” pointer, then that in itself is a terrible bug! EDIT: My understanding is the programmer was correct as the -> operator has higher precedence than the decrement operator.

SYS “OS_Memory”, 20, 0

On my RPCEmu, PRINT !0 still works after running the above call. Also, where can I download ZeroPain?

where can I download ZeroPain?

It’s in the Bonus binaries package.

if (—(*tb)→refcount == 0)

I think that’s an example of trying to do too much in one line.

On my RPCEmu, PRINT !0 still works after running the above call.

If my memory’s correct (and it may not be!), the zero page relocation only occurs on newer hardware (where the Risc PC and therefore RPCEmu are “old” and the Raspberry Pi is “new”).

Ah thanks Chris, because I just checked RiscOS/BuildSys/-/blob/master/Components/ROOL/IOMD32 and there’s no kernel option specified for HiProcVecs specified there (I’ve just been reading Rick’s blog about all this). ; )

I’ll try it again on my Pi later, though that’s running RISC OS Direct at the moment which may be built vector low – I don’t know. I never get the brush crashes on my Pi either though FWIW but I’m beginning to suspect it’s something to do with the way other people are using Paint, in a way that never occurs to me!

On my RPCEmu, PRINT !0 still works

Ah, that’s too old, it won’t support vector relocation. As far as I’m aware, VBAR was introduced in ARMv7?

I’ve just been reading Rick’s blog about all this). ; )

Now that the compatibility page exists and shonky stuff doesn’t randomly drop dead, there’s no reason to use low vector any more. I’ve been running high vector builds for about two years now.

Anyway, exciting news: I believe I have found the smoking g__ – erm, let’s just call it the smoking non-violent defence accessory – you have to watch what you say these days!

I had another look at a couple of ROOL’s crash dumps where it crashed in psprite_drop_translation() and I saw they still had a return address in R14. I subtracted the address of the crash from that to try to find the offset from there to the function that had called psprite_drop_translation(). I looked at that offset in a dump of the ROM I had saved and from my calculations that puts it as psprite_free_brush_blocks(). There was a slight oddity in that the BL that actually calls psprite_drop_translation() seemed to be about eight instructions further down, so I don’t know if my arithmetic was a bit off or some oddity with the ROM, or what?

I don’t think it matters too much though because the good news is that psprite_free_brush_blocks() gets called a lot. Most notably, it’s called when a brush is first selected (in the code from the old OK button) and since I added the Shape / Stamp / Tint options with the transfer function, it even gets called during the brush plot code, for example when the colour used with Shape or Tint is changed. And, more importantly, **drum roll…** there seems to be a bug in it!:

os_error *psprite_free_brush_blocks (void) /*free any sprite specific block*/

{ os_error *error = NULL;

  ftracef0 ("psprite_free_brush_blocks\n");
  if (tools_brushsprite_ptr != NULL)
  { main_window *window;
    /*int i;*/

    /*!!!for (i = 0; i < 8; i++)
    { psprite_drop_translation (&brush_translations [i]);
      brush_translations [i] = NULL;
    }*/

    for (window = main_windows; window != NULL; window = window->link)
      if (window->tag == main_window_is_file)
      { main_sprite *sprite;
        for (sprite = window->data->file.sprites; sprite != NULL;
             sprite = sprite->link)
        { psprite_drop_translation ((main_ttab **) &sprite->toolspace [2]);
          /*sprite->needsnull = 0;*/
            /*not done here*/
        }
      }
  }

  return error;
}

What that’s doing is it’s looping through all the sprite file windows and for each one looping through every sprite in that file and trying to drop what it assumes is a translation table at toolspace[2] – trouble is, as I’ve explained in my wall of text earlier, for sprites that didn’t have an open sprite window when the brush tool was selected, they could have any random junk in toolspace[2], which may often cause an abort!

I hope that’s what’s been going wrong all these years because if so, I should be able to get a fix together. For completeness I still want to see the crash happening on one of my systems. I’ll have a go on the Pi but otherwise it may come to popping up a message box if the ttab pointer is less than &8000!

I’ll go and revise operator precedence

😋

Different languages have different precedence rules, and it’s a pain to re-enter all of it. Brackets are your friends!

For completeness I still want to see the crash happening on one of my systems. I’ll have a go on the Pi but otherwise it may come to popping up a message box if the ttab pointer is less than &8000!

For testing on RPCEmu I’ve just coded that check for sprite->toolspace[2] being less than &8000 (and greater than 0) in psprite_free_brush_blocks() and can confirm that happens pretty much every time if you just:

1. Run Paint and make a new (smallish) sprite.
2. Use the scissor tool on that sprite.
3. Close all sprite windows.
4. Make a second sprite.
5. Use the brush tool on that sprite in Shape mode.
6. Change the foreground colour.
7. Paint some more on that second sprite with the brush tool.

If anyone’s a little bored and running a high vector RISC OS, would you like to try the above steps and tell us if it explodes?

Sometimes using the scissor tool isn’t even needed but it’s a guaranteed way to write a coordinate into the first sprite’s toolspace[2] which will then get misinterpreted as a translation table pointer in step 7! Fix to come hopefullly soon.

I got as far as item 5 before getting "Unrecoverable internal error (abort on data transfer at &FC43C468). I’ll try again as I didn’t perform exactly the commands you specified.

Second attempt (being careful to do the exact things you specified) gave “abort on data transfer at &FC43C460”, again at item 5.

Raspberry Pi 4B, RISC OS 5.28 (16 December 2020).

I got as far as item 5

Thanks. I’d call that a repro. I’m not 100% sure why it might try to drop translation tables at step 5. I think it would do it if you were painting in a colour other than black or if you changed any of the brush settings, chose a different brush sprite or also if you swapped from the brush tool to another tool and back.