Startup crash [@ _sk_start_pipeline_sse2]

RESOLVED FIXED in Firefox 60

Status

()

defect
RESOLVED FIXED
Last year
Last year

People

(Reporter: jbeich, Assigned: jbeich)

Tracking

({crash, regression})

Trunk
mozilla61
x86
FreeBSD
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox-esr52 unaffected, firefox59 unaffected, firefox60 fixed, firefox61 fixed)

Details

Attachments

(1 attachment, 2 obsolete attachments)

59 bytes, text/x-review-board-request
lsalzman
: review+
Details
mozilla-unified changeset 998a57bd05ad snapshot built by Clang 5.0.1 on FreeBSD 10.3 i386. Doesn't affect amd64.

$ rm -rf objdir/tmp/scratch_user; mkdir -p objdir/tmp/scratch_user
$ echo 'user_pref("browser.tabs.remote.autostart", false);' >>objdir/tmp/scratch_user/user.js
$ gdb --args objdir/dist/bin/firefox -no-remote -profile objdir/tmp/scratch_user
[...]
Thread 1 received signal SIGBUS, Bus error.
0x3155e265 in _sk_start_pipeline_sse2 () from objdir/dist/bin/libxul.so
(gdb) backtrace
#0  0x3155e265 in _sk_start_pipeline_sse2 ()
   from objdir/dist/bin/libxul.so
#1  0x31769c03 in SkRasterPipeline::run (this=0xffff74fc, x=0, y=0, w=1, h=1)
    at gfx/skia/skia/src/jumper/SkJumper.cpp:481
#2  0x31f47a2d in SkRasterPipelineBlitter::Create (dst=..., paint=..., alloc=0xffff7494, shaderPipeline=...,
    burstCtx=0x0, is_opaque=true, is_constant=true)
    at gfx/skia/skia/src/core/SkRasterPipelineBlitter.cpp:198
#3  0x31f47295 in SkCreateRasterPipelineBlitter (dst=..., paint=..., ctm=..., alloc=0xffff7494)
    at gfx/skia/skia/src/core/SkRasterPipelineBlitter.cpp:101
#4  0x3157ba27 in SkBlitter::Choose (device=..., matrix=..., origPaint=..., alloc=0xffff7494, drawCoverage=false)
    at gfx/skia/skia/src/core/SkBlitter.cpp:1001
#5  0x31b2cee0 in SkAutoBlitterChoose::choose (this=0xffff7490, dst=..., matrix=..., paint=...,
    drawCoverage=false) at gfx/skia/skia/src/core/SkAutoBlitterChoose.h:34
#6  0x31b23ba8 in SkDraw::drawDevPath (this=0xffff83a8, devPath=..., paint=..., drawCoverage=false,
    customBlitter=0x0, doFill=true, iData=0x0) at gfx/skia/skia/src/core/SkDraw.cpp:969
#7  0x31b25708 in SkDraw::drawPath (this=0xffff83a8, origSrcPath=..., origPaint=..., prePathMatrix=0x0,
    pathIsMutable=false, drawCoverage=false, customBlitter=0x0, iData=0x0)
    at gfx/skia/skia/src/core/SkDraw.cpp:1141
#8  0x3165fcc9 in SkDraw::drawPath (this=0xffff83a8, path=..., paint=..., prePathMatrix=0x0, pathIsMutable=false)
    at gfx/skia/skia/src/core/SkDraw.h:58
#9  0x318bd0fe in SkBitmapDevice::drawPath (this=0x420f9800, path=..., paint=..., prePathMatrix=0x0,
    pathIsMutable=false) at gfx/skia/skia/src/core/SkBitmapDevice.cpp:226
#10 0x318e0823 in SkCanvas::onDrawPath (this=0x4206d000, path=..., paint=...)
    at gfx/skia/skia/src/core/SkCanvas.cpp:2170
#11 0x318dd07f in SkCanvas::drawPath (this=0x4206d000, path=..., paint=...)
    at gfx/skia/skia/src/core/SkCanvas.cpp:1760
