29079 2009-09-09 04:47:14 -0700 ARM compiler does not understand GCC visibility attribute 2009-09-10 06:23:07 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Other Other RESOLVED FIXED Qt P2 Normal --- 0 laszlo.gombos webkit-unassigned hausmann kenneth kent.hansen oldest_to_newest 145704 0 laszlo.gombos 2009-09-09 04:47:14 -0700 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. 145705 1 39263 laszlo.gombos 2009-09-09 04:51:56 -0700 Created attachment 39263 proposed patch 145707 2 laszlo.gombos 2009-09-09 05:06:04 -0700 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); ^ 145742 3 39263 darin 2009-09-09 07:35:31 -0700 Comment on attachment 39263 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. 145799 4 laszlo.gombos 2009-09-09 09:39:10 -0700 (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. 145814 5 kenneth 2009-09-09 10:08:25 -0700 (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? 145822 6 laszlo.gombos 2009-09-09 10:28:32 -0700 (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. 145854 7 laszlo.gombos 2009-09-09 11:14:06 -0700 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. 146177 8 laszlo.gombos 2009-09-10 06:23:07 -0700 Committed as http://trac.webkit.org/changeset/48251 39263 2009-09-09 04:51:56 -0700 2009-09-10 05:46:12 -0700 proposed patch patch_29079.txt text/plain 1152 laszlo.gombos SW5kZXg6IEphdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBKYXZhU2NyaXB0 Q29yZS9DaGFuZ2VMb2cJKHJldmlzaW9uIDQ4MjA0KQorKysgSmF2YVNjcmlwdENvcmUvQ2hhbmdl TG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTMgQEAKKzIwMDktMDktMDkgIExhc3psbyBH b21ib3MgIDxsYXN6bG8uMS5nb21ib3NAbm9raWEuY29tPgorCisgICAgICAgIFJldmlld2VkIGJ5 IE5PQk9EWSAoT09QUyEpLgorCisgICAgICAgIEFSTSBjb21waWxlciBkb2VzIG5vdCB1bmRlcnN0 YW5kIEdDQyB2aXNpYmlsaXR5IGF0dHJpYnV0ZQorICAgICAgICBodHRwczovL2J1Z3Mud2Via2l0 Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MjkwNzkKKworICAgICAgICAqIEFQSS9KU0Jhc2UuaDogTWFr ZSB0aGUgdGVzdCBtb3JlIHNwZWNpZmljIHRvIGhpdCBvbmx5CisgICAgICAgIHRoZSBHQ0MgY29t cGlsZXIKKwogMjAwOS0wOS0wOCAgT2xpdmVyIEh1bnQgIDxvbGl2ZXJAYXBwbGUuY29tPgogCiAg ICAgICAgIFJldmlld2VkIGJ5IE1hY2llaiBTdGFjaG93aWFrLgpJbmRleDogSmF2YVNjcmlwdENv cmUvQVBJL0pTQmFzZS5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT0KLS0tIEphdmFTY3JpcHRDb3JlL0FQSS9KU0Jhc2Uu aAkocmV2aXNpb24gNDgxNjUpCisrKyBKYXZhU2NyaXB0Q29yZS9BUEkvSlNCYXNlLmgJKHdvcmtp bmcgY29weSkKQEAgLTY3LDcgKzY3LDcgQEAgdHlwZWRlZiBzdHJ1Y3QgT3BhcXVlSlNWYWx1ZSog SlNPYmplY3RSZQogI3VuZGVmIEpTX0VYUE9SVAogI2lmIGRlZmluZWQoQlVJTERJTkdfV1hfXykK ICAgICAjZGVmaW5lIEpTX0VYUE9SVAotI2VsaWYgZGVmaW5lZChfX0dOVUNfXykKKyNlbGlmIGRl ZmluZWQoX19HTlVDX18pICYmICFkZWZpbmVkKF9fQ0NfQVJNKSAmJiAhZGVmaW5lZChfX0FSTUND X18pCiAgICAgI2RlZmluZSBKU19FWFBPUlQgX19hdHRyaWJ1dGVfXygodmlzaWJpbGl0eSgiZGVm YXVsdCIpKSkKICNlbGlmIGRlZmluZWQoX1dJTjMyX1dDRSkKICAgICAjaWYgZGVmaW5lZChKU19C VUlMRElOR19KUykK