Fix #251: cgreen-runner handle non-lib files

[fnadeau]

Remove dependency to nm and rather use libbfd, which is the same lib
used by nm. There is no additional depency, but cmake will fail if it
cannot find libbfd. There was no check for nm before.

Unit test still use nm to count the number of symbols. It provides a
double check on this implementation.

Before we would get:
$ ./build/tools/cgreen-runner ./build/tests/libbreadcrumb_tests.so
Running "libbreadcrumb_tests" (9 tests)...
"Breadcrumb": 9 passes in 3ms.
Completed "libbreadcrumb_tests": 9 passes in 3ms.

$ ./build/tools/cgreen-runner ./build/tests/Makefile
/usr/bin/nm: ./build/tests/Makefile: file format not recognized
No tests found in './build/tests/Makefile'.

$ ./build/tools/cgreen-runner /usr/lib/libgit2.so
/usr/bin/nm: /usr/lib/libgit2.so: no symbols
No tests found in '/usr/lib/libgit2.so'.

After this patch we get:
$ ./build/tools/cgreen-runner ./build/tests/libbreadcrumb_tests.so
Running "libbreadcrumb_tests" (9 tests)...
"Breadcrumb": 9 passes in 3ms.
Completed "libbreadcrumb_tests": 9 passes in 3ms.

$ ./build/tools/cgreen-runner ./build/tests/Makefile
No tests found in './build/tests/Makefile'.

$ ./build/tools/cgreen-runner /usr/lib/libgit2.so
No tests found in '/usr/lib/libgit2.so'.

[coveralls]

Coverage decreased (-12.3%) to 83.275% when pulling 873c5c9 on fnadeau:bugfix/cgreen-runner_nm_parsing into 9314651 on cgreen-devs:master.

[thoni56]

Thanks for this PR! It addresses something I/we have been thinking about for a long time. And it seems to be a good implementation with tests and all ;-)

It comes with a backside, though. Cgreen has been designed to be as portable as possible. This adds an extra dependency.

We really want to make sure that writing testcases and running them with a "programmed" runner is possible in as many environments as possible, including standard C-99 embedded environments.

As this PR only affects the cgreen-runner which isn't at the core of Cgreen test execution, I was thinking it could still be adopted.

But there are a couple of things that I want to bring up:

Documentation

There are currently no "special" dependencies so there is no natural place for documenting the new dependency on libbfd or binutils-dev. Could you please consider this and suggest a way to make this visible? This needs to be made visible to package maintainers and normal users alike.

User Experience when libbfd is missing

As it now stands the CMake build stops with

CMake Error: The following variables are used in this project, but they are set to NOTFOUND.
Please set them or make sure they are set and tested correctly in the CMake files:
LIBBFD_BFD_LIBRARY (ADVANCED)

I'm not sure this is good user experience, the message requires a bit of CMake knowledge and Google-fu, in the situation that you just want to compile and get going. It also brings to mind that there are situations where you want to just build the library (in those embedded environments e.g.) but easy support to opt out from building cgreen-runner is probably a different issue. Maybe a new target in the Makefile ;-)

Cygwin (and other platforms) build

On Cygwin there is no binutils-dev package as the one required on Ubuntu, which shows that even if nm is present binutils-dev might not be. But even adding the libbfd package did not allow a successful build. I get a lot of unresolved symbols:

