182923 2018-02-19 03:01:18 -0800 REGRESSION(r227717): Hardcoded page size causing JSC crashes on platforms with page size bigger than 16 KB 2018-06-16 12:01:36 -0700 1 1 1 Unclassified WebKit JavaScriptCore WebKit Nightly Build Unspecified Unspecified RESOLVED FIXED InRadar P2 Normal --- 1 tpopela mcatanzaro berto commit-queue ews-watchlist fpizlo keith_miller mark.lam mcatanzaro msaboff saam webkit-bug-importer oldest_to_newest 1400311 0 tpopela 2018-02-19 03:01:18 -0800 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? 1400339 1 fpizlo 2018-02-19 05:40:36 -0800 It is inaccurate to say that this hardcodes page size. On Mac, the pages are 4KB and this logic all works. 1400355 2 tpopela 2018-02-19 07:05:10 -0800 (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 1400359 3 fpizlo 2018-02-19 07:30:03 -0800 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. 1406202 4 mcatanzaro 2018-03-13 12:56:54 -0700 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. 1406204 5 fpizlo 2018-03-13 12:59:29 -0700 (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. 1406213 6 mcatanzaro 2018-03-13 13:12:47 -0700 (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. 1406363 7 tpopela 2018-03-14 00:39:13 -0700 (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.. 1406371 8 fpizlo 2018-03-14 06:29:27 -0700 (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. 1433726 9 mcatanzaro 2018-06-16 10:11:57 -0700 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. 1433728 10 342883 mcatanzaro 2018-06-16 10:16:44 -0700 Created attachment 342883 Patch 1433729 11 mcatanzaro 2018-06-16 10:17:51 -0700 (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. 1433743 12 342883 mark.lam 2018-06-16 11:21:00 -0700 Comment on attachment 342883 Patch r=me 1433749 13 mcatanzaro 2018-06-16 11:33:59 -0700 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. 1433756 14 342883 commit-queue 2018-06-16 12:00:21 -0700 Comment on attachment 342883 Patch Clearing flags on attachment: 342883 Committed r232909: <https://trac.webkit.org/changeset/232909> 1433757 15 commit-queue 2018-06-16 12:00:23 -0700 All reviewed patches have been landed. Closing bug. 1433758 16 webkit-bug-importer 2018-06-16 12:01:36 -0700 <rdar://problem/41188975> 342883 2018-06-16 10:16:44 -0700 2018-06-16 12:00:21 -0700 Patch bug-182923-20180616121643.patch text/plain 1808 mcatanzaro U3VidmVyc2lvbiBSZXZpc2lvbjogMjMyODk1CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCA4 MjY5NTEwY2NjY2Y5ZTg3YTFiNTY2YjVhNDU1ZDgyZjZlNWJkODUyLi41ZWIwMTNkOTM1Y2IxMjhm MjAwOWZiOTZkMDQ4Y2NhOTcyZDM0OGNkIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxNSBAQAorMjAxOC0wNi0xNiAgTWljaGFlbCBDYXRhbnphcm8gIDxtY2F0YW56YXJvQGln YWxpYS5jb20+CisKKyAgICAgICAgUkVHUkVTU0lPTihyMjI3NzE3KTogSGFyZGNvZGVkIHBhZ2Ug c2l6ZSBjYXVzaW5nIEpTQyBjcmFzaGVzIG9uIHBsYXRmb3JtcyB3aXRoIHBhZ2Ugc2l6ZSBiaWdn ZXIgdGhhbiAxNiBLQgorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5j Z2k/aWQ9MTgyOTIzCisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgVGhlIGJsb2NrU2l6ZSB1c2VkIGJ5IE1hcmtlZEJsb2NrIGlzIGluY29ycmVjdCBvbiBw bGF0Zm9ybXMgd2l0aCBwYWdlcyBsYXJnZXIgdGhhbiAxNiBLQi4KKyAgICAgICAgVXBzdHJlYW0g RmVkb3JhJ3MgcGF0Y2ggdG8gdXNlIGEgc2FmZXIgNjQgS0IgZGVmYXVsdC4gVGhpcyBmaXhlcyBQ b3dlclBDIGFuZCBzMzkweC4KKworICAgICAgICAqIGhlYXAvTWFya2VkQmxvY2suaDoKKwogMjAx OC0wNi0xNSAgU2FhbSBCYXJhdGkgIDxzYmFyYXRpQGFwcGxlLmNvbT4KIAogICAgICAgICBNYWtl IEZvcmNlT1NSRXhpdCBDRkcgcHJ1bmluZyBpbiBieXRlY29kZSBwYXJzZXIgbW9yZSBhZ2dyZXNz aXZlIGJ5IG1ha2luZyB0aGUgb3JpZ2luYWwgYmxvY2sgdG8gaWdub3JlIGJlIHRoZSBwbGFuJ3Mg b3NyRW50cnlCeXRlY29kZUluZGV4CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUv aGVhcC9NYXJrZWRCbG9jay5oIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2hlYXAvTWFya2VkQmxv Y2suaAppbmRleCA3ZDE0ZWVjZmQ4ZDdkMTc4ODM3NGYwYWZlZmU4YTMzNGY2NDNkODA5Li40NDRh ODhmMzI5Mzc5NmY3N2I3OTlmMzRmYTBiZTc5ODJhMTE0MzgzIDEwMDY0NAotLS0gYS9Tb3VyY2Uv SmF2YVNjcmlwdENvcmUvaGVhcC9NYXJrZWRCbG9jay5oCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0 Q29yZS9oZWFwL01hcmtlZEJsb2NrLmgKQEAgLTY2LDcgKzY2LDE0IEBAIHByaXZhdGU6CiAgICAg ZnJpZW5kIGNsYXNzIEhhbmRsZTsKIHB1YmxpYzoKICAgICBzdGF0aWMgY29uc3RleHByIHNpemVf dCBhdG9tU2l6ZSA9IDE2OyAvLyBieXRlcworCisgICAgLy8gQmxvY2sgc2l6ZSBtdXN0IGJlIGF0 IGxlYXN0IGFzIGxhcmdlIGFzIHRoZSBzeXN0ZW0gcGFnZSBzaXplLgorI2lmIENQVShQUEM2NCkg fHwgQ1BVKFBQQzY0TEUpIHx8IENQVShQUEMpIHx8IENQVShVTktOT1dOKQorICAgIHN0YXRpYyBj b25zdGV4cHIgc2l6ZV90IGJsb2NrU2l6ZSA9IDY0ICogS0I7CisjZWxzZQogICAgIHN0YXRpYyBj b25zdGV4cHIgc2l6ZV90IGJsb2NrU2l6ZSA9IDE2ICogS0I7CisjZW5kaWYKKwogICAgIHN0YXRp YyBjb25zdGV4cHIgc2l6ZV90IGJsb2NrTWFzayA9IH4oYmxvY2tTaXplIC0gMSk7IC8vIGJsb2Nr U2l6ZSBtdXN0IGJlIGEgcG93ZXIgb2YgdHdvLgogCiAgICAgc3RhdGljIGNvbnN0ZXhwciBzaXpl X3QgYXRvbXNQZXJCbG9jayA9IGJsb2NrU2l6ZSAvIGF0b21TaXplOwo=