How can my changes be re-integrated upstream?

Bug reports (if you don't/won't have a Github account)
Post Reply
pjshumphreys
Member
Posts: 66
Joined: Sat Feb 06, 2021 2:32 pm

How can my changes be re-integrated upstream?

Post by pjshumphreys »

Hi there,

In the process of working on a project on mine I've cloned z88dk so that I could make a
few changes to fix what seem to me to be bugs and also to add a couple of features.

I'm now wondering, what could be done to merge my amendments back into the upstream version of
z88dk?

Thanks


Here's a list of what I have:


Potential bugs(?):
------------------

1. If you're compiling code for the esxdos zx spectrum disk system and use fopen to
read a file that doesn't exist, the fopen call didn't fail but instead created a new file.

I've made a change that should make the fopen call return null instead:

https://github.com/pjshumphreys/z88dk/c ... a1d9e54f5c


2. Again when compiling using esxdos file io, I found that fread would behave seemingly
incorrectly according to standard C when reading past the end of a file:
I'm not 100% sure but feof should return a non 0 value in such a situation right?

I believe this commit would sort of fix this by checking whether fread actually read
any bytes before flagging an error:

https://github.com/pjshumphreys/z88dk/c ... d9baf312f6


3. I was experiencing some problems with the call stack becoming unbalanced when I used
fseek with esxdos.

This commit seemed to fix that:

https://github.com/pjshumphreys/z88dk/c ... 3fdf0973ca


4. I believe a file's path was changed historically for residos support on a zx
spectrum, but a reference to that file wasn't updated:

Here's my commit to update it:

https://github.com/pjshumphreys/z88dk/c ... c3b31487ba


5. Code related to l_qsort and l_bsearch wasn't being placed into the code_clib section
along with the rest of clib. I appreciate these two functions are z88dk specific, but
they are part of z88dk's c library and not user written code right?

https://github.com/pjshumphreys/z88dk/c ... 42d01f62ec


6. The MSXDOS2 file io library for msx targets doesn't seem to be being compiled by default.
I've been able to get it built and manually put in the right place, but I don't know
how to get it done automatically when the full toolchain is being built.

https://github.com/pjshumphreys/z88dk/c ... c21f226337


7. In the MSXDOS2 file io library, there seems to be a typo where the getcwd.asm file
instead defines a getwd function. The getcwd and getwd functions in general have
different semantics plus there is also a getwd.asm file present in the same folder,
so this seems to be a typo in getcwd.asm

https://github.com/pjshumphreys/z88dk/c ... eab83dff6a

8. When I did a fresh checkout of z88dk to build it from source, I experienced
problems with a couple of tests failing. I got these tests to pass by amending
testsuite/results/Far_Pointer_Call_ieee.opt and testsuite/results/mathops_ieee.opt,
but I'm not sure what the correct output code here should be.

https://github.com/pjshumphreys/z88dk/c ... 677700adc3


New features
------------

9. In order to support the page banking facilities I need for my project, I've added a
new command-line option to manually set the offset that local variables are accessed
from for *all* functions compiled by an invocation of z88dk.

The syntax is

--base=[integer for local variables custom offset]

This will break code if compiled directly to an object file, but my intention with this
is to only compile C code into assembly language so that I can then integrate this asm
code together with my own custom banking trampoline asm in the build process of my project.

The commits to add this are at:

https://github.com/pjshumphreys/z88dk/c ... d16d94e269

and

https://github.com/pjshumphreys/z88dk/c ... 63be8894ff


10. I've added an INCBIN directive to z88dk-z80asm as an alias to the BINARY directive.
I understand the INCBIN directive may have also been added to upstream z88dk recently,
but I thought it might be worth showing my commit to implement it as it's only a 2
line change:

https://github.com/pjshumphreys/z88dk/c ... 868d633250
User avatar
dom
Well known member
Posts: 2072
Joined: Sun Jul 15, 2007 10:01 pm

Re: How can my changes be re-integrated upstream?

Post by dom »

Hi, thank you for these - just in the nick of time: I'm planning on promoting a snapshot tomorrow.

Normally I'd suggest a PR, but since they're small and buried in your tree I've just started to incorporate them directly.

1 and 2: I need to look at these in a bit more detail, I think O_APPEND is now implicit isn't it?

3 and 4: I've made the appropriate corrections: I think with the lseek() one, there's an issue with merging of the function decorators which I'll investigate later on.

5: I've taken directly.

6: this one is slightly baffling. It should be being built already, so I'll take a proper look.

7: good spot!

8: That's worrying - some of those changes are wrong, what OS/cpu are you on?

9: I removed a very similar feature a few years ago: https://github.com/z88dk/z88dk/commit/2 ... 275b9L1471 I think in hindsight I should have implemented it as a #pragma rather than a config option. BTW there should be no need to add the option to zcc - all non recognised options are I think passed through to the compiler.

