Bug 182923 – REGRESSION(r227717): Hardcoded page size causing JSC crashes on platforms with page size bigger than 16 KB

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Tomas Popela]

After r227717 the JSC crashes on architectures that do have 64KB page size with:

1   0x7ffff76b4090 WTFCrash
2   0x7ffff776f0e8 WTF::OSAllocator::decommit(void*, unsigned long)
3   0x7ffff6e7493c JSC::IsoAlignedMemoryAllocator::freeAlignedMemory(void*)
4   0x7ffff6ea00a8 JSC::MarkedBlock::Handle::~Handle()
5   0x7ffff6ea3fe8 JSC::MarkedSpace::freeBlock(JSC::MarkedBlock::Handle*)
6   0x7ffff6dfca70
7   0x7ffff6dfe628
8   0x7ffff6dfcb14 JSC::BlockDirectory::shrink()
9   0x7ffff6ea40c0
10  0x7ffff6eac0ac
11  0x7ffff6ea4114 JSC::MarkedSpace::shrink()
12  0x7ffff6e17e48 JSC::Heap::sweepSynchronously()
13  0x7ffff6e181bc JSC::Heap::collectNow(JSC::Synchronousness, JSC::GCRequest)
14  0x1004d184
15  0x7ffff704cfe0 JSC::LLInt::CLoop::execute(JSC::OpcodeID, void*, JSC::VM*, JSC::ProtoCallFrame*, bool)
16  0x7ffff705e29c vmEntryToJavaScript
17  0x7ffff702c720 JSC::JITCode::execute(JSC::VM*, JSC::ProtoCallFrame*)
18  0x7ffff7007170 JSC::Interpreter::executeProgram(JSC::SourceCode const&, JSC::ExecState*, JSC::JSObject*)
19  0x7ffff729b8a8 JSC::evaluate(JSC::ExecState*, JSC::SourceCode const&, JSC::JSValue, WTF::NakedPtr<JSC::Exception>&)
20  0x1005bb68
21  0x1005d0b8
22  0x1005e5a0
23  0x1005d1a8 jscmain(int, char**)
24  0x1005a3c4 main
25  0x7ffff3685b90
26  0x7ffff3685db8 __libc_start_main

It's because of

    static constexpr size_t blockSize = 16 * KB;

in MarkedBlock.cpp. Ideally the code should use pageSize() from wtf/PageBlock.h. But the code will need to be changed as the current way (static variable) won't work. And I don't know what sollution the JSC devs will prefer. Also I see that it was hardcoded even before, but the code was not crashing (at least in our PPC64(LE) CI).

Filip (or other JSC devs) can you please look at this?

[Filip Pizlo]

It is inaccurate to say that this hardcodes page size. On Mac, the pages are 4KB and this logic all works.

[Tomas Popela]

