We are currently migrating Bugzilla to GitHub issues.
Any changes made to the bug tracker now will be lost, so please do not post new bugs or make changes to them.
When we're done, all bug URLs will redirect to their equivalent location on the new bug tracker.

These patches went into SDL revision control around December of 2005 (see Bug #17), and are in the 1.2.10 release and later.

If Fedora is still shipping 1.2.9, it's really really time to update the package, as 1.2.11 has the same API/ABI but has literally years of bugfixes over 1.2.9.

(But as they are in our Subversion repository and won't need to be further maintained as separate patches by Fedora, it's probably safe to just apply the Gentoo TEXTREL patches to the 1.2.9 sources if bumping to a newer SDL package isn't feasible.)

I'll post this info at the Fedora bugzilla, too.

--ryan.



*** This bug has been marked as a duplicate of bug 17 ***

Oh, wait, the Red Hat bug says there are patches we never imported. Hans, can you verify and post just the missed pieces here?

Thanks,
--ryan.

Created attachment 201 [details]
PATCH: fix the remaining non PIC asm code in SDL

Correct, Fedora has SDL-1.2.10 which indeed contains a few fixes for non PIC-asm, compared to 1.2.9, but which also still contains some non PIC asm. This patch fixes this.

This patch is a combination of:
http://mir2.ovh.net/gentoo-portage/media-libs/libsdl/files/libsdl-1.2.9-PIC-hermes-call-dont-jump.patch

and:
http://mir2.ovh.net/gentoo-portage/media-libs/libsdl/files/libsdl-1.2.9-PIC-yuv-mmx.patch


In combination with removing the unused CPU-detect code.

Notice that:
http://mir2.ovh.net/gentoo-portage/media-libs/libsdl/files/libsdl-1.2.9-PIC-SDL_stretch.patch

Is not applied as it already was applied in 1.2.10

I've confirmed that with this patch all textrel's are gone (according to
eu-findtextrel). I've done some testing with this and all SDL apps still seem
to work fine.

Bumping a bunch of bugs to Priority 1 for consideration for the 1.2.12 release.

--ryan.

*** Bug 127 has been marked as a duplicate of this bug. ***

Created attachment 213 [details]
Updated patch for svn and gcc2...


Here's an updated patch...this one applies cleanly to the latest Subversion, and disables the assembly on gcc2 (since it needs more registers than gcc2 inline assembly will give you).

Testing and comments for this patch are welcome. I'd like it to go into the soon-to-be-ready 1.2.12 release.

--ryan.

As far as my assembler skills go, the patch looks good.

Okay, the good news is that this patch definitely fixes the remaining TEXTRELs.

But...I just went back to verify this and found that we would never hit the MMX path in SDL12/test/testoverlay.c, since the image's width has to be a multiple of 16, or it will use the C fallback code.

The sample.bmp file is not, but you can rescale it slightly in The Gimp or whatnot to 416 pixels wide.

Then you can see with the patch that the MMX codepath is broken.

   ./testoverlay -format YV12 -bpp 16
   ./testoverlay -format YV12 -bpp 32

If I reenable the original MMX code (which is currently disabled in svn since it won't compile with gcc2) by just toggling the #if 0 sections without applying this patch, the MMX code definitely runs and functions.

So this patch needs further work, I'm afraid.  :/

--ryan.

Hmm,

Perhaps it is best then to leave the mmx asm unchanged and disabled as it currently is, and only apply the rest of the patch, then we atleast get a textrel free SDL and no regressions compared to the current SVN.

(In reply to comment #9)
> Perhaps it is best then to leave the mmx asm unchanged and disabled as it
> currently is, and only apply the rest of the patch, then we atleast get a
> textrel free SDL and no regressions compared to the current SVN.

We'll do that if we get down to the end of 1.2.12 and no one's fixed it, and consider the rendering issues a separate bug for 1.2.13.

--ryan.

I've now committed the Hermes changes in svn revision #3107 (#3108 for the 1.3 branch), so the PIC issues should be gone for certain in 1.2.12, but I'm leaving this bug open until I have time to figure out what in the inline asm changes is broken.

To be clear, the existing MMX code works, when enabled, with the changes I just committed, so the bug is definitely in the inline asm that patch changes...the Hermes parts are good either way, so that went into Subversion.

--ryan.

Created attachment 215 [details]
Updated patch with just inline asm


Here's an updated version of the patch against the latest in Subversion that only deals with the (broken!) inline asm. All I did was remove the Hermes patches that are now in Subversion so they don't cause complaints from the patch program.

--ryan.

Okay, I looked into this.

The reason this doesn't work is that we're explicitly using %%ebx without clobbering it, but gcc uses %%ebx as the PIC register, so it replaces some of our "m" variables with references to %%ebx, which it expects to be intact:

   pand   0x550(%ebx),%mm4


If you try to list ebx in the clobber list, GCC complains, because it needs it, and if you try to put (cr) into a "r" variable, GCC complains that it's using every register it's got available.

So, something has to be consolidated into a single register or something.

--ryan.

ugh, i wish i had seen this earlier ...

please revert those hermes changes ... i already got those fixed properly by using ELF hidden symbols.  Fedora needs to update and use the latest yasm or add the patch i posted to the nasm list to their nasm.

as for the inline stuff, i hate it because of the requirement to work with older limited gcc-2 and so cant really help there :(

Ryan, can you look into reverting those patches?  Mike, what's wrong with them?

As for gcc 2, I'm fine with disabling the MMX code for gcc 2 and enabling it for newer compilers.

well, there were two ways to solve the hermes PIC issue ...

the first was to keep everything as global ELF symbols which means having to go through the PLT due to relocations which means doing calls ... this is what i proposed first in the patch mentioned earlier

the second was to make everything hidden ELF symbols which means you can jump directly to the symbol ... this is what i proposed later and was merged into 1.2.11 (if not earlier)

the speed advantages (while argubably somewhat minor), are obvious: you cull symbols from the global ELF table which shouldnt be exported in the first place (thus reducing symbol lookup overhead), and you can do a direct jump rather than having to do an indirect call/ret (thus less stack manipulation)

iirc, if you make it so the inline assembler requires gcc-3+ (and go the C route for gcc-2), it should make things nicer as gcc-2 had restrictions on number of operands you could pass in

I'm okay with whatever, so long as the textrel issues remain resolved and the inline assembly is usable in recent gcc versions...gcc2 is already #ifdef'd out in Attachment #215 [details], so it can use the C fallbacks for all I care (sorry, BeOS).

--ryan.

Can we use call if we're building with old nasm, and code that was in svn if we're using yasm or nasm with the hidden symbol support?

What needs to happen to make the MMX code work with newer gcc?

If the latest version of nasm supports the hidden symbol stuff out of the box, I'm even okay with disabling the hermes code if we're not using nasm or a newer version of nasm.

We should of course document that in the patch notes. :)

Argh, I hate that you can't edit comments. :)

That last should have been "not using yasm or a newer version of nasm"

As per Sam's request on the list, I had a look at the SDL_yuv_mmx.c code, and IMHO that code really should be rewritten to let gcc manage the loop variables. Only the actual MMX operations should be in asm, as gcc can allocate registers to store the most critical loop counters where needed and store pointers in memory when necessary. The resulting code will not be gcc2 compatible, because of gcc2's lack of needed operand constraints.

I could take a whack at this, but I have no idea what that code actually does.

Okay, I cleaned this up for now...I just shuffle data between the stack and %%ebx once per block of data to avoid the conflict. This gets us working MMX inline asm code with no significant slowdown, no textrels, no requirement to upgrade nasm/yasm, and no stomping of the PIC register.

An fixed version of Attachment #215 [details] became svn revision #3212 in the 1.2 branch.

Better attempts are definitely welcome, but this seems like the least invasive fix for the 1.2 branch. Mike, if you want to replace this with something cleaner for the 1.3 branch, I certainly welcome it.

Hans, if you could build the latest in Subversion and make sure the textrels are still gone, I'd appreciate that, too.

--ryan.