[ 19%] Linking C executable cgreen-runner.exe
/usr/lib/gcc/x86_64-pc-cygwin/10/../../../../x86_64-pc-cygwin/bin/ld: /usr/lib/gcc/x86_64-pc-cygwin/10/../../../../lib/libbfd.a(hash.o): in function `bfd_hash_table_init_n':
/usr/src/debug/binutils-2.36.1-2/bfd/hash.c:385: undefined reference to `objalloc_create'
/usr/lib/gcc/x86_64-pc-cygwin/10/../../../../x86_64-pc-cygwin/bin/ld: /usr/src/debug/binutils-2.36.1-2/bfd/hash.c:392: undefined reference to `_objalloc_alloc'
/usr/lib/gcc/x86_64-pc-cygwin/10/../../../../x86_64-pc-cygwin/bin/ld: /usr/lib/gcc/x86_64-pc-cygwin/10/../../../../lib/libbfd.a(hash.o): in function `bfd_hash_table_free':
/usr/src/debug/binutils-2.36.1-2/bfd/hash.c:426: undefined reference to `objalloc_free'
/usr/lib/gcc/x86_64-pc-cygwin/10/../../../../x86_64-pc-cygwin/bin/ld: /usr/src/debug/binutils-2.36.1-2/bfd/hash.c:426: undefined reference to `objalloc_free'
/usr/lib/gcc/x86_64-pc-cygwin/10/../../../../x86_64-pc-cygwin/bin/ld: /usr/lib/gcc/x86_64-pc-cygwin/10/../../../../lib/libbfd.a(hash.o): in function `bfd_hash_insert':
/usr/src/debug/binutils-2.36.1-2/bfd/hash.c:536: undefined reference to `_objalloc_alloc'
/usr/lib/gcc/x86_64-pc-cygwin/10/../../../../x86_64-pc-cygwin/bin/ld: /usr/lib/gcc/x86_64-pc-cygwin/10/../../../../lib/libbfd.a(hash.o): in function `bfd_hash_lookup':
/usr/src/debug/binutils-2.36.1-2/bfd/hash.c:486: undefined reference to `_objalloc_alloc'
/usr/lib/gcc/x86_64-pc-cygwin/10/../../../../x86_64-pc-cygwin/bin/ld: /usr/lib/gcc/x86_64-pc-cygwin/10/../../../../lib/libbfd.a(hash.o): in function `bfd_hash_allocate':
/usr/src/debug/binutils-2.36.1-2/bfd/hash.c:623: undefined reference to `_objalloc_alloc'
...

The objalloc... stuff seems to come from libiberty for which Cygwin seems to have no package. Maybe this is a Cygwin issue...

Any ideas?

I have not tried to build on Msys2 or Darwin. Before including this PR I want to make sure that this builds cleanly on as many platforms as possible, and that end users don't have to go through this dance (I don't mind ;-), Cgreen should be as much plug-and-play as possible. If you are new to unittesting there are enough hurdles to jump over.

Valgrind

I tried to make valgrind on this change as it is fairly major. That did not compile either. Looks like it's missing the libbfd in the linking. (Valgrinding is also important since a user might want to ensure that their SUT is not losing memory, Cgreen should not be the culprit here. I should probably create a CONTRIBUTION.md...)

[matthargett]

libiberty definitely used to be in the Cygwin base distribution 20 years ago. Still, it looks like most of the feedback has been resolved in the follow-up commits. Can you take another look @thoni56 ?

[thoni56]

Thanks, @matthargett, for bringing this back to my attention. I had totally forgotten about this.

@fnadeau thank you for the updates, and sorry for the delay. This works like a charm on Ubuntu, but unfortunately I still have problems building this in Cygwin/64.

• there seems to be no cygwin64-libbfd package in the recent Cygwin distributions (filtering for libbfd only gets me cygwin32-libbfd which seems strange...). Googling shows this page indicating that it is ORPHANED, which might account for it no longer being available. I'll ask on the mailing list.
• CMake indicates -- Found LibBfd: /usr/lib/libbfd.a, which is in the binutils package but I still get linker errors, although different ones now:

/usr/lib/gcc/x86_64-pc-cygwin/11/../../../../x86_64-pc-cygwin/bin/ld: /usr/lib/gcc/x86_64-pc-cygwin/11/../../../../lib/libbfd.a(libbfd.o): in function `_bfd_generic_get_section_contents':
/usr/src/debug/binutils-2.37-2/bfd/libbfd.c:969: undefined reference to `libintl_dgettext'
/usr/lib/gcc/x86_64-pc-cygwin/11/../../../../x86_64-pc-cygwin/bin/ld: /usr/lib/gcc/x86_64-pc-cygwin/11/../../../../lib/libbfd.a(libbfd.o): in function `_bfd_warn_deprecated':
/usr/src/debug/binutils-2.37-2/bfd/libbfd.c:1133: undefined reference to `libintl_dgettext'
/usr/lib/gcc/x86_64-pc-cygwin/11/../../../../x86_64-pc-cygwin/bin/ld: /usr/src/debug/binutils-2.37-2/bfd/libbfd.c:1136: undefined reference to `libintl_dgettext'
/usr/lib/gcc/x86_64-pc-cygwin/11/../../../../x86_64-pc-cygwin/bin/ld: /usr/lib/gcc/x86_64-pc-cygwin/11/../../../../lib/libbfd.a(binary.o): in function `binary_set_section_contents':
/usr/src/debug/binutils-2.37-2/bfd/binary.c:276: undefined reference to `libintl_dgettext'
/usr/lib/gcc/x86_64-pc-cygwin/11/../../../../x86_64-pc-cygwin/bin/ld: /usr/lib/gcc/x86_64-pc-cygwin/11/../../../../lib/libbfd.a(ihex.o): in function `ihex_write_object_contents':
/usr/src/debug/binutils-2.37-2/bfd/ihex.c:843: undefined reference to `libintl_dgettext'
/usr/lib/gcc/x86_64-pc-cygwin/11/../../../../x86_64-pc-cygwin/bin/ld: /usr/lib/gcc/x86_64-pc-cygwin/11/../../../../lib/libbfd.a(ihex.o):/usr/src/debug/binutils-2.37-2/bfd/ihex.c:785: more undefined references to `libintl_dgettext' follow

