diff mbox series

[bug#51863] gnu: webkitgtk: Really disable SSE2 on i686-linux.

Message ID 87a6i5fyi4.fsf@GlaDOS.home
State Accepted
Headers show
Series [bug#51863] gnu: webkitgtk: Really disable SSE2 on i686-linux. | expand

Checks

Context Check Description
cbaines/comparison success View comparision
cbaines/git branch success View Git branch
cbaines/applying patch fail View Laminar job
cbaines/issue success View issue

Commit Message

Diego Nicola Barbato Nov. 15, 2021, 11:29 a.m. UTC 
Hey Guix,

Commit d82fd7c2dd542693988f61fb15c020e3209ac7ec (gnu: webkitgtk: Disable
SSE2 when not on x86_64.) on master breaks webkitgtk on i686-linux.

(I'm Cc-ing Leo, Liliana, and Mark since you were all involved in
upgrading webkitgtk to 2.34.1.)

The offending patch attempts to disable SSE2 by conditionally adding a
phase which removes the call to 'CHECK_FOR_SSE2' from the end of
'Source/cmake/DetectSSE2.cmake'.  This phase doesn't do anything about
'SSE2_SUPPORT_FOUND' being set to 'FALSE' at line 34 in the same file,
which causes the following snippet starting at line 152 in
'Source/cmake/WebKitCompilerFlags.cmake'

--8<---------------cut here---------------start------------->8---
    # Force SSE2 fp on x86 builds.
    if (WTF_CPU_X86 AND NOT CMAKE_CROSSCOMPILING)
        WEBKIT_PREPEND_GLOBAL_COMPILER_FLAGS(-msse2 -mfpmath=sse)
        include(DetectSSE2)
        if (NOT SSE2_SUPPORT_FOUND)
            message(FATAL_ERROR "SSE2 support is required to compile WebKit")
        endif ()
    endif ()
--8<---------------cut here---------------end--------------->8---

to fail with the following error when building for i686.

--8<---------------cut here---------------start------------->8---
CMake Error at Source/cmake/WebKitCompilerFlags.cmake:157 (message):
  SSE2 support is required to compile WebKit
--8<---------------cut here---------------end--------------->8---

The attached patch removes the code for adding the broken 'disable-sse2'
phase.  Instead it disables SSE2 using a patch from Debian and by
setting 'CFLAGS' and 'CXXFLAGS' to '-march=i686' in the
'prepare-build-environment' phase when building for i686.  The latter is
necessary because clang, unlike gcc, defaults to '-march=pentium4',
which enables SSE2.

With this patch I've successfully built webkitgtk and epiphany on
i686-linux and x86_64-linux.

Regards,

Diego

Comments

Liliana Marie Prikler Nov. 15, 2021, 7:29 p.m. UTC | #1 
Hi Diego,

Am Montag, den 15.11.2021, 11:29 +0000 schrieb Diego Nicola Barbato:
> Hey Guix,
> 
> Commit d82fd7c2dd542693988f61fb15c020e3209ac7ec (gnu: webkitgtk:
> Disable SSE2 when not on x86_64.) on master breaks webkitgtk on i686-
> linux.
No, it does not.  It merely fails to fix it ;)

> (I'm Cc-ing Leo, Liliana, and Mark since you were all involved in
> upgrading webkitgtk to 2.34.1.)
> 
> The offending patch attempts to disable SSE2 by conditionally adding
> a phase which removes the call to 'CHECK_FOR_SSE2' from the end of
> 'Source/cmake/DetectSSE2.cmake'.  This phase doesn't do anything
> about 'SSE2_SUPPORT_FOUND' being set to 'FALSE' at line 34 in the
> same file, which causes the following snippet starting at line 152 in
> 'Source/cmake/WebKitCompilerFlags.cmake'
> 
> --8<---------------cut here---------------start------------->8---
>     # Force SSE2 fp on x86 builds.
>     if (WTF_CPU_X86 AND NOT CMAKE_CROSSCOMPILING)
>         WEBKIT_PREPEND_GLOBAL_COMPILER_FLAGS(-msse2 -mfpmath=sse)
>         include(DetectSSE2)
>         if (NOT SSE2_SUPPORT_FOUND)
>             message(FATAL_ERROR "SSE2 support is required to compile
> WebKit")
>         endif ()
>     endif ()
> --8<---------------cut here---------------end--------------->8---
> 
> to fail with the following error when building for i686.
> 
> --8<---------------cut here---------------start------------->8---
> CMake Error at Source/cmake/WebKitCompilerFlags.cmake:157 (message):
>   SSE2 support is required to compile WebKit
> --8<---------------cut here---------------end--------------->8---
> 
> The attached patch removes the code for adding the broken 'disable-
> sse2' phase.  Instead it disables SSE2 using a patch from Debian and
> by setting 'CFLAGS' and 'CXXFLAGS' to '-march=i686' in the
> 'prepare-build-environment' phase when building for i686.  The latter
> is necessary because clang, unlike gcc, defaults to '-
> march=pentium4', which enables SSE2.
These two things don't require combination though, right?  Like one
thing disables the SSE check whereas the other fixes a mostly unrelated
issue, am I correct?

I'd like the comment about the "-march=whatever" thing to be closer to
the setenvs, also if GCC does not require it there ought to be a TODO
or XXX stating that it should be removed on core-updates-frozen.

Otherwise LGTM, cheers!
Maxim Cournoyer Nov. 26, 2021, 4:15 a.m. UTC | #2 
Hello,

Fixed with a variant of Diego's patch in d13d5f3141 on master and
68d47844b1 on core-updates-frozen.

Thanks a lot!

Closing.

Maxim
diff mbox series

Patch

From 5c1f6fe53db3f3916776b9e06909e8fc598e7a53 Mon Sep 17 00:00:00 2001
From: Diego Nicola Barbato <dnbarbato@posteo.de>
Date: Tue, 9 Nov 2021 18:24:57 +0100
Subject: [PATCH] gnu: webkitgtk: Really disable SSE2 on i686-linux.

This is a followup to d82fd7c2dd542693988f61fb15c020e3209ac7ec.

* gnu/packages/patches/webkitgtk-dont-detect-sse2.patch: New file.
* gnu/local.mk (dist_patch_DATA): Add it.
* gnu/packages/webkit.scm (webkitgtk)[source]: Use it.
[arguments]: Remove 'disable-sse2' phase.  When building on i686-linux, insert
'setenv' forms in the 'prepare-build-environment' phase that set both 'CFLAGS'
and 'CXXFLAGS' to '-march=i686'.
---
 gnu/local.mk                                  |  1 +
 .../patches/webkitgtk-dont-detect-sse2.patch  | 28 +++++++++++++++++++
 gnu/packages/webkit.scm                       | 17 +++++------
 3 files changed, 36 insertions(+), 10 deletions(-)
 create mode 100644 gnu/packages/patches/webkitgtk-dont-detect-sse2.patch

diff --git a/gnu/local.mk b/gnu/local.mk
index 208875754b..438b661442 100644
--- a/gnu/local.mk
+++ b/gnu/local.mk
@@ -1899,6 +1899,7 @@  dist_patch_DATA =						\
   %D%/packages/patches/vtk-fix-freetypetools-build-failure.patch	\
   %D%/packages/patches/vtk-8-fix-freetypetools-build-failure.patch	\
   %D%/packages/patches/warsow-qfusion-fix-bool-return-type.patch	\
+  %D%/packages/patches/webkitgtk-dont-detect-sse2.patch		\
   %D%/packages/patches/webkitgtk-share-store.patch		\
   %D%/packages/patches/webkitgtk-bind-all-fonts.patch		\
   %D%/packages/patches/websocketpp-fix-for-cmake-3.15.patch	\
diff --git a/gnu/packages/patches/webkitgtk-dont-detect-sse2.patch b/gnu/packages/patches/webkitgtk-dont-detect-sse2.patch
new file mode 100644
index 0000000000..6be2775480
--- /dev/null
+++ b/gnu/packages/patches/webkitgtk-dont-detect-sse2.patch
@@ -0,0 +1,28 @@ 
+Copied from Debian:
+
+https://sources.debian.org/data/main/w/webkit2gtk/2.34.1-1/debian/patches/dont-detect-sse2.patch
+
+From: Alberto Garcia <berto@igalia.com>
+Subject: Don't check for SSE2 support on i386
+Bug-Debian: https://bugs.debian.org/930935
+Forwarded: no
+Index: webkitgtk/Source/cmake/WebKitCompilerFlags.cmake
+===================================================================
+--- webkitgtk.orig/Source/cmake/WebKitCompilerFlags.cmake
++++ webkitgtk/Source/cmake/WebKitCompilerFlags.cmake
+@@ -148,15 +148,6 @@ if (COMPILER_IS_GCC_OR_CLANG)
+     if (CMAKE_CXX_COMPILER_ID MATCHES "GNU")
+         WEBKIT_PREPEND_GLOBAL_COMPILER_FLAGS(-Wno-expansion-to-defined)
+     endif ()
+-
+-    # Force SSE2 fp on x86 builds.
+-    if (WTF_CPU_X86 AND NOT CMAKE_CROSSCOMPILING)
+-        WEBKIT_PREPEND_GLOBAL_COMPILER_FLAGS(-msse2 -mfpmath=sse)
+-        include(DetectSSE2)
+-        if (NOT SSE2_SUPPORT_FOUND)
+-            message(FATAL_ERROR "SSE2 support is required to compile WebKit")
+-        endif ()
+-    endif ()
+ endif ()
+ 
+ if (COMPILER_IS_GCC_OR_CLANG AND NOT MSVC)
diff --git a/gnu/packages/webkit.scm b/gnu/packages/webkit.scm
index 688e745d95..88a59d7acd 100644
--- a/gnu/packages/webkit.scm
+++ b/gnu/packages/webkit.scm
@@ -236,7 +236,8 @@  (define-public webkitgtk
               (sha256
                (base32
                 "1vix0w24m7mq82rzxrk8xvcrkli44gimzs282xs29q2xf0b16g24"))
-              (patches (search-patches "webkitgtk-share-store.patch"
+              (patches (search-patches "webkitgtk-dont-detect-sse2.patch"
+                                       "webkitgtk-share-store.patch"
                                        "webkitgtk-bind-all-fonts.patch"))))
     (build-system cmake-build-system)
     (outputs '("out" "doc"))
@@ -295,23 +296,19 @@  (define-public webkitgtk
                  (("libWPEBackend-fdo-([\\.0-9]+)\\.so" all version)
                   (string-append wpebackend-fdo "/lib/" all)))
                #t)))
-         ,@(if (string-prefix? "x86_64" (or (%current-target-system)
-                                            (%current-system)))
-               '()
-               '((add-after 'unpack 'disable-sse2
-                   (lambda _
-                     (substitute* "Source/cmake/DetectSSE2.cmake"
-                       (("CHECK_FOR_SSE2\\(\\)") ""))))))
          (add-before 'configure 'prepare-build-environment
            (lambda* (#:key inputs #:allow-other-keys)
              (setenv "CC" "clang")
              (setenv "CXX" "clang++")
              ;; XXX Until we switch back to using GCC,
-             ;; work around <https://bugs.gnu.org/51591>.
+             ;; work around <https://bugs.gnu.org/51591>
+             ;; and compile for i686 without extensions.
              ,@(if (target-x86-32?)
                    '((substitute* "Source/WTF/wtf/CheckedArithmetic.h"
                        (("#define USE_MUL_OVERFLOW 1")
-                        "#define USE_MUL_OVERFLOW 0")))
+                        "#define USE_MUL_OVERFLOW 0"))
+                     (setenv "CFLAGS" "-march=i686")
+                     (setenv "CXXFLAGS" "-march=i686"))
                    '())
              #t))
          (add-after 'install 'move-doc-files

base-commit: d29a9aa54b9df9167f4641dd8ca42c561b460c4c
-- 
2.33.1