1300843 - Quit with an error on Linux machines without SSE2

Status: RESOLVED FIXED

(Core :: Widget: Gtk, defect)

Description

[Ralph Giles (:rillian)]

Per https://bugzilla.mozilla.org/show_bug.cgi?id=1274196#c13 we'd like to do something informative when a user tries to run a build which requires SSE2 support on unsupported hardware. This is only relevant to 32-bit x86 Linux builds; on Windows the installer checks and all Mac hardware supports SSE2.

The suggestion here is that we run a cpuid check early in main() and die with a console message and/or a gui notification so it doesn't look like we've just crashed if someone downloads a build manually, perhaps with a redirection to a supported build for their hardware (https://mozilla.debian.net/)

Upgrades from supported official builds should be blocked through the update service (bug 1271762). This bug is just about informing users who have gotten the wrong build some other way.

Comment 1

[Henri Sivonen (:hsivonen)]

Since Firefox is a GUI app and users might not look at its console output, we should probably use a Gtk dialog if at all feasible.

Are we OK with just hard-coding the message in English so that we don't need to ship localized strings in a format the Gtk could read without relying on any libxul infrastructure?

Comment 2

[Axel Hecht [:Pike]]

Yeah, agree, let's hard-code this, it's such an edge case.

Comment 3

[Henri Sivonen (:hsivonen)]

Attachment: Minimal program to be merged into main().

If we want a dialog but are OK with it being unlocalized, I believe this is about the minimum amount of code we'll need.

I intentionally didn't mention "Firefox" in the UI text to avoid the issue of having to vary the string based on branding level.

I didn't try to actually paste this into nsBrowserApp.cpp, I don't know if that file is allowed to link with Gtk3 directly, and I don't know what the right #ifdefs are for scoping this are.

Comment 4

[Henri Sivonen (:hsivonen)]

(In reply to Henri Sivonen (:hsivonen) from comment #3)
> If we want a dialog but are OK with it being unlocalized, I believe this is
> about the minimum amount of code we'll need.

I missed Axel's comment before I wrote mine, so let's go with unlocalized.

> I didn't try to actually paste this into nsBrowserApp.cpp, I don't know if
> that file is allowed to link with Gtk3 directly, and I don't know what the
> right #ifdefs are for scoping this are.

At least the include paths are not set up to allow nsBrowserApp.cpp to #include <gtk/gtk.h>.

As for #ifdefs:
I suggest we mint a define called MOZ_LINUX_32_SSE2_STARTUP_DIALOG and make the build system define it if we are on 32-bit x86, the widget backend is Gtk3 and MOZILLA_PRESUME_SSE2 is defined.

Then I think the build system should define MOZILLA_PRESUME_SSE2 for 32-bit x86 by default (to make sure developers by default build what Mozilla ships) but provide a mozconfig options for opt-out.

Comment 5

[Henri Sivonen (:hsivonen)]

Attachment: Show a dialog using GTK3

I think this is about as far as it makes sense for me to try to go without help from someone who understands the build system better.

I think the missing pieces are:

 1) Move the definition of MOZ_LINUX_32_SSE2_STARTUP_DIALOG into the build system.
 2) Change the condition in moz.build to check MOZ_LINUX_32_SSE2_STARTUP_DIALOG from the build system.
 3) Fix bug 1274196 at the same time such that by default, -msse2 is used (and MOZ_LINUX_32_SSE2_STARTUP_DIALOG is defined if the other conditions in its current definition hold).
 4) As part of the previous point, provide some sort of --unassume-sse2 option for Linux distros to opt out of -msse2 and defining MOZ_LINUX_32_SSE2_STARTUP_DIALOG.