• I also get -- Could NOT find Valgrind (missing: Valgrind_EXECUTABLE) repeated 26 times, not a big deal and probably related to CMake-(ill-)logic ;-)

I think Cygwin is becoming a bit marginalised nowadays, but I have a couple of project that I still need to build on Cygwin, so I'm reluctant to bring this in unless it works on Cygwin or we have special handling for it. But the only special handling that I can think of is using nm and that defeats the whole purpose of this, so not really an option...

I'll also investigate this on Msys2 64-bit "native", which is basically Cygwin, and see if there is any difference.

[thoni56]

• CMake indicates -- Found LibBfd: /usr/lib/libbfd.a, which is in the binutils package but I still get linker errors, although different ones now:

/usr/lib/gcc/x86_64-pc-cygwin/11/../../../../x86_64-pc-cygwin/bin/ld: /usr/lib/gcc/x86_64-pc-cygwin/11/../../../../lib/libbfd.a(libbfd.o): in function `_bfd_generic_get_section_contents':
/usr/src/debug/binutils-2.37-2/bfd/libbfd.c:969: undefined reference to `libintl_dgettext'
/usr/lib/gcc/x86_64-pc-cygwin/11/../../../../x86_64-pc-cygwin/bin/ld: /usr/lib/gcc/x86_64-pc-cygwin/11/../../../../lib/libbfd.a(libbfd.o): in function `_bfd_warn_deprecated':
/usr/src/debug/binutils-2.37-2/bfd/libbfd.c:1133: undefined reference to `libintl_dgettext'
/usr/lib/gcc/x86_64-pc-cygwin/11/../../../../x86_64-pc-cygwin/bin/ld: /usr/src/debug/binutils-2.37-2/bfd/libbfd.c:1136: undefined reference to `libintl_dgettext'
/usr/lib/gcc/x86_64-pc-cygwin/11/../../../../x86_64-pc-cygwin/bin/ld: /usr/lib/gcc/x86_64-pc-cygwin/11/../../../../lib/libbfd.a(binary.o): in function `binary_set_section_contents':
/usr/src/debug/binutils-2.37-2/bfd/binary.c:276: undefined reference to `libintl_dgettext'
/usr/lib/gcc/x86_64-pc-cygwin/11/../../../../x86_64-pc-cygwin/bin/ld: /usr/lib/gcc/x86_64-pc-cygwin/11/../../../../lib/libbfd.a(ihex.o): in function `ihex_write_object_contents':
/usr/src/debug/binutils-2.37-2/bfd/ihex.c:843: undefined reference to `libintl_dgettext'
/usr/lib/gcc/x86_64-pc-cygwin/11/../../../../x86_64-pc-cygwin/bin/ld: /usr/lib/gcc/x86_64-pc-cygwin/11/../../../../lib/libbfd.a(ihex.o):/usr/src/debug/binutils-2.37-2/bfd/ihex.c:785: more undefined references to `libintl_dgettext' follow

Ok, so these look mostly as an unresolved gettext dependency, maybe from libbfd? Google points here: https://www.gnu.org/software/gettext/FAQ.html#integrating_undefined. Should maybe libbfd set -lintl?

EDIT: Msys2 has the same problem.

[thoni56]

This is more of a status report than anything else...

I've fixed most of the issues I've seen (evident in the not exactly pretty commit sequence here...), but I still have one left. On Ubuntu 20.04 the discoverer unit tests build fine using the (verbose) CMake command:

/usr/bin/cc -fPIC  -std=c99 -Wstrict-prototypes  -shared -Wl,-soname,libdiscoverer_unit_tests.so -o libdiscoverer_unit_tests.so CMakeFiles/discoverer_unit_tests.dir/discoverer_unit_tests.c.o CMakeFiles/discoverer_unit_tests.dir/__/discoverer.c.o  -Wl,-rpath,/home/thoni/Utveckling/cgreen/build/src ../../src/libcgreen.so.1.4.0 -ldl -lbfd -Wl,-Bstatic -liberty -Wl,-Bdynamic -lz -lpthread -lstdc++ -lm -lgcc_s -lgcc -lc -lgcc_s -lgcc -lc

but the exact same step fails on Cygwin:

/usr/bin/cc  -std=c99 -Wstrict-prototypes -shared -Wl,--enable-auto-import -o cygdiscoverer_unit_tests.dll -Wl,--out-implib,libdiscoverer_unit_tests.dll.a -Wl,--major-image-version,0,--minor-image-version,0 CMakeFiles/discoverer_unit_tests.dir/discoverer_unit_tests.c.o CMakeFiles/discoverer_unit_tests.dir/__/discoverer.c.o  ../../src/libcgreen.dll.a -ldl -Wl,-Bstatic -lbfd -liberty -Wl,-Bdynamic -lz -lstdc++ -lgcc_s -lgcc -lcygwin -ladvapi32 -lshell32 -luser32 -lkernel32 -lgcc_s -lgcc -lcygwin -ladvapi32 -lshell32 -luser32 -lkernel32
/usr/lib/gcc/x86_64-pc-cygwin/11/../../../../x86_64-pc-cygwin/bin/ld: CMakeFiles/discoverer_unit_tests.dir/__/discoverer.c.o:discoverer.c:(.text+0x19d): undefined reference to `create_test_item_from'
/usr/lib/gcc/x86_64-pc-cygwin/11/../../../../x86_64-pc-cygwin/bin/ld: CMakeFiles/discoverer_unit_tests.dir/__/discoverer.c.o:discoverer.c:(.rdata$.refptr.destroy_test_item[.refptr.destroy_test_item]+0x0): undefined reference to `destroy_test_item'

Somehow test_item.c is not included in the build on Cygwin...

[thoni56]

Aaarrrgh... Now it builds cleanly... But only now do I realise that since shared libraries on Cygwin are actually PE32+ executables, obviously maybe libbfd won't be able to find any tests in them:

No tests found in 'build/tests/cygunit_tests.dll'.

So, what to do next? Building a fall-back solution for non-bfd environments does seem to be the only option...

I'll investigate if libbfd is supposed to support PE32+ and/or there is some problem with the usage in the runner.

[thoni56]

Yes, libbfd supports the COFF/PE format.

But since the call to bdf_get_dynamic_symtab_upper_bound() is linked to _bfd_long_bfd_n1_error which returns bfd_error_invalid_operation I'm suspecting that DLL's don't have dynamic symbols. This probably means that for DLL's we need to get the symbols using some other BFD calls...

I don't (didn't) know anything about the binary file formats, so I don't know how to explore this further. Do anyone know about a "command line bfd explorer" or something similar?

[thoni56]

We need to distinguish between dynamic (.so) and non-dynamic (.dll) libraries, which can be done looking at the flags.

I'll start with adding that check to split get_symbol_table() into two separate versions.

[thoni56]

Fixed. It was actually only a matter of using non-dynamic counterparts of two bfd-functions. So, ready to merge this now.

(Sorry for re-using this for the fixes, I actually thought that I had created a branch when I saw that it needed some more research, but well...)

[thoni56]

Well, I suppose I should have know with so much fiddling with building on Cygwin, MacOS, ... that my initial concern about portability would come back and bite us.

FYI, as indicated by #300, Debian has forbidden access to libbfd for packages in the distro, from which follows that many other distros will not carry a cgreen that links dynamically links with it.

This means that this, and the related changes that ports it to other platforms, has to be reverted, going back to the more portable nm parsing.

Sad, because it was an interesting approach and should really be the way to go (if it was just portable/stable enough).