[Webkit-unassigned] [Bug 24540] Fix for missing mmap features in Symbian
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Wed Mar 11 19:51:30 PDT 2009
https://bugs.webkit.org/show_bug.cgi?id=24540
mrowe at apple.com changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #28516|review? |review-
Flag| |
------- Comment #2 from mrowe at apple.com 2009-03-11 19:51 PDT -------
(From update of attachment 28516)
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.
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug, or are watching the assignee.
More information about the webkit-unassigned
mailing list