Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

(z80asm) do not link with object files built for a different cpu #2320

Closed
pauloscustodio opened this issue Jun 20, 2023 · 52 comments
Closed
Assignees

Comments

@pauloscustodio
Copy link
Member

No description provided.

@suborb
Copy link
Member

suborb commented Jun 24, 2023

It's not a simple problem - I think we probably want compatibility to parents on the tree (so we can link an 8080 library for the ez80_z80 for example). cbundoc is where we consider +z80 to be now

Untitled Diagram

@pauloscustodio
Copy link
Member Author

Yes, that's true. We need also to take into account the -IXIY flag.

@pauloscustodio
Copy link
Member Author

Feature is implemented, checking for regressions.

Defined behaviour:

  • object file for a different incompatible CPU (according to diagram above) or with different -IXIY option causes an error
  • object file for a different incompatible CPU or with different -IXIY option inside a library is just skipped in the library search, causing undefined symbols further on.

This allows, in theory, for a library to contain the same object file assembled for different CPUs and -IXIY options. For that we need to make the -x option update the library instead of re-creating it from scratch. But that change may break Makefile rules that depend on the timestamp of the library file, e.g. update an asm file, update it in the library for one CPU; now the library is up-to-date, but it may have inside other object files for the same asm for different CPUs out of date.

What do you think?

@suborb
Copy link
Member

suborb commented Jul 31, 2023