(In reply to Filip Pizlo from comment #1)
> It is inaccurate to say that this hardcodes page size. On Mac, the pages are
> 4KB and this logic all works.

Yes I know, but I wrote "crashes on architectures that do have 64KB page size". I will update the bug title to reflect that

[Filip Pizlo]

It’s not accurate to say that MarkedBlock::blockSize is a hardcoded page size. It’s not a page size at all. It’s the block size. 

The solution is certainly not to make blockSize a runtime thing. Block size needs to be a constant.

[Michael Catanzaro]

Tom, do you have a patch that you want to propose for this? Ideally something that does *not* require us to roll out r227717. Even if it's just adding #if CPU() guards around the blockSize declaration.

[Filip Pizlo]

(In reply to Michael Catanzaro from comment #4)
> Tom, do you have a patch that you want to propose for this? Ideally
> something that does *not* require us to roll out r227717. Even if it's just
> adding #if CPU() guards around the blockSize declaration.

I don't think that rolling out r227717 is an option.

[Michael Catanzaro]

(In reply to Filip Pizlo from comment #5)
> I don't think that rolling out r227717 is an option.

Yeah, we shouldn't need to. We're currently using this patch downstream:

https://src.fedoraproject.org/cgit/rpms/webkit2gtk3.git/tree/page-size.patch

So, without understanding anything about the code, this would work:

#if CPU(PPC64) || CPU(PPC64LE) || CPU(PPC) || CPU(S390) || CPU(S390X)
    static constexpr size_t blockSize = 64 * KB;
#else
    static constexpr size_t blockSize = 16 * KB;
#endif

We would need to add new WTF_CPU definitions for s390 and s390x.

It would be nicer if this were not needed.

[Tomas Popela]

(In reply to Michael Catanzaro from comment #6)
> (In reply to Filip Pizlo from comment #5)
> > I don't think that rolling out r227717 is an option.
> 
> Yeah, we shouldn't need to. We're currently using this patch downstream:
> 
> https://src.fedoraproject.org/cgit/rpms/webkit2gtk3.git/tree/page-size.patch
> 
> So, without understanding anything about the code, this would work:
> 
> #if CPU(PPC64) || CPU(PPC64LE) || CPU(PPC) || CPU(S390) || CPU(S390X)
>     static constexpr size_t blockSize = 64 * KB;
> #else
>     static constexpr size_t blockSize = 16 * KB;
> #endif
> 
> We would need to add new WTF_CPU definitions for s390 and s390x.
> 
> It would be nicer if this were not needed.

This is not the exactly right solution (as I wrote in the downstream comment that it's a silly workaround) - the page size should be obtained with pageSize() from WTF that is platform agnostic. I just know that blockSize needs to be aligned with the page size, but I really don't how to "easily" do it, without bigger changes in the code that I really don't quite understand..

[Filip Pizlo]

(In reply to Tomas Popela from comment #7)
> (In reply to Michael Catanzaro from comment #6)
> > (In reply to Filip Pizlo from comment #5)
> > > I don't think that rolling out r227717 is an option.
> > 
> > Yeah, we shouldn't need to. We're currently using this patch downstream:
> > 
> > https://src.fedoraproject.org/cgit/rpms/webkit2gtk3.git/tree/page-size.patch
> > 
> > So, without understanding anything about the code, this would work:
> > 
> > #if CPU(PPC64) || CPU(PPC64LE) || CPU(PPC) || CPU(S390) || CPU(S390X)
> >     static constexpr size_t blockSize = 64 * KB;
> > #else
> >     static constexpr size_t blockSize = 16 * KB;
> > #endif
> > 
> > We would need to add new WTF_CPU definitions for s390 and s390x.
> > 
> > It would be nicer if this were not needed.
> 
> This is not the exactly right solution (as I wrote in the downstream comment
> that it's a silly workaround) - the page size should be obtained with
> pageSize() from WTF that is platform agnostic. I just know that blockSize
> needs to be aligned with the page size, but I really don't how to "easily"
> do it, without bigger changes in the code that I really don't quite
> understand..

As I said before, blockSize really should not be gotten from pageSize because it needs to be a compile time constant. The right solution is to overestimate the blockSize or create an abstraction where GC blocks can allocate blockSize even if pageSize is bigger.

[Michael Catanzaro]

After chatting with Caitlin about this, and based on Filip's comments that the block size must be a compile-time constant and not computed from page size, I propose we just take the simple Fedora patch:

(In reply to Michael Catanzaro from comment #6)
> #if CPU(PPC64) || CPU(PPC64LE) || CPU(PPC) || CPU(UNKNOWN)
>     static constexpr size_t blockSize = 64 * KB;
> #else
>     static constexpr size_t blockSize = 16 * KB;
> #endif

We can also define it at the CMake level and allow overriding it, but that makes cross-compiling more difficult and does not seem useful.

[Michael Catanzaro]

Created attachment 342883 [details]
Patch

[Michael Catanzaro]

(In reply to Tomas Popela from comment #7)
> This is not the exactly right solution (as I wrote in the downstream comment
> that it's a silly workaround) - the page size should be obtained with
> pageSize() from WTF that is platform agnostic.

I think it's the right solution because Filip has expressed a requirement that blockSize be constant and not computed at runtime.

[Mark Lam]

Comment on attachment 342883 [details]
Patch

r=me

[Michael Catanzaro]

Note that I'm assuming overestimating block size will only be a performance issue for CPU(UNKNOWN), and not a correctness issue.

We used to have WTF_CPU_S390 and WTF_CPU_S390X before WTF_CPU_UNKNOWN, which could be brought back if need be (they also need the 64 KB block size), but I assume it's not needed.

[WebKit Commit Bot]

Comment on attachment 342883 [details]
Patch

Clearing flags on attachment: 342883

Committed r232909: <https://trac.webkit.org/changeset/232909>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Radar WebKit Bug Importer]

<rdar://problem/41188975>