Bug 24540

Created attachment 28516 [details] Proposed fix for bug 24540

Comment on attachment 28516 [details] Proposed fix for bug 24540 If every location where mmap is used needs to be if'd, it may make more sense to not set HAVE_MMAP to true for Symbian. Alternatively, you may want to implement a wrapper around mmap / munmap that uses some other mechanism for MAP_ANON and calls through to the system mmap for other flags. This would avoid having to sprinkle #if's through so much code. > Index: JavaScriptCore/interpreter/RegisterFile.cpp > =================================================================== > --- JavaScriptCore/interpreter/RegisterFile.cpp (revision 41604) > +++ JavaScriptCore/interpreter/RegisterFile.cpp (working copy) > @@ -34,7 +34,11 @@ namespace JSC { > RegisterFile::~RegisterFile() > { > #if HAVE(MMAP) > - munmap(m_buffer, ((m_max - m_start) + m_maxGlobals) * sizeof(Register)); > + #if PLATFORM(SYMBIAN) > + fastFree(m_bufferSymbian); > + #else > + munmap(m_buffer, ((m_max - m_start) + m_maxGlobals) * sizeof(Register)); > + #endif > #elif HAVE(VIRTUALALLOC) > VirtualFree(m_buffer, 0, MEM_RELEASE); > #else It doesn't make sense to put this inside the HAVE(MMAP) block given it doesn't even use mmap. We also don't indent code in this manner. > #if HAVE(MMAP) > +#if PLATFORM(SYMBIAN) > + // Symbian's OpenC doesn't support MAP_ANON for mmap. > + // Need to hack with fastMalloc. > + // FIXME --nl-- Not sure, if we really need memory alignment here, > + // but it's safe this way and only wastes very little memory. > + static size_t pagesize = getpagesize(); > + size_t extra = 0; > + if (allocationSize > pagesize) { > + extra = allocationSize - pagesize; > + } > + m_bufferSymbian = fastMalloc(bufferLength + extra); > + > + // Align memory at allocationSize boundary > + uintptr_t ptr = reinterpret_cast<uintptr_t>(m_bufferSymbian); > + size_t adjust = 0; > + if ((ptr & (allocationSizeMask)) != 0) { > + adjust = allocationSize - (ptr & (allocationSizeMask)); > + } > + > + if (!m_bufferSymbian || adjust > (bufferLength + extra)) { > + fprintf(stderr, "Could not allocate register file: %d\n", errno); > + CRASH(); > + } > + ptr += adjust; > + m_buffer = reinterpret_cast<Register*>(ptr); > +#else > m_buffer = static_cast<Register*>(mmap(0, bufferLength, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANON, -1, 0)); > if (m_buffer == MAP_FAILED) { > fprintf(stderr, "Could not allocate register file: %d\n", errno); > CRASH(); > } > +#endif // PLATFORM(SYMBIAN) It doesn't make sense to put this inside the HAVE(MMAP) block given it doesn't even use mmap. It also makes it hard to see the common implementation of the function given the mass of code related to a single obscure platform. > Index: JavaScriptCore/jit/ExecutableAllocatorPosix.cpp > =================================================================== > --- JavaScriptCore/jit/ExecutableAllocatorPosix.cpp (revision 41604) > +++ JavaScriptCore/jit/ExecutableAllocatorPosix.cpp (working copy) > @@ -41,14 +41,24 @@ void ExecutableAllocator::intializePageS > > ExecutablePool::Allocation ExecutablePool::systemAlloc(size_t n) > { > +#if PLATFORM(SYMBIAN) > + // Symbian's OpenC doesn't support MAP_ANON for mmap. > + // Need to hack with fastMalloc. > + ExecutablePool::Allocation alloc = {reinterpret_cast<char*>(fastMalloc(n)), n}; > +#else > ExecutablePool::Allocation alloc = {reinterpret_cast<char*>(mmap(NULL, n, PROT_READ | PROT_WRITE | PROT_EXEC, MAP_PRIVATE | MAP_ANON, -1, 0)), n}; > +#endif > return alloc; > } > > void ExecutablePool::systemRelease(const ExecutablePool::Allocation& alloc) > { > +#if PLATFORM(SYMBIAN) > + fastFree(reinterpret_cast<void*>alloc.pages); > +#else > int result = munmap(alloc.pages, alloc.size); > ASSERT_UNUSED(result, !result); > +#endif > } If you're not going to use the POSIX allocator on Symbian then you should add a new implementation of the ExecutablePool interface for your platform rather than #if'ing the POSIX implementation. > Index: JavaScriptCore/runtime/Collector.cpp > =================================================================== > --- JavaScriptCore/runtime/Collector.cpp (revision 41604) > +++ JavaScriptCore/runtime/Collector.cpp (working copy) I'm skipping over the Collector.cpp changes as I'm not particularly familiar this code. > Index: JavaScriptCore/runtime/Collector.h > =================================================================== > --- JavaScriptCore/runtime/Collector.h (revision 41604) > +++ JavaScriptCore/runtime/Collector.h (working copy) > @@ -93,6 +93,7 @@ namespace JSC { > size_t size; > size_t free; > }; > + > Statistics statistics() const; > > void setGCProtectNeedsLocking(); Seems superfluous. > Index: JavaScriptCore/wtf/TCSystemAlloc.cpp > =================================================================== > --- JavaScriptCore/wtf/TCSystemAlloc.cpp (revision 41604) > +++ JavaScriptCore/wtf/TCSystemAlloc.cpp (working copy) > @@ -169,6 +169,27 @@ static void* TryMmap(size_t size, size_t > if (alignment > pagesize) { > extra = alignment - pagesize; > } > +#if PLATFORM(SYMBIAN) > + // Symbian's OpenC doesn't support MAP_ANON for mmap. > + // Need to hack with fastMalloc(). > + // FIXME: The way we adjust for memory alignment > + // causes a small memory leak of returned result ptr > + // (The adjust size won't be free'd). > + // However, this function is currently not called from anywhere ?! > + void* result = fastMalloc(size + extra); > + > + // Adjust the return memory so it is aligned > + uintptr_t ptr = reinterpret_cast<uintptr_t>(result); > + size_t adjust = 0; > + if ((ptr & (alignment - 1)) != 0) { > + adjust = alignment - (ptr & (alignment - 1)); > + } > + > + if (!result || adjust > (size + extra)) { > + fprintf(stderr, "Could not allocate register file: %d\n", errno); > + CRASH(); > + } > +#else > void* result = mmap(NULL, size + extra, > PROT_READ | PROT_WRITE, > MAP_PRIVATE|MAP_ANONYMOUS, > @@ -192,6 +213,7 @@ static void* TryMmap(size_t size, size_t > if (adjust < extra) { > munmap(reinterpret_cast<void*>(ptr + adjust + size), extra - adjust); > } > +#endif /* PLATFORM(SYMBIAN) */ > > ptr += adjust; > return reinterpret_cast<void*>(ptr); This code seems totally wrong. You can't call fastMalloc from here! TryMmap is called from TCMalloc_SystemAlloc to allocate memory. TCMalloc_SystemAlloc is called by TCMalloc to request more memory from the system. TCMalloc is what implements fastMalloc. So this code attempts to use TCMalloc to allocate memory for TCMalloc.... And again, please include a real name rather than "Nokia User" in your changelogs.

Created attachment 28716 [details] Updated patch for bug 24540 I implemented the changes as suggested by Mark Rowe in his review. In particular: - The PLATFORM(SYMBIAN) conditional changes are now outside the HAVE(MMAP)blocks - I created a new ExecutableAllocatorSymbian.cpp instead of embedding the symbian changes within ExecutableAllocatorPosix.cpp - I removed the patch for TCSystemAlloc. According to the processing logic, it is sufficient to utilize the sbrk feature (SBRK is already set true for Symbian anyway).

Comment on attachment 28716 [details] Updated patch for bug 24540 I'm going to say r- again as the patch looks like it won't compile, and has a number of more minor issues. I'd also prefer that the change were less invasive / better factored so the #if PLATFORM(SYMBIAN) code can be kept to a minimum, or at least following our typical pattern of putting such platform-specific code in separate implementation files. WebCore has better examples of this practice than JavaScriptCore at present, but I think it would be good to apply here too. > @@ -124,7 +128,28 @@ > , m_globalObject(0) > { > size_t bufferLength = (capacity + maxGlobals) * sizeof(Register); > -#if HAVE(MMAP) > +#if PLATFORM(SYMBIAN) > + // Symbian's OpenC doesn't support MAP_ANON for mmap. > + // Need to hack with fastMalloc. > + // We may not really need memory alignment on page boundary (4k), > + // but it's safe this way, with very little overhead. > + static size_t pagesize = getpagesize(); > + m_bufferSymbian = fastMalloc(bufferLength + pagesize); > + > + // Align memory at allocationSize boundary > + uintptr_t ptr = reinterpret_cast<uintptr_t>(m_bufferSymbian); > + size_t adjust = 0; > + if ((ptr & (pagesize - 1)) != 0) { > + adjust = pagesize - (ptr & (pagesize - 1)); > + } > + > + if (!m_bufferSymbian || adjust > pagesize || adjust < 0) { > + fprintf(stderr, "Could not allocate register file: %d\n", errno); > + CRASH(); > + } > + ptr += adjust; > + m_buffer = reinterpret_cast<Register*>(ptr); > +#elif HAVE(MMAP) Can we factor this in a way that doesn't require putting 20 lines of Symbian-specific code in this method? Putting code related to such a niche platform first in the function is also a little obnoxious. > m_buffer = static_cast<Register*>(mmap(0, bufferLength, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANON, -1, 0)); > if (m_buffer == MAP_FAILED) { > fprintf(stderr, "Could not allocate register file: %d\n", errno); > @@ -207,6 +232,9 @@ > #if HAVE(VIRTUALALLOC) > Register* m_maxCommitted; > #endif > +#if PLATFORM(SYMBIAN) > + void* m_bufferSymbian; > +#endif This variable name doesn't describe what the variable represents. The fact it is Symbian-specific is obvious from the fact it's declared in PLATFORM(SYMBIAN) block, so doesn't need to be mentioned in the name. > +#include "config.h" > + > +#include "ExecutableAllocator.h" There shouldn't be a blank line between these two #include's. > + > +#if ENABLE(ASSEMBLER) > + > +#include <sys/mman.h> > +#include <unistd.h> > + > +namespace JSC { > + > +void ExecutableAllocator::intializePageSize() > +{ > + ExecutableAllocator::pageSize = getpagesize(); > +} > + > +ExecutablePool::Allocation ExecutablePool::systemAlloc(size_t n) > +{ > + // Symbian's OpenC doesn't support MAP_ANON for mmap. > + // Need to hack with fastMalloc. > + ExecutablePool::Allocation alloc = {reinterpret_cast<char*>(fastMalloc(n)), n}; > + return alloc; > +} > + > +void ExecutablePool::systemRelease(const ExecutablePool::Allocation& alloc) > +{ > + fastFree(reinterpret_cast<void*>alloc.pages); > +} > + > +} > + > +#endif // HAVE(ASSEMBLER) Does this code even compile? The systemRelease method seems particular suspicious. > @@ -87,6 +92,11 @@ > // a PIC branch in Mach-O binaries, see <rdar://problem/5971391>. > #define MIN_ARRAY_SIZE (static_cast<size_t>(14)) > > +#if PLATFORM(SYMBIAN) > + const size_t MAX_NUM_BLOCKS = 128; // Max size of collector heap set to 8 MB > + static RHeap* userChunk = NULL; > +#endif > + We don't typically indent in this manner. > static void freeHeap(CollectorHeap*); > > #if ENABLE(JSC_MULTIPLE_THREADS) > @@ -128,6 +138,27 @@ > { > ASSERT(globalData); > > +#if PLATFORM(SYMBIAN) > + // Symbian OpenC supports mmap but currently not the MAP_ANON flag. > + // Using fastMalloc() does not properly align blocks on 64k boundaries > + // and previous implementation was flawed/incomplete. > + // UserHeap::ChunkHeap allows allocation of continuous memory and specification > + // of alignment value for (symbian) cells within that heap. > + // > + // Clarification and mapping of terminology: > + // RHeap (created by UserHeap::ChunkHeap below) is continuos memory chunk, > + // which can dynamically grow up to 8 MB, > + // that holds all CollectorBlocks of this session (static). > + // Each symbian cell within RHeap maps to a 64kb aligned CollectorBlock. > + // JSCell objects are maintained as usual within CollectorBlocks. > + if (NULL == userChunk) { > + userChunk = UserHeap::ChunkHeap (NULL, 0, MAX_NUM_BLOCKS * BLOCK_SIZE, BLOCK_SIZE, BLOCK_SIZE); > + if (NULL == userChunk) { > + CRASH(); > + } > + } > +#endif // PLATFORM(SYMBIAN) > + > memset(&primaryHeap, 0, sizeof(CollectorHeap)); > memset(&numberHeap, 0, sizeof(CollectorHeap)); > } > @@ -184,10 +215,18 @@ Besides the coding style issues here (explicit comparisons with NULL, braces around one-line if statements, use of NULL where we prefer 0, and whitespace between function name and arguments), it's rather horrible to have a huge chunk of #if'd code in this otherwise platform-independent method. It would be preferable to factor this in some manner that avoids this. The use of a static in this code doesn't seem thread-safe either. > vm_address_t address = 0; > // FIXME: tag the region as a JavaScriptCore heap when we get a registered VM tag: <rdar://problem/6054788>. > vm_map(current_task(), &address, BLOCK_SIZE, BLOCK_OFFSET_MASK, VM_FLAGS_ANYWHERE, MEMORY_OBJECT_NULL, 0, FALSE, VM_PROT_DEFAULT, VM_PROT_DEFAULT, VM_INHERIT_DEFAULT); > + > #elif PLATFORM(SYMBIAN) > - // no memory map in symbian, need to hack with fastMalloc > - void* address = fastMalloc(BLOCK_SIZE); > + > + // Allocate a 64 kb aligned CollectorBlock > + unsigned char* mask = (unsigned char*)userChunk->Alloc(BLOCK_SIZE); > + if (NULL == mask) { > + CRASH(); > + } > + uintptr_t address = reinterpret_cast<uintptr_t>(mask); > + > memset(reinterpret_cast<void*>(address), 0, BLOCK_SIZE); > + > #elif PLATFORM(WIN_OS) > // windows virtual address granularity is naturally 64k > LPVOID address = VirtualAlloc(NULL, BLOCK_SIZE, MEM_COMMIT | MEM_RESERVE, PAGE_READWRITE); > @@ -231,7 +270,7 @@ > #if PLATFORM(DARWIN) > vm_deallocate(current_task(), reinterpret_cast<vm_address_t>(block), BLOCK_SIZE); > #elif PLATFORM(SYMBIAN) > - fastFree(block); > + userChunk->Free(reinterpret_cast<TAny *>(block)); > #elif PLATFORM(WIN_OS) > VirtualFree(block, 0, MEM_RELEASE); > #elif HAVE(POSIX_MEMALIGN) > @@ -395,6 +434,16 @@ There are a few coding style issues here. The use of C-style casts, braces around one-line if statements, explicit comparisons with NULL and whitespace before the * in type names are ones that jump out at me. > +#elif PLATFORM(SYMBIAN) > + // Needs to be positioned before other platforms to avoid potential ambiguities > + static void* stackBase = 0; > + if (stackBase == 0) { > + TThreadStackInfo info; > + RThread thread; > + thread.StackInfo(info); > + stackBase = (void*)info.iBase; > + } > + return (void*)stackBase; > #elif PLATFORM(WIN_OS) && PLATFORM(X86) && COMPILER(MSVC) > // offset 0x18 from the FS segment register gives a pointer to > // the thread information block for the current thread > @@ -446,15 +495,6 @@ > stackThread = thread; > } > return static_cast<char*>(stackBase) + stackSize; > -#elif PLATFORM(SYMBIAN) > - static void* stackBase = 0; > - if (stackBase == 0) { > - TThreadStackInfo info; > - RThread thread; > - thread.StackInfo(info); > - stackBase = (void*)info.iBase; > - } > - return (void*)stackBase; > #else > #error Need a way to get the stack base on this platform > #endif Why does this code need to move? The ChangeLog doesn't mention it at all.

Created attachment 32409 [details] Update 2 of patch file for bug 24540 Attached patch contains cleaned-up code for collector.cpp, according to suggestions from Mark Rowe in comment #4. For simplicity, I separated the change requests for RegisterFile and ExecutableAllocator from this bug and will submit those independently.

Comment on attachment 32409 [details] Update 2 of patch file for bug 24540 Can this be tested by layout tests? (Ones which crash perhaps?)

Comment on attachment 32409 [details] Update 2 of patch file for bug 24540 > +#if PLATFORM(SYMBIAN) > + const size_t MAX_NUM_BLOCKS = 256; // Max size of collector heap set to 16 MB > + static RHeap* userChunk = NULL; > +#endif > + This should not be indented and 0 instead of NULL should be used, as Mark points out. That's a really simple and minor issue that I can fix while landing. The rest looks good to me.

Landed in r46266