| Summary: | -fvisibility=hidden patch for libX11 | ||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | xorg | Reporter: | Bernie Innocenti <bernie> | ||||||||||||||||||||
| Component: | Lib/Xlib | Assignee: | Bernie Innocenti <bernie> | ||||||||||||||||||||
| Status: | RESOLVED WONTFIX | QA Contact: | Xorg Project Team <xorg-team> | ||||||||||||||||||||
| Severity: | enhancement | ||||||||||||||||||||||
| Priority: | high | CC: | ago, ajax, alan.coopersmith, erik.andren, gajownik, jeremyhu, roland.mainz, tilman | ||||||||||||||||||||
| Version: | git | Keywords: | patch | ||||||||||||||||||||
| Hardware: | All | ||||||||||||||||||||||
| OS: | All | ||||||||||||||||||||||
| Whiteboard: | 2011BRB_Reviewed | ||||||||||||||||||||||
| i915 platform: | i915 features: | ||||||||||||||||||||||
| Attachments: |
|
||||||||||||||||||||||
|
Description
Bernie Innocenti
2005-02-23 20:53:51 UTC
Created attachment 1969 [details] [review] Part 1/4 Created attachment 1970 [details] [review] Part 2/4. Created attachment 1971 [details] [review] Part 3/4. Created attachment 1972 [details] [review] Part 4/4. Oops: /usr/lib/vmware/bin/vmware: symbol lookup error: /usr/lib/vmware/bin/vmware: undefined symbol: XauFileName Please wait for an updated patch. Created attachment 1973 [details] [review] Part 3/4. The patches define APIENTRY __stdcall for Cygwin and MinGW. This chabges the library ABI. Is this really intended? -fvisibility is not available with cygwin gcc. Most likely because that switch only effects ELF binaries. If X11APIENTRY is defined as __stdcall it is required in the .c file as well otherwise defintion and declaration of the function do not match The logic has been copied over from Mesa. Using __stdcall is customary in Windows DLL entries, but we could leave it alone for backwards compatibility. What's the preferred solution? As for repeating __stdcall on function definition, I've just realised this is needed. Since I can't easily test on Cygwin, if we're going to remove __stdcall anyway, I'd prefer to postpne this other change to a later patch. cygwin can not use __stdcall since this would break DLL compatibility. Since there is no real need and benefit from __stdcall anyway (apart from slightly smaller executables) I'd keep native windows builds without __stdcall too. - symbols were __cdecl since ages (even the msvc compiled DLLs must have been without __stdcall) - change breaks ABI - no real benefit from change - need for adjusting function declarations too ==> no __stdcall ever Created attachment 2014 [details] [review] Part 2/4 Don't change the existing ABI by adding __cdecl for Cygwin/MingW. (In reply to comment #10) Sorry for the longish delay... Could you please build on Cygwin just to make sure everything still works? PING! This patch is still unreviewed. Did anyone test it? Before submitting additional patches for other libraries I'd prefer to see this one approved. Of course I'll be glad to rework it if needed. Created attachment 2167 [details] [review] Part 3/4. Restore an "extern" that was removed by mistake from the _Xdebug declaration. On cygwin and mingw it does build ok. I only hope I've removed the old patches completely before applying the new ones. ping and add patch keyword we can use _X_HIDDEN and _X_EXPORT nowadays. so can you please rework part #3 to use those? Sorry about the phenomenal bug spam, guys. Adding xorg-team@ to the QA contact so bugs don't get lost in future. Created attachment 9856 [details] [review] XORG_VISIBILITY macro This autoconf macro will check for gcc 4+, and if present, append -fvisibility=hidden to CFLAGS. Obviously this macro can only be used once a project has marked the public API with _X_EXPORT. Created attachment 9857 [details] [review] Add _X_EXPORT's for libX11 This patch, based on on attachment 2167 [details] [review], adds the necessary _X_EXPORT tags to libX11. I added them to both the headers and the function definitions for completeness' sake. Also you can quickly see what's supposed to be part of the public API when editing the code and what's not. I'd rather not depend on including the headers in the source code to get the _X_EXPORT tags. Some of these functions have prototypes in proto/kbproto. I just won't attach the patch for that, because I'm lazy. It does the obvious thing anyway :P These two patches result in 386 less symbols in libX11.so. Not sure whether I missed a few which are needed. So, someone have a look at this. Ping? (In reply to comment #21) > Ping? Please send these patches to xorg-devel@lists.x.org for review/inclusion. They're going to be continually ignored here. Please update your patches against current git and send to xorg-devel for review. If this isn't done in the next month or so, I'll close this as WONTFIX. I appreciate the effort and am sorry that this has bitrot for so long, but if you do that, the changes will be acted upon. > --- Comment #23 from Jeremy Huddleston <jeremyhu@freedesktop.org> 2011-10-03 19:17:34 PDT --- > Please update your patches against current git and send to xorg-devel for > review. If this isn't done in the next month or so, I'll close this as > WONTFIX. I appreciate the effort and am sorry that this has bitrot for so > long, but if you do that, the changes will be acted upon. > I asked ajax about this a while back, and he said he'd rather not change this stuff. So I guess this is a wontfix. (I had previously sent out some patches, see the thread starting at <1249040982-16446-1-git-send-email-jcristau@debian.org>) Ok, well I prefer annotation, but it is Xlib so maybe it's just too big of a can of worms at this point. |
Use of freedesktop.org services, including Bugzilla, is subject to our Code of Conduct. How we collect and use information is described in our Privacy Policy.