Re: Define JNIEXPORT as visibility default with GCC?
I've bcc'd jdk8-dev and re-added core-libs and hotspot-runtime as that is where the original thread was. David On 9/04/2013 11:49 AM, Coleen Phillimore wrote: Hi Martin, I'm sorry, I lost track of this and thought it was already pushed. The jni_md.h changes look good but I don't really understand why the awt changes were made, or what they do. Since the jdk doesn't usually push using JPRT, I'm afraid to push this directly myself without a review from someone from jdk8-...@openjdk.com. I have cc'ed them. I think someone from the tools and libraries group should review and push this. Thanks, Coleen On 4/8/2013 7:35 PM, Martin Buchholz wrote: friendly ping. I'd like to have an approve to push this (or have someone jprt for me). On Mon, Mar 11, 2013 at 4:57 PM, Martin Buchholz marti...@google.com mailto:marti...@google.com wrote: The latest version of my webrev is here: http://cr.openjdk.java.net/~martin/webrevs/openjdk8/JNIEXPORT/ http://cr.openjdk.java.net/%7Emartin/webrevs/openjdk8/JNIEXPORT/ It includes this line: Reviewed-by: coleenp, ddehaven, dcubed Ok to push? On Fri, Mar 8, 2013 at 10:31 AM, Coleen Phillmore coleen.phillim...@oracle.com mailto:coleen.phillim...@oracle.com wrote: The hotspot definitions of JNIEXPORT don't match in all the files to the JDK definition. I think a hotspot bug should be filed to fix the jni_cpu.h definitions which now none of them match. After someone in core-libs checks this in, we'll update the hotspot files to match the final version and retest -fvisibility=hidden. I don't remember why the JDK version wasn't fixed with the original -fvisibility=hidden work. Coleen On 2/28/2013 3:56 PM, Daniel D. Daugherty wrote: On 2/28/13 11:57 AM, David DeHaven wrote: Has a bug been filed for this? -DrD- As mentioned earlier in this thread... Dan On 2/19/13 5:21 PM, Daniel D. Daugherty wrote: I couldn't find a 'jdk' repo relevant bug for this issue so I filed: 8008509: 6588413 changed JNIEXPORT visibility for GCC on HSX, jdk's jni_md.h needs similar change http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8008509 https://jbs.oracle.com/bugs/browse/JDK-8008509 Coleen did the original work on 6588413 so I added her to the interest list for the new bug. The need for an update to the jdk repo's jni_md.h file was raised during the code review for 6588413, but that detail appears to have been dropped. Dan
Re: Define JNIEXPORT as visibility default with GCC?
The latest version of my webrev is here: http://cr.openjdk.java.net/~martin/webrevs/openjdk8/JNIEXPORT/ It includes this line: Reviewed-by: coleenp, ddehaven, dcubed Ok to push? On Fri, Mar 8, 2013 at 10:31 AM, Coleen Phillmore coleen.phillim...@oracle.com wrote: The hotspot definitions of JNIEXPORT don't match in all the files to the JDK definition. I think a hotspot bug should be filed to fix the jni_cpu.h definitions which now none of them match. After someone in core-libs checks this in, we'll update the hotspot files to match the final version and retest -fvisibility=hidden. I don't remember why the JDK version wasn't fixed with the original -fvisibility=hidden work. Coleen On 2/28/2013 3:56 PM, Daniel D. Daugherty wrote: On 2/28/13 11:57 AM, David DeHaven wrote: Has a bug been filed for this? -DrD- As mentioned earlier in this thread... Dan On 2/19/13 5:21 PM, Daniel D. Daugherty wrote: I couldn't find a 'jdk' repo relevant bug for this issue so I filed: 8008509: 6588413 changed JNIEXPORT visibility for GCC on HSX, jdk's jni_md.h needs similar change http://bugs.sun.com/**bugdatabase/view_bug.do?bug_**id=8008509http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8008509 https://jbs.oracle.com/bugs/**browse/JDK-8008509https://jbs.oracle.com/bugs/browse/JDK-8008509 Coleen did the original work on 6588413 so I added her to the interest list for the new bug. The need for an update to the jdk repo's jni_md.h file was raised during the code review for 6588413, but that detail appears to have been dropped. Dan
Re: Define JNIEXPORT as visibility default with GCC?
Those changes look fine to me. None of those typedefs make sense with JNIEXPORT since they're only used locally, and most are scoped to a single function… I agree with the __has_attribute comment. The next step would be to try setting -fvisibility=hidden and -fvisibility-inlines-hidden :) I encountered a few issues trying to get that working. -DrD- More changes. I discovered that with JNIEXPORT defined to use the attribute, typedefs that included JNIEXPORT would generate a gcc -Wattribute warning warning: visibility attribute ignored. These new warnings are annoying, but arguably, the use of JNIEXPORT in a typedef was a bug all along - JNIEXPORT should be all about actual symbols, not types. (But I couldn't find any actual spec for JNIEXPORT; could somebody please fix that?). So I removed all occurences of JNIEXPORT inside a typedef that caused a -Wattribute warning. http://cr.openjdk.java.net/~martin/webrevs/openjdk8/JNIEXPORT/ Martin On Tue, Mar 5, 2013 at 5:45 PM, Martin Buchholz marti...@google.com wrote: Another rev of this change. I added the condition check for gcc 4.2 (I don't much care either way) I just ran clang for the first time, and was surprised that clang has values of __GNUC__ == 4 and __GNUC_MINOR__ == 2 I think we should keep the __has_attribute check, because it seems clearly correct and recommended by http://clang.llvm.org/docs/LanguageExtensions.html#feature-checking-macros and with luck, __has_attribute will become a minor industry standard from the innovators in llvm-land. I really like the design of their extension mechanisms. On Wed, Feb 27, 2013 at 3:07 PM, Martin Buchholz marti...@google.comwrote: Here's the latest version of the proposed patch: http://cr.openjdk.java.net/~martin/webrevs/openjdk8/JNIEXPORT/ that has been tested, but only with gcc 4.6, but is also written to work with llvm.
Re: Define JNIEXPORT as visibility default with GCC?
The hotspot definitions of JNIEXPORT don't match in all the files to the JDK definition. I think a hotspot bug should be filed to fix the jni_cpu.h definitions which now none of them match. After someone in core-libs checks this in, we'll update the hotspot files to match the final version and retest -fvisibility=hidden. I don't remember why the JDK version wasn't fixed with the original -fvisibility=hidden work. Coleen On 2/28/2013 3:56 PM, Daniel D. Daugherty wrote: On 2/28/13 11:57 AM, David DeHaven wrote: Has a bug been filed for this? -DrD- As mentioned earlier in this thread... Dan On 2/19/13 5:21 PM, Daniel D. Daugherty wrote: I couldn't find a 'jdk' repo relevant bug for this issue so I filed: 8008509: 6588413 changed JNIEXPORT visibility for GCC on HSX, jdk's jni_md.h needs similar change http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8008509 https://jbs.oracle.com/bugs/browse/JDK-8008509 Coleen did the original work on 6588413 so I added her to the interest list for the new bug. The need for an update to the jdk repo's jni_md.h file was raised during the code review for 6588413, but that detail appears to have been dropped. Dan
Re: Define JNIEXPORT as visibility default with GCC?
Another rev of this change. I added the condition check for gcc 4.2 (I don't much care either way) I just ran clang for the first time, and was surprised that clang has values of __GNUC__ == 4 and __GNUC_MINOR__ == 2 I think we should keep the __has_attribute check, because it seems clearly correct and recommended by http://clang.llvm.org/docs/LanguageExtensions.html#feature-checking-macros and with luck, __has_attribute will become a minor industry standard from the innovators in llvm-land. I really like the design of their extension mechanisms. On Wed, Feb 27, 2013 at 3:07 PM, Martin Buchholz marti...@google.comwrote: Here's the latest version of the proposed patch: http://cr.openjdk.java.net/~martin/webrevs/openjdk8/JNIEXPORT/ that has been tested, but only with gcc 4.6, but is also written to work with llvm.
Re: Define JNIEXPORT as visibility default with GCC?
More changes. I discovered that with JNIEXPORT defined to use the attribute, typedefs that included JNIEXPORT would generate a gcc -Wattribute warning warning: visibility attribute ignored. These new warnings are annoying, but arguably, the use of JNIEXPORT in a typedef was a bug all along - JNIEXPORT should be all about actual symbols, not types. (But I couldn't find any actual spec for JNIEXPORT; could somebody please fix that?). So I removed all occurences of JNIEXPORT inside a typedef that caused a -Wattribute warning. http://cr.openjdk.java.net/~martin/webrevs/openjdk8/JNIEXPORT/ Martin On Tue, Mar 5, 2013 at 5:45 PM, Martin Buchholz marti...@google.com wrote: Another rev of this change. I added the condition check for gcc 4.2 (I don't much care either way) I just ran clang for the first time, and was surprised that clang has values of __GNUC__ == 4 and __GNUC_MINOR__ == 2 I think we should keep the __has_attribute check, because it seems clearly correct and recommended by http://clang.llvm.org/docs/LanguageExtensions.html#feature-checking-macros and with luck, __has_attribute will become a minor industry standard from the innovators in llvm-land. I really like the design of their extension mechanisms. On Wed, Feb 27, 2013 at 3:07 PM, Martin Buchholz marti...@google.comwrote: Here's the latest version of the proposed patch: http://cr.openjdk.java.net/~martin/webrevs/openjdk8/JNIEXPORT/ that has been tested, but only with gcc 4.6, but is also written to work with llvm.
Re: Define JNIEXPORT as visibility default with GCC?
Here's the latest version of the proposed patch: http://cr.openjdk.java.net/~martin/webrevs/openjdk8/JNIEXPORT/ that has been tested, but only with gcc 4.6, but is also written to work with llvm. This looks fine, however since 4.2.1 is still used for some builds we need to filter that specific version out or this will cause grief in other groups. The __has_attribute check is also redundant since clang by design pretends to be gcc so the __GNUC__ check should be sufficient (clang claims GCC compatibility as one of it's selling features). LLVM is the backend to the compiler, it doesn't see the source code. My jawt_md.h fix (that I haven't touched in well over month) has a separate jni_md.h for Mac since it has to have it's own headers to properly fix the issue, it wouldn't affect Linux/Solaris builds at all though. I think that would be more appropriate for Mac users, who haven't used 4.2 in some time. If I can actually find some time this week I may be able to post it for review... There's also the matter of the hotspot header being wrong. Has a bug been filed for this? -DrD-
Re: Define JNIEXPORT as visibility default with GCC?
Here's the latest version of the proposed patch: http://cr.openjdk.java.net/~martin/webrevs/openjdk8/JNIEXPORT/ that has been tested, but only with gcc 4.6, but is also written to work with llvm. This looks fine, however since 4.2.1 I meant 4.1.2… -DrD-
Re: Define JNIEXPORT as visibility default with GCC?
On 2/28/13 11:57 AM, David DeHaven wrote: Has a bug been filed for this? -DrD- As mentioned earlier in this thread... Dan On 2/19/13 5:21 PM, Daniel D. Daugherty wrote: I couldn't find a 'jdk' repo relevant bug for this issue so I filed: 8008509: 6588413 changed JNIEXPORT visibility for GCC on HSX, jdk's jni_md.h needs similar change http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8008509 https://jbs.oracle.com/bugs/browse/JDK-8008509 Coleen did the original work on 6588413 so I added her to the interest list for the new bug. The need for an update to the jdk repo's jni_md.h file was raised during the code review for 6588413, but that detail appears to have been dropped. Dan
Re: Define JNIEXPORT as visibility default with GCC?
Here's the latest version of the proposed patch: http://cr.openjdk.java.net/~martin/webrevs/openjdk8/JNIEXPORT/ that has been tested, but only with gcc 4.6, but is also written to work with llvm.
Re: Define JNIEXPORT as visibility default with GCC?
Since the proposed change is user-visible (i.e., not just for building the JDK, but also for building pretty much every JNI blob in existence), wouldn't you want the correct check anyway? Does the JDK have rules about which version of gcc you are allowed to use to build JNI? Jeremy On Mon, Feb 18, 2013 at 1:36 AM, Florian Weimer fwei...@redhat.com wrote: On 02/17/2013 11:55 PM, David Holmes wrote: On 16/02/2013 2:46 AM, Florian Weimer wrote: On 02/15/2013 05:41 PM, Jeremy Manson wrote: Can we just blacklist 4.1.2 explicitly? Folks, per README-build.html, the minimum GCC version is 4.2 on MacOS X and 4.3 everywhere else. Do we really have to bother with 4.1.2 at this point? There are ports that still use these older gcc versions. Oh. Perhaps this information can be added to README-builds.html? -- Florian Weimer / Red Hat Product Security Team
Re: Define JNIEXPORT as visibility default with GCC?
I couldn't find a 'jdk' repo relevant bug for this issue so I filed: 8008509: 6588413 changed JNIEXPORT visibility for GCC on HSX, jdk's jni_md.h needs similar change http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=8008509 https://jbs.oracle.com/bugs/browse/JDK-8008509 Coleen did the original work on 6588413 so I added her to the interest list for the new bug. The need for an update to the jdk repo's jni_md.h file was raised during the code review for 6588413, but that detail appears to have been dropped. Dan On 2/19/13 1:20 PM, Jeremy Manson wrote: Since the proposed change is user-visible (i.e., not just for building the JDK, but also for building pretty much every JNI blob in existence), wouldn't you want the correct check anyway? Does the JDK have rules about which version of gcc you are allowed to use to build JNI? Jeremy On Mon, Feb 18, 2013 at 1:36 AM, Florian Weimer fwei...@redhat.com mailto:fwei...@redhat.com wrote: On 02/17/2013 11:55 PM, David Holmes wrote: On 16/02/2013 2:46 AM, Florian Weimer wrote: On 02/15/2013 05:41 PM, Jeremy Manson wrote: Can we just blacklist 4.1.2 explicitly? Folks, per README-build.html, the minimum GCC version is 4.2 on MacOS X and 4.3 everywhere else. Do we really have to bother with 4.1.2 at this point? There are ports that still use these older gcc versions. Oh. Perhaps this information can be added to README-builds.html? -- Florian Weimer / Red Hat Product Security Team
Re: Define JNIEXPORT as visibility default with GCC?
On 16/02/2013 2:46 AM, Florian Weimer wrote: On 02/15/2013 05:41 PM, Jeremy Manson wrote: Can we just blacklist 4.1.2 explicitly? Folks, per README-build.html, the minimum GCC version is 4.2 on MacOS X and 4.3 everywhere else. Do we really have to bother with 4.1.2 at this point? There are ports that still use these older gcc versions. David
Re: Define JNIEXPORT as visibility default with GCC?
On 15/02/2013 5:26 PM, Jeremy Manson wrote: a) I don't know what's going on behind the scenes, but if this sounds like a good idea to folks, can we open a bug and get the process otherwise rolling? b) Martin, where did the 4.2 restriction come from? Both Apple's site and the gcc wiki say that visibility support arrived in 4.0: From the original push for 6588413 in linux gcc.make: +# version 4 and above support fvisibility=hidden (matches jni_x86.h file) +# except 4.1.2 gives pointless warnings that can't be disabled (afaik) So it was limited on x86 to 2 (which I think was a typo: intended to be =2 or 1 ?) Of course the bsd port copied the linux file. David http://gcc.gnu.org/wiki/Visibility https://developer.apple.com/library/mac/#documentation/DeveloperTools/Conceptual/CppRuntimeEnv/Articles/SymbolVisibility.html On Thu, Feb 14, 2013 at 3:01 PM, David DeHaven david.deha...@oracle.com mailto:david.deha...@oracle.com wrote: +#if defined(__GNUC__) (__GNUC__ 4) || (__GNUC__ == 4) (__GNUC_MINOR__ 2) + #define JNIEXPORT __attribute__((visibility(default))) + #define JNIIMPORT __attribute__((visibility(default))) The default compiler in Xcode 4.1 is llvm-gcc 4.2, it seems. The conditional above excludes that. Is this intentional? It's *is* gcc, with a LLVM backend. Yes, but it identifies itself as GCC 4.2, so the conditional doesn't fire. I assume this was not the intent and the version check is just wrong. It may be that they deliberately stayed with gcc 4.2 to keep parity with clang, which only supports 4.2 (or it may not, because clang supports lots of 4.3+ features). If Xcode is fine with the #define, I suggest to drop the version check completely. We already do not support compiling with GCC versions which are so old that they lack visibility support. If it were Mac only, I'd agree. The same header is currently used for all unix-like OS's (which may change, if I have my way), so Solaris and Linux would also be affected. Most Linux distros have used gcc 4 for quite a while now, I've no idea what Solaris uses and embedded targets are a wild mishmash of whatever someone manages to cobble together, so the simpler __GNUC__ check may still be appropriate. -DrD-
Re: Define JNIEXPORT as visibility default with GCC?
a) I don't know what's going on behind the scenes, but if this sounds like a good idea to folks, can we open a bug and get the process otherwise rolling? b) Martin, where did the 4.2 restriction come from? Both Apple's site and the gcc wiki say that visibility support arrived in 4.0: http://gcc.gnu.org/wiki/Visibility https://developer.apple.com/library/mac/#documentation/DeveloperTools/Conceptual/CppRuntimeEnv/Articles/SymbolVisibility.html I've been poking at http://bugs.sun.com/bugdatabase/view_bug.do?bug_id=7181710 in my spare time (I've not a lot of these days). That issue is, IMHO, an even more glaring issue for what should be obvious reasons. The best solution seems to be to create a new set of headers for export in jdk/src/macosx/javavm/export since the implementation is very system dependent. I could roll this change as part of that as well as clean up some of the #if flow control that would no longer have any effect. I have that mostly done, but I need more time for testing the header and doing JPRT runs to be sure I haven't broken other platforms in the process. That leaves the solaris headers and a separate issue would have to be filed to fix hotspot. -DrD-
Re: Define JNIEXPORT as visibility default with GCC?
a) I don't know what's going on behind the scenes, but if this sounds like a good idea to folks, can we open a bug and get the process otherwise rolling? b) Martin, where did the 4.2 restriction come from? Both Apple's site and the gcc wiki say that visibility support arrived in 4.0: From the original push for 6588413 in linux gcc.make: +# version 4 and above support fvisibility=hidden (matches jni_x86.h file) +# except 4.1.2 gives pointless warnings that can't be disabled (afaik) So it was limited on x86 to 2 (which I think was a typo: intended to be =2 or 1 ?) Of course the bsd port copied the linux file. That makes sense. Is 4.1.2 in use any more? Was the warning due to -fvisibility=hidden or the __attribute__? The attribute should be benign as it's the default setting unless you explicitly set visibility when running the compiler, it only has effect when you compile with visibility != default so I see no harm in using it. -DrD-
Re: Define JNIEXPORT as visibility default with GCC?
Can we just blacklist 4.1.2 explicitly? Jeremy On Fri, Feb 15, 2013 at 2:20 AM, David Holmes david.hol...@oracle.comwrote: On 15/02/2013 5:26 PM, Jeremy Manson wrote: a) I don't know what's going on behind the scenes, but if this sounds like a good idea to folks, can we open a bug and get the process otherwise rolling? b) Martin, where did the 4.2 restriction come from? Both Apple's site and the gcc wiki say that visibility support arrived in 4.0: From the original push for 6588413 in linux gcc.make: +# version 4 and above support fvisibility=hidden (matches jni_x86.h file) +# except 4.1.2 gives pointless warnings that can't be disabled (afaik) So it was limited on x86 to 2 (which I think was a typo: intended to be =2 or 1 ?) Of course the bsd port copied the linux file. David http://gcc.gnu.org/wiki/**Visibility http://gcc.gnu.org/wiki/Visibility https://developer.apple.com/**library/mac/#documentation/** DeveloperTools/Conceptual/**CppRuntimeEnv/Articles/** SymbolVisibility.htmlhttps://developer.apple.com/library/mac/#documentation/DeveloperTools/Conceptual/CppRuntimeEnv/Articles/SymbolVisibility.html On Thu, Feb 14, 2013 at 3:01 PM, David DeHaven david.deha...@oracle.com mailto:david.dehaven@oracle.**com david.deha...@oracle.com wrote: +#if defined(__GNUC__) (__GNUC__ 4) || (__GNUC__ == 4) (__GNUC_MINOR__ 2) + #define JNIEXPORT __attribute__((visibility(** default))) + #define JNIIMPORT __attribute__((visibility(** default))) The default compiler in Xcode 4.1 is llvm-gcc 4.2, it seems. The conditional above excludes that. Is this intentional? It's *is* gcc, with a LLVM backend. Yes, but it identifies itself as GCC 4.2, so the conditional doesn't fire. I assume this was not the intent and the version check is just wrong. It may be that they deliberately stayed with gcc 4.2 to keep parity with clang, which only supports 4.2 (or it may not, because clang supports lots of 4.3+ features). If Xcode is fine with the #define, I suggest to drop the version check completely. We already do not support compiling with GCC versions which are so old that they lack visibility support. If it were Mac only, I'd agree. The same header is currently used for all unix-like OS's (which may change, if I have my way), so Solaris and Linux would also be affected. Most Linux distros have used gcc 4 for quite a while now, I've no idea what Solaris uses and embedded targets are a wild mishmash of whatever someone manages to cobble together, so the simpler __GNUC__ check may still be appropriate. -DrD-
Re: Define JNIEXPORT as visibility default with GCC?
On 02/15/2013 05:41 PM, Jeremy Manson wrote: Can we just blacklist 4.1.2 explicitly? Folks, per README-build.html, the minimum GCC version is 4.2 on MacOS X and 4.3 everywhere else. Do we really have to bother with 4.1.2 at this point? -- Florian Weimer / Red Hat Product Security Team
Re: Define JNIEXPORT as visibility default with GCC?
On 02/14/2013 02:32 AM, Martin Buchholz wrote: +#if defined(__GNUC__) (__GNUC__ 4) || (__GNUC__ == 4) (__GNUC_MINOR__ 2) + #define JNIEXPORT __attribute__((visibility(default))) + #define JNIIMPORT __attribute__((visibility(default))) The default compiler in Xcode 4.1 is llvm-gcc 4.2, it seems. The conditional above excludes that. Is this intentional? If Xcode 4.1 supports visibility properly, you can drop the comparisons because the minimum GCC version on other architectures is already 4.3. (And that's from an outdated README-builds.html—we might require a more recent version in practice.) -- Florian Weimer / Red Hat Product Security Team
Re: Define JNIEXPORT as visibility default with GCC?
+#if defined(__GNUC__) (__GNUC__ 4) || (__GNUC__ == 4) (__GNUC_MINOR__ 2) + #define JNIEXPORT __attribute__((visibility(default))) + #define JNIIMPORT __attribute__((visibility(default))) The default compiler in Xcode 4.1 is llvm-gcc 4.2, it seems. The conditional above excludes that. Is this intentional? It's *is* gcc, with a LLVM backend. -DrD-
Re: Define JNIEXPORT as visibility default with GCC?
This seems like an obvious improvement. There are already bunches of places in the jdk sources that do things like: ./hotspot/src/cpu/x86/vm/jni_x86.h #if defined(SOLARIS) || defined(LINUX) || defined(_ALLBSD_SOURCE) #if defined(__GNUC__) (__GNUC__ 4) || (__GNUC__ == 4) (__GNUC_MINOR__ 2) #define JNIEXPORT __attribute__((visibility(default))) #define JNIIMPORT __attribute__((visibility(default))) #else #define JNIEXPORT #define JNIIMPORT #endif That version check didn't work for me running on Mac OS X (using llvm-gcc 4.2). If that's what hotspot is using then they may not be getting the desired effect. The problem is __GNUC_MINOR__ is 2, so it needs to be changed to = 2. Gnu recommends just using #if __GNUC__ = 4, and everything I've read about it says it's 4.0 and later, so I don't understand why the minor version check is even in there. -DrD-
Re: Define JNIEXPORT as visibility default with GCC?
On 02/14/2013 08:09 PM, David DeHaven wrote: +#if defined(__GNUC__) (__GNUC__ 4) || (__GNUC__ == 4) (__GNUC_MINOR__ 2) + #define JNIEXPORT __attribute__((visibility(default))) + #define JNIIMPORT __attribute__((visibility(default))) The default compiler in Xcode 4.1 is llvm-gcc 4.2, it seems. The conditional above excludes that. Is this intentional? It's *is* gcc, with a LLVM backend. Yes, but it identifies itself as GCC 4.2, so the conditional doesn't fire. If Xcode is fine with the #define, I suggest to drop the version check completely. We already do not support compiling with GCC versions which are so old that they lack visibility support. -- Florian Weimer / Red Hat Product Security Team
Re: Define JNIEXPORT as visibility default with GCC?
Alright, let's use gcc's recommended test, and let's add a test for clang (completely untested): diff --git a/src/solaris/javavm/export/jni_md.h b/src/solaris/javavm/export/jni_md.h --- a/src/solaris/javavm/export/jni_md.h +++ b/src/solaris/javavm/export/jni_md.h @@ -26,8 +26,14 @@ #ifndef _JAVASOFT_JNI_MD_H_ #define _JAVASOFT_JNI_MD_H_ -#define JNIEXPORT -#define JNIIMPORT +#if (defined(__GNUC__) __GNUC__ = 4) || (defined(__has_attribute) __has_attribute(visibility)) + #define JNIEXPORT __attribute__((visibility(default))) + #define JNIIMPORT __attribute__((visibility(default))) +#else + #define JNIEXPORT + #define JNIIMPORT +#endif + #define JNICALL typedef int jint; On Thu, Feb 14, 2013 at 11:07 AM, David DeHaven david.deha...@oracle.comwrote: This seems like an obvious improvement. There are already bunches of places in the jdk sources that do things like: ./hotspot/src/cpu/x86/vm/jni_x86.h #if defined(SOLARIS) || defined(LINUX) || defined(_ALLBSD_SOURCE) #if defined(__GNUC__) (__GNUC__ 4) || (__GNUC__ == 4) (__GNUC_MINOR__ 2) #define JNIEXPORT __attribute__((visibility(default))) #define JNIIMPORT __attribute__((visibility(default))) #else #define JNIEXPORT #define JNIIMPORT #endif That version check didn't work for me running on Mac OS X (using llvm-gcc 4.2). If that's what hotspot is using then they may not be getting the desired effect. The problem is __GNUC_MINOR__ is 2, so it needs to be changed to = 2. Gnu recommends just using #if __GNUC__ = 4, and everything I've read about it says it's 4.0 and later, so I don't understand why the minor version check is even in there. -DrD-
Re: Define JNIEXPORT as visibility default with GCC?
+#if defined(__GNUC__) (__GNUC__ 4) || (__GNUC__ == 4) (__GNUC_MINOR__ 2) + #define JNIEXPORT __attribute__((visibility(default))) + #define JNIIMPORT __attribute__((visibility(default))) The default compiler in Xcode 4.1 is llvm-gcc 4.2, it seems. The conditional above excludes that. Is this intentional? It's *is* gcc, with a LLVM backend. Yes, but it identifies itself as GCC 4.2, so the conditional doesn't fire. I assume this was not the intent and the version check is just wrong. If Xcode is fine with the #define, I suggest to drop the version check completely. We already do not support compiling with GCC versions which are so old that they lack visibility support. If it were Mac only, I'd agree. The same header is currently used for all unix-like OS's (which may change, if I have my way), so Solaris and Linux would also be affected. Most Linux distros have used gcc 4 for quite a while now, I've no idea what Solaris uses and embedded targets are a wild mishmash of whatever someone manages to cobble together, so the simpler __GNUC__ check may still be appropriate. -DrD-
Re: Define JNIEXPORT as visibility default with GCC?
a) I don't know what's going on behind the scenes, but if this sounds like a good idea to folks, can we open a bug and get the process otherwise rolling? b) Martin, where did the 4.2 restriction come from? Both Apple's site and the gcc wiki say that visibility support arrived in 4.0: http://gcc.gnu.org/wiki/Visibility https://developer.apple.com/library/mac/#documentation/DeveloperTools/Conceptual/CppRuntimeEnv/Articles/SymbolVisibility.html On Thu, Feb 14, 2013 at 3:01 PM, David DeHaven david.deha...@oracle.comwrote: +#if defined(__GNUC__) (__GNUC__ 4) || (__GNUC__ == 4) (__GNUC_MINOR__ 2) + #define JNIEXPORT __attribute__((visibility(default))) + #define JNIIMPORT __attribute__((visibility(default))) The default compiler in Xcode 4.1 is llvm-gcc 4.2, it seems. The conditional above excludes that. Is this intentional? It's *is* gcc, with a LLVM backend. Yes, but it identifies itself as GCC 4.2, so the conditional doesn't fire. I assume this was not the intent and the version check is just wrong. It may be that they deliberately stayed with gcc 4.2 to keep parity with clang, which only supports 4.2 (or it may not, because clang supports lots of 4.3+ features). If Xcode is fine with the #define, I suggest to drop the version check completely. We already do not support compiling with GCC versions which are so old that they lack visibility support. If it were Mac only, I'd agree. The same header is currently used for all unix-like OS's (which may change, if I have my way), so Solaris and Linux would also be affected. Most Linux distros have used gcc 4 for quite a while now, I've no idea what Solaris uses and embedded targets are a wild mishmash of whatever someone manages to cobble together, so the simpler __GNUC__ check may still be appropriate. -DrD-
Re: Define JNIEXPORT as visibility default with GCC?
This seems like an obvious improvement. There are already bunches of places in the jdk sources that do things like: ./hotspot/src/cpu/x86/vm/jni_x86.h #if defined(SOLARIS) || defined(LINUX) || defined(_ALLBSD_SOURCE) #if defined(__GNUC__) (__GNUC__ 4) || (__GNUC__ == 4) (__GNUC_MINOR__ 2) #define JNIEXPORT __attribute__((visibility(default))) #define JNIIMPORT __attribute__((visibility(default))) #else #define JNIEXPORT #define JNIIMPORT #endif This is *crazy*. The visibility feature has nothing to do with x86, or SOLARIS, or LINUX, or BSD. JNIEXPORT should be defined to __attribute__((visibility(default))) #if #and #only #if: defined(__GNUC__) (__GNUC__ 4) || (__GNUC__ == 4) (__GNUC_MINOR__ 2) and that change should simply be made to the public exported jdk jni headers Martin On Mon, Feb 11, 2013 at 10:26 AM, Jeremy Manson jeremyman...@google.comwrote: Hi folks, Pardon if this has come up before; a quick search didn't indicate anything, but the mailing list archives are kind of hard to search. I wonder if it makes sense to define JNIEXPORT as meaning __attribute__ ((visibility (default))) when compiling with gcc. Currently, anyone building JNI code with -fvisibility=hidden and a stock Oracle JDK is at a loss: their JNI exports will be hidden along with everything else. I notice that both IcedTea and OS X have made this change independently, and it has been added to Hotspot's JNIEXPORT definition (so HS can be built with -fvisibility=hidden), but the change isn't present in the latest JDK8 bits: http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/933742f4bb4c/src/solaris/javavm/export/jni_md.h The workaround is pretty ugly: people who want to use -fvisibility=hidden have to redefine JNIEXPORT. Upstream, it would be a pretty simple change to jni_md.h, along the lines of: #if defined(__GNUC__) __GNUC__ = 4 #define JNIEXPORT __attribute__ ((visibility (default))) #else #define JNIEXPORT #endif Any thoughts? Jeremy
Re: Define JNIEXPORT as visibility default with GCC?
So, here's an obvious patch. Any reason not to do this? (although we can do even better, as always) http://cr.openjdk.java.net/~martin/webrevs/openjdk8/JNIEXPORT/ # HG changeset patch # User martin # Date 1360805180 28800 # Node ID 0d31e5e0a2c8e4a40c669a6c1c59530aa5f705bb # Parent 397424fe9fb52dd622af409ed2e3f1fc3463ee8d 666: JNIEXPORT should use gcc visibility default Summary: Define JNIEXPORT diff --git a/src/solaris/javavm/export/jni_md.h b/src/solaris/javavm/export/jni_md.h --- a/src/solaris/javavm/export/jni_md.h +++ b/src/solaris/javavm/export/jni_md.h @@ -26,8 +26,14 @@ #ifndef _JAVASOFT_JNI_MD_H_ #define _JAVASOFT_JNI_MD_H_ -#define JNIEXPORT -#define JNIIMPORT +#if defined(__GNUC__) (__GNUC__ 4) || (__GNUC__ == 4) (__GNUC_MINOR__ 2) + #define JNIEXPORT __attribute__((visibility(default))) + #define JNIIMPORT __attribute__((visibility(default))) +#else + #define JNIEXPORT + #define JNIIMPORT +#endif + #define JNICALL typedef int jint; On Wed, Feb 13, 2013 at 5:14 PM, Martin Buchholz marti...@google.comwrote: This seems like an obvious improvement. There are already bunches of places in the jdk sources that do things like: ./hotspot/src/cpu/x86/vm/jni_x86.h #if defined(SOLARIS) || defined(LINUX) || defined(_ALLBSD_SOURCE) #if defined(__GNUC__) (__GNUC__ 4) || (__GNUC__ == 4) (__GNUC_MINOR__ 2) #define JNIEXPORT __attribute__((visibility(default))) #define JNIIMPORT __attribute__((visibility(default))) #else #define JNIEXPORT #define JNIIMPORT #endif This is *crazy*. The visibility feature has nothing to do with x86, or SOLARIS, or LINUX, or BSD. JNIEXPORT should be defined to __attribute__((visibility(default))) #if #and #only #if: defined(__GNUC__) (__GNUC__ 4) || (__GNUC__ == 4) (__GNUC_MINOR__ 2) and that change should simply be made to the public exported jdk jni headers Martin On Mon, Feb 11, 2013 at 10:26 AM, Jeremy Manson jeremyman...@google.comwrote: Hi folks, Pardon if this has come up before; a quick search didn't indicate anything, but the mailing list archives are kind of hard to search. I wonder if it makes sense to define JNIEXPORT as meaning __attribute__ ((visibility (default))) when compiling with gcc. Currently, anyone building JNI code with -fvisibility=hidden and a stock Oracle JDK is at a loss: their JNI exports will be hidden along with everything else. I notice that both IcedTea and OS X have made this change independently, and it has been added to Hotspot's JNIEXPORT definition (so HS can be built with -fvisibility=hidden), but the change isn't present in the latest JDK8 bits: http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/933742f4bb4c/src/solaris/javavm/export/jni_md.h The workaround is pretty ugly: people who want to use -fvisibility=hidden have to redefine JNIEXPORT. Upstream, it would be a pretty simple change to jni_md.h, along the lines of: #if defined(__GNUC__) __GNUC__ = 4 #define JNIEXPORT __attribute__ ((visibility (default))) #else #define JNIEXPORT #endif Any thoughts? Jeremy
Define JNIEXPORT as visibility default with GCC?
Hi folks, Pardon if this has come up before; a quick search didn't indicate anything, but the mailing list archives are kind of hard to search. I wonder if it makes sense to define JNIEXPORT as meaning __attribute__ ((visibility (default))) when compiling with gcc. Currently, anyone building JNI code with -fvisibility=hidden and a stock Oracle JDK is at a loss: their JNI exports will be hidden along with everything else. I notice that both IcedTea and OS X have made this change independently, and it has been added to Hotspot's JNIEXPORT definition (so HS can be built with -fvisibility=hidden), but the change isn't present in the latest JDK8 bits: http://hg.openjdk.java.net/jdk8/jdk8/jdk/file/933742f4bb4c/src/solaris/javavm/export/jni_md.h The workaround is pretty ugly: people who want to use -fvisibility=hidden have to redefine JNIEXPORT. Upstream, it would be a pretty simple change to jni_md.h, along the lines of: #if defined(__GNUC__) __GNUC__ = 4 #define JNIEXPORT __attribute__ ((visibility (default))) #else #define JNIEXPORT #endif Any thoughts? Jeremy