#12 0x2c639811 in mozilla::gfx::DrawTargetSkia::Fill (this=0x41eba580, aPath=0x36032f20, aPattern=...,
    aOptions=...) at gfx/2d/DrawTargetSkia.cpp:976
#13 0x2cda8dbd in CreateBoxShadow (aDestDrawTarget=0x41c3fa50, aMinSize=..., aCornerRadii=0xffff8fb8,
    aBlurRadius=..., aShadowColor=..., aMirrorCorners=true, aOutBlurMargin=...)
    at gfx/thebes/gfxBlur.cpp:544
#14 0x2cd8529e in GetBlur (aDestinationCtx=0x420801c0, aRectSize=..., aBlurRadius=..., aCornerRadii=0xffff8fb8,
    aShadowColor=..., aMirrorCorners=true, aOutBlurMargin=..., aOutSlice=..., aOutMinSize=...)
    at gfx/thebes/gfxBlur.cpp:612
#15 0x2cd848cb in gfxAlphaBoxBlur::BlurRectangle (aDestinationCtx=0x420801c0, aRect=...,
    aCornerRadii=0xffff8fb8, aBlurStdDev=..., aShadowColor=..., aDirtyRect=..., aSkipRect=...)
    at gfx/thebes/gfxBlur.cpp:963
#16 0x30a6e9e0 in nsContextBoxBlur::BlurRectangle (aDestinationCtx=0x420801c0, aRect=...,
    aAppUnitsPerDevPixel=60, aCornerRadii=0xffff8fb8, aBlurRadius=240, aShadowColor=..., aDirtyRect=...,
    aSkipRect=...) at layout/painting/nsCSSRendering.cpp:4601
#17 0x30a6d171 in nsCSSRendering::PaintBoxShadowOuter (aPresContext=0x3fb9ae00, aRenderingContext=...,
    aForFrame=0x41a60ab8, aFrameArea=..., aDirtyRect=..., aOpacity=1)
    at layout/painting/nsCSSRendering.cpp:1771
#18 0x30aaabf2 in nsDisplayBoxShadowOuter::Paint (this=0x41f8d190, aBuilder=0xffffa628, aCtx=0x420801c0)
    at layout/painting/nsDisplayList.cpp:5835
#19 0x30a5ecf8 in mozilla::FrameLayerBuilder::PaintItems (this=0x420cd940, aItems=..., aRect=...,
    aContext=0x420801c0, aBuilder=0xffffa628, aPresContext=0x3fb9ae00, aOffset=..., aXScale=1, aYScale=1,
    aCommonClipCount=0) at layout/painting/FrameLayerBuilder.cpp:6097
#20 0x30a5f80c in mozilla::FrameLayerBuilder::DrawPaintedLayer (aLayer=0x37481600, aContext=0x420801c0,
    aRegionToDraw=..., aDirtyRegion=..., aClip=mozilla::layers::DrawRegionClip::DRAW, aRegionToInvalidate=...,
    aCallbackData=0xffffa628) at layout/painting/FrameLayerBuilder.cpp:6254
#21 0x2cb792a3 in mozilla::layers::ClientPaintedLayer::PaintThebes (this=0x37481600, aReadbackUpdates=0xffff99c0)
    at gfx/layers/client/ClientPaintedLayer.cpp:158
#22 0x2cb7a30c in mozilla::layers::ClientPaintedLayer::RenderLayerWithReadback (this=0x37481600,
    aReadback=0xffff9a30) at gfx/layers/client/ClientPaintedLayer.cpp:314
#23 0x2cb7a37b in non-virtual thunk to mozilla::layers::ClientPaintedLayer::RenderLayerWithReadback(mozilla::layers::ReadbackProcessor*) ()
    at objdir/dist/include/mozilla/layers/LayersTypes.h:291