I did manage to get banking calls to work without needing the offset: https://github.com/z88dk/z88dk/blob/mas ... d_call.asm at the cost of having to use a temporary stack which makes same-bank calling painless.
pjshumphreys
Member
Posts: 66
Joined: Sat Feb 06, 2021 2:32 pm

Re: How can my changes be re-integrated upstream?

Post by pjshumphreys »

1. Is O_APPEND meaningful when attempting to read a file?

I was seeing new files being created and then opened for reading when I used something like the code below to test whether a file exists:

Code: Select all

FILE * readFile = fopen("notthere.txt", "rb");  /* never returned null, but just created a new file and opened that for reading */
6. msxdos2.lib was being built, but I've had to keep manually placing it into z88dk/lib/clibs/ in order to link with it whenever I first build the toolchain. I don't know if all libraries should be ending up in there, but it seems all the other ones built get placed into that folder.

8. I'm on Fedora 31:

Code: Select all

$ uname -a
Linux localhost.localdomain 5.8.18-100.fc31.x86_64 #1 SMP Mon Nov 2 20:32:55 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
9. yes I've seen some of the bank stack work that's been done recently. How big does this bank stack need to be? My project can use a great deal of heap space, and calling into other banks hasn't really been an issue for my trampoline approach up to now. I'd kinda prefer to not use a separate bank stack if the standard toolchain could be made to not need it somehow.

In case you're interested in how I've got things set up, the build script for the zx spectrum variant of my project is at https://github.com/pjshumphreys/querycs ... uild-zx.js and uses node.js to assist in linking things together. Here's my memory map for each of the cpm/msxdos and the various zx spectrum variants (the pages used by my application's non clib code are shared between each of the disk systems BTW):

Code: Select all


cpm/msxdos

+---------------+
| 0x0000-0x00FF | cp/m data, command line text
+---------------+
| 0x0100-0x3FFF | paging code, cp/m or msx 2 file access routines, global variables, function jump table at bottom. extra heap memory at top
+---------------+
| 0x4000-0x7FFF | paged code, text constants (duplicated at the top of each memory page)
+---------------+
| 0x8000-0x???? | heap memory at bottom. stack memory at top
+---------------+
| 0x????-0xFFFF | cpm code
+---------------+

esxdos 48k

+---------------+
| 0x0000-0x1FFF | esxdos/basic rom
+---------------+
| 0x2000-0x3FFF | extra heap memory/basic rom
+---------------+
| 0x4000-0x5B00 | screen
+---------------+
| 0x5???-0x5??? | basic loader and argv[1] (basic variable moved 1 byte down to fit a null terminator)
+---------------+
| 0x5???-0xB??? | heap memory at bottom. stack memory at top
+---------------+
| 0xB???-0xBFFF | function jump table, page swapping code, atexit, interrupt mode 2 handler, global variables, rst 10 based fputc_cons
+---------------+
| 0xC000-0x???? | paged code
+---------------+
| 0x????-0xFFFF | text constants (duplicated in each memory page)
+---------------+


residos 48k

+---------------+
| 0x0000-0x3FFF | extra heap memory/basic rom
+---------------+
| 0x4000-0x5B00 | screen
+---------------+
| 0x5???-0x5??? | basic loader and argv[1] (basic variable moved 1 byte down to fit a null terminator)
+---------------+
| 0x5???-0xB??? | heap memory at bottom. stack memory at top
+---------------+
| 0xB???-0xBFFF | function jump table, page swapping code, atexit, interrupt mode 2 handler, global variables, rst 10 based fputc_cons
+---------------+
| 0xC000-0x???? | paged code
+---------------+
| 0x????-0xFFFF | text constants (duplicated in each memory page)
+---------------+


residos 128k

+---------------+
| 0x0000-0x3FFF | extra heap memory/basic rom
+---------------+
| 0x4000-0x5B00 | extra heap memory
+---------------+
| 0x5???-0x5??? | basic loader and argv[1] (basic variable moved 1 byte down to fit a null terminator)
+---------------+
| 0x5???-0xB??? | heap memory at bottom. stack memory at top
+---------------+
| 0xB???-0xBFFF | page swapping code, atexit, interrupt mode 2 handler, global variables
+---------------+
| 0xC000-0x???? | function jump table, paged code / disk buffer in page 6 / screen, fputc_cons that uses the alternate screen and rom font in page 7
+---------------+
| 0x????-0xFFFF | text constants (duplicated in each memory page)
+---------------+


esxdos 128k

