Skip to content
This repository was archived by the owner on Aug 30, 2022. It is now read-only.

Build shared plugin for Linux amd64 using Docker #82

Merged
merged 1 commit into from
Apr 26, 2018

Conversation

isaachier
Copy link
Contributor

Resolves #81. Will update README soon with instructions.

@isaachier
Copy link
Contributor Author

@rnburn please take a look when you can.

@rnburn
Copy link
Contributor

rnburn commented Apr 10, 2018

That looks good. If you haven't done so already, I would just run ldd to confirm it doesn't have any unexpected shared library dependency and verify that you can actually dynamically load and use it to report spans.

@rnburn
Copy link
Contributor

rnburn commented Apr 10, 2018

Does https://cmake.org/files/v3.11/cmake-3.11.0-Linux-x86_64.sh add the compiler flags to target x86_64?

@isaachier
Copy link
Contributor Author

No that's actually the best way I have seen to install CMake. Otherwise, the debian packages tend to install version 2.8 or something ancient.

I left out the -march=x86_64 just because I didn't understand why they were needed and thought it might be Zipkin specific. Should I add them?

@codecov
Copy link

codecov bot commented Apr 10, 2018

Codecov Report

Merging #82 into master will decrease coverage by 0.43%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #82      +/-   ##
==========================================
- Coverage   88.61%   88.18%   -0.44%     
==========================================
  Files          96       96              
  Lines        2311     2311              
==========================================
- Hits         2048     2038      -10     
- Misses        263      273      +10
Impacted Files Coverage Δ
src/jaegertracing/DynamicLoad.cpp 0% <0%> (-83.34%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41c772c...ee59c62. Read the comment docs.

@isaachier
Copy link
Contributor Author

BTW I did do ldd and it links in everything but libc (which makes sense here, given only -static-libstdc++ -static-libgcc was specified). I'll try writing a sample as part of this experiment that dynamically loads the library and constructs a new tracer from it.

@rnburn
Copy link
Contributor

rnburn commented Apr 10, 2018

That's to ensure the code targets x86_64. If you run on a specific architecture gcc may produce code that uses non-x86_64 instructions. For example, you don't want something like AVX256 instructions if your processor happens to support them.

@rnburn
Copy link
Contributor

rnburn commented Apr 10, 2018

opentracing-cpp has this example that takes a library, configuration file and produces a couple of spans https://github.com/opentracing/opentracing-cpp/blob/master/example/dynamic_load/dynamic_load-example.cpp.

@isaachier
Copy link
Contributor Author

OK I'm going to call it a day. Let me try that example either later tonight or tomorrow. For now, I am pretty confident the build is working.

@isaachier
Copy link
Contributor Author

OK now running into this issue: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=52590.

@rnburn
Copy link
Contributor

rnburn commented Apr 11, 2018

Not sure why, but building like this instead of what I was doing with zipkin seems to avoid the problems with pthreads and, I think, produces a similar result.

Update: Looks like that's because it links in pthreads dynamically. Hopefully, pthreads is something you can rely on being present and stable across linux distributions.

@isaachier
Copy link
Contributor Author

Nice will try a bit later on my laptop.

@isaachier
Copy link
Contributor Author

OK so quick update. I have found that statically linking pthread is a controversial idea. It is about as bad as statically linking libc. Meanwhile, libstdc++ links to pthread but pthread uses weak symbols. For some reason, the symbols remain undefined. In your case @rnburn, can you paste the output of ldd libjaegertracing.so so I can see what is dynamically loaded.

@rnburn
Copy link
Contributor

rnburn commented Apr 12, 2018

Sure

root@f1319ff010ee:/# ldd libjaegertracing_plugin.so
	linux-vdso.so.1 =>  (0x00007fff2d5bc000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f4157f2b000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f4157b4b000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f4158683000)

Looks like it links to pthread dynamically.

@isaachier
Copy link
Contributor Author

isaachier commented Apr 13, 2018

Ya interesting. I think I'll give this another shot at some point tomorrow or over the weekend. Thanks!

@isaachier isaachier force-pushed the shared-plugin-release branch from 693ae5f to 52e01a7 Compare April 13, 2018 03:33
@isaachier
Copy link
Contributor Author

OK quick update. I have a desktop at home running Debian where is crashed consistently, but it was using GCC 6. I upgraded to GCC 7 and instead of a panic, I get EINVAL thrown as a system error the first time a mutex is locked. I'm not sure what's going on but clearly pthreads is part of the issue.

@isaachier
Copy link
Contributor Author

@rnburn I have tried numerous methods, but all of them crash. The issue is the weak symbols and people highly recommend against static linking with pthreads. Can I just keep standard library dynamic and link all other dependencies statically? Envoy is C++ anyway. If a user has an issue, I can work with him or her down the line.

@rnburn
Copy link
Contributor

rnburn commented Apr 20, 2018

I think linking in pthreads dynamically is fine.

But please keep the standard c++ library linked in statically. It won't work with envoy otherwise. (See my question on stackoverflow: https://stackoverflow.com/questions/47841812/how-to-support-dynamic-plugins-when-statically-linking-in-standard-libraries).

It shouldn't be too hard to build this way. The scripts I have here will make such a library: https://github.com/rnburn/cpp-client/tree/plugin/plugin

@isaachier
Copy link
Contributor Author

The problem is static linking in C++11 means libstdc++ "resolves" pthread functions to null. If libstdc++ were linked dynamically too, the problem would probably be avoided. See comment I found about this issue here: https://github.com/mozilla-b2g/minimal_prebuilt/blob/master/ndk/android-ndk-r6/sources/cxx-stl/gnu-libstdc%2B%2B/libs/armeabi/include/bits/gthr-default.h#L41-L48. I have no idea how those scripts work for you or in general. Maybe they are actually avoiding the static linking.

@rnburn
Copy link
Contributor

rnburn commented Apr 20, 2018

Here's the binary: https://github.com/rnburn/cpp-client/blob/plugin-store/plugin/libjaegertracing_plugin.so

I've verified that it works.

@rnburn
Copy link
Contributor

rnburn commented Apr 20, 2018

I have a docker-compose example set up that uses that library: https://github.com/rnburn/ot-chat. If you run it for Jaeger, you'll see that it produces traces just fine.

And the library doesn't have a dynamic dependency on libstdc++:

root@b3d971a760b7:/# ldd libjaegertracing_plugin.so
	linux-vdso.so.1 =>  (0x00007ffd941bf000)
	libpthread.so.0 => /lib/x86_64-linux-gnu/libpthread.so.0 (0x00007f2f72f07000)
	libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007f2f72b27000)
	/lib64/ld-linux-x86-64.so.2 (0x00007f2f7365f000)

@isaachier
Copy link
Contributor Author

isaachier commented Apr 20, 2018

BTW Bazel had to deal with this already for Envoy specifically. See bazelbuild/bazel#2840.

@isaachier
Copy link
Contributor Author

Maybe using gcc ... -lstdc++ like they suggest there will fix this.

@isaachier isaachier force-pushed the shared-plugin-release branch from e32b90d to 4f74933 Compare April 20, 2018 19:57
@rnburn
Copy link
Contributor

rnburn commented Apr 23, 2018

@isaachier - Did that work? If not, is there any reason you can't you can't use this approach?

@isaachier
Copy link
Contributor Author

@rnburn it did not work, but now I suspect my main problem was the build of the dynamic-trace_example code I used as a hook. If that code doesn't respect the same linking pattern, it might try to resolve code that need not be resolved. The problem I have with using your code is that it is CMake/high level. If you run make VERBOSE=1, I'd be happy to run any linking command you suggest to see if it works. Also, I have been working on a debian docker image but I can change that as well.

@rnburn
Copy link
Contributor

rnburn commented Apr 23, 2018

It's passing options that work with the existing cmake build. I don't know what's wrong with that?

But if you want to reproduce what it's doing, it's effectively building the shared library in single step instead of first building a PIC static library and turning that into a shared library.

@isaachier
Copy link
Contributor Author

I see your point. My fear is that CMAKE_SHARED_LINKER_FLAGS might not be propagated correctly (I know the cmake/toolchain.cmake file affects propagated build options). Even if it is, how does the libjaegertracing.so contain the other libraries. It seems you still need dynamic loading excepted for libgcc and libstdc++.

Build issues aside, this only explains why the linking succeeds, but the crash should still be reproducible in your build. How are you testing it?

@rnburn
Copy link
Contributor

rnburn commented Apr 23, 2018

@isaachier - It contains all other libraries because you set up hunter to build all those dependencies statically.

I have an example running with none of the dependencies installed. You can quickly run it with docker-compose if you want to check for yourself. And the CMAKE_SHARED_LINKER_FLAGS options don't need to be propagated to the hunter builds. They only need to be applied when linking the jaeger library.

@isaachier
Copy link
Contributor Author

OK awesome thanks. I'll try that but trying one more thing to salvage this gcc build... I just need to know why! It's driving me mad. Sorry for the delay. Will switch to your build if mine fails again (as I expect it will).

@isaachier
Copy link
Contributor Author

Finally, I think I got it to work! Another bonus of working on this was moving the plugin build to CMake so it isn't only available to Docker users.

@isaachier isaachier force-pushed the shared-plugin-release branch from 1d9cf7f to 89cc375 Compare April 24, 2018 01:24
@rnburn
Copy link
Contributor

rnburn commented Apr 24, 2018

That looks good. I'd only recommend keeping the march options separate from the cmake file so that you could use it to build a plugin for other architectures (e.g. x86-32 and arm)

@isaachier
Copy link
Contributor Author

Right, good point. Updated.

@isaachier isaachier force-pushed the shared-plugin-release branch from 41fefcd to 51baec9 Compare April 26, 2018 14:54
Signed-off-by: Isaac Hier <isaachier@gmail.com>
@isaachier isaachier force-pushed the shared-plugin-release branch from 51baec9 to ee59c62 Compare April 26, 2018 15:47
@isaachier isaachier merged commit 359979c into jaegertracing:master Apr 26, 2018
@isaachier isaachier deleted the shared-plugin-release branch April 26, 2018 20:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants