Bug 29079 – ARM compiler does not understand GCC visibility attribute

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Laszlo Gombos]

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.

[Laszlo Gombos]

Created attachment 39263 [details]
proposed patch

[Laszlo Gombos]

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

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

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

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

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]

Committed as http://trac.webkit.org/changeset/48251