nfroyd, can you implement the missing pieces or point me to the right direction? (It's quite unclear to me where in the build system the changes should go.)

Comment 6

[Henri Sivonen (:hsivonen)]

Oh, and nsBrowserApp.cpp itself should probably be built without -msse2 when MOZ_LINUX_32_SSE2_STARTUP_DIALOG is defined unless we want to live dangerously and hope that no SSE or SSE2 instructions get autoinserted on the code path to the dialog.

Comment 7

[Nathan Froyd [:froydnj]]

(In reply to Henri Sivonen (:hsivonen) from comment #5)
> Created attachment 8790185 [details] [diff] [review]
> Show a dialog using GTK3
> 
> I think this is about as far as it makes sense for me to try to go without
> help from someone who understands the build system better.
> 
> I think the missing pieces are:
> 
>  1) Move the definition of MOZ_LINUX_32_SSE2_STARTUP_DIALOG into the build
> system.

This would have to go in old-configure.in, since the widget toolkit is determined using the old configure.  Something like:

case "$target" in
i*86-*linux*)
    if test "$MOZ_WIDGET_TOOLKIT" = gtk3; then
        if $CC $CFLAGS -E - -dM | grep __SSE2__ > /dev/null; then
           AC_DEFINE(MOZ_LINUX_SSE2_STARTUP_DIALOG)
        fi
    fi
    ;;
esac

I *think* you want CFLAGS in there, because I think you want to pick up any -msse2 that we add to CFLAGS in configure prior to this check.

>  2) Change the condition in moz.build to check
> MOZ_LINUX_32_SSE2_STARTUP_DIALOG from the build system.

I think this means you'd also need AC_SUBST(MOZ_LINUX_SSE2_STARTUP_DIALOG) in the above code, since I don't think you can get access to whatever's been AC_DEFINE'd from configure.  But with AC_SUBST, you can check CONFIG['MOZ_LINUX_SSE2_STARTUP_DIALOG'] in moz.build files.

>  3) Fix bug 1274196 at the same time such that by default, -msse2 is used
> (and MOZ_LINUX_32_SSE2_STARTUP_DIALOG is defined if the other conditions in
> its current definition hold).

I think this just means adding -msse2 to the compiler flags, possibly right around here:

http://dxr.mozilla.org/mozilla-central/source/build/moz.configure/toolchain.configure#351

It's not clear to me whether you'd want to add it there, or whether you'd want to do it somewhere convenient in old-configure.in.  Note that it would have to be done prior to the check in part (1).

>  4) As part of the previous point, provide some sort of --unassume-sse2
> option for Linux distros to opt out of -msse2 and defining
> MOZ_LINUX_32_SSE2_STARTUP_DIALOG.

The option itself can go in $topsrcdir/moz.configure.  Wherever you added -msse2 in part (3) would obviously have to check this, and the configure check in part (1) would have to check the new option as well.

The build system could remove -msse* from C*FLAGS for an entire directory ala:

http://dxr.mozilla.org/mozilla-central/source/gfx/cairo/libpixman/src/Makefile.in#7

so that nsBrowserApp.cpp can be compiled with -msse2.

> nfroyd, can you implement the missing pieces or point me to the right
> direction? (It's quite unclear to me where in the build system the changes
> should go.)

Ideally the above provides the information you're looking for.  Please let me know if anything seems unclear.

Comment 8

[Henri Sivonen (:hsivonen)]

Attachment: Show a dialog using GTK3, v2

(In reply to Nathan Froyd [:froydnj] from comment #7)
> (In reply to Henri Sivonen (:hsivonen) from comment #5)
> > Created attachment 8790185 [details] [diff] [review]
> > Show a dialog using GTK3
> > 
> > I think this is about as far as it makes sense for me to try to go without
> > help from someone who understands the build system better.
> > 
> > I think the missing pieces are:
> > 
> >  1) Move the definition of MOZ_LINUX_32_SSE2_STARTUP_DIALOG into the build
> > system.
> 
> This would have to go in old-configure.in, since the widget toolkit is
> determined using the old configure.  Something like:
> 
> case "$target" in
> i*86-*linux*)
>     if test "$MOZ_WIDGET_TOOLKIT" = gtk3; then
>         if $CC $CFLAGS -E - -dM | grep __SSE2__ > /dev/null; then

That line didn't work, but I cargo culted my way around it with:
if $CC -E -dM -</dev/null | grep -q __SSE2__; then

