Apple2enh / Language card: Fix .cwd_init constructor corrupting code bytes

[colinleroy]

Constructor .cwd_init calls apple2's initcwd, which does an sta/stx to struct mliparam. This struct is stored in BSS, so a few bytes of BSS, which as that time is still possibly containing code destined to be Block-Transfered-Up to the Language Card.

The fix consists of moving the LC code before calling initlib.

[polluks2]

Missing tail call optimization

cc65/libsrc/apple2/crt0.s

Line 159 in 1bb4841

jsr initlib

jmp initlib

[colinleroy]

Detailed explanations of how I tracked the problem, if it helps: https://www.colino.net/wordpress/archives/2023/08/15/tracking-and-killing-a-cc65-corruption-bug/

[oliverschmidt]

https://cc65.github.io/doc/apple2.html#s3 says:

While running main() the Language Card bank 2 is enabled for read access. However while running module constructors/destructors the Language Card is disabled.

This pull request turns that statement wrong. But it's not only about the documentation. This pull request actually breaks existing code that relies on the documented behavior:
https://github.com/cc65/cc65/blob/master/libsrc/apple2/reboot.s#L17-L18
https://github.com/cc65/cc65/blob/master/libsrc/apple2/get_ostype.s#L17

[bbbradsmith]

Constructor .cwd_init calls apple2's initcwd, which does an sta/stx to struct mliparam. This struct is stored in BSS, so a few bytes of BSS, which as that time is still possibly containing code destined to be Block-Transfered-Up to the Language Card.

This sounds like the bug is initcwd itself? Since mliparam is .bss any value set during init is going to be obliterated by zerobss immediately after?

Since BSS is expected to replace the ONCE and LC areas after they're used, and constructors are run before BSS is initialized, they must not use it.

Looking at apple2.cfg it seems that really mliparam should just be stored in INIT (or possibly DATA) instead?

Are there any other constructors erroneously trying to use BSS before it's initialized?

[oliverschmidt]

This sounds like the bug is initcwd itself?

Yes.

Since mliparam is .bss any value set during init is going to be obliterated by zerobss immediately after?

That's not the point...

Since BSS is expected to replace the ONCE and LC areas after they're used, and constructors are run before BSS is initialized, they must not use it.

That's the point! I documented this in https://github.com/cc65/wiki/wiki/Segment-usage-of-constructors

Looking at apple2.cfg it seems that really mliparam should just be stored in INIT (or possibly DATA) instead?

That would be an option. However, I think it's preferable to have https://github.com/cc65/cc65/blob/master/libsrc/apple2/initcwd.s not call https://github.com/cc65/cc65/blob/master/libsrc/apple2/mli.s in the first place.

I'm the Apple II library maintainer and will provide a fix. But please don't hold your breath.

@colinleroy: Thanks for the bug report and the excellent root cause analysis :-))

[colinleroy]

