Bug 156338 - Modify the CXXFLAGS in webkitdirs.pm just on architectures where the flags are supported
Summary: Modify the CXXFLAGS in webkitdirs.pm just on architectures where the flags ar...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2016-04-07 05:02 PDT by Tomas Popela
Modified: 2016-04-12 07:56 PDT (History)
6 users (show)

See Also:


Attachments
Proposed patch (1.83 KB, patch)
2016-04-07 05:05 PDT, Tomas Popela
no flags Details | Formatted Diff | Diff
Modify the CXXFLAGS just on i686 (1.61 KB, patch)
2016-04-12 01:58 PDT, Tomas Popela
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tomas Popela 2016-04-07 05:02:45 PDT
When building from checkout on s390(x) or ppc(64) the build will fail as we are adding "-march=pentium4 -msse2 -mfpmath=sse " into CXXFLAGS in webkitdirs.pm. We need to avoid this step for these architectures.
Comment 1 Tomas Popela 2016-04-07 05:05:40 PDT
Created attachment 275882 [details]
Proposed patch
Comment 2 Michael Catanzaro 2016-04-07 05:13:02 PDT
Comment on attachment 275882 [details]
Proposed patch

I think it would be better to check for i[3,4,5,6]86 to decide whether to add those build flags, so that we don't need to grow this conditional for every imaginable arch.
Comment 3 Michael Catanzaro 2016-04-07 05:20:31 PDT
Comment on attachment 275882 [details]
Proposed patch

View in context: https://bugs.webkit.org/attachment.cgi?id=275882&action=review

> Tools/Scripts/webkitdirs.pm:2019
>          $ENV{'CXXFLAGS'} = "-march=pentium4 -msse2 -mfpmath=sse " . ($ENV{'CXXFLAGS'} || "");

Actually note that since it uses sse2, these flags only work on i686.
Comment 4 Radar WebKit Bug Importer 2016-04-07 05:21:51 PDT
<rdar://problem/25599643>
Comment 5 Tomas Popela 2016-04-07 07:38:30 PDT
(In reply to comment #3)
> Comment on attachment 275882 [details]
> Proposed patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=275882&action=review
> 
> > Tools/Scripts/webkitdirs.pm:2019
> >          $ENV{'CXXFLAGS'} = "-march=pentium4 -msse2 -mfpmath=sse " . ($ENV{'CXXFLAGS'} || "");
> 
> Actually note that since it uses sse2, these flags only work on i686.

So this would effectively leave us with:

-    if ($architecture ne "x86_64" && !isARM() && !isCrossCompilation() && !isAnyWindows()) {
+    if ($architecture eq "i686" && !isCrossCompilation() && !isAnyWindows()) {

but I don't know whether this could break anything for Apple port.
Comment 6 Alex Christensen 2016-04-08 23:09:54 PDT
I don't think this will cause a problem for the Mac cmake work. If it causes a problem we will fix it, but my initial cmake work is only for x86_64
Comment 7 Michael Catanzaro 2016-04-10 15:45:04 PDT
(In reply to comment #5)
> So this would effectively leave us with:
> 
> -    if ($architecture ne "x86_64" && !isARM() && !isCrossCompilation() &&
> !isAnyWindows()) {
> +    if ($architecture eq "i686" && !isCrossCompilation() &&
> !isAnyWindows()) {
> 
> but I don't know whether this could break anything for Apple port.

Let's indeed go with this. If it unexpectedly breaks something, we can simply revert it and use your original patch instead.
Comment 8 Tomas Popela 2016-04-12 01:58:57 PDT
Created attachment 276225 [details]
Modify the CXXFLAGS just on i686
Comment 9 WebKit Commit Bot 2016-04-12 07:56:05 PDT
Comment on attachment 276225 [details]
Modify the CXXFLAGS just on i686

Clearing flags on attachment: 276225

Committed r199350: <http://trac.webkit.org/changeset/199350>
Comment 10 WebKit Commit Bot 2016-04-12 07:56:09 PDT
All reviewed patches have been landed.  Closing bug.