Bug 33570 - [BREWMP] Map FastMalloc to BREW memory allocator
Summary: [BREWMP] Map FastMalloc to BREW memory allocator
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Web Template Framework (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Other
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks: 33564
  Show dependency treegraph
 
Reported: 2010-01-12 19:48 PST by Kwang Yul Seo
Modified: 2010-02-22 11:03 PST (History)
5 users (show)

See Also:


Attachments
FastMalloc for BREW (3.96 KB, patch)
2010-01-12 19:51 PST, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
FastMalloc for BREW (3.78 KB, patch)
2010-01-12 20:00 PST, Kwang Yul Seo
no flags Details | Formatted Diff | Diff
FastMalloc (3.94 KB, patch)
2010-01-22 21:58 PST, Kwang Yul Seo
eric: review-
Details | Formatted Diff | Diff
FastMalloc (1.94 KB, patch)
2010-02-03 21:28 PST, Kwang Yul Seo
eric: review-
Details | Formatted Diff | Diff
FastMalloc (3.93 KB, patch)
2010-02-04 21:35 PST, Kwang Yul Seo
eric: review-
Details | Formatted Diff | Diff
FastMalloc (4.11 KB, patch)
2010-02-22 03:46 PST, Kwang Yul Seo
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Kwang Yul Seo 2010-01-12 19:48:36 PST
FastMalloc can't be ported to BREW. Map fastMalloc/fastFree/fastRealloc to BREW's MALLOC/FREE/REALLOC macros.
Comment 1 Kwang Yul Seo 2010-01-12 19:51:57 PST
Created attachment 46422 [details]
FastMalloc for BREW
Comment 2 WebKit Review Bot 2010-01-12 19:54:22 PST
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
Comment 3 Kwang Yul Seo 2010-01-12 20:00:28 PST
Created attachment 46423 [details]
FastMalloc for BREW

Fix style errors.
Comment 4 Kwang Yul Seo 2010-01-22 21:58:55 PST
Created attachment 47266 [details]
FastMalloc
Comment 5 Eric Seidel (no email) 2010-01-26 14:46:25 PST
Comment on attachment 47266 [details]
FastMalloc

This seems totally wrong.  You should implement the system malloc versions if you want to use system malloc.
Comment 6 Kwang Yul Seo 2010-02-03 21:28:17 PST
Created attachment 48099 [details]
FastMalloc

Okay. This is another try. Replace the system malloc in FastMalloc with BREW's MALLOC/CALLOC/REALLOC/FREE macros.
Comment 7 Eric Seidel (no email) 2010-02-04 16:56:48 PST
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()?
Comment 8 Kwang Yul Seo 2010-02-04 21:23:39 PST
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.
Comment 9 Kwang Yul Seo 2010-02-04 21:35:21 PST
Created attachment 48198 [details]
FastMalloc

Move the code to SystemMallocBrew.h and include it.
Comment 10 Eric Seidel (no email) 2010-02-08 19:21:37 PST
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?
Comment 11 Kwang Yul Seo 2010-02-08 19:41:04 PST
(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.
Comment 12 Eric Seidel (no email) 2010-02-17 15:17:52 PST
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.
Comment 13 Kwang Yul Seo 2010-02-22 03:46:15 PST
Created attachment 49196 [details]
FastMalloc

Add a comment explaining why we need this kind of hack for malloc.
Comment 14 Kwang Yul Seo 2010-02-22 03:49:10 PST
(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.
Comment 15 Eric Seidel (no email) 2010-02-22 10:46:34 PST
Comment on attachment 49196 [details]
FastMalloc

OK.
Comment 16 WebKit Commit Bot 2010-02-22 11:02:57 PST
Comment on attachment 49196 [details]
FastMalloc

Clearing flags on attachment: 49196

Committed r55091: <http://trac.webkit.org/changeset/55091>
Comment 17 WebKit Commit Bot 2010-02-22 11:03:02 PST
All reviewed patches have been landed.  Closing bug.