+---------------+
| 0x0000-0x1FFF | esxdos/basic rom
+---------------+
| 0x2000-0x3FFF | extra heap memory/divmmc memory page cache/basic rom
+---------------+
| 0x4000-0x5B00 | extra heap memory
+---------------+
| 0x5???-0x5??? | basic loader and argv[1] (basic variable moved 1 byte down to fit a null terminator)
+---------------+
| 0x5???-0xB??? | heap memory at bottom. stack memory at top
+---------------+
| 0xB???-0xBFFF | function jump table, page swapping code, atexit, global variables
+---------------+
| 0xC000-0x???? | paged code / screen, fputc_cons that uses the alternate screen and rom font in page 7
+---------------+
| 0x????-0xFFFF | text constants (duplicated in each memory page)
+---------------+


plus3dos

+---------------+
| 0x0000-0x3FFF | basic rom
+---------------+
| 0x4000-0x5B00 | extra heap memory
+---------------+
| 0x5???-0x5??? | basic loader and argv[1] (basic variable moved 1 byte down to fit a null terminator)
+---------------+
| 0x5???-0xB??? | heap memory at bottom. stack memory at top
+---------------+
| 0xB???-0xBFFF | function jump table, page swapping code, atexit, global variables
+---------------+
| 0xC000-0x???? | paged code / disk buffer in page 6 / screen, fputc_cons and font in page 7
+---------------+
| 0x????-0xFFFF | text constants (duplicated in each memory page)
+---------------+
Last edited by pjshumphreys on Sat Feb 06, 2021 8:38 pm, edited 1 time in total.
User avatar
dom
Well known member
Posts: 2072
Joined: Sun Jul 15, 2007 10:01 pm

Re: How can my changes be re-integrated upstream?

Post by dom »

pjshumphreys wrote: Sat Feb 06, 2021 8:37 pm 1. Is O_APPEND meaningful when attempting to read a file?
I was jumping ahead to the writing code, and had a scanning misread on the way!
6. msxdos2.lib was being built, but I've had to keep manually placing it into z88dk/lib/clibs/ in order to link with it whenever I first build the toolchain. I don't know if all libraries should be ending up in there, but it seems all the other ones built get placed into that folder.
That's odd, the install is just a cp $(OUTPUT_DIRECTORY)/*.lib ../lib/clibs! At least it was being built, I can see it ends up in the right place for nightlies.
8. I'm on Fedora 31:

Code: Select all

$ uname -a
Linux localhost.localdomain 5.8.18-100.fc31.x86_64 #1 SMP Mon Nov 2 20:32:55 UTC 2020 x86_64 x86_64 x86_64 GNU/Linux
Okay, that's really strange - the snaps and ci builds cover loads of architectures, and Fedora 31/x86_64 isn't something obscure. I'll set up a local VM and see if I can reproduce.
9. yes I've seen some of the bank stack work that's been done recently. How big does this bank stack need to be? My project can use a great deal of heap space, and calling into other banks hasn't really been an issue for my trampoline approach up to now. I'd kinda prefer to not use a separate bank stack if the standard toolchain could be made to not need it somehow.
The temporary stack is just used to stuff some paging values - user code executes on the main stack, by default CLIB_BANKING_STACK_SIZE is set to 100 (bytes), on +zx each call uses 4 bytes of it (copy of BANKM + real return address), so the 100 bytes allows a call depth of 25 which (of course) is just an arbitrary number I chose.
In case you're interested in how I've got things set up, the build script for the zx spectrum variant of my project is at https://github.com/pjshumphreys/querycs ... uild-zx.js and uses node.js to assist in linking things together.
That does sound interesting, I'll take a look to see how you're doing things. It's nice to see the cross target code being really tested.
pjshumphreys
Member
Posts: 66
Joined: Sat Feb 06, 2021 2:32 pm

Re: How can my changes be re-integrated upstream?

Post by pjshumphreys »

Thanks for your help with this stuff by the way. It's very much appreciated :)
User avatar
dom
Well known member
Posts: 2072
Joined: Sun Jul 15, 2007 10:01 pm

Re: How can my changes be re-integrated upstream?

Post by dom »

dom wrote: Sat Feb 06, 2021 9:48 pmOkay, that's really strange - the snaps and ci builds cover loads of architectures, and Fedora 31/x86_64 isn't something obscure. I'll set up a local VM and see if I can reproduce.
Which I've now done (well an lxc container). Your fork is generating the same code as master (which makes sense given the tiny code delta). What that does mean is that your changes to mathops_ieee.opt should be reverted back to vanilla - I'm not sure where your changes to Far_Pointer_Call_ieee.opt have got to.

With a clean checkout of your tree do you get the same results as me?

I've figured out the msxdos2.lib issue and put in a temporary patch for it.
pjshumphreys
Member
Posts: 66
Joined: Sat Feb 06, 2021 2:32 pm

Re: How can my changes be re-integrated upstream?

Post by pjshumphreys »

I've just done

Code: Select all

git clone  --recursive https://github.com/z88dk/z88dk.git
cd z88dk
./build.sh
and the tests all pass. Doing the same on my fork gives an error when running the tests. I've obviously broken my fork somehow (probably during a rebase). Sorry for the trouble on that one.
Post Reply