| Summary: | Remove compile time define for SEPARATED_HEAP | ||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Oliver Hunt <oliver> | ||||||||||||||
| Component: | New Bugs | Assignee: | Oliver Hunt <oliver> | ||||||||||||||
| Status: | RESOLVED FIXED | ||||||||||||||||
| Severity: | Normal | CC: | achristensen, benjamin, cdumez, cmarcelo, commit-queue, keith_miller, mark.lam, msaboff, ossy, rniwa, ryanhaddad, saam | ||||||||||||||
| Priority: | P2 | ||||||||||||||||
| Version: | WebKit Nightly Build | ||||||||||||||||
| Hardware: | Unspecified | ||||||||||||||||
| OS: | Unspecified | ||||||||||||||||
| Bug Depends on: | 155537, 155555, 156505 | ||||||||||||||||
| Bug Blocks: | |||||||||||||||||
| Attachments: |
|
||||||||||||||||
|
Description
Oliver Hunt
2016-03-15 13:26:09 PDT
Created attachment 274119 [details]
Patch
Comment on attachment 274119 [details]
Patch
r=me
The efl EWS is not happy. Can you fix the efl build failure before landing? Created attachment 274142 [details]
patch for landing
Comment on attachment 274142 [details]
patch for landing
r=me
Committed r198235: <http://trac.webkit.org/changeset/198235> This change broke the iOS 9 build: <https://build.webkit.org/builders/Apple%20iOS%209%20Release%20%28Build%29/builds/3308> /Volumes/Data/slave/ios-9-release/build/Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:91:18: error: unused variable 'startOfFixedWritableMemoryPool' [-Werror,-Wunused-variable] static uintptr_t startOfFixedWritableMemoryPool; ^ 1 error generated. (In reply to comment #7) > /Volumes/Data/slave/ios-9-release/build/Source/JavaScriptCore/jit/ > ExecutableAllocatorFixedVMPool.cpp:91:18: error: unused variable > 'startOfFixedWritableMemoryPool' [-Werror,-Wunused-variable] > static uintptr_t startOfFixedWritableMemoryPool; > ^ > 1 error generated. Fixed in r198240: <http://trac.webkit.org/r198240>. (In reply to comment #8) > (In reply to comment #7) > > /Volumes/Data/slave/ios-9-release/build/Source/JavaScriptCore/jit/ > > ExecutableAllocatorFixedVMPool.cpp:91:18: error: unused variable > > 'startOfFixedWritableMemoryPool' [-Werror,-Wunused-variable] > > static uintptr_t startOfFixedWritableMemoryPool; > > ^ > > 1 error generated. > > Fixed in r198240: <http://trac.webkit.org/r198240>. Put a better fix in afterwards -- i was waiting for it to clear all the myriad of iOS configs :-/ More fixed in https://trac.webkit.org/r198241 (In reply to comment #6) > Committed r198235: <http://trac.webkit.org/changeset/198235> It caused a serious regression on ARMv7 Thumb2 Linux, see bug155537 for details. Looks like this is causing crashes on iOS. I think we should roll it out for now. Reverted r198235, r198240, r198241, and r198252 for reason: Causing crashes on ARM Committed r198291: <http://trac.webkit.org/changeset/198291> (In reply to comment #13) > Reverted r198235, r198240, r198241, and r198252 for reason: > > Causing crashes on ARM > > Committed r198291: <http://trac.webkit.org/changeset/198291> This roll out seems to have fixed PLT on iOS 9. Created attachment 275740 [details]
Patch
Attachment 275740 [details] did not pass style-queue:
ERROR: Source/WebCore/ChangeLog:8: You should remove the 'No new tests' and either add and list tests, or explain why no new tests were possible. [changelog/nonewtests] [5]
Total errors found: 1 in 18 files
If any of these errors are false positives, please file a bug against check-webkit-style.
Created attachment 276032 [details]
Patch
Comment on attachment 276032 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276032&action=review > Source/JavaScriptCore/assembler/ARM64Assembler.h:2486 > - void linkJump(AssemblerLabel from, AssemblerLabel to) > + void linkJump(AssemblerLabel from, void* executableCode, AssemblerLabel to) > { > ASSERT(from.isSet()); > ASSERT(to.isSet()); > - relinkJumpOrCall<false>(addressOf(from), addressOf(to)); > + relinkJumpOrCall<false>(addressOf(from), addressOf(executableCode, from), addressOf(to)); > } > > static void linkJump(void* code, AssemblerLabel from, void* to) > { > ASSERT(from.isSet()); > - relinkJumpOrCall<false>(addressOf(code, from), to); > + relinkJumpOrCall<false>(addressOf(code, from), addressOf(code, from), to); > } The part of the patch that actually "remove compile time define for SEPARATED_HEAP" looks good to me (with a few suggestions below). However, I have several concerns: 1. In this patch, you've made a lot of changes to some of the linking code, and this is not documented / explained in the ChangeLog. 2. I can see why you would want to pass the X address in addition to the W address. However, sometimes, you pass the same address as both values (e.g. line 2485 above), and sometimes you don't (e.g. line 2479 above). It's not clear to me why you're passing the same values. Can you explain why the difference, or specifically, why is it ever correct to pass the same values for the X address and the W address? What am I not getting? Also, why does some linkJump()s (above line 2475) not need the X address at all? 3. Have you run benchmarks to make sure that there's no perf regression? Threading the extra X address arg thru the linking functions may not add up to any meaningful perturbation of perf results, but it is extra work for the linker, and I'd feel better if we have some perf number to back it up. > Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:60 > +#if CPU(ARM64) && PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 > +#define USE_EXECUTE_ONLY_JIT_WRITE_FUNCTION 1 > +#endif Can we enclose this in "#if HAVE(REMAP_JIT)" and "#endif"? According to the code below, if is meaningless to USE(EXECUTE_ONLY_JIT_WRITE_FUNCTION) while !HAVE(REMAP_JIT). Isn't that true? If so, let's make it explicit. > Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:100 > +#if !USE(EXECUTE_ONLY_JIT_WRITE_FUNCTION) > +static uintptr_t startOfFixedWritableMemoryPool; > #endif Ditto. Enclose in "#if HAVE(REMAP_JIT)" and "#endif". > Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:299 > +#else // !OS(DARWIN) nit: can you change this comment to be "// OS(DARWIN) && HAVE(REMAP_JIT)" so that we know that it matches the #if above. > Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:302 > + ASSERT_UNUSED(startOfFixedWritableMemoryPool, !startOfFixedWritableMemoryPool); By putting the USE(EXECUTE_ONLY_JIT_WRITE_FUNCTION) stuff under the guard of #if HAVE(REMAP_JIT), you can get rid of this. > Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:304 > +#endif nit: can you change this comment to be "// OS(DARWIN) && HAVE(REMAP_JIT)" so that we know that it matches the #if above. (In reply to comment #18) > Comment on attachment 276032 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=276032&action=review > > > Source/JavaScriptCore/assembler/ARM64Assembler.h:2486 > > - void linkJump(AssemblerLabel from, AssemblerLabel to) > > + void linkJump(AssemblerLabel from, void* executableCode, AssemblerLabel to) > > { > > ASSERT(from.isSet()); > > ASSERT(to.isSet()); > > - relinkJumpOrCall<false>(addressOf(from), addressOf(to)); > > + relinkJumpOrCall<false>(addressOf(from), addressOf(executableCode, from), addressOf(to)); > > } > > > > static void linkJump(void* code, AssemblerLabel from, void* to) > > { > > ASSERT(from.isSet()); > > - relinkJumpOrCall<false>(addressOf(code, from), to); > > + relinkJumpOrCall<false>(addressOf(code, from), addressOf(code, from), to); > > } > > The part of the patch that actually "remove compile time define for > SEPARATED_HEAP" looks good to me (with a few suggestions below). > > However, I have several concerns: > > 1. In this patch, you've made a lot of changes to some of the linking code, > and this is not documented / explained in the ChangeLog. The linking code changes are needed to make this actually work with runtime enabling/disabling. With the runtime control of separation we always use a side buffer during linkage, and that makes relative branch offsets wrong. > > 2. I can see why you would want to pass the X address in addition to the W > address. However, sometimes, you pass the same address as both values (e.g. > line 2485 above), and sometimes you don't (e.g. line 2479 above). It's not > clear to me why you're passing the same values. Can you explain why the > difference, or specifically, why is it ever correct to pass the same values > for the X address and the W address? What am I not getting? > > Also, why does some linkJump()s (above line 2475) not need the X address > at all? > To avoid needless (risky) code duplication we always use the same core branch linking functions. The same argument variants are used for repatching instructions after the original linkage, so they are passing in the actual executable code pointer so the executable and the "writable" addresses are the same. My concern with having a separate version of the actual "write to memory" portion of the patch is that it introduces a _very_ subtle foot gun where you get an incorrect branch computation. > 3. Have you run benchmarks to make sure that there's no perf regression? > Threading the extra X address arg thru the linking functions may not add up > to any meaningful perturbation of perf results, but it is extra work for the > linker, and I'd feel better if we have some perf number to back it up. I've run all jsc-benchmarks in the jsc shell and there wasn't a perf hit on device, or on x86-32/64. Even when actually using the remapping. > > > Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:60 > > +#if CPU(ARM64) && PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 > > +#define USE_EXECUTE_ONLY_JIT_WRITE_FUNCTION 1 > > +#endif > > Can we enclose this in "#if HAVE(REMAP_JIT)" and "#endif"? According to the > code below, if is meaningless to USE(EXECUTE_ONLY_JIT_WRITE_FUNCTION) while > !HAVE(REMAP_JIT). Isn't that true? If so, let's make it explicit. Ok, to this comment and the others. Do you want me to repost for rereview? Created attachment 276106 [details]
Patch
(In reply to comment #19) > (In reply to comment #18) > > Comment on attachment 276032 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=276032&action=review > > > > > Source/JavaScriptCore/assembler/ARM64Assembler.h:2486 > > > - void linkJump(AssemblerLabel from, AssemblerLabel to) > > > + void linkJump(AssemblerLabel from, void* executableCode, AssemblerLabel to) > > > { > > > ASSERT(from.isSet()); > > > ASSERT(to.isSet()); > > > - relinkJumpOrCall<false>(addressOf(from), addressOf(to)); > > > + relinkJumpOrCall<false>(addressOf(from), addressOf(executableCode, from), addressOf(to)); > > > } > > > > > > static void linkJump(void* code, AssemblerLabel from, void* to) > > > { > > > ASSERT(from.isSet()); > > > - relinkJumpOrCall<false>(addressOf(code, from), to); > > > + relinkJumpOrCall<false>(addressOf(code, from), addressOf(code, from), to); > > > } > > > > The part of the patch that actually "remove compile time define for > > SEPARATED_HEAP" looks good to me (with a few suggestions below). > > > > However, I have several concerns: > > > > 1. In this patch, you've made a lot of changes to some of the linking code, > > and this is not documented / explained in the ChangeLog. > > The linking code changes are needed to make this actually work with runtime > enabling/disabling. With the runtime control of separation we always use a > side buffer during linkage, and that makes relative branch offsets wrong. I understand that. I just think the ChangeLog deserves a comment or two on these details. > > > > 2. I can see why you would want to pass the X address in addition to the W > > address. However, sometimes, you pass the same address as both values (e.g. > > line 2485 above), and sometimes you don't (e.g. line 2479 above). It's not > > clear to me why you're passing the same values. Can you explain why the > > difference, or specifically, why is it ever correct to pass the same values > > for the X address and the W address? What am I not getting? > > > > Also, why does some linkJump()s (above line 2475) not need the X address > > at all? > > > > To avoid needless (risky) code duplication we always use the same core > branch linking functions. The same argument variants are used for repatching > instructions after the original linkage, so they are passing in the actual > executable code pointer so the executable and the "writable" addresses are > the same. My concern with having a separate version of the actual "write to > memory" portion of the patch is that it introduces a _very_ subtle foot gun > where you get an incorrect branch computation. I agree with not having 2 separate versions. My concern is that it is not very clear just from reading the code why we sometimes do one thing and sometimes do another. I think this lack of clarity can lead to bugs being introduced later. To summarize, this is what I think you said: we want to use the same linking functions both for original linkage (different W X addresses) and also for repatching (only use X addresses). I'll try to go through the patch again with your comments in mind. Maybe it'll be clearer this time. Please add this info to the ChangeLog as well. > > 3. Have you run benchmarks to make sure that there's no perf regression? > > Threading the extra X address arg thru the linking functions may not add up > > to any meaningful perturbation of perf results, but it is extra work for the > > linker, and I'd feel better if we have some perf number to back it up. > > I've run all jsc-benchmarks in the jsc shell and there wasn't a perf hit on > device, or on x86-32/64. Even when actually using the remapping. Usually, we post the perf results either in a bugzilla comment or as an attachment so that the reviewer can confirm that. That said, a comment in the ChangeLog to indicate that you saw no perf regression with this patch is adequate. This is useful info to have in the event the we have multiple revisions of build failures, and later discover a perf regression. Comments like these help us to focus on patches that have not been vetted instead of spending time on the ones that have already been perf tested. > > > > > Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:60 > > > +#if CPU(ARM64) && PLATFORM(IOS) && __IPHONE_OS_VERSION_MIN_REQUIRED >= 100000 > > > +#define USE_EXECUTE_ONLY_JIT_WRITE_FUNCTION 1 > > > +#endif > > > > Can we enclose this in "#if HAVE(REMAP_JIT)" and "#endif"? According to the > > code below, if is meaningless to USE(EXECUTE_ONLY_JIT_WRITE_FUNCTION) while > > !HAVE(REMAP_JIT). Isn't that true? If so, let's make it explicit. > > Ok, to this comment and the others. Do you want me to repost for rereview? It would be nice to have a new patch with an updated ChangeLog plus the suggested fixes. I see that you've already done that (but still need the ChangeLog comments on what was changed to make linking work). Thanks. Created attachment 276108 [details]
Patch
Comment on attachment 276108 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=276108&action=review r=me with some added comments in the code (for the benefit of future readers of this code) to explain why some link / relink / jump functions are doing the right thing to pass the same address for both the write and executable addresses. > Source/JavaScriptCore/assembler/ARM64Assembler.h:2485 > + relinkJumpOrCall<false>(addressOf(code, from), addressOf(code, from), to); Please add a comment here explaining why it's ok to pass the same address for the Write address and the Execute address. Or alternatively if the comment is long-ish, add the long comment at the top of the file, and just add a short comment here to see that top comment. That way, you can add the "see top comment" at other places that passes the same value as well. > Source/JavaScriptCore/assembler/ARM64Assembler.h:2491 > + linkJumpOrCall<true>(addressOf(code, from) - 1, addressOf(code, from) - 1, to); Ditto. > Source/JavaScriptCore/assembler/ARM64Assembler.h:2661 > + relinkJumpOrCall<false>(reinterpret_cast<int*>(from), reinterpret_cast<const int*>(from), to); A comment up saying that "relinks always writes to the same address as the executable address" would be good. > Source/JavaScriptCore/assembler/ARM64Assembler.h:2667 > + relinkJumpOrCall<true>(reinterpret_cast<int*>(from) - 1, reinterpret_cast<const int*>(from) - 1, to); Ditto. > Source/JavaScriptCore/assembler/ARMv7Assembler.h:2194 > + linkJumpAbsolute(location, location, to); This one needs a comment. > Source/JavaScriptCore/assembler/ARMv7Assembler.h:2215 > + linkJumpAbsolute(reinterpret_cast<uint16_t*>(from), reinterpret_cast<uint16_t*>(from), to); Needs a comment. > Source/JavaScriptCore/assembler/ARMv7Assembler.h:2280 > + linkJumpT4(ptr, ptr, to); Needs a comment. > Source/JavaScriptCore/assembler/ARMv7Assembler.h:2284 > + linkBX(ptr, ptr, to); Ditto. > Source/JavaScriptCore/assembler/ARMv7Assembler.h:2290 > cacheFlush(ptr - 2, sizeof(uint16_t) * 2); Ditto. > Source/JavaScriptCore/assembler/ARMv7Assembler.h:2720 > - > + Please undo. Committed r199299: <http://trac.webkit.org/changeset/199299> Build fix in http://trac.webkit.org/changeset/199339 It would be nice if someone could verify that I did not change the intent of the code. (In reply to comment #25) > Build fix in http://trac.webkit.org/changeset/199339 > It would be nice if someone could verify that I did not change the intent of > the code. That did have a meaningful change -- what platform broke? this passed all the EWS bots? If absolutely necessary we can have a #if for the platform that doesn't have memset_s Re-opened since this is blocked by bug 156505 I'm reverting that change. (In reply to comment #26) > (In reply to comment #25) > > Build fix in http://trac.webkit.org/changeset/199339 > > It would be nice if someone could verify that I did not change the intent of > > the code. > > That did have a meaningful change -- what platform broke? this passed all > the EWS bots? > > If absolutely necessary we can have a #if for the platform that doesn't have > memset_s Which platform? The Apple Mac cmake build: /Volumes/Data/slave/elcapitan-cmake-debug/build/Source/JavaScriptCore/jit/ExecutableAllocatorFixedVMPool.cpp:213:9: error: use of undeclared identifier 'memset_s' memset_s(&writableAddr, sizeof(writableAddr), 0, sizeof(writableAddr)); ^ 1 error generated. https://developer.apple.com/library/mac/documentation/Darwin/Reference/ManPages/man3/memset_s.3.html says the following: SYNOPSIS #define __STDC_WANT_LIB_EXT1__ 1 #include <string.h> The question is why "normal" Apple Mac build defines __STDC_WANT_LIB_EXT1__ and why and how the Apple Mac cmake build doesn't. There isn't any occurence of __STDC_WANT_LIB_EXT1_ in WebKit trunk, except the changelog of the reverted buildfix. Just to document, https://trac.webkit.org/changeset/199530/trunk/Source/JavaScriptCore/PlatformMac.cmake fixed this Apple Mac cmake issue. |