Bug 229235 – REGRESSION(r279256): Crash in JSC::FTL::saveAllRegisters

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Michael Catanzaro]

Unfortunately I've had to downgrade WebKitGTK 2.33.3 (r281074) -> 2.33.2 (r278597) in the GNOME runtime due to a FTL crash. This occurs 100% of the time when loading any article on https://arstechnica.com/. It also occurs 100% when loading chat rooms in https://gnome.element.io/.

(gdb) bt
#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:49
#1  0x00007ff13f048867 in __GI_abort () at abort.c:79
#2  0x00007ff13d6ef668 in std::__replacement_assert(char const*, int, char const*, char const*)
    (__file=<optimized out>, __line=<optimized out>, __function=<optimized out>, __condition=<optimized out>)
    at /usr/include/c++/11.1.0/x86_64-unknown-linux-gnu/bits/c++config.h:504
#3  0x00007ff13df0c63c in std::array<unsigned int, 1ul>::operator[](unsigned long) const (this=0x7fef83ac91d0, __n=0)
    at /usr/include/c++/11.1.0/array:193
#4  std::array<unsigned int, 1ul>::operator[](unsigned long) const (__n=<optimized out>, this=<optimized out>)
    at /usr/include/c++/11.1.0/array:193
#5  WTF::Bitmap<32ul, unsigned int>::get(unsigned long, WTF::Dependency) const
    (dependency=..., n=<optimized out>, this=<optimized out>) at WTF/Headers/wtf/Bitmap.h:164
#6  JSC::RegisterSet::get(JSC::Reg) const (reg=..., this=<optimized out>)
    at ../Source/JavaScriptCore/jit/RegisterSet.h:99
#7  JSC::FTL::(anonymous namespace)::Regs::Regs (this=<optimized out>)
    at ../Source/JavaScriptCore/ftl/FTLSaveRestore.cpp:83
#8  JSC::FTL::saveAllRegisters(JSC::AssemblyHelpers&, char*)
    (jit=..., scratchMemory=scratchMemory@entry=0x7ff1368609c8 "\360\206a\304\360\177")
    at ../Source/JavaScriptCore/ftl/FTLSaveRestore.cpp:111
#9  0x00007ff13df14071 in JSC::FTL::genericGenerationThunkGenerator(JSC::VM&, JSC::FunctionPtr<(WTF::PtrTag)1>, char const*, unsigned int, JSC::FTL::FrameAndStackAdjustmentRequirement, JSC::PtrTag)
    (vm=<optimized out>, generationFunction=..., name=<optimized out>, extraPopsToRestore=<optimized out>, frameAndStackAdjustmentRequirement=(unknown: 0x83ac92c0), resultTag=<optimized out>)
    at ../Source/JavaScriptCore/ftl/FTLThunks.cpp:81
#10 0x00007ff13df14a26 in JSC::FTL::osrExitGenerationThunkGenerator(JSC::VM&) (vm=<optimized out>)
    at WTF/Headers/wtf/PtrTag.h:308
#11 0x00007ff13e0af39e in operator() (__closure=<synthetic pointer>)
    at ../Source/JavaScriptCore/jit/JITThunks.cpp:170
#12 JSC::JITThunks::ctiStubImpl<JSC::JITThunks::ctiStub(JSC::VM&, JSC::ThunkGenerator)::<lambda()> >(JSC::ThunkGenerator, struct {...}) (this=0x7ff136aabf28, key=<optimized out>, generateThunk=...)
    at ../Source/JavaScriptCore/jit/JITThunks.cpp:159
#13 0x00007ff13e0af5c2 in JSC::JITThunks::ctiStub(JSC::VM&, JSC::MacroAssemblerCodeRef<(WTF::PtrTag)26129> (*)(JSC::VM&)) (this=<optimized out>, vm=<optimized out>, generator=<optimized out>)
    at ../Source/JavaScriptCore/jit/JITThunks.cpp:169
#14 0x00007ff13e527f52 in JSC::VM::getCTIStub(JSC::MacroAssemblerCodeRef<(WTF::PtrTag)26129> (*)(JSC::VM&))
    (this=<optimized out>, generator=<optimized out>) at /usr/include/c++/11.1.0/bits/unique_ptr.h:173
#15 0x00007ff13defce84 in operator() (linkBuffer=<optimized out>, __closure=<optimized out>)
    at ../Source/JavaScriptCore/ftl/FTLOSRExitHandle.cpp:52
