Ticket #4584 (closed defect: fixed)

Opened 2 months ago

Last modified 4 weeks ago

Fix tests on non-GNU ld toolchains

Reported by: zaytsev Owned by: zaytsev
Priority: major Milestone: 4.8.33
Component: tests Version: master
Keywords: Cc:
Blocked By: Blocking: #3542, #4170
Branch state: merged Votes for changeset: committed-master

Description

Currently, the test suite is broken on non-GNU ld toolchains because of the way the unit tests are written with check. The tests need to mock some internal mc functions or even external library functions like fork.

I have researched the options and found the following approaches:

Multiple symbol definitions

This is currently used in mc. The object files contain symbols with the same name as the function being mocked. The --allow-multiple-definition (-z muldefs) option is used and the linker takes the first definition it sees:

Normally when a symbol is defined multiple times, the linker will report a fatal error. These options allow multiple definitions and the first definition will be used.

This approach mostly works until it doesn't (#3542), resulting in extremely confusing and hard to debug errors. Worst of all, other linkers, such as the one used on macOS, simply don't allow multiple symbol definitions at all, so we are limited to the GNU linker.

Runtime GOT poisoning

This is by far my favorite approach from a theoretical standpoint. It doesn't require any changes to the source code and should work reliably for both internal and external calls. The downside is that it's very platform-specific. I have only found one library that does this, but:

  1. Platform support is limited (especially on ARM64 / PPC)
  2. It doesn't seem to be actively maintained
  3. It's not present in distributions

https://github.com/Snaipe/Mimick

LD_PRELOAD

Another option is to put all the mock functions in a separate shared object and use the dynamic linker preload mechanism to inject them into the test executables. This is like the above approach, but only supported by the dynamic loader.

This doesn't sound too bad to me, but it's a lot of work and platform specific. Also, what to do about platforms with static linking (think AIX?). Weird and rare platforms are also where testing is most useful for finding bugs.

Function pointers

One can hide all functions that should be mocked behind function pointers and switch them in tests. All library functions will need wrappers. Sounds very inelegant and tedious to me.

Weak symbols

This is a bit like function pointers, in that you have to wrap all library functions, but at least there is no juggling with pointers, and maybe marking functions in source code that are mocked in tests is not a bad thing after all. Seems to be widely supported (at least on Linux, macOS, Solaris, ...).

I'm not sure it's a good thing to have weak symbols in the binary, but dynamic linkers should actually treat them as strong, and if you want to mess with mc and already have the required level of access, you could use LD_PRELOAD.

Solution

I've managed to get the test suite to run on macOS and fix the problems on Linux with the weak symbol approach. I think this is the way to go.

Change History

comment:1 Changed 2 months ago by zaytsev

  • Blocking 3542 added

comment:2 Changed 2 months ago by zaytsev

  • Blocking 4170 added

comment:3 Changed 6 weeks ago by zaytsev

  • Status changed from new to accepted
  • Owner set to zaytsev
  • Branch state changed from no branch to on review

Branch: 4584_tests_weak_symbols
Initial changeset:bcf3b8c37c4c76f51e02d27cdb7c2a3d9e6b3ca4

With these changes the tests run on macOS.

comment:4 Changed 4 weeks ago by zaytsev

Rebased on current master and resolved conflicts, changeset:340a00fa0963a59dbf00bddf7e6f15f47bb792cd .

comment:5 Changed 4 weeks ago by andrew_b

  • Votes for changeset set to andrew_b
  • Branch state changed from on review to approved

The first commit message doesn't have a header with ticket number.
The last commit is not relevant to this ticket. It should be in the cleanup branch or directly in master.

comment:6 Changed 4 weeks ago by zaytsev

  • Status changed from accepted to testing
  • Votes for changeset changed from andrew_b to committed-master
  • Resolution set to fixed
  • Branch state changed from approved to merged

Unrelated commit moved to master, commit message for the first commit was changed.

Merged to master as changeset:65fee2daf707a6afff7a1449637207067082da2e.

Version 0, edited 4 weeks ago by zaytsev (next)

comment:7 Changed 4 weeks ago by zaytsev

  • Status changed from testing to closed
Note: See TracTickets for help on using tickets.