#24 0x2cb9f16b in mozilla::layers::ClientContainerLayer::RenderLayer (this=0x41cefa00)
    at gfx/layers/client/ClientContainerLayer.h:58
#25 0x2cb9f30e in non-virtual thunk to mozilla::layers::ClientContainerLayer::RenderLayer() ()
    at objdir/dist/include/mozilla/layers/ContentClient.h:257
#26 0x2cb7644a in mozilla::layers::ClientLayerManager::EndTransactionInternal (this=0x37490520,
, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*)>, aCallbackData=0xffffa628) at gfx/layers/client/ClientLayerManager.cpp:359
#27 0x2cb768af in mozilla::layers::ClientLayerManager::EndTransaction (this=0x37490520,
    aCallback=0x30a5efa0 <mozilla::FrameLayerBuilder::DrawPaintedLayer(mozilla::layers::PaintedLayer*, gfxContext*, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, mozilla::layers::DrawRegionClip, mozilla::gfx::IntRegionTyped<mozilla::gfx::UnknownUnits> const&, void*)>, aCallbackData=0xffffa628, aFlags=mozilla::layers::LayerManager::END_DEFAULT)
    at gfx/layers/client/ClientLayerManager.cpp:423
#28 0x30a999e2 in nsDisplayList::PaintRoot (this=0xffffa610, aBuilder=0xffffa628, aCtx=0x0, aFlags=13)
    at layout/painting/nsDisplayList.cpp:2779
#29 0x305bdc2d in nsLayoutUtils::PaintFrame (aRenderingContext=0x0, aFrame=0x40f79010, aDirtyRegion=...,
    aBackstop=4293388520, aBuilderMode=nsDisplayListBuilderMode::PAINTING,
    aFlags=(nsLayoutUtils::PaintFrameFlags::PAINT_WIDGET_LAYERS | nsLayoutUtils::PaintFrameFlags::PAINT_EXISTING_TRANSACTION | nsLayoutUtils::PaintFrameFlags::PAINT_NO_COMPOSITE))
    at layout/base/nsLayoutUtils.cpp:4016
#30 0x305209a2 in mozilla::PresShell::Paint (this=0x28892800, aViewToPaint=0x3fbfeb80, aDirtyRegion=...,
    aFlags=1) at layout/base/PresShell.cpp:6435
#31 0x2ffc08bb in nsViewManager::ProcessPendingUpdatesPaint (this=0x400656a0, aWidget=0x3a3a2640)
    at view/nsViewManager.cpp:480
#32 0x2ffc03a8 in nsViewManager::ProcessPendingUpdatesForView (this=0x400656a0, aView=0x3fbfeb80,
    aFlushDirtyRegion=true) at view/nsViewManager.cpp:412
#33 0x2ffc17e4 in nsViewManager::ProcessPendingUpdates (this=0x400656a0)
    at view/nsViewManager.cpp:1102
#34 0x304cc230 in nsRefreshDriver::Tick (this=0x3a21c600, aNowEpoch=1521761518002803, aNowTime=...)
    at layout/base/nsRefreshDriver.cpp:2064
#35 0x304d48ae in mozilla::RefreshDriverTimer::TickDriver (driver=0x3a21c600, jsnow=1521761518002803, now=...)
    at layout/base/nsRefreshDriver.cpp:341
#36 0x304d458f in mozilla::RefreshDriverTimer::TickRefreshDrivers (this=0x3a20d1c0, aJsNow=1521761518002803,
    aNow=..., aDrivers=...) at layout/base/nsRefreshDriver.cpp:311
#37 0x304d43a6 in mozilla::RefreshDriverTimer::Tick (this=0x3a20d1c0, jsnow=1521761518002803, now=...)
    at layout/base/nsRefreshDriver.cpp:333
#38 0x304d786f in mozilla::VsyncRefreshDriverTimer::RunRefreshDrivers (this=0x3a20d1c0, aTimeStamp=...)
    at layout/base/nsRefreshDriver.cpp:774
