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

TARGET_ARCH = $(GCC_MTUNE)=i586 does not actually produce a backwards compatible allegro.a #5

Open
ArtificialRaccoon opened this issue Oct 5, 2022 · 6 comments

Comments

@ArtificialRaccoon
Copy link

While -mtune=i586 should produce a binary compatible with i586 and earlier CPU's, this does not appear to be the case with the Allegro library. This resulted in a "gotcha" scenario for me when I moved from testing on DOSBOX to real hardware (and better VMs such as 86Box and VirtualBox), where anything built with this build of the Allegro library would immediately segfault.

Setting TARGET_ARCH=i386 in makefile.dj solves the segfault issue, and an executable built with this Allegro library work on real hardware and virtual machines other than DOSBOX.

@manxorist
Copy link

manxorist commented Oct 5, 2022

When not setting -march=something, GCC will default to a -march value set when building GCC itself. Conventionally, if you are invoking a GCC via i586-pc-msdosdjgpp-gcc this is probably i586 for your GCC. So this may be what is happening by default for you.

It is not actually clear from your report whether you did set -march=i386 and/or -mtune=i386 to fix the apparent problem.
Setting only -mtune=i386 may maybe appear to work by accident, but GCC may still generate i586 instructions due to its internal defaults. You need to additionally set -march=i386 in order to make it explicitly avoid any non-386 instructions.

Disclaimer:
I am not maintaining this particular allegro fork, but I am also using it (or rather my own fork of the fork) for openmpt123 and other projects.

@manxorist
Copy link

As for the actual issue, I agree that the current default at

TARGET_ARCH = $(GCC_MTUNE)=i586
should likely better be a more explicit TARGET_ARCH = -march=i386 -m80387 -mtune=pentium instead of TARGET_ARCH = $(GCC_MTUNE)=i586.

@ArtificialRaccoon
Copy link
Author

@manxorist Thank you for the heads up about -mtune and GCC; this was very helpful to know/learn. :)

@msikma
Copy link
Owner

msikma commented Oct 19, 2022

As for the actual issue, I agree that the current default at

TARGET_ARCH = $(GCC_MTUNE)=i586

should likely better be a more explicit TARGET_ARCH = -march=i386 -m80387 -mtune=pentium instead of TARGET_ARCH = $(GCC_MTUNE)=i586.

Do you think it would be a good idea to change this?

To be completely honest, I don't actually have that much experience with C compiler settings. I did the minimum needed to get it cross compiling on my setup. But if this is a more useful default it might be nice to change it.

@manxorist
Copy link

As for the actual issue, I agree that the current default at

TARGET_ARCH = $(GCC_MTUNE)=i586

should likely better be a more explicit TARGET_ARCH = -march=i386 -m80387 -mtune=pentium instead of TARGET_ARCH = $(GCC_MTUNE)=i586.

Do you think it would be a good idea to change this?

Yes.
Given that that aspect of the Makefile was added over 22 years ago, it was probably assumed that -march=i386 would be the default, which does not hold any more for all compiler builds, It should be
TARGET_ARCH = -march=i386 -m80387 $(GCC_MTUNE)=pentium
to still be compatible with -mcpu= instead of mtune= for very old GCC.
I do not know when exactly -m80387 was added in GCC though. It's documented for GCC 7, but GCC does not seem to error with it even down to 4.1 (I have not tested any earlier).

To be completely honest, I don't actually have that much experience with C compiler settings. I did the minimum needed to get it cross compiling on my setup.

Yeah, uhm, there are some people keeping allegro 4.2 forks around, each with some the same and some other set of small patches, I do so also myself, but this is not public on github at the moment. It maybe would make some sense to consolidate these efforts of various people in some way or another.

@msikma
Copy link
Owner

msikma commented Nov 2, 2022

Thanks! I'm currently traveling but when I'm back I'll look into this.

Although initially I just put this repo up for my own use, given that other people have found it and commented on it, I'm happy to make some more adjustments to make it more useful. To me this was always an experiment and I'm not very well versed in C let alone the intricacies of how to properly set up a compiler for an older system. So I appreciate the help.

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

No branches or pull requests

3 participants