Bug 28886

pedralho, any luck to build w/ jit, yacc and friends all off ? please also inform revision you were at.

(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

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.

(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*)'

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

Created attachment 39186 [details] Adding the __clear_cache header according to the comment above. This patch fixes this bug.

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

Created attachment 39188 [details] Updated ToT, added ifdefs and changelog according to loki04 suggestions in #qtwebkit.

Should this be guarded <= a GCC version? Or is this expected that this symbol will always be missing and need our own extern declaration?

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

http://gcc.gnu.org/onlinedocs/gcc-4.1.2/gccint/Miscellaneous-routines.html Misc function for gcc 4.1.2

Probably not related, but just in case: http://code.google.com/p/android/issues/detail?id=1803

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

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

(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

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 .

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

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.

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

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

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.

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

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.

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

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

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

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.

(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 on attachment 39450 [details] incorporate Gabor's comments. This is now obsolete thanks to Gabor after http://trac.webkit.org/changeset/48527. Laszlo.

Just r+ing the last patch, since it is not clear to me from the comments whether the first is still required by anyone.

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

Landed in @48702. http://trac.webkit.org/changeset/48702

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 on attachment 40240 [details] Remove __clear_cache which is an internal function of GCC Thanks Gabor!

Committed r48824: <http://trac.webkit.org/changeset/48824>