Bug 33570 – [BREWMP] Map FastMalloc to BREW memory allocator

• Status: RESOLVED
• Resolution: FIXED
• Component: Web Template Framework

[Kwang Yul Seo]

FastMalloc can't be ported to BREW. Map fastMalloc/fastFree/fastRealloc to BREW's MALLOC/FREE/REALLOC macros.

[Kwang Yul Seo]

Created attachment 46422 [details]
FastMalloc for BREW

[WebKit Review Bot]

Attachment 46422 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/wtf/brew/FastMallocBrew.cpp:36:  Code inside a namespace should not be indented.  [whitespace/indent] [4]
JavaScriptCore/wtf/brew/FastMallocBrew.cpp:44:  n_elements is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/brew/FastMallocBrew.cpp:44:  element_size is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/brew/FastMallocBrew.cpp:83:  n_elements is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
JavaScriptCore/wtf/brew/FastMallocBrew.cpp:83:  element_size is incorrectly named. Don't use underscores in your identifier names.  [readability/naming] [4]
Total errors found: 5

[Kwang Yul Seo]

Created attachment 46423 [details]
FastMalloc for BREW

Fix style errors.

[Kwang Yul Seo]

Created attachment 47266 [details]
FastMalloc

[Eric Seidel (no email)]

Comment on attachment 47266 [details]
FastMalloc

This seems totally wrong.  You should implement the system malloc versions if you want to use system malloc.

[Kwang Yul Seo]

Created attachment 48099 [details]
FastMalloc

Okay. This is another try. Replace the system malloc in FastMalloc with BREW's MALLOC/CALLOC/REALLOC/FREE macros.

[Eric Seidel (no email)]

Comment on attachment 48099 [details]
FastMalloc

Hmmm.

Why use a #define instead of  just naming the functions malloc, calloc, etc?

Maybe this code should go in its own header, which FastMalloc.cpp should include.  Something like SystemMallocBrew.h?

How do other codebases which port to BREW deal with this strange lack of malloc()?

[Kwang Yul Seo]

BREW uses RVCT or GCC for target. So malloc, calloc, realloc and free are provided by these two compilers. However, because BREW runtime does not initialize the heap for application, calling malloc crashes the system. To share system heap, BREW applications must use MALLOC, CALLOC, REALLOC and FREE.


(In reply to comment #7)
> (From update of attachment 48099 [details])
> Hmmm.
> 
> Why use a #define instead of  just naming the functions malloc, calloc, etc?
> 

Naming the function malloc causes a compile error if stdlib.h is included.

void* malloc(size_t)' was declared `extern' and later `static' error.


> Maybe this code should go in its own header, which FastMalloc.cpp should
> include.  Something like SystemMallocBrew.h?
> 

I think it is a good idea.


> How do other codebases which port to BREW deal with this strange lack of
> malloc()?

One simple approach is to replace malloc, calloc, realloc and free with their corresponding counterparts MALLOC, CALLOC, REALLOC and FREE using macros.

[Kwang Yul Seo]

Created attachment 48198 [details]
FastMalloc

Move the code to SystemMallocBrew.h and include it.

[Eric Seidel (no email)]

So BREW has a stdlib.h which defines malloc?  But it just doesn't have a malloc() implementation?

In which case, why aren't we just providing a malloc implementation?  Why doesn't BREW?

[Kwang Yul Seo]

(In reply to comment #10)
> So BREW has a stdlib.h which defines malloc?  But it just doesn't have a
> malloc() implementation?
> 
> In which case, why aren't we just providing a malloc implementation?  Why
> doesn't BREW?

The situation is a bit complicated here. BREW does have malloc() implementation as compilers provide it. However, malloc() does not work correctly because the BREW loader does not initialize the heap base address and size properly. BREW applications must use macros like MALLOC and FREE to access the system heap.

[Eric Seidel (no email)]

Comment on attachment 48198 [details]
FastMalloc

Please add a comment before the #defines explaining why they're the right thing to do. Similar perhaps to how you explained things in teh bug.  That although BREW defines "malloc()" you can't use it because the loader does not initialize the base address properly.  (Is this a bug in brew?  If so, is there a bug url tracking it?  If so, you should include it.)

Once we have enough justification in the code, we can land this kind of hack.  As is I think this code would just confuse future folks trying to read it.

[Kwang Yul Seo]

Created attachment 49196 [details]
FastMalloc

Add a comment explaining why we need this kind of hack for malloc.

[Kwang Yul Seo]

(In reply to comment #12)
> (From update of attachment 48198 [details])
> Please add a comment before the #defines explaining why they're the right thing
> to do. Similar perhaps to how you explained things in teh bug.  That although
> BREW defines "malloc()" you can't use it because the loader does not initialize
> the base address properly.  (Is this a bug in brew?  If so, is there a bug url
> tracking it?  If so, you should include it.)
> 
> Once we have enough justification in the code, we can land this kind of hack. 
> As is I think this code would just confuse future folks trying to read it.

Thank you for your comment, Eric. I added a comment as you suggested. I don't think it is a bug in BREW. In BREW, many C functions are prohibited just to make the loader simple.

[Eric Seidel (no email)]

Comment on attachment 49196 [details]
FastMalloc

OK.

[WebKit Commit Bot]

Comment on attachment 49196 [details]
FastMalloc

Clearing flags on attachment: 49196

Committed r55091: <http://trac.webkit.org/changeset/55091>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.