#16 WTF::SharedTaskFunctor<void(JSC::LinkBuffer&), JSC::FTL::OSRExitHandle::emitExitThunk(JSC::FTL::State&, JSC::CCallHelpers&)::<lambda(JSC::LinkBuffer&)> >::run(JSC::LinkBuffer &) (this=0x7ff0446795a0, arguments#0=...)
    at WTF/Headers/wtf/SharedTask.h:91
#17 0x00007ff13d786900 in JSC::LinkBuffer::performFinalization() (this=this@entry=0x7ff084007fa0)
    at ../Source/JavaScriptCore/assembler/LinkBuffer.cpp:476
#18 0x00007ff13d786979 in JSC::LinkBuffer::finalizeCodeWithoutDisassemblyImpl() (this=0x7ff084007fa0)
    at ../Source/JavaScriptCore/assembler/LinkBuffer.cpp:71
#19 0x00007ff13df033f8 in JSC::LinkBuffer::finalizeCodeWithoutDisassembly<(WTF::PtrTag)357>() (this=<optimized out>)
    at ../Source/JavaScriptCore/assembler/LinkBuffer.h:310
#20 JSC::FTL::link(JSC::FTL::State&) (state=...) at ../Source/JavaScriptCore/ftl/FTLLink.cpp:165
#21 0x00007ff13dc47903 in JSC::DFG::Plan::compileInThreadImpl() (this=0x7ff084043c40)
    at ../Source/JavaScriptCore/dfg/DFGPlan.cpp:476
#22 0x00007ff13e08be76 in JSC::JITPlan::compileInThread(JSC::JITWorklistThread*)
    (this=0x7ff084043c40, thread=thread@entry=0x7ff084418cf0) at ../Source/JavaScriptCore/jit/JITPlan.cpp:165
#23 0x00007ff13e0dad8a in JSC::JITWorklistThread::work() (this=0x7ff084418cf0)
    at ../Source/JavaScriptCore/jit/JITWorklistThread.cpp:123
#24 0x00007ff13e767a39 in operator() (__closure=<optimized out>) at ../Source/WTF/wtf/AutomaticThread.cpp:229
#25 WTF::Detail::CallableWrapper<WTF::AutomaticThread::start(const WTF::AbstractLocker&)::<lambda()>, void>::call(void--Type <RET> for more, q to quit, c to continue without paging--
) (this=0x7ff084091a50) at ../Source/WTF/wtf/Function.h:53
#26 0x00007ff13e78ad5e in WTF::Function<void ()>::operator()() const (this=<synthetic pointer>)
    at ../Source/WTF/wtf/Function.h:79
#27 WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) (newThreadContext=0x7ff0840923f0)
    at ../Source/WTF/wtf/Threading.cpp:187
#28 0x00007ff13e7e76dd in WTF::wtfThreadEntryPoint(void*) (context=<optimized out>)
    at ../Source/WTF/wtf/posix/ThreadingPOSIX.cpp:241
#29 0x00007ff13cff53ba in start_thread (arg=0x7fef83acb640) at pthread_create.c:481
#30 0x00007ff13f127ad3 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

I'll attach a full backtrace as well.

Unfortunately I am *not* able to reproduce this crash with my local build of WebKit trunk outside flatpak. I'll try building r281074 to see if I can reproduce there. I can only bisect it if I can reproduce it in my own local build.

[Michael Catanzaro]

Created attachment 435763 [details]
Full backtrace

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #0) 
> Unfortunately I am *not* able to reproduce this crash with my local build of
> WebKit trunk outside flatpak. I'll try building r281074 to see if I can
> reproduce there. I can only bisect it if I can reproduce it in my own local
> build.

Unfortunately my local build of r281074 works fine. :S

[Michael Catanzaro]

Workaround: the crashes go away when I set JSC_useFTLJIT=0.

[Michael Catanzaro]

Oh and there is this console output:

/usr/include/c++/11.1.0/array:196: constexpr const value_type& std::array<_Tp, _Nm>::operator[](std::array<_Tp, _Nm>::size_type) const [with _Tp = unsigned int; long unsigned int _Nm = 1; std::array<_Tp, _Nm>::const_reference = const unsigned int&; std::array<_Tp, _Nm>::size_type = long unsigned int]: Assertion '__n < this->size()' failed.

We can see from the backtrace that __n is 0, so presumably the array's size must be 0 as well.

[Fabian Bornschein]