#39 0x304d6874 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::TickRefreshDriver (
    this=0x3a207920, aVsyncTimestamp=...) at layout/base/nsRefreshDriver.cpp:687
#40 0x304d2dd8 in mozilla::VsyncRefreshDriverTimer::RefreshDriverVsyncObserver::ParentProcessVsyncNotifier::Run (
    this=0x3cc0e180) at layout/base/nsRefreshDriver.cpp:533
#41 0x2aaf42fe in nsThread::ProcessNextEvent (this=0x2884a5e0, aMayWait=false, aResult=0xffffcc61)
    at xpcom/threads/nsThread.cpp:1040
#42 0x2ab1f054 in NS_ProcessNextEvent (aThread=0x2884a5e0, aMayWait=false)
    at xpcom/threads/nsThreadUtils.cpp:517
#43 0x2b6b5b1e in mozilla::ipc::MessagePump::Run (this=0x288ccbb0, aDelegate=0x2880f480)
    at ipc/glue/MessagePump.cpp:97
#44 0x2b5ad481 in MessageLoop::RunInternal (this=0x2880f480)
    at ipc/chromium/src/base/message_loop.cc:326
#45 0x2b5ad406 in MessageLoop::RunHandler (this=0x2880f480)
    at ipc/chromium/src/base/message_loop.cc:319
#46 0x2b5ad3bb in MessageLoop::Run (this=0x2880f480)
    at ipc/chromium/src/base/message_loop.cc:299
#47 0x3002a8e2 in nsBaseAppShell::Run (this=0x3a13fe20) at widget/nsBaseAppShell.cpp:157
#48 0x32b99de9 in nsAppStartup::Run (this=0x28b9a070)
    at toolkit/components/startup/nsAppStartup.cpp:290
#49 0x32d1aec7 in XREMain::XRE_mainRun (this=0xffffd480) at toolkit/xre/nsAppRunner.cpp:4727
#50 0x32d1c2ab in XREMain::XRE_main (this=0xffffd480, argc=4, argv=0xffffdafc, aConfig=...)
    at toolkit/xre/nsAppRunner.cpp:4872
#51 0x32d1cbd7 in XRE_main (argc=4, argv=0xffffdafc, aConfig=...)
    at toolkit/xre/nsAppRunner.cpp:4964
#52 0x32d35fd2 in mozilla::BootstrapImpl::XRE_main (this=0x288153e0, argc=4, argv=0xffffdafc, aConfig=...)
    at toolkit/xre/Bootstrap.cpp:49
#53 0x0804e305 in do_main (argc=4, argv=0xffffdafc, envp=0xffffdb10)
    at browser/app/nsBrowserApp.cpp:231
#54 0x0804dde4 in main (argc=4, argv=0xffffdafc, envp=0xffffdb10)
    at browser/app/nsBrowserApp.cpp:304
(gdb) disassemble
Dump of assembler code for function _sk_start_pipeline_sse2:
   0x3155e220 <+0>:     push   %ebp
   0x3155e221 <+1>:     mov    %esp,%ebp
   0x3155e223 <+3>:     push   %ebx
   0x3155e224 <+4>:     push   %edi
   0x3155e225 <+5>:     push   %esi
   0x3155e226 <+6>:     sub    $0x5c,%esp
   0x3155e229 <+9>:     mov    0xc(%ebp),%eax
   0x3155e22c <+12>:    cmp    0x14(%ebp),%eax
   0x3155e22f <+15>:    jae    0x3155e2d6 <_sk_start_pipeline_sse2+182>
   0x3155e235 <+21>:    mov    0x10(%ebp),%edx
   0x3155e238 <+24>:    mov    0x8(%ebp),%ecx
   0x3155e23b <+27>:    mov    0x18(%ebp),%esi
   0x3155e23e <+30>:    mov    (%esi),%edi
   0x3155e240 <+32>:    add    $0x4,%esi
   0x3155e243 <+35>:    lea    0x4(%ecx),%ecx
   0x3155e246 <+38>:    mov    %ecx,-0x10(%ebp)
   0x3155e249 <+41>:    cmp    %edx,-0x10(%ebp)
   0x3155e24c <+44>:    mov    0x8(%ebp),%ecx
   0x3155e24f <+47>:    mov    %ecx,-0x68(%ebp)
   0x3155e252 <+50>:    mov    %eax,-0x14(%ebp)
   0x3155e255 <+53>:    mov    %eax,-0x64(%ebp)
   0x3155e258 <+56>:    movl   $0x0,-0x60(%ebp)
   0x3155e25f <+63>:    lea    -0x58(%ebp),%eax
   0x3155e262 <+66>:    xorps  %xmm0,%xmm0
