WebKit
[ARM][NEON] FELightningNEON.cpp fails to build, NEON fast path seems unused
0d3344e17d258106617b0e6d783d073b188a2548
[ARM][NEON] FELightningNEON.cpp fails to build, NEON fast path seems unused https://bugs.webkit.org/show_bug.cgi?id=241182 Reviewed by NOBODY (OOPS!). Move the NEON fast path for the SVG lighting filter effects into FELightingSoftwareApplier, and arrange to actually use them by forwarding calls to applyPlatformGeneric() into applyPlatformNeon(). Some changes were needed to adapt platformApplyNeon() to the current state of filters after r286140. This was not detected because the code bitrotted due to it being guarded with CPU(ARM_TRADITIONAL), which does not get used much these days: CPU(ARM_THUMB2) is more common. It should be possible to use the NEON fast paths also in Thumb mode, but that is left for a follow-up fix. * Source/WebCore/platform/graphics/cpu/arm/filters/FELightingNEON.cpp: (WebCore::FELightingSoftwareApplier::platformApplyNeonWorker): (WebCore::FELightingSoftwareApplier::getPowerCoefficients): (WebCore::FELighting::platformApplyNeonWorker): Deleted. (WebCore::FELighting::getPowerCoefficients): Deleted. * Source/WebCore/platform/graphics/cpu/arm/filters/FELightingNEON.h: (WebCore::FELightingSoftwareApplier::applyPlatformNeon): (WebCore::FELighting::platformApplyNeon): Deleted. * Source/WebCore/platform/graphics/filters/DistantLightSource.h: * Source/WebCore/platform/graphics/filters/FELighting.h: * Source/WebCore/platform/graphics/filters/PointLightSource.h: * Source/WebCore/platform/graphics/filters/SpotLightSource.h: * Source/WebCore/platform/graphics/filters/software/FELightingSoftwareApplier.h:
I am marking this as draft at the moment because I want to still test that the lightning filters with the NEON code enabled work well on a Raspberry Pi, but I expect they will be fine so I am already uploading the patch for initial review 😸
Also I have filed bug #241183 to try and enable building this code when using the Thumb instruction set, and not only for CPU(ARM_TRADITIONAL). That way the code will be built and exercised regularly, which will prevent bitrot.
I have removed the “Draft” tag now that I have tested this on a Raspberry Pi 3B, with an image built using Buildroot targeting the traditional ARM ISA instead of Thumb2 (which would have been the default). Render looks the same as a build using the generic code 🥳
@shallawa Could you take a look at the patch? Thanks!
Thanks for fixing this after I broke it.
The change looks good but I think it will be better if you get a separate applier for NEON. Instead of having the code of two different appliers in one class, we can split them in two classes. And FELighting::createSoftwareApplier() can decide which one to create. Any shared structures like LightingData can be moved to a separate header files and can be used by the two appliers. I think this will make things cleaner if we decide to add different appliers in the future. I would suggest the name of the new applier to be something like FELightingSoftwareApplierForNeon, FELightingNeonSoftwareApplier or just FELightingNeonApplier.
Thanks for fixing this after I broke it.
The change looks good but I think it will be better if you get a separate applier for NEON. Instead of having the code of two different appliers in one class, we can split them in two classes. And FELighting::createSoftwareApplier() can decide which one to create. Any shared structures like LightingData can be moved to a separate header files and can be used by the two appliers. I think this will make things cleaner if we decide to add different appliers in the future. I would suggest the name of the new applier to be something like FELightingSoftwareApplierForNeon, FELightingNeonSoftwareApplier or just FELightingNeonApplier.
Thanks for all the suggestions, I'll try to get the patch updated in the coming days 😺
I'm getting this error building with this PR applied against 2.38.0. Any ideas?
[5593/5622] Building CXX object Source/WebKit/CMakeFiles/WebKit.dir/__/__/DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-38.cpp.o
In file included from /usr/local/tmp/crew/webkit2gtk_5.20221010163636.dir/webkitgtk-2.38.0/Source/WebKit/NetworkProcess/cache/NetworkCacheBlobStorage.h:30,
from /usr/local/tmp/crew/webkit2gtk_5.20221010163636.dir/webkitgtk-2.38.0/Source/WebKit/NetworkProcess/cache/NetworkCacheStorage.h:28,
from /usr/local/tmp/crew/webkit2gtk_5.20221010163636.dir/webkitgtk-2.38.0/Source/WebKit/NetworkProcess/cache/NetworkCacheEntry.h:28,
from /usr/local/tmp/crew/webkit2gtk_5.20221010163636.dir/webkitgtk-2.38.0/Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerSoftUpdateLoader.h:30,
from /usr/local/tmp/crew/webkit2gtk_5.20221010163636.dir/webkitgtk-2.38.0/Source/WebKit/NetworkProcess/NetworkSession.h:37,
from /usr/local/tmp/crew/webkit2gtk_5.20221010163636.dir/webkitgtk-2.38.0/Source/WebKit/WebProcess/WebCoreSupport/soup/WebFrameNetworkingContext.cpp:30,
from /usr/local/tmp/crew/webkit2gtk_5.20221010163636.dir/webkitgtk-2.38.0/builddir/DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-38.cpp:4:
/usr/local/tmp/crew/webkit2gtk_5.20221010163636.dir/webkitgtk-2.38.0/Source/WebKit/NetworkProcess/cache/NetworkCacheKey.h: In static member function ‘static unsigned int WTF::NetworkCacheKeyHash::hash(const WebKit::NetworkCache::Key&)’:
/usr/local/tmp/crew/webkit2gtk_5.20221010163636.dir/webkitgtk-2.38.0/Source/WebKit/NetworkProcess/cache/NetworkCacheKey.h:113:17: warning: cast from ‘std::array<unsigned char, 20>::const_pointer’ {aka ‘const unsigned char*’} to ‘const unsigned int*’ increases required alignment of target type [-Wcast-align]
113 | return *reinterpret_cast<const unsigned*>(key.hash().data());
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
[5594/5622] Building CXX object Source/WebKit/CMakeFiles/WebKit.dir/__/__/DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-39.cpp.o
[5595/5622] Building CXX object Source/WebKit/CMakeFiles/WebKit.dir/WebProcess/WebPage/libwpe/AcceleratedSurfaceLibWPE.cpp.o
[5596/5622] Building CXX object Source/WebKit/CMakeFiles/webkit2gtkinjectedbundle.dir/WebProcess/InjectedBundle/API/glib/WebKitInjectedBundleMain.cpp.o
[5597/5622] Building CXX object Source/WebKit/CMakeFiles/WebKit.dir/__/__/DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-40.cpp.o
[5598/5622] Building CXX object Source/WebKit/CMakeFiles/WebKit.dir/__/__/DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-41.cpp.o
[5599/5622] Building CXX object Source/WebKit/CMakeFiles/WebKit.dir/__/__/DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-43.cpp.o
[5600/5622] Building CXX object Source/WebKit/CMakeFiles/WebKit.dir/WebProcess/WebPage/WebPage.cpp.o
[5601/5622] Building CXX object Source/WebKit/CMakeFiles/WebKit.dir/__/__/DerivedSources/WebKit/unified-sources/UnifiedSource-54928a2b-42.cpp.o
[5602/5622] Linking CXX shared library lib/libwebkit2gtk-5.0.so.0.0.0
FAILED: lib/libwebkit2gtk-5.0.so.0.0.0
: && /usr/local/lib/ccache/bin/c++ -fPIC -fdiagnostics-color=always -Wextra -Wall -pipe -Wno-odr -Wno-stringop-overread -Wno-stringop-overflow -Wno-nonnull -Wno-array-bounds -Wno-expansion-to-defined -Wno-noexcept-type -Wno-psabi -Wno-misleading-indentation -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wno-tautological-compare -O2 -pipe -ffat-lto-objects -fPIC -fuse-ld=mold -flto -fno-strict-aliasing -fno-exceptions -fno-rtti -O3 -DNDEBUG -flto=auto -fno-fat-lto-objects -Wl,--no-undefined -flto -Wl,--disable-new-dtags -Wl,--version-script,/usr/local/tmp/crew/webkit2gtk_5.20221010163636.dir/webkitgtk-2.38.0/Source/WebKit/webkitglib-symbols.map -shared -Wl,-soname,libwebkit2gtk-5.0.so.0 -o lib/libwebkit2gtk-5.0.so.0.0.0 @CMakeFiles/WebKit.rsp && :
/usr/local/tmp/crew/webkit2gtk_5.20221010163636.dir/webkitgtk-2.38.0/Source/ThirdParty/ANGLE/src/compiler/preprocessor/Tokenizer.h:25: note: type name ‘angle::pp::Tokenizer::Context’ should match type name ‘sh::TParseContext’
25 | struct Context
|
/usr/local/tmp/crew/webkit2gtk_5.20221010163636.dir/webkitgtk-2.38.0/Source/ThirdParty/ANGLE/src/compiler/translator/ParseContext.h:33: note: the incompatible type is defined here
33 | class TParseContext : angle::NonCopyable
|
mold: error: undefined symbol: WebCore::feLightingConstantsForNeon()
>>> referenced by <artificial>
>>> /usr/local/tmp/ccapKWqu.ltrans98.ltrans.o:(WebCore::FELightingSoftwareApplier::apply(WebCore::Filter const&, WTF::Vector<WTF::Ref<WebCore::FilterImage, WTF::RawPtrTraits<WebCore::FilterImage> >, 0u, WTF::CrashOnOverflow, 16u, WTF::FastMalloc> const&, WebCore::FilterImage&) const)
mold: error: undefined symbol: WebCore::FELightingSoftwareApplier::platformApplyNeonWorker(WebCore::FELightingPaintingDataForNeon*)
>>> referenced by <artificial>
>>> /usr/local/tmp/ccapKWqu.ltrans98.ltrans.o:(WebCore::FELightingSoftwareApplier::apply(WebCore::Filter const&, WTF::Vector<WTF::Ref<WebCore::FilterImage, WTF::RawPtrTraits<WebCore::FilterImage> >, 0u, WTF::CrashOnOverflow, 16u, WTF::FastMalloc> const&, WebCore::FilterImage&) const)
mold: error: undefined symbol: neonDrawLighting
>>> referenced by <artificial>
>>> /usr/local/tmp/ccapKWqu.ltrans98.ltrans.o:(WebCore::FELightingSoftwareApplier::apply(WebCore::Filter const&, WTF::Vector<WTF::Ref<WebCore::FilterImage, WTF::RawPtrTraits<WebCore::FilterImage> >, 0u, WTF::CrashOnOverflow, 16u, WTF::FastMalloc> const&, WebCore::FilterImage&) const)
mold: error: undefined symbol: WebCore::FELightingSoftwareApplier::getPowerCoefficients(float)
>>> referenced by <artificial>
>>> /usr/local/tmp/ccapKWqu.ltrans98.ltrans.o:(WebCore::FELightingSoftwareApplier::apply(WebCore::Filter const&, WTF::Vector<WTF::Ref<WebCore::FilterImage, WTF::RawPtrTraits<WebCore::FilterImage> >, 0u, WTF::CrashOnOverflow, 16u, WTF::FastMalloc> const&, WebCore::FilterImage&) const)>>> referenced by <artificial>
>>> /usr/local/tmp/ccapKWqu.ltrans98.ltrans.o:(WebCore::FELightingSoftwareApplier::apply(WebCore::Filter const&, WTF::Vector<WTF::Ref<WebCore::FilterImage, WTF::RawPtrTraits<WebCore::FilterImage> >, 0u, WTF::CrashOnOverflow, 16u, WTF::FastMalloc> const&, WebCore::FilterImage&) const)
collect2: error: ld returned 1 exit status
Full build log: webkit2gtk_5-2.38.0-armv7l.log
I'm also using this variation on the patch from https://bugs.webkit.org/show_bug.cgi?id=226557#c27 to make sure gcc > 11 isn't an issue.
e.g.
diff --git a/Source/cmake/WebKitCompilerFlags.cmake b/Source/cmake/WebKitCompilerFlags.cmake
index 77ebb802ebb03450b5e96629a47b6819a68672c6..d49d6e43d7eeb6673c624e00eadf3edfca0674eb 100644
--- a/Source/cmake/WebKitCompilerFlags.cmake
+++ b/Source/cmake/WebKitCompilerFlags.cmake
@@ -143,6 +143,13 @@ if (COMPILER_IS_GCC_OR_CLANG)
WEBKIT_PREPEND_GLOBAL_CXX_FLAGS(-Wno-nonnull)
endif ()
+ # This triggers warnings in wtf/Packed.h, a header that is included in many places. It does not
+ # respect ignore warning pragmas and we cannot easily suppress it for all affected files.
+ # https://bugs.webkit.org/show_bug.cgi?id=226557
+ if (CMAKE_CXX_COMPILER_ID MATCHES "GNU" AND ${CMAKE_CXX_COMPILER_VERSION} VERSION_GREATER_EQUAL "11.0")
+ WEBKIT_PREPEND_GLOBAL_CXX_FLAGS(-Wno-stringop-overread)
+ endif ()
+
# -Wexpansion-to-defined produces false positives with GCC but not Clang
# https://bugs.webkit.org/show_bug.cgi?id=167643#c13
if (CMAKE_CXX_COMPILER_ID MATCHES "GNU")
Sorry for polluting this PR.
I am actually getting that first error, and I don't think it is a compiler issue. (I'm applying the above gcc11 patch too and it doesn't help. The errors are from undefined symbols, which seem to be in the header files properly, e.g. https://github.com/WebKit/WebKit/blob/0d3344e17d258106617b0e6d783d073b188a2548/Source/WebCore/platform/graphics/cpu/arm/filters/FELightingNEON.h#L91
Any ideas on what I'm doing wrong?
mold: error: undefined symbol:
did you also try lld/some other linker? it would be a mold bug if the others work (just a random guess)
I'll try another linker next! Mold worked fine on x86_64 for me.
@rui314 I'll let you know in a couple of days if building works without mold. ;)
This is what I am seeing with gold linker and GCC 12.2:
[5602/5622] Linking CXX shared library lib/libwebkit2gtk-5.0.so.0.0.0
FAILED: lib/libwebkit2gtk-5.0.so.0.0.0
: && /usr/local/bin/c++ -fPIC -fdiagnostics-color=always -Wextra -Wall -pipe -Wno-odr -Wno-stringop-overread -Wno-stringop-overread -Wno-stringop-overflow -Wno-nonnull -Wno-array-bounds -Wno-expansion-to-defined -Wno-noexcept-type -Wno-psabi -Wno-misleading-indentation -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wno-tautological-compare -O2 -pipe -ffat-lto-objects -fPIC -fuse-ld=gold -flto -fno-strict-aliasing -fno-exceptions -fno-rtti -O3 -DNDEBUG -flto=auto -fno-fat-lto-objects -Wl,--no-undefined -flto -Wl,--disable-new-dtags -Wl,--version-script,/usr/local/tmp/crew/webkit2gtk_5.20221014223727.dir/webkitgtk-2.38.0/Source/WebKit/webkitglib-symbols.map -shared -Wl,-soname,libwebkit2gtk-5.0.so.0 -o lib/libwebkit2gtk-5.0.so.0.0.0 @CMakeFiles/WebKit.rsp && :
/usr/local/tmp/crew/webkit2gtk_5.20221014223727.dir/webkitgtk-2.38.0/Source/ThirdParty/ANGLE/src/compiler/translator/ParseContext.h:33: note: type name ‘sh::TParseContext’ should match type name ‘angle::pp::Tokenizer::Context’
33 | class TParseContext : angle::NonCopyable
|
/usr/local/tmp/crew/webkit2gtk_5.20221014223727.dir/webkitgtk-2.38.0/Source/ThirdParty/ANGLE/src/compiler/preprocessor/Tokenizer.h:25: note: the incompatible type is defined here
25 | struct Context
|
/usr/local/tmp/ccIlIlbC.ltrans98.ltrans.o:<artificial>:function WebCore::FELightingSoftwareApplier::apply(WebCore::Filter const&, WTF::Vector<WTF::Ref<WebCore::FilterImage, WTF::RawPtrTraits<WebCore::FilterImage> >, 0u, WTF::CrashOnOverflow, 16u, WTF::FastMalloc> const&, WebCore::FilterImage&) const: error: undefined reference to 'WebCore::feLightingConstantsForNeon()' /usr/local/tmp/ccIlIlbC.ltrans98.ltrans.o:<artificial>:function WebCore::FELightingSoftwareApplier::apply(WebCore::Filter const&, WTF::Vector<WTF::Ref<WebCore::FilterImage, WTF::RawPtrTraits<WebCore::FilterImage> >, 0u, WTF::CrashOnOverflow, 16u, WTF::FastMalloc> const&, WebCore::FilterImage&) const: error: undefined reference to 'neonDrawLighting' /usr/local/tmp/ccIlIlbC.ltrans98.ltrans.o:<artificial>:function WebCore::FELightingSoftwareApplier::apply(WebCore::Filter const&, WTF::Vector<WTF::Ref<WebCore::FilterImage, WTF::RawPtrTraits<WebCore::FilterImage> >, 0u, WTF::CrashOnOverflow, 16u, WTF::FastMalloc> const&, WebCore::FilterImage&) const: error: undefined reference to 'WebCore::FELightingSoftwareApplier::getPowerCoefficients(float)' /usr/local/tmp/ccIlIlbC.ltrans98.ltrans.o:<artificial>:function WebCore::FELightingSoftwareApplier::apply(WebCore::Filter const&, WTF::Vector<WTF::Ref<WebCore::FilterImage, WTF::RawPtrTraits<WebCore::FilterImage> >, 0u, WTF::CrashOnOverflow, 16u, WTF::FastMalloc> const&, WebCore::FilterImage&) const: error: undefined reference to 'WebCore::FELightingSoftwareApplier::platformApplyNeonWorker(WebCore::FELightingPaintingDataForNeon*)' /usr/local/tmp/ccIlIlbC.ltrans98.ltrans.o:<artificial>:function WebCore::FELightingSoftwareApplier::apply(WebCore::Filter const&, WTF::Vector<WTF::Ref<WebCore::FilterImage, WTF::RawPtrTraits<WebCore::FilterImage> >, 0u, WTF::CrashOnOverflow, 16u, WTF::FastMalloc> const&, WebCore::FilterImage&) const: error: undefined reference to 'WebCore::FELightingSoftwareApplier::getPowerCoefficients(float)'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.
I won't be able to work on this any time soon. If anyone feels like picking it, please go ahead 😉
(FYI I couldn't work around these issues, so we ended up just building w/o -mfpu=neon and using -mfpu=vfpv3-d16 to match the debian armhf build setup and that fixes our webkit2gtk builds.)
When I apply this patch to the ARCH linux ARM PKGBUILD, it seems to fully compile all the way to end but fails to link with:
5541/5634] Linking CXX shared library lib/libwebkit2gtk-5.0.so.0.0.0
FAILED: lib/libwebkit2gtk-5.0.so.0.0.0
: && /usr/bin/c++ -fPIC -fdiagnostics-color=always -Wextra -Wall -pipe -Wno-odr -Wno-stringop-overread -Wno-stringop-overflow -Wno-nonnull -Wno-array-bounds -Wno-expansion-to-defined -Wno-noexcept-type -Wno-psabi -Wno-misleading-indentation -Wno-maybe-uninitialized -Wwrite-strings -Wundef -Wpointer-arith -Wmissing-format-attribute -Wformat-security -Wcast-align -Wno-tautological-compare -march=armv7-a -mfloat-abi=hard -mfpu=neon -O2 -pipe -fstack-protector-strong -fno-plt -fexceptions -Wp,-D_FORTIFY_SOURCE=2 -Wformat -Werror=format-security -fstack-clash-protection -Wp,-D_GLIBCXX_ASSERTIONS -g -fvar-tracking-assignments -ffile-prefix-map=/mnt/src/odroid/alarm/PKGBUILD/webkit2gtk-5.0/src=/usr/src/debug/webkit2gtk-5.0 -g0 -fno-strict-aliasing -fno-exceptions -fno-rtti -O3 -DNDEBUG -Wl,--no-undefined -Wl,-O1,--sort-common,--as-needed,-z,relro,-z,now -Wl,--disable-new-dtags -Wl,--version-script,/mnt/src/odroid/alarm/PKGBUILD/webkit2gtk-5.0/src/webkitgtk-2.38.3/Source/WebKit/webkitglib-symbols.map -shared -Wl,-soname,libwebkit2gtk-5.0.so.0 -o lib/libwebkit2gtk-5.0.so.0.0.0 @CMakeFiles/WebKit.rsp && :
/usr/bin/ld: Source/WebCore/CMakeFiles/WebCore.dir/./__/__/WebCore/DerivedSources/unified-sources/UnifiedSource-3c72abbe-43.cpp.o: in function `WebCore::FELightingSoftwareApplier::applyPlatformNeon(WebCore::FELightingSoftwareApplier::LightingData const&, WebCore::LightSource::PaintingData const&)':
UnifiedSource-3c72abbe-43.cpp:(.text._ZN7WebCore25FELightingSoftwareApplier17applyPlatformNeonERKNS0_12LightingDataERKNS_11LightSource12PaintingDataE[_ZN7WebCore25FELightingSoftwareApplier17applyPlatformNeonERKNS0_12LightingDataERKNS_11LightSource12PaintingDataE]+0x88): undefined reference to `WebCore::feLightingConstantsForNeon()'
/usr/bin/ld: UnifiedSource-3c72abbe-43.cpp:(.text._ZN7WebCore25FELightingSoftwareApplier17applyPlatformNeonERKNS0_12LightingDataERKNS_11LightSource12PaintingDataE[_ZN7WebCore25FELightingSoftwareApplier17applyPlatformNeonERKNS0_12LightingDataERKNS_11LightSource12PaintingDataE]+0x16c): undefined reference to `neonDrawLighting'
/usr/bin/ld: UnifiedSource-3c72abbe-43.cpp:(.text._ZN7WebCore25FELightingSoftwareApplier17applyPlatformNeonERKNS0_12LightingDataERKNS_11LightSource12PaintingDataE[_ZN7WebCore25FELightingSoftwareApplier17applyPlatformNeonERKNS0_12LightingDataERKNS_11LightSource12PaintingDataE]+0x2a0): undefined reference to `WebCore::FELightingSoftwareApplier::getPowerCoefficients(float)'
/usr/bin/ld: UnifiedSource-3c72abbe-43.cpp:(.text._ZN7WebCore25FELightingSoftwareApplier17applyPlatformNeonERKNS0_12LightingDataERKNS_11LightSource12PaintingDataE[_ZN7WebCore25FELightingSoftwareApplier17applyPlatformNeonERKNS0_12LightingDataERKNS_11LightSource12PaintingDataE]+0x5d4): undefined reference to `WebCore::FELightingSoftwareApplier::getPowerCoefficients(float)'
/usr/bin/ld: UnifiedSource-3c72abbe-43.cpp:(.text._ZN7WebCore25FELightingSoftwareApplier17applyPlatformNeonERKNS0_12LightingDataERKNS_11LightSource12PaintingDataE[_ZN7WebCore25FELightingSoftwareApplier17applyPlatformNeonERKNS0_12LightingDataERKNS_11LightSource12PaintingDataE]+0xbb8): undefined reference to `WebCore::FELightingSoftwareApplier::platformApplyNeonWorker(WebCore::FELightingPaintingDataForNeon*)'
collect2: error: ld returned 1 exit status
ninja: build stopped: subcommand failed.
==> ERROR: A failure occurred in build().
Aborting...
Any ideas?
