RESOLVED FIXED 29079
ARM compiler does not understand GCC visibility attribute
https://bugs.webkit.org/show_bug.cgi?id=29079
Summary ARM compiler does not understand GCC visibility attribute
Laszlo Gombos
Reported 2009-09-09 04:47:14 PDT
Thi will result in a compilation warning which pollutes the build output. In JavaScriptCore/API/JSBase.h instead of just testing for defined(__GNUC__), there should also be a check to exclude the ARM compiler. Essentially what we want here is a test for COMPILER(GCC), but since Platform.h is not visible from JavaScriptCore/API/JSBase.h the logic for testing for COMPILER(GCC) needs to be repeated.
Attachments
proposed patch (1.12 KB, patch)
2009-09-09 04:51 PDT, Laszlo Gombos
no flags
Laszlo Gombos
Comment 1 2009-09-09 04:51:56 PDT
Created attachment 39263 [details] proposed patch
Laszlo Gombos
Comment 2 2009-09-09 05:06:04 PDT
The warning message - that this patch is trying to address - is the following: "\webkit\JavaScriptCore\API\JSObjectRef.h", line 596: Warning: #1207-D: attribute "visibility" ignored JS_EXPORT bool JSObjectSetPrivate(JSObjectRef object, void* data); ^
Darin Adler
Comment 3 2009-09-09 07:35:31 PDT
Comment on attachment 39263 [details] proposed patch Is it really the ARM issue that makes this not support visibility? Or is it version-specific? r=me, but I suspect this is not as good a condition as it could be.
Laszlo Gombos
Comment 4 2009-09-09 09:39:10 PDT
(In reply to comment #3) > (From update of attachment 39263 [details]) > Is it really the ARM issue that makes this not support visibility? Or is it > version-specific? r=me, but I suspect this is not as good a condition as it > could be. Darin, thanks for the review. This is an issue with the compiler syntax. ARM compiler does support visibility declarations (which is different from GCC so the extra test is needed). For now we do not really need the visibility decaration for the ARM ports now so my patch leaves the JS_EXPORT empty for ARM.
Kenneth Rohde Christiansen
Comment 5 2009-09-09 10:08:25 PDT
(In reply to comment #4) > (In reply to comment #3) > > (From update of attachment 39263 [details] [details]) > > Is it really the ARM issue that makes this not support visibility? Or is it > > version-specific? r=me, but I suspect this is not as good a condition as it > > could be. > > Darin, thanks for the review. > > This is an issue with the compiler syntax. ARM compiler does support visibility > declarations (which is different from GCC so the extra test is needed). For now > we do not really need the visibility decaration for the ARM ports now so my > patch leaves the JS_EXPORT empty for ARM. Who is "we"? Boston Team? Will this affect compiling Qt 4.6 for ARM in general?
Laszlo Gombos
Comment 6 2009-09-09 10:28:32 PDT
(In reply to comment #5) > (In reply to comment #4) > > (In reply to comment #3) > > > (From update of attachment 39263 [details] [details] [details]) > > > Is it really the ARM issue that makes this not support visibility? Or is it > > > version-specific? r=me, but I suspect this is not as good a condition as it > > > could be. > > > > Darin, thanks for the review. > > > > This is an issue with the compiler syntax. ARM compiler does support visibility > > declarations (which is different from GCC so the extra test is needed). For now > > we do not really need the visibility decaration for the ARM ports now so my > > patch leaves the JS_EXPORT empty for ARM. > > Who is "we"? Boston Team? Will this affect compiling Qt 4.6 for ARM in general? Kenneth, sorry for the vague use of words. The change only has an impact for the ARMCC compiler (also known as ADS or RVCT). As far as I know only the Symbian port using the RVCT compiler, although in theory other ports/platforms might be able to use it as well. This change has no impact on those ARM ports (including the QtWebKit ARM ports) that are using GCC.
Laszlo Gombos
Comment 7 2009-09-09 11:14:06 PDT
Added Kent Hansen and Simon Hausmann to the CC list and set commit-queue- to give a change for Kent and Simon to look at this patch.
Laszlo Gombos
Comment 8 2009-09-10 06:23:07 PDT
Note You need to log in before you can comment on or make changes to this bug.