=> 0x3155e265 <+69>:    movaps %xmm0,0x30(%eax)
   0x3155e269 <+73>:    movaps %xmm0,0x20(%eax)
   0x3155e26d <+77>:    movaps %xmm0,0x10(%eax)
   0x3155e271 <+81>:    movaps %xmm0,(%eax)
   0x3155e274 <+84>:    mov    %ecx,%eax
   0x3155e276 <+86>:    lea    -0x68(%ebp),%ebx
   0x3155e279 <+89>:    ja     0x3155e2a4 <_sk_start_pipeline_sse2+132>
   0x3155e27b <+91>:    sub    $0x8,%esp
   0x3155e27e <+94>:    xorps  %xmm0,%xmm0
   0x3155e281 <+97>:    xorps  %xmm1,%xmm1
   0x3155e284 <+100>:   xorps  %xmm2,%xmm2
   0x3155e287 <+103>:   xorps  %xmm3,%xmm3
   0x3155e28a <+106>:   push   %esi
   0x3155e28b <+107>:   push   %ebx
   0x3155e28c <+108>:   call   *%edi
   0x3155e28e <+110>:   mov    0x10(%ebp),%edx
   0x3155e291 <+113>:   add    $0x10,%esp
   0x3155e294 <+116>:   mov    -0x68(%ebp),%ecx
   0x3155e297 <+119>:   lea    0x4(%ecx),%eax
   0x3155e29a <+122>:   mov    %eax,-0x68(%ebp)
   0x3155e29d <+125>:   add    $0x8,%ecx
   0x3155e2a0 <+128>:   cmp    %edx,%ecx
   0x3155e2a2 <+130>:   jbe    0x3155e27b <_sk_start_pipeline_sse2+91>
   0x3155e2a4 <+132>:   mov    %edx,%ecx
   0x3155e2a6 <+134>:   sub    %eax,%ecx
   0x3155e2a8 <+136>:   je     0x3155e2c9 <_sk_start_pipeline_sse2+169>
   0x3155e2aa <+138>:   mov    %ecx,-0x60(%ebp)
   0x3155e2ad <+141>:   sub    $0x8,%esp
   0x3155e2b0 <+144>:   xorps  %xmm0,%xmm0
   0x3155e2b3 <+147>:   xorps  %xmm1,%xmm1
   0x3155e2b6 <+150>:   xorps  %xmm2,%xmm2
   0x3155e2b9 <+153>:   xorps  %xmm3,%xmm3
   0x3155e2bc <+156>:   push   %esi
   0x3155e2bd <+157>:   lea    -0x68(%ebp),%eax
   0x3155e2c0 <+160>:   push   %eax
   0x3155e2c1 <+161>:   call   *%edi
   0x3155e2c3 <+163>:   mov    0x10(%ebp),%edx
   0x3155e2c6 <+166>:   add    $0x10,%esp
   0x3155e2c9 <+169>:   mov    -0x14(%ebp),%eax
   0x3155e2cc <+172>:   inc    %eax
   0x3155e2cd <+173>:   cmp    0x14(%ebp),%eax
   0x3155e2d0 <+176>:   jne    0x3155e249 <_sk_start_pipeline_sse2+41>
   0x3155e2d6 <+182>:   add    $0x5c,%esp
   0x3155e2d9 <+185>:   pop    %esi
   0x3155e2da <+186>:   pop    %edi
   0x3155e2db <+187>:   pop    %ebx
   0x3155e2dc <+188>:   pop    %ebp
   0x3155e2dd <+189>:   ret