>            AC_DEFINE(MOZ_LINUX_SSE2_STARTUP_DIALOG)
>         fi
>     fi
>     ;;
> esac
> 
> I *think* you want CFLAGS in there, because I think you want to pick up any
> -msse2 that we add to CFLAGS in configure prior to this check.
> 
> >  2) Change the condition in moz.build to check
> > MOZ_LINUX_32_SSE2_STARTUP_DIALOG from the build system.
> 
> I think this means you'd also need AC_SUBST(MOZ_LINUX_SSE2_STARTUP_DIALOG)
> in the above code, since I don't think you can get access to whatever's been
> AC_DEFINE'd from configure.  But with AC_SUBST, you can check
> CONFIG['MOZ_LINUX_SSE2_STARTUP_DIALOG'] in moz.build files.

It seems I also need to assign 1 to the variable.

> >  3) Fix bug 1274196 at the same time such that by default, -msse2 is used
> > (and MOZ_LINUX_32_SSE2_STARTUP_DIALOG is defined if the other conditions in
> > its current definition hold).
> 
> I think this just means adding -msse2 to the compiler flags, possibly right
> around here:
> 
> http://dxr.mozilla.org/mozilla-central/source/build/moz.configure/toolchain.
> configure#351

I didn't get to this part today.

Thank you.

Comment 9

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d3fe795d8898

Comment 10

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=48d3f66b74d5

Comment 11

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=aa5183136dcd

Comment 12

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f7d3e07dc395

Comment 13

[Henri Sivonen (:hsivonen)]

Attachment: Show a dialog using GTK3, v3

Comment 14

[Henri Sivonen (:hsivonen)]

(In reply to Nathan Froyd [:froydnj] from comment #7)
> >  3) Fix bug 1274196 at the same time such that by default, -msse2 is used
> > (and MOZ_LINUX_32_SSE2_STARTUP_DIALOG is defined if the other conditions in
> > its current definition hold).
> 
> I think this just means adding -msse2 to the compiler flags, possibly right
> around here:
> 
> http://dxr.mozilla.org/mozilla-central/source/build/moz.configure/toolchain.
> configure#351

How do I gain access to a boolean indicating whether the --without-sse2 option was set? What's in the current patch fails on 32-bit Linux, because without_sse2 is not a valid name within check_compiler(). (Doing set_config() doesn't help, because CONFIG seems to be unavailable for reading here.)

Comment 15

[Henri Sivonen (:hsivonen)]

Also, if I try to add flags in check_compiler() without trying to check for --without-sse2, adding any of -msse, -msse2 or -mfpmath=sse makes configure err with:
ERROR: Unknown compiler or compiler not supported.

Why might that be?

Comment 16

[Henri Sivonen (:hsivonen)]

Attachment: Show a dialog using GTK3, v4

I'm giving up on changing the build defaults in this bug. This patch assumes that bug 1274196 will end up adding -msse -msse2 -mfpmath=sse to CFLAGS and CXXFLAGS on 32-bit x86 Linux for Mozilla-official builds.

r? gps for the build system stuff and r? karlt for the gtk3 usage.

I don't have 32-bit x86 hardware without SSE2 and I was unable to make VirtualBox fake the absence of SSE2. It would be good if someone with access to actual 32-bit x86 hardware without SSE2 could test that the try build https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-0a7ddd08830da643e24f6838ef06673e2f2a3139/try-linux/firefox-51.0a1.en-US.linux-i686.tar.bz2 actually shows the error dialog.

Comment 17

[Gregory Szorc [:gps]]

Comment on attachment 8792308 [details] [diff] [review]
Show a dialog using GTK3, v4

Review of attachment 8792308 [details] [diff] [review]:
-----------------------------------------------------------------

I left a comment. But I'm going to redirect this review to glandium since it touches configure and I'm not sure what state configure is in to know if this change should go in moz.configure instead.

::: browser/app/Makefile.in
@@ +32,5 @@
> +CFLAGS := $(filter-out -msse,$(CFLAGS))
> +CXXFLAGS := $(filter-out -mfpmath=sse,$(CXXFLAGS))
> +CFLAGS := $(filter-out -mfpmath=sse,$(CFLAGS))
> +CFLAGS += -mno-sse -mno-sse2 -mfpmath=387
> +CXXFLAGS += -mno-sse -mno-sse2 -mfpmath=387

