| Message ID | 1438748380-5528-1-git-send-email-simon.marchi@polymtl.ca |
|---|---|
| State | Superseded |
| Headers | show |
On 05/08/2015 06:19, Simon Marchi wrote: > This patch fixes this autobuild failure: > > http://autobuild.buildroot.net/results/e14/e14e1700d4fe359c56be57587bdb509e002e5753/build-end.log > > I am not 100% sure of what happens (I am not a C++ expert at all), but > here it is anyway. I assume that when the definition of > TCPSocket::ReadDescriptor is embedded in the class definition, no actual > implementation is generated (all usages of it are inline). No, the actual implementation is still generated. However, the build system pass the -fvisibility-inlines-hidden option, so it is not exported ... This bug should be present in the whole library, i'm surprised that only break in this case.
On 5 August 2015 at 03:58, Nicolas Cavallari <Nicolas.Cavallari@green-communications.fr> wrote: > No, the actual implementation is still generated. However, the build > system pass the -fvisibility-inlines-hidden option, so it is not > exported ... Ahh thanks for the tip. I should have looked at the compiler flags in the first place, this one would have been obvious. > This bug should be present in the whole library, i'm surprised that > only break in this case. My guess is it's the only method that has this problem that is called from a shared library. In any case, do you think that making the implementation non-inline is the right fix here?
Dear Simon Marchi, On Wed, 5 Aug 2015 00:19:40 -0400, Simon Marchi wrote: > This patch fixes this autobuild failure: > > http://autobuild.buildroot.net/results/e14/e14e1700d4fe359c56be57587bdb509e002e5753/build-end.log > > I am not 100% sure of what happens (I am not a C++ expert at all), but > here it is anyway. I assume that when the definition of > TCPSocket::ReadDescriptor is embedded in the class definition, no actual > implementation is generated (all usages of it are inline). When object > libolaopenpixelcontrol.so tries to reference it somehow, it results in > an undefined reference. Moving the definition in the cpp retains it. If > you have a clear explanation of what happens, I'd be happy to read it. > > Signed-off-by: Simon Marchi <simon.marchi@polymtl.ca> Could you report the problem to the upstream ola project and see what they say? Also, they have a 0.9.7 release, maybe you want to try it? Thomas
On 8 August 2015 at 06:44, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Could you report the problem to the upstream ola project and see what > they say? Also, they have a 0.9.7 release, maybe you want to try it? I discussed it a bit on their IRC channel, it seems like the right fix will be to move the implementations to the .cpp files. I filed a bug: https://github.com/OpenLightingProject/ola/issues/880 The 0.9.7 release does not change anything about that problem, but we could still bump the version. I am preparing a patch to cleanup the plugin management of the package, if it hasn't been bump by the time I send my patch, I'll do it then.
Dear Simon Marchi, On Sun, 9 Aug 2015 23:11:03 -0400, Simon Marchi wrote: > On 8 August 2015 at 06:44, Thomas Petazzoni > <thomas.petazzoni@free-electrons.com> wrote: > > Could you report the problem to the upstream ola project and see what > > they say? Also, they have a 0.9.7 release, maybe you want to try it? > > I discussed it a bit on their IRC channel, it seems like the right fix > will be to move the implementations to the .cpp files. > > I filed a bug: https://github.com/OpenLightingProject/ola/issues/880 Shouldn't we simply remove -fvisibility-inlines-hidden ? > The 0.9.7 release does not change anything about that problem, but we > could still bump the version. I am preparing a patch to cleanup the > plugin management of the package, if it hasn't been bump by the time I > send my patch, I'll do it then. Sure, but that's going to be material for after 2015.08. What I was looking for right now for 2015.08 is a fix for the build issue. Thanks! Thomas
On 10 August 2015 at 08:44, Thomas Petazzoni <thomas.petazzoni@free-electrons.com> wrote: > Dear Simon Marchi, > > On Sun, 9 Aug 2015 23:11:03 -0400, Simon Marchi wrote: >> On 8 August 2015 at 06:44, Thomas Petazzoni >> <thomas.petazzoni@free-electrons.com> wrote: >> > Could you report the problem to the upstream ola project and see what >> > they say? Also, they have a 0.9.7 release, maybe you want to try it? >> >> I discussed it a bit on their IRC channel, it seems like the right fix >> will be to move the implementations to the .cpp files. >> >> I filed a bug: https://github.com/OpenLightingProject/ola/issues/880 > > Shouldn't we simply remove -fvisibility-inlines-hidden ? Do you mean remove it as a patch in the buildroot package, or upstream? I asked in the bug report if removing the switch upstream would be a possibility, how much impact it has on runtime performance (startup time). >> The 0.9.7 release does not change anything about that problem, but we >> could still bump the version. I am preparing a patch to cleanup the >> plugin management of the package, if it hasn't been bump by the time I >> send my patch, I'll do it then. > > Sure, but that's going to be material for after 2015.08. What I was > looking for right now for 2015.08 is a fix for the build issue. As an immediate fix, the patch I sent works. Although it only moves the problematic method. It would be possible for another method to break with another combination of compiler/platform. Removing the switch for the buildroot package would probably work as well (I am testing right now) and would prevent other similar breaks. Would you like me to send a patch immediately that does that instead? Thanks, Simon
Dear Simon Marchi, On Mon, 10 Aug 2015 13:45:31 -0400, Simon Marchi wrote: > >> I discussed it a bit on their IRC channel, it seems like the right fix > >> will be to move the implementations to the .cpp files. > >> > >> I filed a bug: https://github.com/OpenLightingProject/ola/issues/880 > > > > Shouldn't we simply remove -fvisibility-inlines-hidden ? > > Do you mean remove it as a patch in the buildroot package, or upstream? I was thinking in the Buildroot package for now, while the discussion with upstream is on-going. > I asked in the bug report if removing the switch upstream would be a > possibility, how much impact it has on runtime performance (startup > time). Yep, seen that, that's good. > As an immediate fix, the patch I sent works. Although it only moves > the problematic method. It would be possible for another method to > break with another combination of compiler/platform. > > Removing the switch for the buildroot package would probably work as > well (I am testing right now) and would prevent other similar breaks. > Would you like me to send a patch immediately that does that instead? To be honest, I really don't know what is the best between changing the method implementation, or removing the switch. Just let me know which you think is the best workaround for now, until upstream solves the problem. Thanks! Thomas
diff --git a/package/ola/Fix-link-ola-network-TCPSocket-ReadDescriptor.patch b/package/ola/Fix-link-ola-network-TCPSocket-ReadDescriptor.patch new file mode 100644 index 0000000..8c9385b --- /dev/null +++ b/package/ola/Fix-link-ola-network-TCPSocket-ReadDescriptor.patch @@ -0,0 +1,28 @@ +Index: ola-0.9.6/common/network/TCPSocket.cpp +=================================================================== +--- ola-0.9.6.orig/common/network/TCPSocket.cpp ++++ ola-0.9.6/common/network/TCPSocket.cpp +@@ -163,6 +163,10 @@ TCPSocket* TCPSocket::Connect(const Sock + return socket; + } + ++ola::io::DescriptorHandle TCPSocket::ReadDescriptor() const ++{ ++ return m_handle; ++} + + // TCPAcceptingSocket + // ------------------------------------------------ +Index: ola-0.9.6/include/ola/network/TCPSocket.h +=================================================================== +--- ola-0.9.6.orig/include/ola/network/TCPSocket.h ++++ ola-0.9.6/include/ola/network/TCPSocket.h +@@ -47,7 +47,7 @@ class TCPSocket: public ola::io::Connect + + ~TCPSocket() { Close(); } + +- ola::io::DescriptorHandle ReadDescriptor() const { return m_handle; } ++ ola::io::DescriptorHandle ReadDescriptor() const; + ola::io::DescriptorHandle WriteDescriptor() const { return m_handle; } + bool Close(); +
This patch fixes this autobuild failure: http://autobuild.buildroot.net/results/e14/e14e1700d4fe359c56be57587bdb509e002e5753/build-end.log I am not 100% sure of what happens (I am not a C++ expert at all), but here it is anyway. I assume that when the definition of TCPSocket::ReadDescriptor is embedded in the class definition, no actual implementation is generated (all usages of it are inline). When object libolaopenpixelcontrol.so tries to reference it somehow, it results in an undefined reference. Moving the definition in the cpp retains it. If you have a clear explanation of what happens, I'd be happy to read it. Signed-off-by: Simon Marchi <simon.marchi@polymtl.ca> --- ...link-ola-network-TCPSocket-ReadDescriptor.patch | 28 ++++++++++++++++++++++ 1 file changed, 28 insertions(+) create mode 100644 package/ola/Fix-link-ola-network-TCPSocket-ReadDescriptor.patch