Re: [libvirt] [PATCH 2/2] Revert "Prevent more compiler optimization of mockable functions"

On Wed, Jul 12, 2017 at 01:10:08PM +0200, Martin Kletzander wrote: > On Wed, Jul 12, 2017 at 11:14:16AM +0100, Daniel P. Berrange wrote: > > This reverts commit e4b980c853d2114b25fa805a84ea288384416221. > > > > When a binary links against a .a archive (as opposed to a shared library), > > any symbols which are marked as 'weak' get silently dropped. As a result > > when the binary later runs, those 'weak' functions have an address of > > 0x0 and thus crash when run. > > > > This happened with virtlogd and virtlockd because they don't link to > > libvirt.so, but instead just libvirt_util.a and libvirt_rpc.a. The > > virRandomBits symbols was weak and so left out of the virtlogd & > > virtlockd binaries, despite being required by virHashTable functions. > > > > Various other binaries like libvirt_lxc, libvirt_iohelper, etc also > > link directly to .a files instead of libvirt.so, so are potentially > > at risk of dropping symbols leading to a later runtime crash. > > > > This is normal linker behaviour because a weak symbol is not treated > > as undefined, so nothing forces it to be pulled in from the .a You > > have to force the linker to pull in weak symbols using -u$SYMNAME > > which is not a practical approach. > > > > How is this done by gnulib (or libc) when most their functions are weak > aliases anyway? Can't we use the same approach they have? > virtlo{g,ck}d link with libgnu.la as well and there is no problem with > that, right? So I guess this _must_ be solvable somehow, IMHO. > > I'm just curious how that works. > > Martin I guess we would have to do something like the following, but for every function. diff --git i/src/util/virrandom.c w/src/util/virrandom.c index 41daa404b273..3d9fe7f85d97 100644 --- i/src/util/virrandom.c +++ w/src/util/virrandom.c @@ -99,7 +99,7 @@ VIR_ONCE_GLOBAL_INIT(virRandom) * * Return: a random number with @nbits entropy */ -uint64_t virRandomBits(int nbits) +static uint64_t __virRandomBits(int nbits) { uint64_t ret = 0; int32_t bits; @@ -125,6 +125,7 @@ uint64_t virRandomBits(int nbits) virMutexUnlock(&randomLock); return ret; } +uint64_t virRandomBits(int nbits) ATTRIBUTE_MOCKABLE __attribute__((alias("__virRandomBits"))); /** diff --git i/src/util/virrandom.h w/src/util/virrandom.h index 990a456addf7..abda95aef506 100644 --- i/src/util/virrandom.h +++ w/src/util/virrandom.h @@ -24,7 +24,7 @@ # include "internal.h" -uint64_t virRandomBits(int nbits) ATTRIBUTE_MOCKABLE; +uint64_t virRandomBits(int nbits); double virRandom(void); uint32_t virRandomInt(uint32_t max); int virRandomBytes(unsigned char *buf, size_t buflen) -- And of course that could be macrofied so that ATTRIBUTE_MOCKABLE takes function or something, etc. I like this more than reverting the patches.