I'm not a fan of doing this here. I'd rather we set a variable in configure containing the flags to support non-SSE configurations.

Comment 18

[Karl Tomlinson (:karlt)]

Comment on attachment 8792308 [details] [diff] [review]
Show a dialog using GTK3, v4

gtk_init(nullptr, nullptr) and gtk_window_new(GTK_WINDOW_TOPLEVEL) instead of
gtk_application_* would avoid the need for connecting to the "activate"
signal, but what you have here should work fine.

>+  (void) g_application_run(G_APPLICATION(app), 0, nullptr);

Not clear why this cast is necessary, but no space between the cast and the
operand.

Comment 19

[Mike Hommey [:glandium]]

Comment on attachment 8792308 [details] [diff] [review]
Show a dialog using GTK3, v4

Review of attachment 8792308 [details] [diff] [review]:
-----------------------------------------------------------------

This can't work this way. The firefox executable contains libmozglue and jemalloc, all of which would still be built with SSE if we were to default to SSE. You can be sure that jemalloc *is* going to be invoked to display those gtk dialogs. Building all the things that end up in the firefox executable without SSE is also not very appealing, because the list of those things varies. Also, that would be a bummer.

I think an approach that would have more success would be to build a separate executable that is built without jemalloc, without libmozglue, and without anything else, and that does the Gtk heavy lifting. And that executable would be called with execve() first thing in main(), if CPUID says the CPU doesn't support SSE. That still keeps an open door for static initializers using SSE crashing firefox before this can be called, though. A little more security would be added if the CPUID/execve check happened in a static initializer. Obviously, the static initializer would need to be guaranteed non-SSE... putting it in a separate file and compiling /that/ file with flags to disable SSE would help. I don't remember in what order ld puts the static initializers, but it should also be possible to trick it into putting that static initializer first.

