Re: Review Request 110563: Crash fix: hide symbols from static lib QtUitools.a (generically by new macro KDE4_HIDE_SYMBOLS_FROM_STATIC_LIBS)

2014-09-17 Thread Friedrich W. H. Kossebau

---
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/110563/
---

(Updated Sept. 17, 2014, 7:36 nachm.)


Status
--

This change has been discarded.


Review request for Build System, kdelibs, Alexander Neundorf, and Thiago 
Macieira.


Repository: kdelibs


Description
---

Like discussed in the thread Crashes with libQtUiTools.a if linked multiple 
times into the same process (with Bsymbolic-functions flag) on kde-core-devel 
( http://lists.kde.org/?t=13682986311r=1w=2 ) symbols from QtUitools.a 
are not hidden by default in Qt4 and thus will be added to the public symbols 
of the module/shared lib they are linked to. And thus can appear multiple times 
in the same process, resulting in symbol clashes and leading to problems at 
least with the Bsymbolic-functions flag or when being possibly incompatible 
versions.

Attached patch sees to solve that problem, by adding a macro 
KDE4_HIDE_SYMBOLS_FROM_STATIC_LIBS which should add any needed linker flags 
depending on the platform/linker used. 

Only issue is that instead of some variable I had to use QtUiTools.a as I 
found no variable which would resolve to that. E.g. ${QT_QTUITOOLS_LIBRARY} 
resolves to Qt4::QtUiTools for me. Any idea what to use there, in case 
another platform needs another name/prefix here?

Patch is against 4.10 branch, so I hope to get this in 4.10.4

http://lxr.kde.org/search?v=4.10-branchfilestring=string=QT_QTUITOOLS_LIBRARY 
shows that there are some more places where the symbols need hiding, but I 
first want feedback on the proposed approach.


Diffs
-

  cmake/modules/FindKDE4Internal.cmake cb63285 
  cmake/modules/KDE4Macros.cmake 3db4e24 
  kjsembed/kjsembed/CMakeLists.txt d70f260 
  kross/modules/CMakeLists.txt d245fd8 
  kross/qts/CMakeLists.txt d8cb4a5 
  plasma/CMakeLists.txt 674550d 

Diff: https://git.reviewboard.kde.org/r/110563/diff/


Testing
---


Thanks,

Friedrich W. H. Kossebau



Re: Review Request 110563: Crash fix: hide symbols from static lib QtUitools.a (generically by new macro KDE4_HIDE_SYMBOLS_FROM_STATIC_LIBS)

2013-05-27 Thread Friedrich W. H. Kossebau


 On May 22, 2013, 6:28 p.m., Alexander Neundorf wrote:
  The documentation for the macro should be at the top of 
  FindKDE4Internal.cmake, where all the documentation is.
  
  For the technical side: this flag is new to me. Is it the only possible 
  solution ? Thiago ?
 
 Thomas Lübking wrote:
  For the technical side: this flag is new to me.
 I wasn't aware it's used and grepping the Makefiles of kdelibs, workspace 
 and runtime didn't show it here, btw.
 
 What it does:
 http://www.technovelty.org/c/what-exactly-does-bsymblic-do.html
 
 My 2¢
 There're various issues with it and i dare to claim that you more or less 
 want to use it alongside --dynamic-list only.
 Alternatively one would use 
 __attribute__((visibility([hidden|protected]))) or the -version-script 
 switch to protect/accelerate *certain* funtion/calls. (protected visibility 
 is afair gcc only, though)
 
 If downstream applies it, i'd frankly tell downstream to rtfm and not 
 just push everything claimed to make it fasta!!!
 
 This is by no means sth. one should apply uninformed since it has 
 impact on the runtime behavior that the developer needs to know about 
 (whoops, my trick to shadow friend qt_foo() to gain access to 
 protected/private d-bar only works sometiems - yes, one should not hack, 
 but one should also be aware that this hack can legally fail and not wonder 
 why it works here and on distro X but fails on distro Y)

Now the KDE packages for OpenSUSE are said to have been created with that flag 
already for a while, given Hrvoje Senjan saying we are using it 4 years 
already, this is first known issue it's caused so far 
(https://bugzilla.novell.com/show_bug.cgi?id=819437#c14)

http://qt-project.org/wiki/Performance_Tip_Startup_Time says: Use 
-Bsymbolic-functions for your shared libraries., more or less, with no big 
warnings. (Please note your warnings there, to have more downstream rtfm :) )

Surely -Bsymbolic-functions should be used with care.


Still, we have the problem that QtUitools.a exposes all its symbols on Linux  
similar. Which means symbol clashes if loaded multiple times in the same 
process. With and without -Bsymbolic-functions. One could fix QtUitools.a for 
4.8.future. Or see to do on our side what can be done, i.e. explicitely hide 
those symbols when linking the now existing versions of the static lib 
QtUitools.a into our code, so any of our code does not even try to lookup its 
symbols in the wrong place.

Which is what the proposed patch does, offer the option to mark an extern 
static library to be linked without exposing its symbols, on those platform 
where it is needed.

What other solution would there be?


- Friedrich W. H.


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110563/#review32984
---


On May 22, 2013, 6:45 p.m., Friedrich W. H. Kossebau wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/110563/
 ---
 
 (Updated May 22, 2013, 6:45 p.m.)
 
 
 Review request for Build System, kdelibs, Alexander Neundorf, and Thiago 
 Macieira.
 
 
 Description
 ---
 
 Like discussed in the thread Crashes with libQtUiTools.a if linked multiple 
 times into the same process (with Bsymbolic-functions flag) on 
 kde-core-devel ( http://lists.kde.org/?t=13682986311r=1w=2 ) symbols 
 from QtUitools.a are not hidden by default in Qt4 and thus will be added to 
 the public symbols of the module/shared lib they are linked to. And thus can 
 appear multiple times in the same process, resulting in symbol clashes and 
 leading to problems at least with the Bsymbolic-functions flag or when being 
 possibly incompatible versions.
 
 Attached patch sees to solve that problem, by adding a macro 
 KDE4_HIDE_SYMBOLS_FROM_STATIC_LIBS which should add any needed linker flags 
 depending on the platform/linker used. 
 
 Only issue is that instead of some variable I had to use QtUiTools.a as I 
 found no variable which would resolve to that. E.g. ${QT_QTUITOOLS_LIBRARY} 
 resolves to Qt4::QtUiTools for me. Any idea what to use there, in case 
 another platform needs another name/prefix here?
 
 Patch is against 4.10 branch, so I hope to get this in 4.10.4
 
 http://lxr.kde.org/search?v=4.10-branchfilestring=string=QT_QTUITOOLS_LIBRARY
  shows that there are some more places where the symbols need hiding, but I 
 first want feedback on the proposed approach.
 
 
 Diffs
 -
 
   cmake/modules/FindKDE4Internal.cmake cb63285 
   cmake/modules/KDE4Macros.cmake 3db4e24 
   kjsembed/kjsembed/CMakeLists.txt d70f260 
   kross/modules/CMakeLists.txt d245fd8 
   kross/qts/CMakeLists.txt d8cb4a5 
   plasma/CMakeLists.txt 674550d 
 
 Diff: 

Re: Review Request 110563: Crash fix: hide symbols from static lib QtUitools.a (generically by new macro KDE4_HIDE_SYMBOLS_FROM_STATIC_LIBS)

2013-05-27 Thread Thomas Lübking


 On May 22, 2013, 6:28 p.m., Alexander Neundorf wrote:
  The documentation for the macro should be at the top of 
  FindKDE4Internal.cmake, where all the documentation is.
  
  For the technical side: this flag is new to me. Is it the only possible 
  solution ? Thiago ?
 
 Thomas Lübking wrote:
  For the technical side: this flag is new to me.
 I wasn't aware it's used and grepping the Makefiles of kdelibs, workspace 
 and runtime didn't show it here, btw.
 
 What it does:
 http://www.technovelty.org/c/what-exactly-does-bsymblic-do.html
 
 My 2¢
 There're various issues with it and i dare to claim that you more or less 
 want to use it alongside --dynamic-list only.
 Alternatively one would use 
 __attribute__((visibility([hidden|protected]))) or the -version-script 
 switch to protect/accelerate *certain* funtion/calls. (protected visibility 
 is afair gcc only, though)
 
 If downstream applies it, i'd frankly tell downstream to rtfm and not 
 just push everything claimed to make it fasta!!!
 
 This is by no means sth. one should apply uninformed since it has 
 impact on the runtime behavior that the developer needs to know about 
 (whoops, my trick to shadow friend qt_foo() to gain access to 
 protected/private d-bar only works sometiems - yes, one should not hack, 
 but one should also be aware that this hack can legally fail and not wonder 
 why it works here and on distro X but fails on distro Y)
 
 Friedrich W. H. Kossebau wrote:
 Now the KDE packages for OpenSUSE are said to have been created with that 
 flag already for a while, given Hrvoje Senjan saying we are using it 4 years 
 already, this is first known issue it's caused so far 
 (https://bugzilla.novell.com/show_bug.cgi?id=819437#c14)
 
 http://qt-project.org/wiki/Performance_Tip_Startup_Time says: Use 
 -Bsymbolic-functions for your shared libraries., more or less, with no big 
 warnings. (Please note your warnings there, to have more downstream rtfm :) )
 
 Surely -Bsymbolic-functions should be used with care.
 
 
 Still, we have the problem that QtUitools.a exposes all its symbols on 
 Linux  similar. Which means symbol clashes if loaded multiple times in the 
 same process. With and without -Bsymbolic-functions. One could fix 
 QtUitools.a for 4.8.future. Or see to do on our side what can be done, i.e. 
 explicitely hide those symbols when linking the now existing versions of the 
 static lib QtUitools.a into our code, so any of our code does not even try to 
 lookup its symbols in the wrong place.
 
 Which is what the proposed patch does, offer the option to mark an extern 
 static library to be linked without exposing its symbols, on those platform 
 where it is needed.
 
 What other solution would there be?

It does usually rather not cause the kind of issue where you'd say oh, yes - 
that's because of the linker behavior and the better solution was to upstream 
version tag symbols - so one gets consistent behavior and also has more fine 
grained control over protection/acceleration.

This general consideration aside, in the particular incident Thiago already 
pointed that QtUitools hides symbols in Qt5 (in the mailing ist, iirc) so it's 
certainly the proper solution until then - i didn't mean to question that 
aspect.

The only problem would be the limited presence of that linker option (seems GNU 
ld only and also on certain architectures) - still better than nothing, until 
and if the next Qt version would backport hiding the symbols as Thiago also ... 
implied ... ;-)


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110563/#review32984
---


On May 22, 2013, 6:45 p.m., Friedrich W. H. Kossebau wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/110563/
 ---
 
 (Updated May 22, 2013, 6:45 p.m.)
 
 
 Review request for Build System, kdelibs, Alexander Neundorf, and Thiago 
 Macieira.
 
 
 Description
 ---
 
 Like discussed in the thread Crashes with libQtUiTools.a if linked multiple 
 times into the same process (with Bsymbolic-functions flag) on 
 kde-core-devel ( http://lists.kde.org/?t=13682986311r=1w=2 ) symbols 
 from QtUitools.a are not hidden by default in Qt4 and thus will be added to 
 the public symbols of the module/shared lib they are linked to. And thus can 
 appear multiple times in the same process, resulting in symbol clashes and 
 leading to problems at least with the Bsymbolic-functions flag or when being 
 possibly incompatible versions.
 
 Attached patch sees to solve that problem, by adding a macro 
 KDE4_HIDE_SYMBOLS_FROM_STATIC_LIBS which should add any needed 

Re: Review Request 110563: Crash fix: hide symbols from static lib QtUitools.a (generically by new macro KDE4_HIDE_SYMBOLS_FROM_STATIC_LIBS)

2013-05-22 Thread Friedrich W. H. Kossebau

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110563/
---

(Updated May 22, 2013, 11:19 a.m.)


Review request for Build System, kdelibs, Alexander Neundorf, and Thiago 
Macieira.


Changes
---

Put Crash keyword in title, to get more attention.
Also set Alex and Thiago explicitely as reviewers.

Want to get this in ASAP, before 4.10.4


Summary (updated)
-

Crash fix: hide symbols from static lib QtUitools.a (generically by new macro 
KDE4_HIDE_SYMBOLS_FROM_STATIC_LIBS)


Description
---

Like discussed in the thread Crashes with libQtUiTools.a if linked multiple 
times into the same process (with Bsymbolic-functions flag) on kde-core-devel 
( http://lists.kde.org/?t=13682986311r=1w=2 ) symbols from QtUitools.a 
are not hidden by default in Qt4 and thus will be added to the public symbols 
of the module/shared lib they are linked to. And thus can appear multiple times 
in the same process, resulting in symbol clashes and leading to problems at 
least with the Bsymbolic-functions flag or when being possibly incompatible 
versions.

Attached patch sees to solve that problem, by adding a macro 
KDE4_HIDE_SYMBOLS_FROM_STATIC_LIBS which should add any needed linker flags 
depending on the platform/linker used. 

Only issue is that instead of some variable I had to use QtUiTools.a as I 
found no variable which would resolve to that. E.g. ${QT_QTUITOOLS_LIBRARY} 
resolves to Qt4::QtUiTools for me. Any idea what to use there, in case 
another platform needs another name/prefix here?

Patch is against 4.10 branch, so I hope to get this in 4.10.4

http://lxr.kde.org/search?v=4.10-branchfilestring=string=QT_QTUITOOLS_LIBRARY 
shows that there are some more places where the symbols need hiding, but I 
first want feedback on the proposed approach.


Diffs
-

  cmake/modules/FindKDE4Internal.cmake cb63285 
  cmake/modules/KDE4Macros.cmake 3db4e24 
  kjsembed/kjsembed/CMakeLists.txt d70f260 
  kross/modules/CMakeLists.txt d245fd8 
  kross/qts/CMakeLists.txt d8cb4a5 
  plasma/CMakeLists.txt 674550d 

Diff: http://git.reviewboard.kde.org/r/110563/diff/


Testing
---


Thanks,

Friedrich W. H. Kossebau



Re: Review Request 110563: Crash fix: hide symbols from static lib QtUitools.a (generically by new macro KDE4_HIDE_SYMBOLS_FROM_STATIC_LIBS)

2013-05-22 Thread Alexander Neundorf

---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110563/#review32984
---


The documentation for the macro should be at the top of FindKDE4Internal.cmake, 
where all the documentation is.

For the technical side: this flag is new to me. Is it the only possible 
solution ? Thiago ?

- Alexander Neundorf


On May 22, 2013, 11:19 a.m., Friedrich W. H. Kossebau wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/110563/
 ---
 
 (Updated May 22, 2013, 11:19 a.m.)
 
 
 Review request for Build System, kdelibs, Alexander Neundorf, and Thiago 
 Macieira.
 
 
 Description
 ---
 
 Like discussed in the thread Crashes with libQtUiTools.a if linked multiple 
 times into the same process (with Bsymbolic-functions flag) on 
 kde-core-devel ( http://lists.kde.org/?t=13682986311r=1w=2 ) symbols 
 from QtUitools.a are not hidden by default in Qt4 and thus will be added to 
 the public symbols of the module/shared lib they are linked to. And thus can 
 appear multiple times in the same process, resulting in symbol clashes and 
 leading to problems at least with the Bsymbolic-functions flag or when being 
 possibly incompatible versions.
 
 Attached patch sees to solve that problem, by adding a macro 
 KDE4_HIDE_SYMBOLS_FROM_STATIC_LIBS which should add any needed linker flags 
 depending on the platform/linker used. 
 
 Only issue is that instead of some variable I had to use QtUiTools.a as I 
 found no variable which would resolve to that. E.g. ${QT_QTUITOOLS_LIBRARY} 
 resolves to Qt4::QtUiTools for me. Any idea what to use there, in case 
 another platform needs another name/prefix here?
 
 Patch is against 4.10 branch, so I hope to get this in 4.10.4
 
 http://lxr.kde.org/search?v=4.10-branchfilestring=string=QT_QTUITOOLS_LIBRARY
  shows that there are some more places where the symbols need hiding, but I 
 first want feedback on the proposed approach.
 
 
 Diffs
 -
 
   cmake/modules/FindKDE4Internal.cmake cb63285 
   cmake/modules/KDE4Macros.cmake 3db4e24 
   kjsembed/kjsembed/CMakeLists.txt d70f260 
   kross/modules/CMakeLists.txt d245fd8 
   kross/qts/CMakeLists.txt d8cb4a5 
   plasma/CMakeLists.txt 674550d 
 
 Diff: http://git.reviewboard.kde.org/r/110563/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Friedrich W. H. Kossebau
 




Re: Review Request 110563: Crash fix: hide symbols from static lib QtUitools.a (generically by new macro KDE4_HIDE_SYMBOLS_FROM_STATIC_LIBS)

2013-05-22 Thread Thomas Lübking


 On May 22, 2013, 6:28 p.m., Alexander Neundorf wrote:
  The documentation for the macro should be at the top of 
  FindKDE4Internal.cmake, where all the documentation is.
  
  For the technical side: this flag is new to me. Is it the only possible 
  solution ? Thiago ?

 For the technical side: this flag is new to me.
I wasn't aware it's used and grepping the Makefiles of kdelibs, workspace and 
runtime didn't show it here, btw.

What it does:
http://www.technovelty.org/c/what-exactly-does-bsymblic-do.html

My 2¢
There're various issues with it and i dare to claim that you more or less want 
to use it alongside --dynamic-list only.
Alternatively one would use __attribute__((visibility([hidden|protected]))) 
or the -version-script switch to protect/accelerate *certain* funtion/calls. 
(protected visibility is afair gcc only, though)

If downstream applies it, i'd frankly tell downstream to rtfm and not just 
push everything claimed to make it fasta!!!

This is by no means sth. one should apply uninformed since it has impact on 
the runtime behavior that the developer needs to know about (whoops, my trick 
to shadow friend qt_foo() to gain access to protected/private d-bar only works 
sometiems - yes, one should not hack, but one should also be aware that this 
hack can legally fail and not wonder why it works here and on distro X but 
fails on distro Y)


- Thomas


---
This is an automatically generated e-mail. To reply, visit:
http://git.reviewboard.kde.org/r/110563/#review32984
---


On May 22, 2013, 6:45 p.m., Friedrich W. H. Kossebau wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 http://git.reviewboard.kde.org/r/110563/
 ---
 
 (Updated May 22, 2013, 6:45 p.m.)
 
 
 Review request for Build System, kdelibs, Alexander Neundorf, and Thiago 
 Macieira.
 
 
 Description
 ---
 
 Like discussed in the thread Crashes with libQtUiTools.a if linked multiple 
 times into the same process (with Bsymbolic-functions flag) on 
 kde-core-devel ( http://lists.kde.org/?t=13682986311r=1w=2 ) symbols 
 from QtUitools.a are not hidden by default in Qt4 and thus will be added to 
 the public symbols of the module/shared lib they are linked to. And thus can 
 appear multiple times in the same process, resulting in symbol clashes and 
 leading to problems at least with the Bsymbolic-functions flag or when being 
 possibly incompatible versions.
 
 Attached patch sees to solve that problem, by adding a macro 
 KDE4_HIDE_SYMBOLS_FROM_STATIC_LIBS which should add any needed linker flags 
 depending on the platform/linker used. 
 
 Only issue is that instead of some variable I had to use QtUiTools.a as I 
 found no variable which would resolve to that. E.g. ${QT_QTUITOOLS_LIBRARY} 
 resolves to Qt4::QtUiTools for me. Any idea what to use there, in case 
 another platform needs another name/prefix here?
 
 Patch is against 4.10 branch, so I hope to get this in 4.10.4
 
 http://lxr.kde.org/search?v=4.10-branchfilestring=string=QT_QTUITOOLS_LIBRARY
  shows that there are some more places where the symbols need hiding, but I 
 first want feedback on the proposed approach.
 
 
 Diffs
 -
 
   cmake/modules/FindKDE4Internal.cmake cb63285 
   cmake/modules/KDE4Macros.cmake 3db4e24 
   kjsembed/kjsembed/CMakeLists.txt d70f260 
   kross/modules/CMakeLists.txt d245fd8 
   kross/qts/CMakeLists.txt d8cb4a5 
   plasma/CMakeLists.txt 674550d 
 
 Diff: http://git.reviewboard.kde.org/r/110563/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Friedrich W. H. Kossebau