I experience the same thing and get the same error

Webkit version 2.33.3 (from https://webkitgtk.org/releases/webkitgtk-2.33.3.tar.xz)
+ https://bugs.webkit.org/show_bug.cgi?id=229152#c18 patch applied

Build options:
    -DPORT=GTK \
    -DCMAKE_BUILD_TYPE=Release \
    -DCMAKE_INSTALL_PREFIX=/usr \
    -DCMAKE_INSTALL_LIBDIR=lib \
    -DCMAKE_INSTALL_LIBEXECDIR=lib \
    -DCMAKE_SKIP_RPATH=ON \
    -DENABLE_GTKDOC=ON \
    -DENABLE_MINIBROWSER=ON \
    -DUSE_SOUP2=ON

Happen to me on any mastodon timeline, as well as the given sites https://arstechnica.com/, https://gnome.element.io/

The line 

/usr/include/c++/11.1.0/array:196: constexpr const value_type& std::array<_Tp, _Nm>::operator[](std::array<_Tp, _Nm>::size_type) const [with _Tp = unsigned int; long unsigned int _Nm = 1; std::array<_Tp, _Nm>::const_reference = const unsigned int&; std::array<_Tp, _Nm>::size_type = long unsigned int]: Assertion '__n < this->size()' failed.

also appears.

2.33.2 (also from the tar.xz) works just fine

[Michael Catanzaro]

Hi Fabian, what distribution are you using?

[Fabian Bornschein]

Ahh sorry. It's on Arch Linux for me.

my bt
==================================================

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:49
#1  0x00007fbfe89ab862 in __GI_abort () at abort.c:79
#2  0x00007fbfe6e0caca in std::__replacement_assert(char const*, int, char const*, char const*) (__file=<optimized out>, __line=<optimized out>, __function=<optimized out>, __condition=<optimized out>) at /usr/include/c++/11.1.0/x86_64-pc-linux-gnu/bits/c++config.h:504
#3  0x00007fbfe776fb2c in std::array<unsigned int, 1ul>::operator[](unsigned long) const (__n=<optimized out>, this=<optimized out>) at /usr/include/c++/11.1.0/array:196
#4  WTF::Bitmap<32ul, unsigned int>::get(unsigned long, WTF::Dependency) const (dependency=..., n=<optimized out>, this=<optimized out>) at /usr/src/debug/build/WTF/Headers/wtf/Bitmap.h:164
#5  JSC::RegisterSet::get(JSC::Reg) const (reg=..., this=<optimized out>) at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/jit/RegisterSet.h:99
#6  JSC::FTL::(anonymous namespace)::Regs::Regs (this=<optimized out>) at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/ftl/FTLSaveRestore.cpp:83
#7  JSC::FTL::saveAllRegisters(JSC::AssemblyHelpers&, char*) (jit=..., scratchMemory=scratchMemory@entry=0x7fbfe026e688 "pC_,\277\177") at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/ftl/FTLSaveRestore.cpp:111
#8  0x00007fbfe7778f8c in JSC::FTL::genericGenerationThunkGenerator(JSC::VM&, JSC::FunctionPtr<(WTF::PtrTag)1>, char const*, unsigned int, JSC::FTL::FrameAndStackAdjustmentRequirement, JSC::PtrTag)
    (vm=<optimized out>, generationFunction=..., name=<optimized out>, extraPopsToRestore=<optimized out>, frameAndStackAdjustmentRequirement=(unknown: 0xd37fc300), resultTag=<optimized out>) at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/ftl/FTLThunks.cpp:81
#9  0x00007fbfe7779a86 in JSC::FTL::osrExitGenerationThunkGenerator(JSC::VM&) (vm=<optimized out>) at /usr/src/debug/build/WTF/Headers/wtf/PtrTag.h:308
#10 0x00007fbfe7952796 in operator() (__closure=<synthetic pointer>) at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/jit/JITThunks.cpp:170
#11 JSC::JITThunks::ctiStubImpl<JSC::JITThunks::ctiStub(JSC::VM&, JSC::ThunkGenerator)::<lambda()> >(JSC::ThunkGenerator, struct {...}) (this=0x7fbfe04aaf28, key=<optimized out>, generateThunk=...) at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/jit/JITThunks.cpp:159
#12 0x00007fbfe79529d2 in JSC::JITThunks::ctiStub(JSC::VM&, JSC::MacroAssemblerCodeRef<(WTF::PtrTag)26129> (*)(JSC::VM&)) (this=<optimized out>, vm=<optimized out>, generator=<optimized out>) at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/jit/JITThunks.cpp:169
#13 0x00007fbfe7e828a3 in JSC::VM::getCTIStub(JSC::MacroAssemblerCodeRef<(WTF::PtrTag)26129> (*)(JSC::VM&)) (this=<optimized out>, generator=<optimized out>) at /usr/include/c++/11.1.0/bits/unique_ptr.h:173
#14 0x00007fbfe775ee65 in operator() (linkBuffer=<optimized out>, __closure=<optimized out>) at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/ftl/FTLOSRExitHandle.cpp:52
#15 WTF::SharedTaskFunctor<void(JSC::LinkBuffer&), JSC::FTL::OSRExitHandle::emitExitThunk(JSC::FTL::State&, JSC::CCallHelpers&)::<lambda(JSC::LinkBuffer&)> >::run(JSC::LinkBuffer &) (this=0x7fbef4017630, arguments#0=...) at /usr/src/debug/build/WTF/Headers/wtf/SharedTask.h:91
#16 0x00007fbfe6eb1a70 in JSC::LinkBuffer::performFinalization() (this=this@entry=0x7fbf5c640960) at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/assembler/LinkBuffer.cpp:476
#17 0x00007fbfe6eb1ae9 in JSC::LinkBuffer::finalizeCodeWithoutDisassemblyImpl() (this=0x7fbf5c640960) at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/assembler/LinkBuffer.cpp:71
#18 0x00007fbfe7766a0c in JSC::LinkBuffer::finalizeCodeWithoutDisassembly<(WTF::PtrTag)357>() (this=<optimized out>) at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/assembler/LinkBuffer.h:310
#19 JSC::FTL::link(JSC::FTL::State&) (state=...) at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/ftl/FTLLink.cpp:165
#20 0x00007fbfe743539d in JSC::DFG::Plan::compileInThreadImpl() (this=0x7fbed0434380) at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/dfg/DFGPlan.cpp:476
#21 0x00007fbfe7929019 in JSC::JITPlan::compileInThread(JSC::JITWorklistThread*) (this=0x7fbed0434380, thread=thread@entry=0x7fbf5c676a68) at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/jit/JITPlan.cpp:165
#22 0x00007fbfe797f28d in JSC::JITWorklistThread::work() (this=0x7fbf5c676a68) at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/jit/JITWorklistThread.cpp:123
#23 0x00007fbfe80e4471 in operator() (__closure=0x7fbf3440b038) at /usr/src/debug/webkitgtk-2.33.3/Source/WTF/wtf/AutomaticThread.cpp:229
#24 WTF::Detail::CallableWrapper<WTF::AutomaticThread::start(const WTF::AbstractLocker&)::<lambda()>, void>::call(void) (this=0x7fbf3440b030) at /usr/src/debug/webkitgtk-2.33.3/Source/WTF/wtf/Function.h:53
#25 0x00007fbfe810bd23 in WTF::Function<void ()>::operator()() const (this=<synthetic pointer>) at /usr/src/debug/webkitgtk-2.33.3/Source/WTF/wtf/Function.h:79
#26 WTF::Thread::entryPoint(WTF::Thread::NewThreadContext*) (newThreadContext=0x7fbf5c676318) at /usr/src/debug/webkitgtk-2.33.3/Source/WTF/wtf/Threading.cpp:187
#27 0x00007fbfe817e45e in WTF::wtfThreadEntryPoint(void*) (context=<optimized out>) at /usr/src/debug/webkitgtk-2.33.3/Source/WTF/wtf/posix/ThreadingPOSIX.cpp:241
#28 0x00007fbfe6755259 in start_thread (arg=0x7fbed37fe640) at pthread_create.c:481
#29 0x00007fbfe8a835e3 in clone () at ../sysdeps/unix/sysv/linux/x86_64/clone.S:95

[Carlos Garcia Campos]

I see gcc 11.1 in both cases, maybe it's related.

[Zan Dobersek]

> constexpr const value_type& std::array<_Tp, _Nm>::operator[](std::array<_Tp, _Nm>::size_type) const [with _Tp = unsigned int; long unsigned int _Nm = 1; std::array<_Tp, _Nm>::const_reference = const unsigned int&; std::array<_Tp, _Nm>::size_type = long unsigned int]: Assertion '__n < this->size()' failed

This is an access into a std::array<unsigned int, 1>, as determined in WTF::Bitmap. Per backtrace, __n should be 0, but for whatever reason the assertion still pops off.

The assertion in std::array's operator[] is new for libstdc++ 11.1.0, it's not used in previous versions.

[Zan Dobersek]

With GCC 10, when an OOB assert is placed in Bitmap<>::get(), it's triggered later in FTL::saveAllRegisters(), when saving all the FPR registers. When passing the MacroAssembler::lastRegister() value to Regs::nextFPRegister(), this method immediately collects the index value of the next FP register, which ends up being 32, which causes access beyond the bounds of Bitmap's std::array<>.

An additional check in Regs::nextFPRegister() would fix that:

```
        while (next <= MacroAssembler::lastFPRegister() && special.get(next))
            next = MacroAssembler::nextFPRegister(next);
```

But this doesn't exactly match the backtraces.

[Fabian Bornschein]

(In reply to Carlos Garcia Campos from comment #8)
> I see gcc 11.1 in both cases, maybe it's related.

I can confirm this. GCC10 rebuild works fine for me.

[Adrian Perez]

I just built trunk today (r281492) on Arch Linux with GCC 11.1.0 and I cannot
hit this issue anymore. Maybe some of the commits that touched JSC recently
have had the side effect of fixing this?

[Adrian Perez]

(In reply to Adrian Perez from comment #12)
> I just built trunk today (r281492) on Arch Linux with GCC 11.1.0 and I cannot
> hit this issue anymore. Maybe some of the commits that touched JSC recently
> have had the side effect of fixing this?

I went ahead and tested a build of WebKitGTK 2.33.3 — still no crash, I've been
using the MiniBrowser to read Ars Technica, and chat using Element for half an
hour, so I suppose I should have hit the problem by now ¯\_(ツ)_/¯

[Radar WebKit Bug Importer]

<rdar://problem/82337517>

[Michael Catanzaro]

(In reply to Adrian Perez from comment #13)
> I went ahead and tested a build of WebKitGTK 2.33.3 — still no crash, I've
> been
> using the MiniBrowser to read Ars Technica, and chat using Element for half
> an
> hour, so I suppose I should have hit the problem by now ¯\_(ツ)_/¯

Yes, this crash occurs immediately when using either page. My guess is it occurs whenever FTL tier is reached, but that's just a blind guess.

[Fabian Bornschein]

(In reply to Adrian Perez from comment #13)
> I went ahead and tested a build of WebKitGTK 2.33.3 — still no crash, I've
> been

Unfortunately I'm on a limited connection and unable to checkout the full webkit repo to see if trunk is fine.

@Adrian If you have some time, this is my PKGBUILD where, after build and installing, I hit the problem:
https://gitlab.com/fabis_cafe/gnome-unstable/-/tree/webkit2gtk_b/webkit2gtk

[Michael Catanzaro]

Thanks to Zan's investigation, I guessed that I might be able to reproduce if I did: -DCMAKE_CXX_FLAGS="-D_GLIBCXX_ASSERTIONS"

And I guessed correctly! So I will bisect this now.

[Michael Catanzaro]

I'm on the last stages of my bisect. It probably broke in r279256. Will update the bug title after I've confirmed for sure.

[Yusuke Suzuki]

(In reply to Michael Catanzaro from comment #18)
> I'm on the last stages of my bisect. It probably broke in r279256. Will
> update the bug title after I've confirmed for sure.

@Michael Can you reproduce this crash after https://trac.webkit.org/changeset/280578/webkit ?

[Michael Catanzaro]

(In reply to Yusuke Suzuki from comment #19)
> @Michael Can you reproduce this crash after
> https://trac.webkit.org/changeset/280578/webkit ?

Yes, that commit predates 2.33.3.

(In reply to Michael Catanzaro from comment #18)
> I'm on the last stages of my bisect. It probably broke in r279256. Will
> update the bug title after I've confirmed for sure.

I've narrowed the first bad commit down to r279254-r279263. This is a small enough range that I'm very confident it will turn out to be r279256 "Use ldp and stp more for saving / restoring registers on ARM64." It's the only JSC commit in this range, and it notably touches FTL::saveAllRegisters.

[Michael Catanzaro]

(In reply to Michael Catanzaro from comment #20)
> I'm very confident it will turn out to be r279256

Confirmed.

[Zan Dobersek]

(In reply to Adrian Perez from comment #12)
> I just built trunk today (r281492) on Arch Linux with GCC 11.1.0 and I cannot
> hit this issue anymore. Maybe some of the commits that touched JSC recently
> have had the side effect of fixing this?

For the assertion to actually trigger, the `-D_GLIBCXX_ASSERTIONS` define is required in CFLAGS and CXXFLAGS. With that, I can reproduce the libstdc++ assert on a GCC 11 build with yesterday's main.

[Zan Dobersek]

Created attachment 436769 [details]
Patch

[Zan Dobersek]

(In reply to Zan Dobersek from comment #23)
> Created attachment 436769 [details]
> Patch

I was able to reproduce the problem on a JetStream run, for a JSCOnly build. The last-register check I described before fixed the problem, so this patch applies that check for both GPRReg and FPRReg iteration.

[Michael Catanzaro]

Comment on attachment 436769 [details]
Patch

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

Thank you, Zan!

> Source/JavaScriptCore/ftl/FTLSaveRestore.cpp:93
>      GPRReg nextRegister(GPRReg current)
>      {
>          auto next = MacroAssembler::nextRegister(current);
> -        while (special.get(next))
> +        while (next <= MacroAssembler::lastRegister() && special.get(next))
>              next = MacroAssembler::nextRegister(next);
>          return next;
>      }

Hi Mark, does this look OK?

[Yusuke Suzuki]

Comment on attachment 436769 [details]
Patch

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

>> Source/JavaScriptCore/ftl/FTLSaveRestore.cpp:93
>>      }
> 
> Hi Mark, does this look OK?

This fix seems not OK to me since we are returning non-valid register if we are reaching to MacroAssembler::lastRegister() (and if it is special register).
Can you investigate what is happening here? Why are we exhausting the registers?
The pasted backtrace is,

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:49
#1  0x00007fbfe89ab862 in __GI_abort () at abort.c:79
#2  0x00007fbfe6e0caca in std::__replacement_assert(char const*, int, char const*, char const*) (__file=<optimized out>, __line=<optimized out>, __function=<optimized out>, __condition=<optimized out>) at /usr/include/c++/11.1.0/x86_64-pc-linux-gnu/bits/c++config.h:504
#3  0x00007fbfe776fb2c in std::array<unsigned int, 1ul>::operator[](unsigned long) const (__n=<optimized out>, this=<optimized out>) at /usr/include/c++/11.1.0/array:196
#4  WTF::Bitmap<32ul, unsigned int>::get(unsigned long, WTF::Dependency) const (dependency=..., n=<optimized out>, this=<optimized out>) at /usr/src/debug/build/WTF/Headers/wtf/Bitmap.h:164
#5  JSC::RegisterSet::get(JSC::Reg) const (reg=..., this=<optimized out>) at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/jit/RegisterSet.h:99
#6  JSC::FTL::(anonymous namespace)::Regs::Regs (this=<optimized out>) at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/ftl/FTLSaveRestore.cpp:83
#7  JSC::FTL::saveAllRegisters(JSC::AssemblyHelpers&, char*) (jit=..., scratchMemory=scratchMemory@entry=0x7fbfe026e688 "pC_,\277\177") at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/ftl/FTLSaveRestore.cpp:111


So, in FTLSaveRestore.cpp:111, we define

Regs regs;

and this Regs constructor is doing,

   77     Regs()
   78     {
   79         special = RegisterSet::stackRegisters();
   80         special.merge(RegisterSet::reservedHardwareRegisters());
   81
   82         first = MacroAssembler::firstRegister();
   83         while (special.get(first))
   84             first = MacroAssembler::nextRegister(first);
   85     }

from the code, this is impossible to get exceeding first register since it is `MacroAssembler::firstRegister()`.
And we are not including all registers into special registers. So MacroAssember::nextRegister must succeed.

[Yusuke Suzuki]

Comment on attachment 436769 [details]
Patch

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

>>> Source/JavaScriptCore/ftl/FTLSaveRestore.cpp:93
>>>      }
>> 
>> Hi Mark, does this look OK?
> 
> This fix seems not OK to me since we are returning non-valid register if we are reaching to MacroAssembler::lastRegister() (and if it is special register).
> Can you investigate what is happening here? Why are we exhausting the registers?
> The pasted backtrace is,
> 
> #0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:49
> #1  0x00007fbfe89ab862 in __GI_abort () at abort.c:79
> #2  0x00007fbfe6e0caca in std::__replacement_assert(char const*, int, char const*, char const*) (__file=<optimized out>, __line=<optimized out>, __function=<optimized out>, __condition=<optimized out>) at /usr/include/c++/11.1.0/x86_64-pc-linux-gnu/bits/c++config.h:504
> #3  0x00007fbfe776fb2c in std::array<unsigned int, 1ul>::operator[](unsigned long) const (__n=<optimized out>, this=<optimized out>) at /usr/include/c++/11.1.0/array:196
> #4  WTF::Bitmap<32ul, unsigned int>::get(unsigned long, WTF::Dependency) const (dependency=..., n=<optimized out>, this=<optimized out>) at /usr/src/debug/build/WTF/Headers/wtf/Bitmap.h:164
> #5  JSC::RegisterSet::get(JSC::Reg) const (reg=..., this=<optimized out>) at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/jit/RegisterSet.h:99
> #6  JSC::FTL::(anonymous namespace)::Regs::Regs (this=<optimized out>) at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/ftl/FTLSaveRestore.cpp:83
> #7  JSC::FTL::saveAllRegisters(JSC::AssemblyHelpers&, char*) (jit=..., scratchMemory=scratchMemory@entry=0x7fbfe026e688 "pC_,\277\177") at /usr/src/debug/webkitgtk-2.33.3/Source/JavaScriptCore/ftl/FTLSaveRestore.cpp:111
> 
> 
> So, in FTLSaveRestore.cpp:111, we define
> 
> Regs regs;
> 
> and this Regs constructor is doing,
> 
>    77     Regs()
>    78     {
>    79         special = RegisterSet::stackRegisters();
>    80         special.merge(RegisterSet::reservedHardwareRegisters());
>    81
>    82         first = MacroAssembler::firstRegister();
>    83         while (special.get(first))
>    84             first = MacroAssembler::nextRegister(first);
>    85     }
> 
> from the code, this is impossible to get exceeding first register since it is `MacroAssembler::firstRegister()`.
> And we are not including all registers into special registers. So MacroAssember::nextRegister must succeed.

Let's consider about the state when this function is called.
first should be eax because of `static constexpr RegisterID firstRegister() { return X86Registers::eax; }`. And then, special must not include eax since this is not a reserved registers.
So... why is `MacroAssembler::nextRegister` called?

[Michael Catanzaro]

The problem is RegisterSet::reservedHardwareRegisters is returning an empty RegisterSet, so "special" only contains rsp and rbp (returned by RegisterSet::stackRegisters).

I tried this debug:

RegisterSet reserved = RegisterSet::reservedHardwareRegisters();
reserved.forEach([](const Reg& reg) { WTFLogAlways("%s", reg.debugName()); });

and it printed nothing.

Whereas:

special.forEach([](const Reg& reg) { WTFLogAlways("%s", reg.debugName()); });

prints "rsp" followed by "rbp", unchanged both before and after this line:

special.merge(RegisterSet::reservedHardwareRegisters());

So next step is to debug RegisterSet::reservedHardwareRegisters.

[Zan Dobersek]

The backtraces pasted here so far are faulty, here's one from a non-optimized build:

#0  __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:50
#1  0x00007ffff75a3537 in __GI_abort () at abort.c:79
#2  0x0000555555626b19 in std::__replacement_assert () at /usr/include/x86_64-linux-gnu/c++/11/bits/c++config.h:514
#3  0x00005555556d91dd in std::array<unsigned int, 1ul>::operator[] () at /usr/include/c++/11/array:196
#4  0x00005555556d3c42 in WTF::Bitmap<32ul, unsigned int>::get () at WTF/Headers/wtf/Bitmap.h:164
#5  0x00005555556cc3ad in JSC::RegisterSet::get () at /work/webkit/git/Source/JavaScriptCore/jit/RegisterSet.h:99
#6  0x0000555556f0af46 in nextFPRegister () at /work/webkit/git/Source/JavaScriptCore/ftl/FTLSaveRestore.cpp:98
#7  0x0000555556f0b128 in JSC::FTL::saveAllRegisters () at /work/webkit/git/Source/JavaScriptCore/ftl/FTLSaveRestore.cpp:142

The problem comes from the FPR storing loop at the end, specifically when handling the MacroAssembler::lastFPRegister() value. At the end of that loop, Regs::nextFPRegister() is called with that value.

But immediately, Regs::nextFPRegister() retrieves the MacroAssembler::nextFPRegister() value of that last register, and calls RegisterSet::get() with that FPRegister.

In raw numbers, MacroAssembler::lastFPRegister() is FPRegisterID(15), MacroAssembler::nextFPRegister() for that value returns FPRegisterID(16). RegisterSet::get() implicitly converts that FPRegisterID to a Reg object, which internally holds index of 32 (the 16 that's the FPRegisterID's value that's additionally offset by the count of existing GP registers, which is also 16). RegisterSet::get() then uses that index value to access into the zero-indexed Bitmap (and the underlying one-sized std::array<uint32_t>) at which point the assert happens because of the out-of-bounds access.

So the additional condition in Regs::nextRegister() and Regs::nextFPRegister() would prevent accessing into RegisterSet beyond the valid register range. For Regs::nextRegister() call with MacroAssembler::lastRegister() (= RegisterID(15)), RegisterSet::get() is called with RegisterID(16), but the Reg object for that register ID ends up with an index of 16, which doesn't go out of the Bitmap's bounds, but it does end up querying the state belonging to FPRegisterID(0). Whereas for Regs::nextFPRegister() and FPRegisterID(15), things go sideways.

[Mark Lam]

Zan's debug stack trace does explain what actually went wrong.  Looking at the loops in FTLSaveRestore.cpp in my original patch in http://trac.webkit.org/r279256, I see where I had introduced this bug which allows us to now iterate past MacroAssembler::lastRegister() and MacroAssembler::lastFPRegister() respectively.

In practice though, I think this bug would not cause any issues: Reg is instantiated on the stack.  We'll read past the end of the Bitmap in Reg until we find the first uint32_t after that has a set bit.  This will result in an index exceeding MacroAssembler::lastRegister() or MacroAssembler::lastFPRegister(), and the loops in saveAllRegisters() and restoreAllRegisters() will eventually reject the invalid RegisterID or FPRegisterID.  However, because GCC added the assertion to std::array, you're now seeing a crash.

As for the fix, it is unfortunate that each look iteration now includes 2 comparisons against MacroAssembler::lastRegister() or MacroAssembler::lastFPRegister().  Instead, can you just remove Regs::nextRegister() and Regs::nextFPRegister(), and restore the original loop structure in http://trac.webkit.org/r279256?  For example,

```
    for (MacroAssembler::FPRegisterID reg = MacroAssembler::firstFPRegister(); reg <= MacroAssembler::lastFPRegister(); reg = MacroAssembler::nextFPRegister(reg)) {
        if (regs.special.get(reg))
            continue;
        spooler.storeFPR({ reg, static_cast<ptrdiff_t>(offsetOfFPR(reg)) });
    }
    spooler.finalizeFPR();
```

[Zan Dobersek]

The save/restore iterations can be simplified as suggested, which is nice. Regs::nextFPRegister() can be removed, Regs::nextRegister() not really since it's still used for initializing different GPR values.

I still adjusted Regs::nextRegsiter() to also use a for-loop and access into RegisterSet inside that loop, only for RegisterID values below or equal to MacroAssembler::lastRegister().

[Zan Dobersek]

Created attachment 436895 [details]
Patch

[Michael Catanzaro]

Comment on attachment 436895 [details]
Patch

Unfortunately there are 189 failures on the jsc-armv7-tests EWS.  Several tests are crashing with exit code 139 (139 - 128 = 11, so that's SIGSEGV), while others are crashing with exit code 134 (134 - 128 = 6, SIGABRT). Unfortunately the EWS doesn't provide any backtraces.

[Zan Dobersek]

I don't know what's going on there, the run for this patch only "produced" one failure whereas the 189 failures are apparently expected as they are listed as pre-existing, as also evident on other jobs.

Plus, the proposed change is inside FTL which isn't used on 32-bit JIT targets like the ARMv7, so there should be no changes in behavior there.

[Michael Catanzaro]

(In reply to Zan Dobersek from comment #34)
> Plus, the proposed change is inside FTL which isn't used on 32-bit JIT
> targets like the ARMv7, so there should be no changes in behavior there.

Good point. Maybe we just got unlucky and the test is flaky. I've clicked "Retry failed builds."

[Mark Lam]

Comment on attachment 436895 [details]
Patch

r=me.  Don't know if the jsc-mips-tests failure is real, but I can't see why this patch can possibly cause any failures.

[EWS]

Committed r281843 (241176@main): <https://commits.webkit.org/241176@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 436895 [details].