(This is where GNU/Linux really sucks. Some architectures have additional ELF headers that indicate the platform/ABI compatibility, but even if that existed for SSE on x86, nothing actually cares about it to report it nicely to users. So application developers that care need to do it on their own, and it's a major PITA)

Comment 20

[Mike Hommey [:glandium]]

Also note that the switch from Gtk+2 to Gtk+3 meant that people with Gtk+2 but not Gtk+3 installed got a non starting Firefox. As in, they'd click on the icon and Firefox wouldn't show up. Only if they ran it from the command line would they see something like:

XPCOMGlueLoad error for file libmozgtk.so:
libgtk-3.so.0: cannot open shared object file: No such file or directory
Couldn't load XPCOM.

(https://support.mozilla.org/en-US/questions/1121133)

Tangent: Your patch, adding a Gtk+3 dependency on the firefox executable would also change this behavior to, instead, display:
firefox: error while loading shared libraries: libgtk-3.so.0: cannot open shared object file: No such file or directory.

Which is not very different, but still different.

Anyways, the point is that we've already gone through a painful-ish user experience. People running Firefox on computers without SSE2 are more than probably very much fewer than those running Firefox on a computer without Gtk+3 installed. So I'm willing to question whether showing a Gtk+ dialog is worth the effort all things considered. What I'm trying to say is that while it *is* nicer to fail early instead of randomly depending on where the first SSE2 the program will try to execute has been put by the compiler, I'm not at all convinced that conveying that failure via a Gtk+ dialog is /that/ helpful, when we've made Firefox not start on way more systems without such a dialog, only a message in a terminal.

Comment 21

[Ralph Giles (:rillian)]

I'm fine with the argument that this isn't worth doing, and we should WONTFIX the bug, but that we did a terrible job with this in previous transitions is no argument for doing a terrible job this time.

If we expect static initializers to crash us, I think the only thing we can do here is a wrapper which checks before invoking the actual firefox.

Comment 22

[Mike Hommey [:glandium]]

Note that if SSE instructions *are* generated in static initializers in the firefox executable before any attempt at doing it gracefully, then it's a win, because it would happen early, not at an inconvenient time. But nothing guarantees it would happen.

I'm also not saying this should be WONTFIX. I'm saying displaying a *Gtk* dialog box for it is probably not worth the effort, and that a fprintf(stderr) would, IMHO, be good enough.

> but that we did a terrible job with this in previous transitions is no argument for doing a terrible job this time.

Note that I only mentioned the last iteration of that (being Gtk), but essentially every single such transition has been handled in a similar way (requirement bumps for glib, glibc, libstdc++, etc.). They don't happen often, and we usually avoid them when we can. When they do happen, they only affect a small population. Arguably, those populations, even small, were larger than the population that will be affected by a drop of non-SSE2. *BUT* these previous instances would fail on non-supported systems in a more controlled way, not at the will of the compiler.

Comment 23

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=7a8ccf979bd2

Comment 24

[Henri Sivonen (:hsivonen)]

Attachment: Print an error to stderr

(In reply to Mike Hommey [:glandium] from comment #19)
> This can't work this way. The firefox executable contains libmozglue and
> jemalloc, all of which would still be built with SSE if we were to default
> to SSE. You can be sure that jemalloc *is* going to be invoked to display
> those gtk dialogs.
(In reply to Mike Hommey [:glandium] from comment #22)
> I'm also not saying this should be WONTFIX. I'm saying displaying a *Gtk*
> dialog box for it is probably not worth the effort, and that a
> fprintf(stderr) would, IMHO, be good enough.

Should we expect fprintf not to involve jemalloc, e.g. for buffering? Here's an fprintf version.

Sadly, users who click a launcher icon in their desktop environment won't see the stderr message.

Comment 25

[Henri Sivonen (:hsivonen)]

Comment on attachment 8797165 [details] [diff] [review]
Print an error to stderr

Review of attachment 8797165 [details] [diff] [review]:
-----------------------------------------------------------------

::: old-configure.in
@@ +5226,5 @@
> +dnl ========================================================
> +
> +case "$target" in
> +i?86-*-linux*)
> +    if test "$MOZ_WIDGET_TOOLKIT" = gtk3; then

Note to self: Remove the toolkit check.

Comment 26

[Mike Hommey [:glandium]]

(In reply to Henri Sivonen (:hsivonen) from comment #24)
> Created attachment 8797165 [details] [diff] [review]
> Print an error to stderr
> 
> (In reply to Mike Hommey [:glandium] from comment #19)
> > This can't work this way. The firefox executable contains libmozglue and
> > jemalloc, all of which would still be built with SSE if we were to default
> > to SSE. You can be sure that jemalloc *is* going to be invoked to display
> > those gtk dialogs.
> (In reply to Mike Hommey [:glandium] from comment #22)
> > I'm also not saying this should be WONTFIX. I'm saying displaying a *Gtk*
> > dialog box for it is probably not worth the effort, and that a
> > fprintf(stderr) would, IMHO, be good enough.
> 
> Should we expect fprintf not to involve jemalloc, e.g. for buffering? Here's
> an fprintf version.

Mmmm true. Use write()?

> Sadly, users who click a launcher icon in their desktop environment won't
> see the stderr message.

cf. second half of comment 22

Comment 27

[Henri Sivonen (:hsivonen)]

Attachment: write() an error to stderr

Comment 28

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45b358eef94b

Comment 29

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=2d8fce8ce724

Comment 30

[Henri Sivonen (:hsivonen)]

Attachment: write() an error to stderr, v2

Comment 31

[Henri Sivonen (:hsivonen)]

Attachment: write() an error to stderr, v3

Comment 32

[Henri Sivonen (:hsivonen)]

ajones, could you, please, find someone who has Linux on real non-SSE2 x86 hardware and who'd be willing to test if https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-1a2ac537f0df17fcdf33675c1787e1a3f18e4e6e/try-linux/firefox-52.0a1.en-US.linux-i686.tar.bz2 outputs a message about SSE2 to stderr and exits?

(As noted earlier, I don't have access to such hardware and was unable to fake it with VirtualBox.)

Comment 33

[Алексей Шилин]

(In reply to Henri Sivonen (:hsivonen) from comment #32)
> ajones, could you, please, find someone who has Linux on real non-SSE2 x86
> hardware and who'd be willing to test if
> https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> 1a2ac537f0df17fcdf33675c1787e1a3f18e4e6e/try-linux/firefox-52.0a1.en-US.
> linux-i686.tar.bz2 outputs a message about SSE2 to stderr and exits?
> 
> (As noted earlier, I don't have access to such hardware and was unable to
> fake it with VirtualBox.)

I've tested it on non-SSE2 hardware and it works like expected: prints the message and quits.

Comment 34

[Henri Sivonen (:hsivonen)]

Comment on attachment 8798376 [details] [diff] [review]
write() an error to stderr, v3

(In reply to Алексей Шилин from comment #33)
> (In reply to Henri Sivonen (:hsivonen) from comment #32)
> > ajones, could you, please, find someone who has Linux on real non-SSE2 x86
> > hardware and who'd be willing to test if
> > https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> > 1a2ac537f0df17fcdf33675c1787e1a3f18e4e6e/try-linux/firefox-52.0a1.en-US.
> > linux-i686.tar.bz2 outputs a message about SSE2 to stderr and exits?
> > 
> > (As noted earlier, I don't have access to such hardware and was unable to
> > fake it with VirtualBox.)
> 
> I've tested it on non-SSE2 hardware and it works like expected: prints the
> message and quits.

Thank you for testing!

Comment 35

[Mike Hommey [:glandium]]

Comment on attachment 8798376 [details] [diff] [review]
write() an error to stderr, v3

Review of attachment 8798376 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/app/Makefile.in
@@ +25,5 @@
>  
> +# If we are trying to show an error dialog about the lack of SSE2 support,
> +# make sure that code itself doesn't use SSE2.
> +ifdef MOZ_LINUX_32_SSE2_STARTUP_ERROR
> +CXXFLAGS := $(filter-out -msse2,$(CXXFLAGS))

You can do it all in one pass: $(filter-out -msse2 -msse -mfpmath=sse,...)

@@ +31,5 @@
> +CXXFLAGS := $(filter-out -msse,$(CXXFLAGS))
> +CFLAGS := $(filter-out -msse,$(CFLAGS))
> +CXXFLAGS := $(filter-out -mfpmath=sse,$(CXXFLAGS))
> +CFLAGS := $(filter-out -mfpmath=sse,$(CFLAGS))
> +CFLAGS += -mno-sse -mno-sse2 -mfpmath=387

It seems to me you could add -march=pentiumpro here and change the -march in the mozconfig to e.g. pentium-m.

::: browser/app/nsBrowserApp.cpp
@@ +49,5 @@
> +static bool
> +IsSSE2Available()
> +{
> +  // The rest of the app has been compiled to assume that SSE2 is present
> +  // unconditionally, so we can't use SSE.h to query for SSE2 support. Instead,

actually... you could... if you included the cpp file (wrapping it in an anonymous namespace to avoid symbol contamination)

@@ +79,5 @@
> +  // point in trying to recover from errors.
> +  MOZ_UNUSED(write(STDERR_FILENO,
> +                   sSSE2Message,
> +                   MOZ_ARRAY_LENGTH(sSSE2Message) - 1));
> +  MOZ_UNUSED(close(STDERR_FILENO));

why close? it's not fwrite, you don't need buffers to be flushed with fflush or fclose.

@@ +401,5 @@
>  {
> +#ifdef MOZ_LINUX_32_SSE2_STARTUP_ERROR
> +  // Doing this before the startup time stamp, so that we don't need to worry
> +  // about an SSE2 instruction getting introduced by the compiler into
> +  // mozilla::TimeStampt::Now() somehow.

typo

It would be even better in a static initializer, as mentioned in a previous comment. You can either use a class constructor and a static class instance, or a __attribute__((constructor)) function.

::: old-configure.in
@@ +5227,5 @@
> +
> +case "$target" in
> +i?86-*-linux*)
> +    if $CC $CFLAGS -E -dM -</dev/null | grep -q __SSE2__; then
> +        AC_DEFINE(MOZ_LINUX_32_SSE2_STARTUP_ERROR)

Adding the define in browser/app/moz.build would be better.

@@ +5229,5 @@
> +i?86-*-linux*)
> +    if $CC $CFLAGS -E -dM -</dev/null | grep -q __SSE2__; then
> +        AC_DEFINE(MOZ_LINUX_32_SSE2_STARTUP_ERROR)
> +        MOZ_LINUX_32_SSE2_STARTUP_ERROR=1
> +        AC_SUBST(MOZ_LINUX_32_SSE2_STARTUP_ERROR)

I think this would be better as an opt-in from the mozconfig. So you'd only need an AC_SUBST here, and to export MOZ_LINUX_32_SSE2_STARTUP_ERROR from our 32-bits linux mozconfig.

Comment 36

[Henri Sivonen (:hsivonen)]

Attachment: Use an ELF constructor to perform the check

Comment 37

[Henri Sivonen (:hsivonen)]

Attachment: Use an ELF constructor to perform the check, v2

Comment 38

[Henri Sivonen (:hsivonen)]

Comment on attachment 8802484 [details] [diff] [review]
Use an ELF constructor to perform the check, v2

(In reply to Mike Hommey [:glandium] from comment #35)
> You can do it all in one pass: $(filter-out -msse2 -msse -mfpmath=sse,...)

Done.

> It seems to me you could add -march=pentiumpro here and change the -march in
> the mozconfig to e.g. pentium-m.

Done. I moved -march from $CC/$CXX to $CFLAGS/$CXXFLAGS to be able to filter it out here.

> ::: browser/app/nsBrowserApp.cpp
> @@ +49,5 @@
> > +static bool
> > +IsSSE2Available()
> > +{
> > +  // The rest of the app has been compiled to assume that SSE2 is present
> > +  // unconditionally, so we can't use SSE.h to query for SSE2 support. Instead,
> 
> actually... you could... if you included the cpp file (wrapping it in an
> anonymous namespace to avoid symbol contamination)

I've not done that, because SSE.cpp caches the results, which is wasteful considering that we need only the SSE2 value and only once. It seems less messy to just inline the 9 lines here. Since the way CPUID works on legacy processors won't change, it seems like there isn't a good maintainability argument for not duplicating the intrinsic invocation. That is, the probability of this function needing maintenance before we either require SSE3 or just retire the check is close to zero.

> @@ +79,5 @@
> > +  // point in trying to recover from errors.
> > +  MOZ_UNUSED(write(STDERR_FILENO,
> > +                   sSSE2Message,
> > +                   MOZ_ARRAY_LENGTH(sSSE2Message) - 1));
> > +  MOZ_UNUSED(close(STDERR_FILENO));
> 
> why close? it's not fwrite, you don't need buffers to be flushed with fflush
> or fclose.

Removed.

> @@ +401,5 @@
> >  {
> > +#ifdef MOZ_LINUX_32_SSE2_STARTUP_ERROR
> > +  // Doing this before the startup time stamp, so that we don't need to worry
> > +  // about an SSE2 instruction getting introduced by the compiler into
> > +  // mozilla::TimeStampt::Now() somehow.
> 
> typo
> 
> It would be even better in a static initializer, as mentioned in a previous
> comment. You can either use a class constructor and a static class instance,
> or a __attribute__((constructor)) function.

Switched to __attribute__((constructor)).

I'm not sure if it's ok to exit() from a constructor, so I set a boolean and did and early return from main().

> ::: old-configure.in
> @@ +5227,5 @@
> > +
> > +case "$target" in
> > +i?86-*-linux*)
> > +    if $CC $CFLAGS -E -dM -</dev/null | grep -q __SSE2__; then
> > +        AC_DEFINE(MOZ_LINUX_32_SSE2_STARTUP_ERROR)
> 
> Adding the define in browser/app/moz.build would be better.

Done.

> @@ +5229,5 @@
> > +i?86-*-linux*)
> > +    if $CC $CFLAGS -E -dM -</dev/null | grep -q __SSE2__; then
> > +        AC_DEFINE(MOZ_LINUX_32_SSE2_STARTUP_ERROR)
> > +        MOZ_LINUX_32_SSE2_STARTUP_ERROR=1
> > +        AC_SUBST(MOZ_LINUX_32_SSE2_STARTUP_ERROR)
> 
> I think this would be better as an opt-in from the mozconfig. So you'd only
> need an AC_SUBST here, and to export MOZ_LINUX_32_SSE2_STARTUP_ERROR from
> our 32-bits linux mozconfig.

Done.

Comment 39

[Henri Sivonen (:hsivonen)]

Attachment: Use an ELF constructor to perform the check, v3

Comment 40

[Henri Sivonen (:hsivonen)]

exit() from an __attribute__((constructor)) function seems to work, so I switched to that after all.

Comment 41

[Henri Sivonen (:hsivonen)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a4ad0aaf352c

Comment 42

[Mike Hommey [:glandium]]

Comment on attachment 8802495 [details] [diff] [review]
Use an ELF constructor to perform the check, v3

Review of attachment 8802495 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/app/nsBrowserApp.cpp
@@ +50,5 @@
> +static bool
> +IsSSE2Available()
> +{
> +  // The rest of the app has been compiled to assume that SSE2 is present
> +  // unconditionally, so we can't use SSE.h to query for SSE2 support. Instead,

Note that my comment in the previous review didn't mean you had to do it, but you should adjust the comment here, because, as I said, you could, so it's not the reason to do what you're doing here anymore.

@@ +81,5 @@
> +  // point in trying to recover from errors.
> +  MOZ_UNUSED(write(STDERR_FILENO,
> +                   sSSE2Message,
> +                   MOZ_ARRAY_LENGTH(sSSE2Message) - 1));
> +  exit(255);

_exit would be even better (directly exits the process, without running any destructors)

Comment 43

[Mike Hommey [:glandium]]

Comment on attachment 8802495 [details] [diff] [review]
Use an ELF constructor to perform the check, v3

Review of attachment 8802495 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/app/Makefile.in
@@ +25,5 @@
>  
> +# If we are trying to show an error dialog about the lack of SSE2 support,
> +# make sure that code itself doesn't use SSE2.
> +ifdef MOZ_LINUX_32_SSE2_STARTUP_ERROR
> +CFLAGS := $(filter-out -march=pentium-m -msse -msse2 -mfpmath=sse,$(CFLAGS))

Afterthoughts: -march=% than -march=pentium-m would be better, and you don't need to touch CFLAGS, only CXXFLAGS. Although it would be better to something like:
CXXFLAGS := $(filter-out ...,$(CXXFLAGS))
CXX := $(filter-out ...,$(CXX))

That would cover cases touching either variable.

Comment 44

[Henri Sivonen (:hsivonen)]

Attachment: Use an ELF constructor to perform the check,4

Comment 45

[Henri Sivonen (:hsivonen)]

Алексей Шилин,

Could you, please, test the build 
https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-c1c72d1b910ba5d1f98395d2eb3cc65788020262/try-linux/firefox-52.0a1.en-US.linux-i686.tar.bz2
on non-SSE2 hardware to check that it still prints the message?

(This is to verify that addressing the review comments didn't accidentally break the intended behavior.)

Comment 46

[Алексей Шилин]

(In reply to Henri Sivonen (:hsivonen) from comment #45)
> Алексей Шилин,
> 
> Could you, please, test the build 
> https://archive.mozilla.org/pub/firefox/try-builds/[e-mail]-
> c1c72d1b910ba5d1f98395d2eb3cc65788020262/try-linux/firefox-52.0a1.en-US.
> linux-i686.tar.bz2
> on non-SSE2 hardware to check that it still prints the message?
> 
> (This is to verify that addressing the review comments didn't accidentally
> break the intended behavior.)

Sorry, didn't have time to test it earlier.

Yes, it still prints the message and quits i.e. works like expected.

Comment 47

[Henri Sivonen (:hsivonen)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/09d9c83ae3a9477ce74f3ab62da8e8d0bbae0f2a
Bug 1300843 - Print an error on 32-bit Linux in the absence of SSE2. r=glandium.

Comment 48

[Henri Sivonen (:hsivonen)]

(In reply to Алексей Шилин from comment #46)
> Yes, it still prints the message and quits i.e. works like expected.

Thank you. Landed.

The patch landed here doesn't do anything on its own. I'll coordinate with releng on the timing to land bug 1274196.

Comment 49

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/09d9c83ae3a9

Comment 50

[Gerry Chang [:gchang]]

We've built 51 RC. Mark 51 won't fix.