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

on i386 libSDL.so contains non PIC code #293

Closed
SDLBugzilla opened this issue Feb 10, 2021 · 0 comments
Closed

on i386 libSDL.so contains non PIC code #293

SDLBugzilla opened this issue Feb 10, 2021 · 0 comments

Comments

@SDLBugzilla
Copy link
Collaborator

This bug report was migrated from our old Bugzilla tracker.

These attachments are available in the static archive:

Reported in version: HG 1.2
Reported for operating system, platform: Linux, x86

Comments on the original bug report:

On 2007-03-19 13:55:51 +0000, Hans de Goede wrote:

The asm code used on i386 is non PIC, leading to so called text relocations, for more info on why this is bad see:
http://thread.gmane.org/gmane.linux.gentoo.devel/33992
http://article.gmane.org/gmane.linux.gentoo.devel/34037
http://people.redhat.com/drepper/nonselsec.pdf

I've merged a couple of patches from gentoo into one patch for inclusion into the Fedora version of SDL, but the Fedora maintainer would rather see this solved upstream, so here I am. See
https://bugzilla.redhat.com/bugzilla/show_bug.cgi?id=179407

For the Fedora bug report including an attached patch.

On 2007-03-19 15:19:55 +0000, Ryan C. Gordon wrote:

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 ***

On 2007-03-19 15:29:14 +0000, Ryan C. Gordon wrote:

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.

On 2007-03-20 00:54:30 +0000, Hans de Goede wrote:

Created attachment 201
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.

On 2007-06-02 13:59:01 +0000, Ryan C. Gordon wrote:

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

--ryan.

On 2007-06-15 00:36:02 +0000, Ryan C. Gordon wrote:

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

On 2007-06-15 00:39:52 +0000, Ryan C. Gordon wrote:

Created attachment 213
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.

On 2007-06-16 23:29:36 +0000, Hans de Goede wrote:

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

On 2007-06-17 04:39:47 +0000, Ryan C. Gordon wrote:

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.

On 2007-06-17 12:41:03 +0000, Hans de Goede wrote:

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.

On 2007-06-18 00:37:53 +0000, Ryan C. Gordon wrote:

(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.

On 2007-06-27 03:17:58 +0000, Ryan C. Gordon wrote:

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.

On 2007-06-27 03:20:19 +0000, Ryan C. Gordon wrote:

Created attachment 215
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.

On 2007-07-05 01:08:48 +0000, Ryan C. Gordon wrote:

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.

On 2007-07-05 22:18:16 +0000, Mike Frysinger wrote:

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 :(

On 2007-07-05 23:23:07 +0000, Sam Lantinga wrote:

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.

On 2007-07-06 00:06:20 +0000, Mike Frysinger wrote:

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

On 2007-07-06 02:14:38 +0000, Ryan C. Gordon wrote:

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, so it can use the C fallbacks for all I care (sorry, BeOS).

--ryan.

On 2007-07-06 02:30:15 +0000, Sam Lantinga wrote:

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?

On 2007-07-06 02:33:27 +0000, Sam Lantinga wrote:

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. :)

On 2007-07-06 02:34:35 +0000, Sam Lantinga wrote:

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

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

On 2007-07-08 11:09:12 +0000, Alex Volkov wrote:

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.

On 2007-07-10 23:26:46 +0000, Ryan C. Gordon wrote:

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 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.

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

1 participant