Bug 28886 - Build error using JIT with gcc 4.1.2 and ARM v5
Summary: Build error using JIT with gcc 4.1.2 and ARM v5
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Platform (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other Linux
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2009-09-01 12:46 PDT by Andre Pedralho
Modified: 2009-09-28 11:57 PDT (History)
11 users (show)

See Also:


Attachments
Adding the __clear_cache header according to the comment above. (558 bytes, patch)
2009-09-08 08:06 PDT, Andre Pedralho
no flags Details | Formatted Diff | Diff
Updated ToT, added ifdefs and changelog according to loki04 suggestions in #qtwebkit. (1.12 KB, patch)
2009-09-08 08:34 PDT, Andre Pedralho
eric: review-
Details | Formatted Diff | Diff
Be more specific with guards. (2.01 KB, patch)
2009-09-11 07:26 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff
incorporate Gabor's comments. (1.95 KB, patch)
2009-09-11 10:51 PDT, Laszlo Gombos
no flags Details | Formatted Diff | Diff
Avoid __clear_cache built-in function if NO_CLEAR_CACHE define is set (2.20 KB, patch)
2009-09-22 08:01 PDT, Gabor Loki
no flags Details | Formatted Diff | Diff
Avoid __clear_cache built-in function if DISABLE_BUILTIN_CLEAR_CACHE define is set (2.34 KB, patch)
2009-09-22 09:23 PDT, Gabor Loki
barraclough: review+
Details | Formatted Diff | Diff
Remove __clear_cache which is an internal function of GCC (1.82 KB, patch)
2009-09-28 09:19 PDT, Gabor Loki
hausmann: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andre Pedralho 2009-09-01 12:46:12 PDT
g++ -c -pipe -Wreturn-type -fno-strict-aliasing -ffunction-sections
-fdata-sections -O2 -D_REENTRANT -fPIC -DENABLE_YARR=1
-DENABLE_YARR_JIT=1 -DENABLE_JIT=1 -DBUILDING_QT__=1 -DUSE_SYSTEM_MALLOC
-DNDEBUG -DQT_MAKEDLL -DHAVE_STDINT_H -DBUILD_WEBKIT
-DENABLE_JAVASCRIPT_DEBUGGER=1 -DENABLE_DATABASE=1
-DENABLE_EVENTSOURCE=1 -DENABLE_OFFLINE_WEB_APPLICATIONS=1
-DENABLE_DOM_STORAGE=1 -DENABLE_ICONDATABASE=1
-DENABLE_CHANNEL_MESSAGING=1 -DENABLE_SQLITE=1
-DENABLE_DASHBOARD_SUPPORT=0 -DENABLE_FILTERS=0 -DENABLE_XPATH=1
-DENABLE_XSLT=0 -DENABLE_WCSS=0 -DENABLE_WML=0 -DENABLE_SHARED_WORKERS=0
-DENABLE_WORKERS=1 -DENABLE_XHTMLMP=0 -DENABLE_DATAGRID=1 -DENABLE_SVG=1
-DENABLE_SVG_FONTS=1 -DENABLE_SVG_FOREIGN_OBJECT=1
-DENABLE_SVG_ANIMATION=1 -DENABLE_SVG_AS_IMAGE=1 -DENABLE_SVG_USE=1
-DENABLE_RUBY=1 -DENABLE_VIDEO=1 -DENABLE_DATALIST=1
-DENABLE_NETSCAPE_PLUGIN_API=1 -DWTF_USE_JAVASCRIPTCORE_BINDINGS=1
-DWTF_CHANGES=1 -DBUILDING_QT__ -DBUILDING_JavaScriptCore -DBUILDING_WTF
-DENABLE_PLUGIN_PACKAGE_SIMPLE_HASH=1 -DXP_UNIX -DQT_NO_DEBUG
-DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_CORE_LIB -DQT_SHARED
-I/usr/share/qt4/mkspecs/linux-g++ -I../../../WebCore
-I/usr/include/qt4/QtCore -I/usr/include/qt4/QtNetwork
-I/usr/include/qt4/QtGui -I/usr/include/qt4 -I../../../WebCore/bridge/qt
-I../../../WebCore/page/qt -I../../../WebCore/platform/graphics/qt
-I../../../WebCore/platform/network/qt -I../../../WebCore/platform/qt
-I../../../WebKit/qt/WebCoreSupport -I../../../WebCore
-I../../../WebCore/accessibility -I../../../WebCore/bindings/js
-I../../../WebCore/bridge -I../../../WebCore/bridge/c
-I../../../WebCore/css -I../../../WebCore/dom
-I../../../WebCore/dom/default -I../../../WebCore/editing
-I../../../WebCore/history -I../../../WebCore/html
-I../../../WebCore/html/canvas -I../../../WebCore/inspector
-I../../../WebCore/loader -I../../../WebCore/loader/appcache
-I../../../WebCore/loader/archive -I../../../WebCore/loader/icon
-I../../../WebCore/notifications -I../../../WebCore/page
-I../../../WebCore/page/animation -I../../../WebCore/platform
-I../../../WebCore/platform/animation
-I../../../WebCore/platform/graphics
-I../../../WebCore/platform/graphics/filters
-I../../../WebCore/platform/graphics/transforms
-I../../../WebCore/platform/image-decoders
-I../../../WebCore/platform/network -I../../../WebCore/platform/sql
-I../../../WebCore/platform/text -I../../../WebCore/plugins
-I../../../WebCore/rendering -I../../../WebCore/rendering/style
-I../../../WebCore/storage -I../../../WebCore/svg
-I../../../WebCore/svg/animation -I../../../WebCore/svg/graphics
-I../../../WebCore/svg/graphics/filters -I../../../WebCore/wml
-I../../../WebCore/workers -I../../../WebCore/xml -Igenerated/release
-I../../../JavaScriptCore -I../../../../webkit
-I../../../JavaScriptCore/assembler -I../../../JavaScriptCore/bytecode
-I../../../JavaScriptCore/bytecompiler
-I../../../JavaScriptCore/debugger -I../../../JavaScriptCore/interpreter
-I../../../JavaScriptCore/jit -I../../../JavaScriptCore/parser
-I../../../JavaScriptCore/profiler -I../../../JavaScriptCore/runtime
-I../../../JavaScriptCore/wrec -I../../../JavaScriptCore/wtf
-I../../../JavaScriptCore/wtf/unicode -I../../../JavaScriptCore/yarr
-I../../../JavaScriptCore/API
-I../../../JavaScriptCore/ForwardingHeaders -Igenerated/release
-I../../../WebKit/qt/Api -I../../../JavaScriptCore/pcre
-I/home/root/webkit/WebKitBuild/Release/JavaScriptCore/tmp
-I/usr/src/3rdparty/sqlite/ -I/usr/include/qt4/phonon
-I/usr/X11R6/include -I. -I../../../WebCore -I. -o obj/release/JSBase.o
../../../JavaScriptCore/API/JSBase.cpp
../../../JavaScriptCore/jit/ExecutableAllocator.h: In static member
function 'static void JSC::ExecutableAllocator::cacheFlush(void*, size_t)':
../../../JavaScriptCore/jit/ExecutableAllocator.h:189: error:
'__clear_cache' was not declared in this scope
make[1]: *** [obj/release/JSBase.o] Error 1

The error persists even building with the command below:
WebKitTools/Scripts/build-webkit --qt ENABLE_YARR=1 ENABLE_YARR_JIT=1 ENABLE_JIT=1 WTF_USE_JSVALUE32=1
Comment 1 Antonio Gomes 2009-09-03 04:41:24 PDT
pedralho, any luck to build w/ jit, yacc and friends all off ?

please also inform revision you were at.
Comment 2 Andre Pedralho 2009-09-03 06:21:57 PDT
(In reply to comment #1)
> pedralho, any luck to build w/ jit, yacc and friends all off ?
> 
> please also inform revision you were at.

No luck, I reseted my tree to ToT (svn r48016) and tried to rebuild getting the following error:

g++ -c -pipe -Wreturn-type -fno-strict-aliasing -ffunction-sections -fdata-sections -O2 -D_REENTRANT -fPIC -DENABLE_VIDEO=0 -DBUILDING_QT__=1 -DUSE_SYSTEM_MALLOC -DNDEBUG -DQT_MAKEDLL -DHAVE_STDINT_H -DBUILD_WEBKIT -DENABLE_JAVASCRIPT_DEBUGGER=1 -DENABLE_DATABASE=1 -DENABLE_EVENTSOURCE=1 -DENABLE_OFFLINE_WEB_APPLICATIONS=1 -DENABLE_DOM_STORAGE=1 -DENABLE_ICONDATABASE=1 -DENABLE_CHANNEL_MESSAGING=1 -DENABLE_SQLITE=1 -DENABLE_DASHBOARD_SUPPORT=0 -DENABLE_FILTERS=0 -DENABLE_XPATH=1 -DENABLE_XSLT=0 -DENABLE_WCSS=0 -DENABLE_WML=0 -DENABLE_SHARED_WORKERS=0 -DENABLE_WORKERS=1 -DENABLE_XHTMLMP=0 -DENABLE_DATAGRID=1 -DENABLE_SVG=1 -DENABLE_SVG_FONTS=1 -DENABLE_SVG_FOREIGN_OBJECT=1 -DENABLE_SVG_ANIMATION=1 -DENABLE_SVG_AS_IMAGE=1 -DENABLE_SVG_USE=1 -DENABLE_RUBY=1 -DENABLE_DATALIST=1 -DENABLE_NETSCAPE_PLUGIN_API=1 -DWTF_USE_JAVASCRIPTCORE_BINDINGS=1 -DWTF_CHANGES=1 -DBUILDING_QT__ -DBUILDING_JavaScriptCore -DBUILDING_WTF -DENABLE_PLUGIN_PACKAGE_SIMPLE_HASH=1 -DXP_UNIX -DQT_NO_DEBUG -DQT_GUI_LIB -DQT_NETWORK_LIB -DQT_CORE_LIB -DQT_SHARED -I/usr/share/qt4/mkspecs/linux-g++ -I../../../WebCore -I/usr/include/qt4/QtCore -I/usr/include/qt4/QtNetwork -I/usr/include/qt4/QtGui -I/usr/include/qt4 -I../../../WebCore/bridge/qt -I../../../WebCore/page/qt -I../../../WebCore/platform/graphics/qt -I../../../WebCore/platform/network/qt -I../../../WebCore/platform/qt -I../../../WebKit/qt/WebCoreSupport -I../../../WebCore -I../../../WebCore/accessibility -I../../../WebCore/bindings/js -I../../../WebCore/bridge -I../../../WebCore/bridge/c -I../../../WebCore/css -I../../../WebCore/dom -I../../../WebCore/dom/default -I../../../WebCore/editing -I../../../WebCore/history -I../../../WebCore/html -I../../../WebCore/html/canvas -I../../../WebCore/inspector -I../../../WebCore/loader -I../../../WebCore/loader/appcache -I../../../WebCore/loader/archive -I../../../WebCore/loader/icon -I../../../WebCore/notifications -I../../../WebCore/page -I../../../WebCore/page/animation -I../../../WebCore/platform -I../../../WebCore/platform/animation -I../../../WebCore/platform/graphics -I../../../WebCore/platform/graphics/filters -I../../../WebCore/platform/graphics/transforms -I../../../WebCore/platform/image-decoders -I../../../WebCore/platform/network -I../../../WebCore/platform/sql -I../../../WebCore/platform/text -I../../../WebCore/plugins -I../../../WebCore/rendering -I../../../WebCore/rendering/style -I../../../WebCore/storage -I../../../WebCore/svg -I../../../WebCore/svg/animation -I../../../WebCore/svg/graphics -I../../../WebCore/svg/graphics/filters -I../../../WebCore/wml -I../../../WebCore/workers -I../../../WebCore/xml -Igenerated/release -I../../../JavaScriptCore -I../../../../mainline -I../../../JavaScriptCore/assembler -I../../../JavaScriptCore/bytecode -I../../../JavaScriptCore/bytecompiler -I../../../JavaScriptCore/debugger -I../../../JavaScriptCore/interpreter -I../../../JavaScriptCore/jit -I../../../JavaScriptCore/parser -I../../../JavaScriptCore/profiler -I../../../JavaScriptCore/runtime -I../../../JavaScriptCore/wrec -I../../../JavaScriptCore/wtf -I../../../JavaScriptCore/wtf/unicode -I../../../JavaScriptCore/yarr -I../../../JavaScriptCore/API -I../../../JavaScriptCore/ForwardingHeaders -Igenerated/release -I../../../WebKit/qt/Api -I../../../JavaScriptCore/pcre -I/home/root/bluebox/webkit/mainline/WebKitBuild/Release/JavaScriptCore/tmp -I/usr/src/3rdparty/sqlite/ -I/usr/X11R6/include -I. -I../../../WebCore -I. -o obj/release/JSBase.o ../../../JavaScriptCore/API/JSBase.cpp
../../../JavaScriptCore/jit/ExecutableAllocator.h: In static member function 'static void JSC::ExecutableAllocator::cacheFlush(void*, size_t)':
../../../JavaScriptCore/jit/ExecutableAllocator.h:189: error: '__clear_cache' was not declared in this scope
make[1]: *** [obj/release/JSBase.o] Error 1
make[1]: Leaving directory `/home/root/bluebox/webkit/mainline/WebKitBuild/Release/WebCore'
make: *** [sub-WebCore-make_default-ordered] Error 2
Comment 3 Zoltan Herczeg 2009-09-03 08:24:59 PDT
I think this cause the issue:

> '__clear_cache' was not declared in this scope
> make[1]: *** [obj/release/JSBase.o] Error 1

This is a gcc builtin function. Please check if its interface is declared.

see the "static void cacheFlush(void* code, size_t size)" in
JavaScriptCore/jit/ExecutableAllocator.h

This function calls a kernel utility to flush data cache memory (unfortunately the required mrc instruction is only executable in kernel level).

There is a native NAPI implementation in the code if __clear_cache is not declared in your gcc. If you have other solution for flushing the data cache, just call that function.
Comment 4 Andre Pedralho 2009-09-04 15:15:01 PDT
(In reply to comment #3)
> I think this cause the issue:
> 
> This is a gcc builtin function. Please check if its interface is declared.
> 
I copied a simple test (from here http://code.google.com/p/android/issues/detail?id=1803) and it builds and runs fine in the same environment.

#include <stdio.h>

int main(int argc, char **argv)
{
    printf("attempting toolchain clearcache syscall:\n");
    __clear_cache(0, 0);
    return 0;                                        
}

I couldn't find the __clear_cache in any system header file, but could find it in 4.1.2/libgcc.a and 4.1.2/libgcc_s.so.

Then I added an "extern void __clear_cache (char *beg, char *end);" before using __clear_cache in ExecutableAllocator.h. The result:

obj/release/ARMAssembler.o: In function `JSC::ARMAssembler::linkBranch(void*, JSC::ARMAssembler::JmpSrc, void*, int)':
ARMAssembler.cpp:(.text+0x1b8): undefined reference to `JSC::__clear_cache(char*, char*)'
Comment 5 Zoltan Herczeg 2009-09-05 00:28:34 PDT
> I couldn't find the __clear_cache in any system header file, but could find it
> in 4.1.2/libgcc.a and 4.1.2/libgcc_s.so.

Good!

> Then I added an "extern void __clear_cache (char *beg, char *end);" before
> using __clear_cache in ExecutableAllocator.h. The result:
> 
> obj/release/ARMAssembler.o: In function `JSC::ARMAssembler::linkBranch(void*,
> JSC::ARMAssembler::JmpSrc, void*, int)':
> ARMAssembler.cpp:(.text+0x1b8): undefined reference to
> `JSC::__clear_cache(char*, char*)'

__clear_cache is a C function not C++, and does not belongs to JSC namespace.

try this (outside of "namespace JSC", for example put it before the #include-s in a C++ file)

extern "C" {
    void __clear_cache (char *beg, char *end);
}

(You don't need to put extern before a function declaration. The semicolon does the magic.)
Comment 6 Andre Pedralho 2009-09-08 08:06:33 PDT
Created attachment 39186 [details]
Adding the __clear_cache header according to the comment above.

This patch fixes this bug.
Comment 7 Zoltan Herczeg 2009-09-08 08:33:37 PDT
(In reply to comment #6)
> Created an attachment (id=39186) [details]
> Adding the __clear_cache header according to the comment above.
> 
> This patch fixes this bug.

Please put #if PLATFORM(ARM) around the definition. Anyway, do you know why this function is not defined in your gcc? Are you tested it on your platform?
Comment 8 Andre Pedralho 2009-09-08 08:34:11 PDT
Created attachment 39188 [details]
Updated ToT, added ifdefs and changelog according to loki04 suggestions in #qtwebkit.
Comment 9 Eric Seidel (no email) 2009-09-08 10:07:48 PDT
Should this be guarded <= a GCC version?  Or is this expected that this symbol will always be missing and need our own extern declaration?
Comment 10 Zoltan Herczeg 2009-09-08 10:26:32 PDT
(In reply to comment #9)
> Should this be guarded <= a GCC version?  Or is this expected that this symbol
> will always be missing and need our own extern declaration?

I was wondering about it myself. If __clear_cache is defined in the same way, it doesn't matter duplicating it. However, what's happen if not? (I saw a macro definition for __clear_cache somewhere else, which replaces it another function. Unfortunately I didn't remember where) The strange thing is for me, that GCC 4.1.2 should define this function... Especially because only its forward declaration is missing, not the function body.
Comment 11 Zoltan Herczeg 2009-09-08 10:28:39 PDT
http://gcc.gnu.org/onlinedocs/gcc-4.1.2/gccint/Miscellaneous-routines.html

Misc function for gcc 4.1.2
Comment 12 Eric Seidel (no email) 2009-09-08 10:29:18 PDT
Probably not related, but just in case: http://code.google.com/p/android/issues/detail?id=1803
Comment 13 Eric Seidel (no email) 2009-09-08 10:32:15 PDT
Searching for __clear_cache in gcc's bugzilla yields no results: http://gcc.gnu.org/bugzilla/  It seems we should file one and reference it in whatever fix we take.
Comment 14 Eric Seidel (no email) 2009-09-08 14:55:33 PDT
Comment on attachment 39188 [details]
Updated ToT, added ifdefs and changelog according to loki04 suggestions in #qtwebkit.

We really should file a bug with GCC (per their public bug tracker) and link to it in this ChangeLog.
Comment 15 Andre Pedralho 2009-09-08 21:26:20 PDT
(In reply to comment #14)
> (From update of attachment 39188 [details])
> We really should file a bug with GCC (per their public bug tracker) and link to
> it in this ChangeLog.

It is not a GCC bug. I could build* and run the example given in the link in comment #12:

 #include <stdio.h>

 int main(int argc, char **argv)
 {
    printf("attempting toolchain clearcache syscall:\n");
    __clear_cache(0, 0);
    return 0;
 }

* no extra arguments given to build nor run.

ps. There were no need even to add the __clear_cache header definition.
Comment 16 Andre Pedralho 2009-09-09 05:28:12 PDT
(In reply to comment #14)
> (From update of attachment 39188 [details])
> We really should file a bug with GCC (per their public bug tracker) and link to
> it in this ChangeLog.

I did the same test in comment #15 but using a .cpp file and building using g++:

teste.cpp:6: error: '__clear_cache' was not declared in this scope
Comment 17 Gabor Loki 2009-09-09 06:39:39 PDT
I did a quick test with 4.3.2, and the example works with gcc and g++ as well.
I am going to examine what this is about gcc 4.1.2 .
Comment 18 Gabor Loki 2009-09-09 07:56:35 PDT
(In reply to comment #17)
> I did a quick test with 4.3.2, and the example works with gcc and g++ as well.
> I am going to examine what this is about gcc 4.1.2 .

Well, this is not a WebKit bug. G++ 4.1.1 from CodeSourcery (2006q3) also has this bug. The most common error, which could cause this kind of error, is that in the first compilation of gcc have to be compiled without headers and the 2nd one should use library headers. I do not think that they did a wrong packaging/configuring.
Comment 19 Laszlo Gombos 2009-09-11 07:17:02 PDT
I think Andre Pedralho or someone else should not only check that it compiles and links, but also that the __clear_cache actually clears the instruction cache. As it was pointed in the Android related discussion (http://code.google.com/p/android/issues/detail?id=1803) the __clear_cache call could be a no-op, which would than break the JIT functionality.

Also maybe WebKit can do a bit better guarding the __clear_cache call and the fallback assembly code. Fill post a patch for consideration.
Comment 20 Laszlo Gombos 2009-09-11 07:26:12 PDT
Created attachment 39430 [details]
Be more specific with guards.

1./ It looks to me that not only the __clear_cache call but also the fallback assembly assumes GCC (as the assembly syntax is compiler specific) - so maybe this whole code section should be guarded with COMPILER(GCC).

2./ It might be better to use the CLEAR_INSN_CACHE guard to test if __clear_cache is available than testing for a specific GCC version. Andre can you see if CLEAR_INSN_CACHE is defined in your environment.

3./ The fallback ARM code seems to call a Linux system call and as such it would only work on Linux, so maybe we should have a guard for that as well. 

4./ I would also suggest #error messages for the platforms currently not supported
Comment 21 Gabor Loki 2009-09-11 09:31:13 PDT
> 2./ It might be better to use the CLEAR_INSN_CACHE guard to test if
> __clear_cache is available than testing for a specific GCC version. Andre can
> you see if CLEAR_INSN_CACHE is defined in your environment.

CLEAR_INSN_CACHE is definied in gcc/config/arm/linux-eabi.h (GCC source), but it is not explicitly exported. You can check the predefinied macros with 'gcc -c -E -dM empty.file' command.

So, we are not able to use CLEAR_INSN_CACHE. The current guard it better. The _clear_cache function is available since GCC 3.4.6 from lib1funcs.asm.

Btw, please use '#else' directive instead of '#elif' without expression.
Comment 22 Laszlo Gombos 2009-09-11 10:51:01 PDT
Created attachment 39450 [details]
incorporate Gabor's comments.

Gabor thanks for the review. If CLEAR_INSN_CACHE test does not work than the patch is less useful, but maybe still valid.
Comment 23 Andre Pedralho 2009-09-14 12:01:56 PDT
Sorry by the delay!

(In reply to comment #19)
> I think Andre Pedralho or someone else should not only check that it compiles
> and links, but also that the __clear_cache actually clears the instruction
> cache. As it was pointed in the Android related discussion
> (http://code.google.com/p/android/issues/detail?id=1803) the __clear_cache call
> could be a no-op, which would than break the JIT functionality.
>
I could run it using strace and it calls both operation (the g++ __clear_cache and th system __asm __volatile...) defined in http://code.google.com/p/android/issues/detail?id=1803

cacheflush(0, 0, 0, 0x1, 0x40022db0)    = 0
cacheflush(0, 0, 0, 0xf0002, 0x40022db0) = 0

> Also maybe WebKit can do a bit better guarding the __clear_cache call and the
> fallback assembly code. Fill post a patch for consideration.


(In reply to comment #20)
> Created an attachment (id=39430) [details]
> Be more specific with guards.
> 
> 2./ It might be better to use the CLEAR_INSN_CACHE guard to test if
> __clear_cache is available than testing for a specific GCC version. Andre can
> you see if CLEAR_INSN_CACHE is defined in your environment.
>
No, it is not defined. So I don't know if __clear_cache is really clearing cache. The code below builds fine:

#ifdef CLEAR_INSN_CACHE
    xxxxxx
#endif
Comment 24 Gabor Loki 2009-09-22 08:01:06 PDT
Created attachment 39925 [details]
Avoid __clear_cache built-in function if NO_CLEAR_CACHE define is set

This patch adds an additional check for __clear_cache. If NO_CLEAR_CACHE is set the execution will skip __clear_cache case.

I have also fixed a silly typo in the inline assembly.
Comment 25 Gabor Loki 2009-09-22 08:04:38 PDT
(In reply to comment #22)
> Created an attachment (id=39450) [details]

Laszlo, I think your patch is obsolete now. I did a small refactoring earlier.
Comment 26 Darin Adler 2009-09-22 08:21:16 PDT
Comment on attachment 39925 [details]
Avoid __clear_cache built-in function if NO_CLEAR_CACHE define is set

The change log mentions only one change, but there's also a change to the assembly code. Is that second change unintentional or is it the change log that needs to be updated?
Comment 27 Andre Pedralho 2009-09-22 08:37:46 PDT
(In reply to comment #24)
> Created an attachment (id=39925) [details]
> Avoid __clear_cache built-in function if NO_CLEAR_CACHE define is set
> 
> This patch adds an additional check for __clear_cache. If NO_CLEAR_CACHE is set
> the execution will skip __clear_cache case.
> 
> I have also fixed a silly typo in the inline assembly.

As I told you in #qtwebkit: we are fixing the issue disabling the feature,
aren't we? I know it is a very specific case (ARM and GCC 4.1.2).

Couldn't we do something using CLEAR_INSN_CACHE (as we don't have it defined)?
BTW, does anybody have it defined?

- #elif PLATFORM(ARM) && COMPILER(GCC) && (GCC_VERSION >= 30406)
+ #elif PLATFORM(ARM) && COMPILER(GCC) && (GCC_VERSION >= 30406) &&
defined(CLEAR_INSN_CACHE)   
     static void cacheFlush(void* code, size_t size)
     {   
         __clear_cache(reinterpret_cast<char*>(code),
reinterpret_cast<char*>(code) + size);
     }
- #elif PLATFORM(ARM_TRADITIONAL) && PLATFORM(LINUX)
+ #elif PLATFORM(ARM_TRADITIONAL) && PLATFORM(LINUX) ||
!defined(CLEAR_INSN_CACHE)
     static void cacheFlush(void* code, size_t size)
     {   
         asm volatile (
             "push    {r7}\n"
             "mov     r0, %0\n"
Comment 28 Gabor Loki 2009-09-22 08:54:42 PDT
(In reply to comment #26)
> The change log mentions only one change, but there's also a change to the
> assembly code.

You are right. There was a small typo in the inline asm. I will update the ChangeLog if this solution is fine for Andre as well.
Comment 29 Gabor Loki 2009-09-22 09:23:51 PDT
Created attachment 39927 [details]
Avoid __clear_cache built-in function if DISABLE_BUILTIN_CLEAR_CACHE define is set

Updated the name of the define and the ChangeLog entry as well.
Comment 30 Andre Pedralho 2009-09-22 11:01:40 PDT
(In reply to comment #28)
> (In reply to comment #26)
> > The change log mentions only one change, but there's also a change to the
> > assembly code.
> 
> You are right. There was a small typo in the inline asm. I will update the
> ChangeLog if this solution is fine for Andre as well.

It is fine for me. Thanks again Loki!

The new define name (DISABLE_BUILTIN_CLEAR_CACHE) is more intuitive than the older (NO_CLEAR_CACHE) for what it really does and unfortunately my suggestion in comment #23 would not work as CLEAR_INSN_CACHE is a GCC internal define.
Comment 31 Laszlo Gombos 2009-09-22 12:49:54 PDT
Comment on attachment 39450 [details]
incorporate Gabor's comments.

This is now obsolete thanks to Gabor after http://trac.webkit.org/changeset/48527. Laszlo.
Comment 32 Gavin Barraclough 2009-09-23 15:33:41 PDT
Just r+ing the last patch, since it is not clear to me from the comments whether the first is still required by anyone.
Comment 33 Gabor Loki 2009-09-23 21:57:47 PDT
(In reply to comment #32)
> Just r+ing the last patch, since it is not clear to me from the comments
> whether the first is still required by anyone.

AFAIK Andre is happy with the last patch. 

The attachment 39188 [details] (the first patch) is not a good approach to achieve access to clear cache.

I will file a bug report about this in GCC's bugzilla.
Comment 34 Zoltan Horvath 2009-09-23 23:12:48 PDT
Landed in @48702. http://trac.webkit.org/changeset/48702
Comment 35 Gabor Loki 2009-09-28 09:19:35 PDT
Created attachment 40240 [details]
Remove __clear_cache which is an internal function of GCC

Finally I got an concrete answer from GCC about __clear_cache.

Although __clear_cache is exported from GCC, it is an internal function. GCC makes no promises about it. So, we should remove it.

The remaining flushing mechanism will be the inline assembly on ARM-Linux.
Comment 36 Simon Hausmann 2009-09-28 11:56:26 PDT
Comment on attachment 40240 [details]
Remove __clear_cache which is an internal function of GCC

Thanks Gabor!
Comment 37 Simon Hausmann 2009-09-28 11:57:59 PDT
Committed r48824: <http://trac.webkit.org/changeset/48824>