End of assembler dump.
(gdb) info registers
eax            0xffff6b68       -38040
ecx            0x0      0
edx            0x1      1
ebx            0x35eeb000       904835072
esp            0xffff6b58       0xffff6b58
ebp            0xffff6bc0       0xffff6bc0
esi            0xffff6c18       -37864
edi            0x3155e491       827712657
eip            0x3155e265       0x3155e265 <_sk_start_pipeline_sse2+69>
eflags         0x210206 [ PF IF RF ID ]
cs             0x33     51
ss             0x3b     59
ds             <unavailable>
es             <unavailable>
fs             <unavailable>
gs             <unavailable>

https://searchfox.org/mozilla-central/source/gfx/skia/skia/src/jumper/SkJumper_generated.S#36148-36219
https://skia.googlesource.com/skia/+log/chrome/m59..chrome/m66/src/jumper/SkJumper_generated.S
When built by GCC 6.4.0 it also crashes in SSE2 code but unrelated to Skia:
https://pastebin.mozilla.org/9080697
Dimitry, does this look related to https://lists.freebsd.org/pipermail/freebsd-current/2012-November/037563.html ?
If you want to try reproduce downstream, see https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=226476
Flags: needinfo?(dimitry)
It looks like Skia expects the i386 ABI to respect the fact that the stack is 16 byte aligned. And FreeBSD i386 does not seem to respect this? Is there any way to alter the FreeBSD build flags for Firefox to accommodate this?
Flags: needinfo?(jbeich)
Posted patch workaround (obsolete) — Splinter Review
-mstack-alignment=16 should be enough (like in FFmpeg) but only together with -mstackrealign actually helped. Clang defaults to 4-byte alignment but most platforms except BSDs switched to 16-byte, following GCC.

https://reviews.llvm.org/diffusion/L/browse/llvm/tags/RELEASE_600/final/lib/Target/X86/X86Subtarget.cpp;328295$243-245
Flags: needinfo?(jbeich)
Attachment #8961659 - Flags: feedback?(lsalzman)
(In reply to Jan Beich from comment #4)
> Created attachment 8961659 [details] [diff] [review]
> workaround
> 
> -mstack-alignment=16 should be enough (like in FFmpeg) but only together
> with -mstackrealign actually helped. Clang defaults to 4-byte alignment but
> most platforms except BSDs switched to 16-byte, following GCC.

You mean, most platforms simply broke their existing SysV ABI, without any regard for backwards compatibility. :)

Note that FreeBSD ports also patches ffmpeg to use -mstackrealign, because of the same issue:

https://svnweb.freebsd.org/ports/head/multimedia/ffmpeg/files/patch-configure?revision=452570&view=markup
Flags: needinfo?(dimitry)
(In reply to Dimitry Andric from comment #5)
> (In reply to Jan Beich from comment #4)
> > Created attachment 8961659 [details] [diff] [review]
> > workaround
> > 
> > -mstack-alignment=16 should be enough (like in FFmpeg) but only together
> > with -mstackrealign actually helped. Clang defaults to 4-byte alignment but
> > most platforms except BSDs switched to 16-byte, following GCC.
> 
> You mean, most platforms simply broke their existing SysV ABI, without any
> regard for backwards compatibility. :)

That's called "cleaning up old cruft and really fixing past mistakes" ;)
Comment on attachment 8961659 [details] [diff] [review]
workaround

I like this as a solution since SkJumper.cpp is the single jumping-off point into the assembly that has the alignment constraints, -mstackrealign neatly bridges us to it without a lot of fuss. Also avoids having to adjust build flags on Firefox as a whole. Ship it!
Attachment #8961659 - Flags: review+
Attachment #8961659 - Flags: feedback?(lsalzman)
Attachment #8961659 - Flags: feedback+
Dimitry, do other Clang platforms like Android or Windows that don't explicitly set stackAlignment = 16 need -mstackrealign as well?
Flags: needinfo?(dimitry)
(In reply to Jan Beich from comment #8)
> Dimitry, do other Clang platforms like Android or Windows that don't
> explicitly set stackAlignment = 16 need -mstackrealign as well?

The default stackAlignment is indeed set to 4, and is only set to 16 when the target is Darwin, Linux, Solaris or kFreeBSD, or when in 64-bit mode.  For Windows 32-bit, the default alignment will also be 4 bytes.  I don't know what Microsoft's own compilers do here.

For Android, it appears that the target triples are of the form "cpu-linux-android*", e.g. "arm-linux-androideabi" or "i686-linux-android", so in that case it will still be identified as Linux (which it of course is).  Therefore its stack alignment will be 16 bytes too, even for 32-bit.
Flags: needinfo?(dimitry)
Posted patch workaround (obsolete) — Splinter Review
Thanks. I was confused by isTargetLinux vs. isTargetAndroid.

As the previous patch was fine this version only improves wording:
- Expand inline comment to note where the target exclusion list originates
- Adjust commit message to imply the code calling SSE2 was realigned

Keeping r+ from comment 7.
Attachment #8961659 - Attachment is obsolete: true
Keywords: checkin-needed
(In reply to Jan Beich from comment #11)
> Created attachment 8961977 [details] [diff] [review]
> workaround
> 
> Thanks. I was confused by isTargetLinux vs. isTargetAndroid.
> 
> As the previous patch was fine this version only improves wording:
> - Expand inline comment to note where the target exclusion list originates
> - Adjust commit message to imply the code calling SSE2 was realigned
> 
> Keeping r+ from comment 7.

I think it would be simpler here if we just got rid of the OS_TARGET condition. Just always using -mstack-alignment=16 and -mstackrealign if we're doing the x86/clang combo should be harmless.
Keywords: checkin-needed
Comment on attachment 8962014 [details]
Bug 1448189 - Realign stack before using SSE2 in Skia.

Changes since attachment 8961977 [details] [diff] [review]:
- Addressed comment 11
Attachment #8961977 - Attachment is obsolete: true
Comment on attachment 8962014 [details]
Bug 1448189 - Realign stack before using SSE2 in Skia.

https://reviewboard.mozilla.org/r/230856/#review236346
Attachment #8962014 - Flags: review?(lsalzman) → review+
FWIW (I'm not a reviewer), Jan's fix looks good to me.
Keywords: checkin-needed
Assignee: nobody → jbeich
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/afbeca340d12
Realign stack before using SSE2 in Skia. r=lsalzman
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/afbeca340d12
Status: NEW → RESOLVED
Closed: Last year
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1448654
Pushed by lsalzman@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ec40368b843a
follow-up - Don't use -mstackrealign with clang-cl. r=jbeich
Comment on attachment 8962014 [details]
Bug 1448189 - Realign stack before using SSE2 in Skia.

Approval Request Comment
[Feature/Bug causing the regression]: bug 1444506
[User impact if declined]: Firefox crashes on FreeBSD/OpenBSD x86.
[Is this code covered by automated tests?]: Yes, crashtests but not on Tier3 platforms.
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: No
[List of other uplifts needed for the feature/fix]: mozilla-central changeset ec40368b843a
[Is the change risky?]: No
[Why is the change risky/not risky?]: Can only break build or fail crashtests on Android x86 or Linux x86 ASAN in case of a compiler bug.
[String changes made/needed]: None
Attachment #8962014 - Flags: approval-mozilla-beta?
Duplicate of this bug: 1448654
No longer depends on: 1448654
Comment on attachment 8962014 [details]
Bug 1448189 - Realign stack before using SSE2 in Skia.

fix stack alignment on bsd with clang, beta60+
Attachment #8962014 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.