--- Building ixiy crt Library ---
[4181](https://github.com/z88dk/z88dk/actions/runs/5708483393/job/15466196318#step:8:4182)

[4182](https://github.com/z88dk/z88dk/actions/runs/5708483393/job/15466196318#step:8:4183)
make --no-print-directory -C z80_crt0s
[4183](https://github.com/z88dk/z88dk/actions/runs/5708483393/job/15466196318#step:8:4184)
make[1]: Nothing to be done for 'all'.
[4184](https://github.com/z88dk/z88dk/actions/runs/5708483393/job/15466196318#step:8:4185)
make --no-print-directory -C math/fix16
[4185](https://github.com/z88dk/z88dk/actions/runs/5708483393/job/15466196318#step:8:4186)
TYPE=ixiy z88dk-z80asm -d -I/home/runner/work/z88dk/z88dk/lib/config//../ -mz80 -IXIY -DSTANDARDESCAPECHARS -x/home/runner/work/z88dk/z88dk/libsrc//z80iy_crt0 @/home/runner/work/z88dk/z88dk/libsrc//../libsrc//z80.lst
[4186](https://github.com/z88dk/z88dk/actions/runs/5708483393/job/15466196318#step:8:4187)
font/font_4x8/font_4x8.lst:1: error: -IXIY incompatible: file font/font_4x8/obj/z80/x/home/runner/work/z88dk/z88dk/libsrc/_DEVELOPMENT/font/font_4x8/_font_4x8_64_minix.o compiled for IX=IX, incompatible with IX=IY
[4187](https://github.com/z88dk/z88dk/actions/runs/5708483393/job/15466196318#step:8:4188)
  ^---- font/font_4x8/obj/**/*.o
[4188](https://github.com/z88dk/z88dk/actions/runs/5708483393/job/15466196318#step:8:4189)
make: *** [Makefile:2085: z80iy_crt0.lib] Error 1

That is annoying since they're just data - I'll update that branch to build those files for multiple CPUs. However I'm surprised it's failing at library creation time since "object file for a different incompatible CPU or with different -IXIY option inside a library is just skipped in the library search, causing undefined symbols further on.". However it might solve the question you raised!

@pauloscustodio
Copy link
Member Author

However I'm surprised it's failing at library creation time

Yes, at library creation time the compilation options used are compared with the object files, and they must match. It is the same as if building a binary.

The skipping is when linking in libraries.

@suborb
Copy link
Member

suborb commented Aug 1, 2023

I’m working through the libsrc errors - I’ve found quite a few.

Most issues so far have been caused by ixiy in the case that the index registers aren’t used.

@pauloscustodio
Copy link
Member Author

Ok, thanks.

@suborb
Copy link
Member

suborb commented Aug 1, 2023

The libraries should now build, but I need to create a dedicated z180_crt0.lib.

-mz80 is at "cbundoc" in the diagram, even though crt0 code is at "Documented"

@pauloscustodio
Copy link
Member Author

Does it make sense to create a new cpu z80_strict with just the documented opcodes?

@suborb
Copy link
Member

suborb commented Aug 1, 2023

Yes, that would be wonderful!

pauloscustodio added a commit that referenced this issue Aug 2, 2023
The library z88dk-z80asm.lib replaces the old
z88dk-z80asm-<cpu>-<ixiy>.lib
@pauloscustodio
Copy link
Member Author

Committed to the issue_2320 branch the new CPU z80_strict, and the usage of one single z88dk-z80asm.lib support library instead of one per cpu-ixiy combination.

Github workflow is failing for Ubuntu:

zcc +zx -vn ../graphics/spirograph.c -lndos --math32 -create-app -ospirograph.bin
../graphics/spirograph.c::x::0::0:25: error: undefined symbol: l_f32_sub
  ^---- l_f32_sub
../graphics/spirograph.c::x::0::0:25: error: undefined symbol: cos_fastcall
  ^---- cos_fastcall
../graphics/spirograph.c::x::0::0:25: error: undefined symbol: l_f32_mul
  ^---- l_f32_mul

@suborb
Copy link
Member

suborb commented Aug 2, 2023

That one is an easy one to solve - bad naming of the z80n library.

Just want to check one thing (relevant for the maths libraries). If I compile to .o the files needed for the various CPUs and then create a library with -m* it will include all files into a single library file?

If that library is created from the following tree:

obj/z80
obj/z80n
obj/z180
obj/ez80_z80

With a glob wildcard obj/**/*.o will z80asm search for the best CPU match rather than the first CPU match? Eg suppose the z80 files were first in the library and I was linking for ez80_z80 would it preferentially use ez80_z80 then z180 then z80 object files?

Related question - if it does do this, is there an effect on linking time? (In the past we had very slow linking on windows for example).

@pauloscustodio
Copy link
Member Author

With a glob wildcard obj/**/*.o will z80asm search for the best CPU match rather than the first CPU match?

Yes, it will get the best CPU match, because the object files are written to the library file from more specific to more generic CPU order. The actual order is fixed in the code: z80n, z80, ez80_z80, z180, z80_strict, 8085, r3k, gbz80, ez80, r2ka and 8080

is there an effect on linking time?

Yes, there will be some impact, as more object files are skipped while searching for symbols. But as the whole library is slurped into memory, the search is memory-only and should be fast. (this slurping into memory was the solution to the windows slow liking times)

To lessen this problem I am currently working on adding a string table to the end of each object file, removing repetitions (e.g. file name) and making the object file smaller. I am also considering adding a hash code to each symbol to make the lookup faster. This is all happening in the issue_2320 branch, so version 18 of the object files is not stable. (the master branch has version 17)

@suborb
Copy link
Member

suborb commented Aug 2, 2023

Okay, that's cool - I'll create fat maths libraries once the branch is merged.

Branch is failing on z80asm tests now - so I’ll hand it back to you @pauloscustodio

@pauloscustodio
Copy link
Member Author

Fixed the problems with the tests, now we are getting some undefined library symbols (error_zc, _font_8x8_rom, l_ret)...
back to you, @suborb, I think

@suborb
Copy link
Member

suborb commented Aug 4, 2023

This is going to hurt. It's a problem with sdcc_iy where the library is compiled with -IXIY but user code isn't.

I can't remember what sdcc_iy is meant to be, sdcc uses ix as the frame register, is the library intended to use iy internally to avoid having to save/restore ix? Help @feilipu ?

@feilipu
Copy link
Collaborator

feilipu commented Aug 4, 2023

AFAIK, the sdcc_iy library does just that. It uses iy internally. So that ix is used solely by the compiler.

But, I've entirely avoided the index registers in my work so can't give examples.

@suborb
Copy link
Member

suborb commented Aug 4, 2023

Thanks for the confirmation. In which case it's an outlier case that I'm not certain how to fix without a lot of pain.

To solve it we need to introduce a new z80asm define INDEX=IX/IY, remove the assembly with -IXIY and then replace all uses of the index registers with that define. Taking special care around __SDCC_IY which deliberately reverses the semantics so -IXIY gets the right index register for ROM calls.

dom@dom-macmini _DEVELOPMENT % find . -type f -name '*.asm' |xargs grep ix | wc
   11766   52660  926212
dom@dom-macmini _DEVELOPMENT % find . -type f -name '*.asm' |xargs grep iy | wc
    2899   12705  234727

Any alternate ideas very welcome!

@pauloscustodio
Copy link
Member Author

pauloscustodio commented Aug 4, 2023 via email

@suborb
Copy link
Member

suborb commented Aug 4, 2023

I think what's actually needed is a "soft -IXIY" flag - that does the reversal but doesn't affect the output CPU in the object file. Changing and checking almost 15000 ixy uses is going to lead to bugs.

@pauloscustodio
Copy link
Member Author

I think what's actually needed is a "soft -IXIY" flag

Agree, swap but write object file as not swapped. Any suggestion on the name? -IXIY-soft?

@suborb
Copy link
Member

suborb commented Aug 8, 2023

Interesting, the l_ functions are from z80_crt0.lib - not sure how it's ended up with the wrong CPU since it was working beforehand.

Ah, I have z80_crt0.lib compiling with -mz80_strict - maybe that's it

@suborb
Copy link
Member

suborb commented Aug 8, 2023

Ah, I have z80_crt0.lib compiling with -mz80_strict - maybe that's it

That is the problem: z80_crt0.lib is partially assembled with -mz80 but the l_ routines are assembled with -mz80_strict.

When creating the library, we use -mz80 and it seems that there's been a change which causes the -mz80_strict modules to be excluded from the library.

So, it looks like the library -m* changed broke something?

@pauloscustodio
Copy link
Member Author

I solved a merge conflct between zcc -mz80 -mz80_strict. Maybe i selected the wrong line?

@suborb
Copy link
Member

suborb commented Aug 8, 2023

I solved a merge conflct between zcc -mz80 -mz80_strict. Maybe i selected the wrong line?

The bit of the library building is all done by invoking z80asm. Simple example:

a.asm

SECTION code2

a:
        ld      hl,16384

b.asm:

MODULE b
SECTION code


b:
        ld      hl,32768

Create library:

z88dk-z80asm -mz80 a.asm
z88dk-z80asm -mz80_strict b.asm
z88dk-z80asm -xlib -mz80  *.o
z88dk-z80nm lib.lib
Library file lib.lib at $0000: Z80LMF18
Object  file lib.lib at $0014: Z80RMF18
  Name: a
  CPU:  z80
  Section "": 0 bytes
  Section code2: 3 bytes
  Symbols:

%

@suborb
Copy link
Member

suborb commented Aug 8, 2023

I'm guessing it previously worked as a side-effect, so I'll change it back to z80. However, a log would be useful!

@suborb
Copy link
Member

suborb commented Aug 8, 2023

Fixed -m* to allow your use case.

I've run into a different issue now:

genmath.lst:3: error: file open: obj/ixiy/c/sccz80/sgn.o
  ^---- obj/ixiy/**/*.o
obj/ixiy/c/sccz80/sgn.o: Too many open files
libc++abi: terminating due to uncaught exception of type std::__1::__fs::filesystem::filesystem_error: filesystem error: in directory_iterator::directory_iterator(...): Too many open files ["obj/ez80_z80"]

% find obj -name '*.o' | wc
     359     359    9002
% ulimit -a
-t: cpu time (seconds)              unlimited
-f: file size (blocks)              unlimited
-d: data seg size (kbytes)          unlimited
-s: stack size (kbytes)             8176
-c: core file size (blocks)         0
-v: address space (kbytes)          unlimited
-l: locked-in-memory size (kbytes)  unlimited
-u: processes                       1333
-n: file descriptors                256

genmath.lst:

obj/z80/**/*.o
obj/z80n/**/*.o
obj/ixiy/**/*.o
obj/ez80_z80/**/*.o

Normally the libraries can have 1000s of files - is -m* keeping files open for some reason?

@suborb
Copy link
Member

suborb commented Aug 8, 2023

Another problem, this time with libsrc/fcntl/dummy which builds ndos.lib. It's built with -m*.

Rebuilding the library (with .o files alongside .asm files) results in only 8080 (IXIY) ending up in the final library. Doing a make clean builds it correctly.

This is a regression on the behaviour from a couple of days ago.

@suborb
Copy link
Member

suborb commented Aug 8, 2023

I'm guessing it previously worked as a side-effect, so I'll change it back to z80. However, a log would be useful!

Actually, this is turning out to be a bigger issue, eg the m100 is an 8085 machine, but m100_clib.lib previously used to contain a mix of 8080 and 8085 code (rightly or wrongly), with the recent change it just contains 8085 routines.

This also affects +zxn which is mostly z80 code, and only smattering of z80n

@pauloscustodio
Copy link
Member Author

Let me check each of these in turn...

pauloscustodio added a commit that referenced this issue Aug 9, 2023
Without -m* z80asm now accepts all the passed object files and adds them
to the library in the order given.

With -m* z80asm selects the object files for each CPU-IXIY combination
in turn.

The -v option now tells if an object file is skipped.
@pauloscustodio
Copy link
Member Author

pauloscustodio commented Aug 9, 2023

Interesting, the l_ functions are from z80_crt0.lib - not sure how it's ended up with the wrong CPU since it was working beforehand.

Ah, I have z80_crt0.lib compiling with -mz80_strict - maybe that's it

Was a bug in building libraries; now -x without -m* accepts all objects given as input and packs them together.

Correction: -x without -m* accepts all objects given as input for compatible CPU and packs them together.

@pauloscustodio
Copy link
Member Author

Fixed -m* to allow your use case.

I've run into a different issue now:

genmath.lst:3: error: file open: obj/ixiy/c/sccz80/sgn.o
  ^---- obj/ixiy/**/*.o
obj/ixiy/c/sccz80/sgn.o: Too many open files
libc++abi: terminating due to uncaught exception of type std::__1::__fs::filesystem::filesystem_error: filesystem error: in directory_iterator::directory_iterator(...): Too many open files ["obj/ez80_z80"]

% find obj -name '*.o' | wc
     359     359    9002
% ulimit -a
-t: cpu time (seconds)              unlimited
-f: file size (blocks)              unlimited
-d: data seg size (kbytes)          unlimited
-s: stack size (kbytes)             8176
-c: core file size (blocks)         0
-v: address space (kbytes)          unlimited
-l: locked-in-memory size (kbytes)  unlimited
-u: processes                       1333
-n: file descriptors                256

genmath.lst:

obj/z80/**/*.o
obj/z80n/**/*.o
obj/ixiy/**/*.o
obj/ez80_z80/**/*.o

Normally the libraries can have 1000s of files - is -m* keeping files open for some reason?

Filehandle leak while checking for the correct object file version and bailing out without closing file. fixed.

@pauloscustodio
Copy link
Member Author

I solved a merge conflct between zcc -mz80 -mz80_strict. Maybe i selected the wrong line?

The bit of the library building is all done by invoking z80asm. Simple example:

a.asm

SECTION code2

a:
        ld      hl,16384

b.asm:

MODULE b
SECTION code


b:
        ld      hl,32768

Create library:

z88dk-z80asm -mz80 a.asm
z88dk-z80asm -mz80_strict b.asm
z88dk-z80asm -xlib -mz80  *.o
z88dk-z80nm lib.lib
Library file lib.lib at $0000: Z80LMF18
Object  file lib.lib at $0014: Z80RMF18
  Name: a
  CPU:  z80
  Section "": 0 bytes
  Section code2: 3 bytes
  Symbols:

%

Fixed.

@pauloscustodio
Copy link
Member Author

It's getting late... tomorrow evening I'll have a go with the next issues.

pauloscustodio added a commit that referenced this issue Aug 9, 2023
@pauloscustodio
Copy link
Member Author

Another problem, this time with libsrc/fcntl/dummy which builds ndos.lib. It's built with -m*.

Rebuilding the library (with .o files alongside .asm files) results in only 8080 (IXIY) ending up in the final library. Doing a make clean builds it correctly.

This is a regression on the behaviour from a couple of days ago.

The current behaviour is the following:

  • z80asm is called with -m* and a bunch of *.o files
  • before the library is built, during the assembly phase, z80asm checks if all the *.o files are for the same CPU-IXIY; if not and if a .asm file is available, it re-assembles that .asm file to get the "correct" CPU-IXIY
  • when the library is built, all object files are now of the same type.

This is clearly not what is wanted, and is a consequence of z80asm trying to be clever in selecting either an asm file or an object file depending on what exists and on the -d flag. I think that it should only handle the files passed to it and leave the cleverness to the Makefiles. But these checks are there since before my time maintaining it and I did not want to break stuff.

@suborb, I would like to hear your view.

pauloscustodio added a commit that referenced this issue Aug 9, 2023
When given a list of object files for incompatible CPUs, if the .asm files exist, z80asm assembles
them first and then creates the library, with all the object files for the same CPU

Behaviour is not correct, need to decide what use cases make sense.
@pauloscustodio
Copy link
Member Author

pauloscustodio commented Aug 9, 2023

The tests are now passing, except for the msbuild ones that depend on the nightly built, that still has the wrong libraries.

Did you manage to create the fat libraries? Shall we wait for that before merging?

@suborb
Copy link
Member

suborb commented Aug 10, 2023

I took a night off, so I've not done the fat library work - I can do it later.

Regarding -d - I think for m* date checking ought not to be done.

Given that the check is only for date and not for CPU type then it's actually quite "dangerous" for z80asm to have this behaviour so it probably ought to be at least disabled by default if not removed.

pauloscustodio added a commit that referenced this issue Aug 10, 2023
When user supplies a *.o, read object file, issue error if not found or not compatible CPU.
(was searching for a .asm file if .o was not found)

When user supplies a *.asm file, always assemble.
(was using an existing .o file if up-to-date)

When user supplies a basename, try first basename.asm and then basename.o.

With -d, the user may supply either the .asm or .o, and z80asm will use the .o if up-to-date. If the
.o is for a different CPU, an error message is output.

With -m*, -d is not allowed.
@pauloscustodio
Copy link
Member Author

Cleanup of command line parsing done: -d and -m* together are now forbidden, and the assembler always uses either the .o or the .asm files supplied to it; -d is the only exception where either a .o or .asm may be supplied and z80asm selects the .o if up-to-date, or else the .asm.

Now back to you, @suborb:

TYPE=z80 z88dk-z80asm -d -I/Users/runner/work/z88dk/z88dk/lib/config//../ -mz80 -DSTANDARDESCAPECHARS -x/Users/runner/work/z88dk/z88dk/libsrc//z80_crt0 @/Users/runner/work/z88dk/z88dk/libsrc//../libsrc//z80.lst
/Users/runner/work/z88dk/z88dk/libsrc//../libsrc//z80.lst:13: error: CPU incompatible: file stdio/ansi/ansifont_f3.o compiled for z80n, incompatible with z80

@suborb
Copy link
Member

suborb commented Aug 10, 2023

Thanks @pauloscustodio - There's a few cases of this so may take me a while to get through them all.

@suborb
Copy link
Member

suborb commented Aug 11, 2023

It was fairly painful, but all done - that diagnostic/error was just what was needed - the incorrect files fell into a few camps:

  • Indirection files (so defc func = alternate_func)
  • Data files
  • IXIY swapped files that didn't use the index registers
  • Some of the ansi engine files were referenced by source path rather than the already generated .o files

So I'd say there was no actual negative effect of these mistakes.

@pauloscustodio
Copy link
Member Author

Thanks @suborb. The tests are all passing, except for the msbuild ones that need the libraries from the nightly run, so I think we are ready to merge.

Do you want to create a fat library before the merge to check the impact?

@suborb
Copy link
Member

suborb commented Aug 11, 2023

I‘m happy to leave fat libraries until later - there’s enough churn and potential breakage in this feature as is!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants