C++ compiling & CMake integration & Linking order of static dependencies

GeoffreyRobert commented 4 years ago

Hi, I had some issues using your library in a small project of mine. I solved those issues and I thought I'd share here.

Setup

Ubuntu 19.10 gcc 9.2.1 cmake 3.17.0 meson 0.53.2 ninja 1.9.0

Issue compiling the standalone project (standard)

$ cd mini-async-log-c
$ meson build --buildtype=release
$ ninja -C build

I get the following error:

In file included from ../subprojects/base_library/include/bl/base/thread.h:12,
                 from ../subprojects/base_library/test/src/bl/base/time_test.c:6:
../subprojects/base_library/include/bl/base/impl/thread_c11.h: At top level:
../subprojects/base_library/include/bl/base/impl/thread_c11.h:52:48: error: unknown type name ‘bl_tss_cleanup_func’
   52 | static inline bl_err bl_tss_init (bl_tss* key, bl_tss_cleanup_func cleanup)
      |                                                ^~~~~~~~~~~~~~~~~~~

Cause

It seems that there is a discrepancy in the implementation files when using C11 threads instead of POSIX threads in the base-library dependency: bl/base/impl/thread_C11.h bl/base/impl/tss_posix.h

The implementation choice is defined in bl/base/thread.h which depends on bl/base/platform.h and since I use a recent version of gcc (I guess ?) the choice is made to enable BL_HAS_C11_THREAD.

NB: Interestingly this issue didn't arise when I tried to compile logger-bench and base-library as a standalone but I didn't dig much farther. NBB: malcpp did not compile with logger-bench though :(

Solution

I first chose the quick and dirty way of switching the preprocessor directives in bl/base/thread.h to give priority to the use of POSIX threads. I ended up with this error:

c++ -Isubprojects/base_library/b3ec909@@bl-nonblock-mpmc-bpm-relacy@exe -Isubprojects/base_library -I../subprojects/base_library -I../subprojects/base_library/include -I../subprojects/base_library/src -I../subprojects/base_library/test/src -I../subprojects/base_library/gitmodules/relacy -fdiagnostics-color=always -pipe -D_FILE_OFFSET_BITS=64 -Wall -Winvalid-pch -Wnon-virtual-dtor -std=c++11 -O3 -pthread -MD -MQ 'subprojects/base_library/b3ec909@@bl-nonblock-mpmc-bpm-relacy@exe/test_src_bl_mpmc_bpm_relacy_mpmc_bpm_relacy.cpp.o' -MF 'subprojects/base_library/b3ec909@@bl-nonblock-mpmc-bpm-relacy@exe/test_src_bl_mpmc_bpm_relacy_mpmc_bpm_relacy.cpp.o.d' -o 'subprojects/base_library/b3ec909@@bl-nonblock-mpmc-bpm-relacy@exe/test_src_bl_mpmc_bpm_relacy_mpmc_bpm_relacy.cpp.o' -c ../subprojects/base_library/test/src/bl/mpmc_bpm_relacy/mpmc_bpm_relacy.cpp
../subprojects/base_library/test/src/bl/mpmc_bpm_relacy/mpmc_bpm_relacy.cpp:304:10: fatal error: relacy/relacy.hpp: No such file or directory
  304 | #include <relacy/relacy.hpp>
      |          ^~~~~~~~~~~~~~~~~~~

With this error, I gave up the idea of using a C threading implementation since the logger would eventually be used in a C++ library.

Issue compiling the standalone project (C++ implementation)

I use the -xc++ flag to force gcc to compile c++ code regardless of the compiled file:

$ cd mini-async-log-c
$ CFLAGS=-xc++ meson build --buildtype=release
$ ninja -C build

I end up with a bunch of compiling errors related to the use of cmocka as unit testing framework (not reproduced here). Next try without building the tests:

$ cd mini-async-log-c
$ CFLAGS=-xc++ meson build --buildtype=release -Dbare=true
$ ninja -C build

I get this kind of error:

../subprojects/base_library/src/bl/base/dynamic_string_replace.c:178:1: error: jump to label ‘destroy_bl_dynarray’
  178 | destroy_bl_dynarray:
      | ^~~~~~~~~~~~~~~~~~~
../subprojects/base_library/src/bl/base/dynamic_string_replace.c:162:10: note:   from here
  162 |     goto destroy_bl_dynarray;
      |          ^~~~~~~~~~~~~~~~~~~
../subprojects/base_library/src/bl/base/dynamic_string_replace.c:164:9: note:   crosses initialization of ‘char* rptr_prev’
  164 |   char* rptr_prev   = s->da.str + bl_dstr_len (s);
      |         ^~~~~~~~~

Cause

This was caused by the use of goto labels that jumped over the declaration with initialization of temporary variables which seems to be allowed in C but not in C++. In the malc project:

In the base-library project:

Solution

I went the q-a-d way again and (stupidly) rewrote the branching logic in those code segments using flags to emulate the goto mechanism. I later stumbled on one of your commit or in-code commentary specifying that the goto mechanism was intended to improve code readability and it did... quite a lot.

I gave another try at solving this and simply created local scopes for the temporary variables jumped over by gotos. It did solve the goto issue.

Issue compiling the standalone project (C++ implementation #2)

So, again:

$ cd mini-async-log-c
$ CFLAGS=-xc++ meson build --buildtype=release -Dbare=true
$ ninja -C build

And this time I'm met with this kind of error:

../subprojects/base_library/src/bl/serial/serial_posix.c: In function ‘bl_err bl_serial_init(bl_serial**, bl_uword, const bl_alloc_tbl*)’:
../subprojects/base_library/include/bl/base/allocator.h:54:29: error: invalid conversion from ‘void*’ to ‘bl_serial*’ [-fpermissive]
   54 |   (bl_alloc_tbl_ptr)->alloc ((bytes), (bl_alloc_tbl_ptr))
      |   ~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~
      |                             |
      |                             void*

I didn't search this one through a lot and directly went:

$ cd mini-async-log-c
$ CFLAGS="-xc++ -fpermissive" meson build --prefix=/ --buildtype=release -Dbare=true
$ ninja -C build

And thus, I was able to compile the logger. Now to the CMake integration.

CMake integration & Linking order

This part is more for those interested by using this logger inside a CMake project. So, having everything compiled, I installed it with the complete compile + install command being:

$ cd mini-async-log-c
$ CFLAGS="-xc++ -fpermissive" meson build --prefix=/ --buildtype=release -Dbare=true
$ ninja -C build
$ DESTDIR=$PWD/install ninja -C build install

And, based on the README, I wrote my Findmini-async-log-c.cmake and managed to gather everything as STATIC IMPORTED libraries. I created an INTERFACE target to regroup everything under the same umbrella like this:

add_library(mini-async-log-c INTERFACE IMPORTED)
target_link_libraries(mini-async-log-c
    INTERFACE
        malc
        malcpp
        bl-base
        bl-nonblock
        bl-time-extras
        Threads::Threads
)

I used your example hello-malcpp.cpp inside my project to test the integration. In doing so, I did stumble on some linking error like:

/usr/bin/ld: /home//mini-async-log-c/install/lib/x86_64-linux-gnu/libmalcpp.a(src_malcpp_destinations.cpp.o): in function `malcpp::array_dst::size() const':
destinations.cpp:(.text+0x99): undefined reference to `malc_array_dst_size'

Which appeared to be related to my wrong ordering of static libraries when creating my umbrella target.
Furthermore I got some errors looking like:

/usr/bin/ld: /home//mini-async-log-c/install/lib/x86_64-linux-gnu/libmalc.a(src_malc_entry_parser.c.o): in function `push_obj_data(void*, char const*, char const*, malc_obj_log_data const*)':
entry_parser.c:(.text+0x598): undefined reference to `bl_itostr_dyn_arr'

Those errors stemed from the dependency on bl-tostr library from base-library, I added it to the mini-async-log-c target and got this target which, linked with the example, could compile:

add_library(mini-async-log-c INTERFACE IMPORTED)
target_link_libraries(mini-async-log-c
    INTERFACE
        malcpp
        malc
        bl-tostr
        bl-nonblock
        bl-time-extras
        bl-base
        Threads::Threads
)

And that's it !

Sorry for the big chunk of text, I hope you'll find some interesting feedback in it. And thanks for releasing this project, now having everything setup, I can't wait to give it an in-depth try.

NB: I attached the Findmini-async-log-c.cmake for those interested. You can use it as such:

list(INSERT CMAKE_MODULE_PATH 0 "path/to/find/modules")
set(mini-async-log-c_ROOT "abs/path/to/mini-async-log-c" CACHE PATH "Location of the mini-async-log-c library")
find_package(mini-async-log-c MODULE)

RafaGago commented 4 years ago

Thanks for reporting! I'll see when I have some time and I'll try to fix these issues.

RafaGago commented 4 years ago

Maybe I should use NIX to pin the build tools...

RafaGago commented 4 years ago

I read your message:

I have to look at compiling with gcc 9.2, that shoudn't happen.
The missing relacy.hpp header could have been caused because you didn't initialize the submodules recursively. The header could fail to compile, but not be missing:

git submodule update --init --recursive

Some other issues WRT compiling C11 code as C++ are expected. There are subtle differences between the languages, as you found on C++ pointers don't explicitly cast to void*.
Notice that for compiling this from CMake you can use ExternalProjectsAdd as done from the logger-bench project. The URI can be remote or local I think. Or you can just install the project and use pkgconfig:

https://stackoverflow.com/questions/29191855/what-is-the-proper-way-to-use-pkg-config-from-cmake

RafaGago commented 4 years ago

I installed both gcc and g++9 on Ubuntu 18.04 and I wasn't able to reproduce.

CC=gcc-9 CXX=g++-9 meson build

Do you face problems if you clone a clean repo from Ubuntu 19, and run:

git submodule update --init --recursive
meson build
ninja -C build

Github's CI is enabled in this project and is passing on "ubuntu-latest", which I guess is 19.04.

GeoffreyRobert commented 4 years ago

Hi, thanks for the reply.

I tried the following commands:

$ git clone <malc-url> malc
$ cd malc
$ git branch
* master
$ git submodule update --init --recursive
$ gcc-9 --version
gcc-9 (Ubuntu 9.2.1-9ubuntu2) 9.2.1 20191008
$ g++-9 --version
g++-9 (Ubuntu 9.2.1-9ubuntu2) 9.2.1 20191008
$ CC=gcc-9 CXX=g++-9 meson build
$ ninja -C build

I still get the following error:

In file included from ../subprojects/base_library/include/bl/base/thread.h:12,
                 from ../subprojects/base_library/test/src/bl/base/time_test.c:6:
../subprojects/base_library/include/bl/base/impl/thread_c11.h: At top level:
../subprojects/base_library/include/bl/base/impl/thread_c11.h:52:48: error: unknown type name ‘bl_tss_cleanup_func’
   52 | static inline bl_err bl_tss_init (bl_tss* key, bl_tss_cleanup_func cleanup)
      |                                                ^~~~~~~~~~~~~~~~~~~

There is this issue with the typedefs used with C11 threads in the base-library dependency: bl/base/impl/thread_C11.h

If I Crl+F this header for bl_tss_cleanup_func I find only one result of it being used as a parameter and AFAIK it is not defined in included headers. What do you think about this ?

RafaGago commented 4 years ago

I see, you are right on this. This is a clear bug.

I might not be able to reproduce because this depends on libc too, not gcc/g++ only.

RafaGago commented 4 years ago

If you download the attached patch and

cd subprojects/base_library
tar -xf <path>/patch.tar.gz
git am 0001-Fix-broken-C11-thread-local-storage-implementation.patch
cd -

Are you able to compile or are more broken things? I wrote the C11 thin wrapper part when it wasn't the default, so its mostly untested.

Otherwise a temporary workaround is to change line 92 on "subprojects/include/bl/base/platform.h", to:

#define BL_HAS_C11_THREAD 0

So you get the pthread based implementation. The only reason for me to implement the C11 part ahead of time was in case e.g. Windows starts supporting it sometime, as it's not POSIX. If we don't solve this bug here I will just disable the (broken) C11 implementation until I use some distro that has it enabled by default and learn the lesson to be a late adopter, not an early one.

patch.tar.gz

GeoffreyRobert commented 4 years ago

Hi, thanks for you quick reply and sorry for mine being late. So I applied your patch and still had some errors.

First build error:

In file included from ../subprojects/base_library/include/bl/base/thread.h:12,
                 from ../subprojects/base_library/test/src/bl/base/time_test.c:6:
../subprojects/base_library/include/bl/base/impl/thread_c11.h: In function ‘bl_mutex_destroy’:
../subprojects/base_library/include/bl/base/impl/thread_c11.h:87:35: error: invalid use of void expression
   87 |   return bl_thread_error_convert (mtx_destroy (m));
      |                                   ^~~~~~~~~~~~~~~

Because mtx_destroy returns void.

Second build error:

In file included from ../src/malc/memory.h:11,
                 from ../src/malc/malc.c:24:
../src/malc/tls_buffer.h: At top level:
../src/malc/tls_buffer.h:51:34: error: expected ‘=’, ‘,’, ‘;’, ‘asm’ or ‘__attribute__’ before ‘tls_buffer_out_of_scope_destroy’
   51 | extern void bl_tss_dtor_callconv tls_buffer_out_of_scope_destroy (void* opaque);
      |                                  ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The macro bl_tss_dtor_callconv was removed from thread_c11.h in the patch.

So I modified the patch (attached), tested it on a clean repo and everything compile perfectly. Thank you very much for your help. patch.tar.gz

RafaGago commented 4 years ago

Thanks for fixing this out. d6e32de71ee826c0d6fd8868e57283d0dba89082 solves your issue, so you don't have to patch anymore.

git pull && git submodule update --init --recursive

GeoffreyRobert commented 4 years ago

Great, I think this can be closed. I'll open another issue dedicated to some failed tests from test-all-variants.sh once I investigate it a bit further.

RafaGago / mini-async-log-c