Bug 33570

Created attachment 46422 [details] FastMalloc for BREW

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

Created attachment 46423 [details] FastMalloc for BREW Fix style errors.

Created attachment 47266 [details] FastMalloc

Comment on attachment 47266 [details] FastMalloc This seems totally wrong. You should implement the system malloc versions if you want to use system malloc.

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 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()?

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.

Created attachment 48198 [details] FastMalloc Move the code to SystemMallocBrew.h and include it.

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?

(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 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.

Created attachment 49196 [details] FastMalloc Add a comment explaining why we need this kind of hack for malloc.

(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 on attachment 49196 [details] FastMalloc OK.

Comment on attachment 49196 [details] FastMalloc Clearing flags on attachment: 49196 Committed r55091: <http://trac.webkit.org/changeset/55091>

All reviewed patches have been landed. Closing bug.