Hi @oliverschmidt and @bbbradsmith . Thanks a lot for your input. I have pushed a different version of the fix, storing mlidata in the DATA segment instead (not INIT, as this cwd.s is used during the whole program's life).

There are apparently no other constructors using bss in apple2/ or common/; but there are on other platforms according to grep -i '\.bss' $(grep -rl '\.constructor' libsrc/*).

[colinleroy]

Hi,

[data segment] would be an option. However, I think it's preferable to have https://github.com/cc65/cc65/blob/master/libsrc/apple2/initcwd.s not call https://github.com/cc65/cc65/blob/master/libsrc/apple2/mli.s in the first place.

That seems more complicated? What would be the drawback of storing mliparam in .data?

@colinleroy: Thanks for the bug report and the excellent root cause analysis :-))

Thanks, that means a lot! (I really outdid myself to find it)

[bbbradsmith]

I have pushed a different version of the fix, storing mlidata in the DATA segment instead (not INIT, as this cwd.s is used during the whole program's life).

I believe the correct segment is INIT, which I suspected but oliverschmidt's documentation link clarified. BSS is uninitialized memory, and INIT is the uninitialized alternative for this purpose. DATA is not appropriate because mlidata does not contain an initial value, and it would cause the build to produce unnecessary storage of initialization values for it.

[colinleroy]

I believe the correct segment is INIT, which I suspected but oliverschmidt's documentation link clarified. BSS is uninitialized memory, and INIT is the uninitialized alternative for this purpose. DATA is not appropriate because mlidata does not contain an initial value, and it would cause the build to produce unnecessary storage of initialization values for it.

I see, thanks. I thought the things in the INIT segment were later given back to code storage? Also (probably) cwd.s expects this struct to be initialized?
I can absolutely update the patch again.

[colinleroy]

Hi @oliverschmidt,

That would be an option. However, I think it's preferable to have https://github.com/cc65/cc65/blob/master/libsrc/apple2/initcwd.s not call https://github.com/cc65/cc65/blob/master/libsrc/apple2/mli.s in the first place.

Did you mean something like that ? master...colinleroy:cc65:initcwd-no-mli-s
I don't think it's better than moving the mliparam's segment, because it duplicates quite a bit of checks and saves/restores from mli.s... But maybe I did it badly?

I'm the Apple II library maintainer and will provide a fix. But please don't hold your breath.

TBH I'd love to get "my" bug fixed by myself (hence the looking around for other ways of fixing it) 😃

[oliverschmidt]

TBH I'd love to get "my" bug fixed by myself (hence the looking around for other ways of fixing it) 😃

Okay then...

DATA vs. INIT

If you look at https://github.com/cc65/cc65/blob/master/cfg/apple2.cfg#L28-L29 they are more or less identical. And they both don't get overwritten by BSS, see https://github.com/cc65/cc65/blob/master/cfg/apple2.cfg#L18

So for Apple II specific code as we talk about here, it doesn't make a difference if DATA or INIT is used. However, we want to do things "right". Look at i.e. https://github.com/cc65/cc65/blob/master/cfg/atari-cart.cfg#L27-L28. This is for producing a ROM, here everything is either:

• Read-Only
• Copied from ROM to RAM
• Initialized from scratch in RAM
  So here, DATA is copied, see https://github.com/cc65/cc65/blob/master/libsrc/atari/cartstart.s#L14-L16, but INIT is not initialized at all.

So taking the ROM scenario into account, https://github.com/cc65/wiki/wiki/Segment-usage-of-constructors hopefully starts to make sense. If we follow the approach to move mliparam then INIT is the correct segment.

mliparam in INIT vs. initcwd modification

If you look at https://github.com/cc65/cc65/blob/master/libsrc/apple2/mli.inc#L78-L90 you see that mliparam is 16 bytes. Moving mliparam to INIT (or DATA) makes every cc65 program that uses ProDOS 16 bytes larger. It's NOT requiring more RAM, but more disk space.

On the other hand, making initcwd larger (by not calling into callmli as-is) makes only cc65 programs larger that use getcwd() or chdir(). But here, it's both disk space and RAM.

I have an idea how to approach that in a clever way, but I need to run right now. I'll explain later...

[colinleroy]

Thank you veeeeery much for the detailed information! I now understand much better the reason you would prefer not using callmli on initcwd, so that only the programs using chdir/getcwd get larger.

Edit: I have made a batch of tests to quantify things (mostly with my other branch modifying initcwd.s to avoid calling callmli) and here are the numbers:

Programs not using chdir / getcwd:
mliparams in INIT: +18 bytes on disk (the struct is 18 bytes in fact), no change in RAM
not using mli.s in initcwd: no change at all

Programs using chdir / getcwd:
mliparams in INIT: +18 bytes on disk (the struct is 18 bytes in fact), no change in RAM
not using mli.s in initcwd: +15 bytes on disk, +15 bytes in RAM.

(The latest one drops to +1/+1 bytes if I remove the random counter save/restore from initcwd... But I suppose it's there in mli.s for a reason - Google didn't help me)

I updated my master branch for this PR with the second solution, which seems indeed better, and hope I'll be able to understand your clever idea and implement it :)

[colinleroy]

Hi @oliverschmidt,
Thanks again for the updated information about the possible optimization (which for some reason I see in my email, but not on this page :/) . I have pushed that to master so that it reflects on this PR, and we're now at +9 bytes on disk and in RAM.

If the RNDH/L save/restore is unnessary in initcwd, we could even get rid of the bug and save 3 bytes (both disk/RAM) from origin/master :)

[oliverschmidt]

If the RNDH/L save/restore is unnessary in initcwd, we could even get rid of the bug and save 3 bytes (both disk/RAM) from origin/master :)

It's always a good approach to check out the Git history of a file that contains things you don't understand. Here it's a9a102b. This is even especially important for a constructor, because a program might very well call randomize() before wait for user input. Then the only values available are the once from user input before the program was run.

[bbbradsmith]

Ah, I didn't realize INIT was not able to avoid producing disk space in this configuration. I feel like it's probably possible,but I can see how it would be tricky to make work. (Inserted before MAIN, perhaps? ...but that adds some complications.)

Should the type of INIT not be bss instead of rw, if this is what it should mean semantically? It would prevent it from being accidentally used with initialization in case someone did want to attempt this program size optimization later.

[oliverschmidt]

I'll explain later...

Because of https://github.com/cc65/cc65/blob/master/libsrc/apple2/mli.s#L20, callmli is placed in the DATA segment (https://github.com/cc65/cc65/blob/master/libsrc/apple2/mli.s#L16). In general, everything is fine about callmli. The only problem is the mliparam https://github.com/cc65/cc65/blob/master/libsrc/apple2/mli.s#L36 address. So we could fix exactly that :-)

We (you) could change https://github.com/cc65/cc65/blob/master/libsrc/apple2/mli.s#L36 to have a label. I propose mliptr. That label could be made available by inserting it between mliparam and callmli at https://github.com/cc65/cc65/blob/master/libsrc/apple2/mli.inc#L130-L131.

Then one could set mliptr to the own mliparam before https://github.com/cc65/cc65/blob/master/libsrc/apple2/initcwd.s#L18 and set mliptr back to global mliparam after https://github.com/cc65/cc65/blob/master/libsrc/apple2/initcwd.s#L21.

I would consider this a clean solution. However, it requires 20 additional bytes code which is more than the roll-your-own-callmli if I'm not mistaken :-(

[oliverschmidt]

Ah, I didn't realize INIT was not able to avoid producing disk space in this configuration. I feel like it's probably possible,but I can see how it would be tricky to make work. (Inserted before MAIN, perhaps? ...but that adds some complications.)

I won't keep you from doing it... I have no overview on the potential problems. Things that come into my mind right away that you might overlook:

• https://github.com/cc65/cc65/blob/master/libsrc/apple2/extra/iobuf-0800.s
• https://github.com/cc65/cc65/blob/master/cfg/apple2-overlay.cfg

Additionally this would mean that the actual load/start address wouldn't be a well known value anymore. I personally have no issue with that at all, but there are quite some users who prefer to ignore our AppleSingle metadata handling and manage that metadata manually.

And finally the value in https://github.com/cc65/cc65/blob/master/libsrc/apple2/mainargs.s#L15-L17 wouldn't be a well known one anymore.

Should the type of INIT not be bss instead of rw, if this is what it should mean semantically? It would prevent it from being accidentally used with initialization in case someone did want to attempt this program size optimization later.

I don't know if the linker is willing to write a memory area that contains a segment of type bss to a file. If yes, then this change might be desirable. Maybe you want to take care of trying it...

[colinleroy]

Hi @oliverschmidt,
Would you like to merge this solution, or would you rather I try the one with an mliptr ?
Thanks!

[oliverschmidt]

The mliptr approach was more or less a proposal to you as you were unhappy with the code duplication. But you seem to not have a strong opinion. I tend to favor the smaller solution. Merging...

[oliverschmidt]

So you now know how it is to contribute to cc65: More details, more discussions, more effort than anticipated ;-)

[colinleroy]

The mliptr approach was more or less a proposal to you as you were unhappy with the code duplication. But you seem to not have a strong opinion. I tend to favor the smaller solution. Merging...

Yes, I'd rather have avoided it, but not at the cost of the solution being bigger :)

So you now know how it is to contribute to cc65: More details, more discussions, more effort than anticipated ;-)

But mostly, more things learnt!

Thank youuuu :)

[oliverschmidt]

But mostly, more things learnt!

I always (try to) invest into the ones eager to learn - and you seem very eager!

Thank youuuu :)

Thank you for finding and fixing the issue :-)