Ticket #4584 (closed defect: fixed)
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:
- Platform support is limited (especially on ARM64 / PPC)
- It doesn't seem to be actively maintained
- 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: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.
git log --oneline 09320d5..65fee2d