Menu
▾
▴
[Tcl-bugs] [ tcl-Patches-2959069 ] Support for -fvisibility=hidden
|
From: SourceForge.net <no...@so...> - 2010-04-29 13:03:40
|
Patches item #2959069, was opened at 2010-02-25 23:43 Message generated for change (Comment added) made by nijtmans You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=310894&aid=2959069&group_id=10894 Please note that this message will contain a full copy of the comment thread, including the initial issue submission, for this request, not just the latest update. Category: 52. Portability Support Group: None >Status: Pending Resolution: Fixed Priority: 5 Private: No Submitted By: Jan Nijtmans (nijtmans) Assigned to: Daniel A. Steffen (das) Summary: Support for -fvisibility=hidden Initial Comment: Here is a patch, which makes the MODULE_SCOPE machinery unnecessary when using gcc. It makes everything compile with the gcc option -fvisibility=hidden (when available), so the EXPORT macro controls which symbols are exported. I am very interested to know whether Darwin, on which MODULE_SCOPE is currently translated by "__private_extern__" has a similar compiler options, making all symbols hidden by default, except the ones explicitely exported. Then MODULE_SCOPE can fully be deprecated. ---------------------------------------------------------------------- >Comment By: Jan Nijtmans (nijtmans) Date: 2010-04-29 15:03 Message: Adding additional warning options seems out-of-scope for this Issue. If everything works fine, this simply should be closed. Agreed? ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2010-03-03 14:17 Message: Maybe we should consider adding -Wstrict-prototypes -Wold-style-definition -Wmissing-prototypes (at least for gcc in debug mode; don't need to worry random builders, but do want to be able to audit for blunders) ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2010-03-03 10:50 Message: Thanks, Donal. But it might be that, because of this change, some symbols that were exported before, are not exported any more. I found and fixed 2 such cases in Tk. Every such difference is a bug, unrelated to this change! I can see 2 possibilities for this to happen: - A missing #include, so the EXTERN in this include is not picked up by the compiler - symbols which neither have EXTERN nor MODULE_SCOPE. So, could anyone do a build (both Tcl and Tk) with the current "configure" script and the previous one, and compare the list of exported symbols. It should be exactly the same! ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2010-03-03 10:34 Message: Seems to function correctly for me on OSX (as in "a clean configure and build with it works"). Great! ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2010-03-03 00:58 Message: Patch was still not 100% correct, but now it is. DKF's and DAS's remark handled. Checked in for HEAD. Assigning to das, could you please check if everything works on mac as well? I am pretty sure it works as is, but checking never hurts. Thanks! ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2010-03-02 23:35 Message: New version of patch, reverting to the current behavior if -fvisibility=hidden is not available. So, no regression. ---------------------------------------------------------------------- Comment By: Donal K. Fellows (dkf) Date: 2010-02-26 10:52 Message: Since we're going to be supporting all sorts of different compilers (on some platforms I use, I've got gcc 2.* still) for a long time across lots of platforms, I can't see that removing MODULE_SCOPE is necessarily a good move. The "get the overall visibility right" is an audit requirement, and we have make targets to support it. If you want to rewrite configure so as to allow for the case you describe (compiler flag, etc.) then that could be trialled. But that's a configure change and not a change to the Tcl and Tk sources. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2010-02-26 10:41 Message: This means, in fact, that the title of this Issue is wrong. The goal is not to phase out MODULE_SCOPE, the goal is to add support for compilers who, like gcc 4.x have a command line option to make all symbols hidden except especially decorated ones. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2010-02-26 10:37 Message: >All that said, I am strongly in favor of using -fvisibility=hidden when >available, I just think it would be a shame to loose the extra flexibility >the current scheme affords us w.r.t non-gcc compilers/linkers. Yes, I fully agree with that. My goal is first get gcc 4.x working with -fvisibility=hidden, and for all other compilers keep using the current MODULE_SCOPE method. Then later we can always extend it to let other compilers use their -fvisiliby=hidden variant. ---------------------------------------------------------------------- Comment By: Daniel A. Steffen (das) Date: 2010-02-26 08:15 Message: as mentioned, IMO it would be better to have a configure check for __attribute__ ((visibility("default"))), gcc > 3 is not the only compiler that supports this, so the check in tcl.h will miss some cases and be a regression over the current state of things. as far as discovering mistakenly exported symbols, that's what the 'checkexports' and 'checkstubs' makefile targets are there for... I try to remember to run these shortly before a release to make sure that we didn't forget any decorations. All that said, I am strongly in favor of using -fvisibility=hidden when available, I just think it would be a shame to loose the extra flexibility the current scheme affords us w.r.t non-gcc compilers/linkers. ---------------------------------------------------------------------- Comment By: Jan Nijtmans (nijtmans) Date: 2010-02-26 00:53 Message: Thanks for the reaction! - The handling of __attribute__((__visibility__("default"))) is done in tcl.h, and this is already available. Also, the needed -DBUILD_tcl and -DBUILD_tk is already part of the current Makefile.in. It should work for Darwin as is.... > After all the work adding MODULE_SCOPE everywhere, where is the > need/benefit to removing it again? I wouldn't start removing it, until it's been well tested on all platforms. (I called it 'deprecate', not 'remove'). However, being obliged to decorate all exported symbols with EXTERN and all others with MODULE_SCOPE, that's asking for trouble forgetting one. Before starting with this, I compared the list of Tk exported symbols, and found two problematic symbols. One that didn't respect the EXTERN because of a missing include (TkpCmapStressed), another symbol that was not supposed to be exported at all (TkSetTransientFor). I fixed those two, but with -fvisibility=hidden, it would already have been discovered long ago.... Anyway, it's not high priority. I agree that the attribute should be kept as fallback. Any more remarks? ---------------------------------------------------------------------- Comment By: Daniel A. Steffen (das) Date: 2010-02-26 00:02 Message: with recent enough gcc, we use visibility hidden on Darwin as well, but with older gcc that did not support visibility attributes yet we fall back to the Apple extension private_extern. MODULE_SCOPE affords us the flexibility to accommodate compiler/linkers that don't have an option to hide symbols by default and only export certain marked symbols. After all the work adding MODULE_SCOPE everywhere, where is the need/benefit to removing it again? Note that there are compilers (e.g. clang) that support __attribute__((__visibility__("hidden"))) but do not have the same commandline flags as gcc, your patch would be a regression there, the check for the attribute should remain as a fallback if the test for -fvisibility=hidden fails. Also, the current patch does not seem to test for __attribute__((__visibility__("default"))), which you will presumably need for the EXPORT macro ? (also not part of the patch) ---------------------------------------------------------------------- You can respond by visiting: https://sourceforge.net/tracker/?func=detail&atid=310894&aid=2959069&group_id=10894 |