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

x86: update x86inc #456

Merged
merged 1 commit into from
Feb 22, 2021
Merged

x86: update x86inc #456

merged 1 commit into from
Feb 22, 2021

Conversation

CoffeeFlux
Copy link
Contributor

This should fix the warning introduced with nasm 2.15

@TheOneric
Copy link
Member

It seems like we'll also need to update our ASFLAGS along with this update; looking at the macros we set:

  • HAVE_ALIGNED_STACK was removed, instead STACK_ALIGNMENT can be used where necessary I guess
  • PIC is now handled differently (can we just always set it now that it is force-disabled for win32?)
  • HAVE_CPUNOP was removed

@CoffeeFlux
Copy link
Contributor Author

HAVE_ALIGNED_STACK checks have been replaced with STACK_ALIGNMENT == 16.

PIC is no longer defined in the configure scripts, since it seems like the existing behavior will just be preserved with the new x86inc internal logic. If we want to always set it I can do that instead, but I don't have context on how this impacts 32-bit linux so it's not my call to make.

HAVE_CUPNOP is no longer defined.

@TheOneric
Copy link
Member

PIC is no longer defined in the configure scripts, since it seems like the existing behavior will just be preserved with the new x86inc internal logic. If we want to always set it I can do that instead, but I don't have context on how this impacts 32-bit linux

Afaik it should be fine on all supported i386 Linux kernels and to my knowledge non-PIC code being included can break ASLR and potentially cause issues in shared libs. Not being PIC was also the reason ASM got disabled in Debian's i386-builds of libass.

However I'm not completely sure that always enabling PIC won't cause problems on some other non-Windows system and I realised it is possible for packagers to pass ASFLAGS=-DPIC to configure to enable it if it is preferred.
Given that this better matches the current behaviour, it's probably safer to leave the decision about PIC on i386 to x86inc's defaults and user ASFLAGS instead of always enabling it, as I previously hinted at. So I think this should be fine.


By changing configure.ac this pr now conflicts with #452. As #452 basically touches almost everything in configure.ac, I'd prefer to merge #452 before this one, as merging it the other way around would result in all of #452's commits needing to be adjusted, meaning new errors could slip in and I'd have to reverify it on all 6 systems.
Merging #452 first on the other hand only requires a few lines of this patch to be adjusted.

configure.ac Outdated Show resolved Hide resolved
@rcombs
Copy link
Member

rcombs commented Jan 9, 2021

The reason lots of code doesn't support PIC on i386 (including ffmpeg, which debian seems to have no issue continuing to package with assembly enabled) is that there's no IP-relative addressing mode, so doing data loads ends up being really complex and expensive (it requires use of at least one extra register, and i386 is already pretty starved for those). I don't see much reason to try to port stuff to PIC-i386 at this point tbh; it's not like there are a whole lot of people clamoring for it when just about everybody not on windows has moved to x86_64.

@CoffeeFlux
Copy link
Contributor Author

I'm sympathetic to your reasons for merging #452 first, particularly given I've had the meson PR sitting on the vine for a couple years now, but that's a rather large PR making mostly cosmetic changes and this one fixes an immediate issue. But well, not my call either way.

Will check on the stack alignment.

libass/x86/rasterizer.asm Outdated Show resolved Hide resolved
@TheOneric
Copy link
Member

With #452 merged, and relying on x86inc.asm alignment defaults, I think the configure.ac-part should now look like this:

diff --git a/configure.ac b/configure.ac
index 2e5cae6..6cb4948 100644
--- a/configure.ac
+++ b/configure.ac
@@ -196,7 +196,7 @@ AS_IF([test "x$enable_asm" != xno], [
             X64=true
             BITS=64
             BITTYPE=x32
-            ASFLAGS="$ASFLAGS -DARCH_X86_64=1 -DPIC"
+            ASFLAGS="$ASFLAGS -DARCH_X86_64=1"
         ],
         [x86_64-*|amd64-*], [
             AS=nasm
@@ -204,7 +204,7 @@ AS_IF([test "x$enable_asm" != xno], [
             X64=true
             BITS=64
             BITTYPE=64
-            ASFLAGS="$ASFLAGS -DARCH_X86_64=1 -DPIC"
+            ASFLAGS="$ASFLAGS -DARCH_X86_64=1"
         ],
         [ # default
             INTEL=false
@@ -219,25 +219,18 @@ AS_IF([test "x$enable_asm" != xno], [
         ], [
             AS_CASE([$host],
                 [*darwin*], [
-                    ASFLAGS="$ASFLAGS -f macho$BITTYPE -DPREFIX -DHAVE_ALIGNED_STACK=1"
+                    ASFLAGS="$ASFLAGS -f macho$BITTYPE -DPREFIX -DSTACK_ALIGNMENT=16"
                 ],
                 [*linux*|*solaris*|*haiku*], [
-                    ASFLAGS="$ASFLAGS -f elf$BITTYPE -DHAVE_ALIGNED_STACK=1"
+                    ASFLAGS="$ASFLAGS -f elf$BITTYPE -DSTACK_ALIGNMENT=16"
                 ],
                 [*dragonfly*|*bsd*], [
                     ASFLAGS="$ASFLAGS -f elf$BITTYPE"
-                    AS_IF([test "x$BITS" = x64], [
-                        ASFLAGS="$ASFLAGS -DHAVE_ALIGNED_STACK=1"
-                    ], [
-                        ASFLAGS="$ASFLAGS -DHAVE_ALIGNED_STACK=0"
-                    ])
                 ],
                 [*cygwin*|*mingw*], [
                     ASFLAGS="$ASFLAGS -f win$BITTYPE"
-                    AS_IF([test "x$BITS" = x64], [
-                        ASFLAGS="$ASFLAGS -DHAVE_ALIGNED_STACK=1"
-                    ], [
-                        ASFLAGS="$ASFLAGS -DHAVE_ALIGNED_STACK=0 -DPREFIX"
+                    AS_IF([test "x$BITS" = x32], [
+                        ASFLAGS="$ASFLAGS -DPREFIX"
                     ])
                 ],
                 [ # default
@@ -251,7 +244,7 @@ AS_IF([test "x$enable_asm" != xno], [
                     ))
                 ]
             )
-            ASFLAGS="$ASFLAGS -DHAVE_CPUNOP=0 -Dprivate_prefix=ass"
+            ASFLAGS="$ASFLAGS -Dprivate_prefix=ass"
             AC_MSG_CHECKING([if $AS supports vpmovzxwd])
             echo "vpmovzxwd ymm0, xmm0" > conftest.asm
             AS_IF([$AS conftest.asm $ASFLAGS -o conftest.o >conftest.log 2>&1], [

This should fix the warnings introduced with nasm 2.15
@astiob astiob merged commit a93e17f into libass:master Feb 22, 2021
@astiob astiob added this to the 0.15.1 milestone Feb 22, 2021
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

Successfully merging this pull request may close these issues.

5 participants