[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]

Re: [libvirt] [PATCH] Rewrite the way mockable functions are handled.



On Thu, Jul 13, 2017 at 12:25:50PM +0100, Daniel P. Berrange wrote:
On Thu, Jul 13, 2017 at 01:14:12PM +0200, Martin Kletzander wrote:
On Wed, Jul 12, 2017 at 05:06:22PM +0100, Daniel P. Berrange wrote:
> Currently any mockable functions are marked with attributes
> noinline, noclone and weak. This prevents the compiler from
> optimizing away the impl of these functions.
>
> It has an unfortunate side effect with the libtool convenience
> libraries, if executables directly link to them. For example
> virlockd, virlogd both link to libvirt_util.la  When this is
> done, the linker does not consider weak symbols as being
> undefined, so it never copies them into the final executable.
>
> In this new approach, we stop annotating the headers entirely.
> Instead we create a weak function alias in the source.
>
>   int fooImpl(void) {
>      ..the real code..
>   }
>
>   int foo(void) __attribute__((noinline, noclone, weak, alias("fooImpl"))
>
> If any functions in the same file call "foo", this prevents the
> optimizer from inlining any part of fooImpl. When linking to the
> libtool convenience static library, we also get all the symbols
> present. Finally the test suite can just directly define a
> 'foo' function in its source, removing the need to use LD_PRELOAD
> (though removal of LD_PRELOADS is left for a future patch).
>

What are you using for tag navigation?  With this macro-defined
functions I cannot easily jump to them (the main reason why I don't like
macros defining functions).

Grep & historic knowledge :-)

                             I would very much prefer if
ATTRIBUTE_MOCKABLE just took a parameter like this:

#define ATTRIBUTE_MOCKABLE(func) __attribute__((noinline, noclone, weak, alias(#func "Impl"))

(written by hand, don't take mu word for it working out of the box) and
the original function would be left untouched.

Yeah, that is certainly a valid alternative. I was not entirely happy
with the results I get here. As long as we don't mind repeating the
arg list in both places, your approach is more attractive, despite
the duplication.


I think we don't if we do:

#define VIR_MOCKABLE(ret, name) typeof(name ## Impl) name \
   __attribute__((noinline, weak, __alias__(#name "Impl")));

or can't this be done for functions?  It worked when I tried it now.

I have one more idea, though.  So that we don't have to double-type the
actual definition of the Impl function, we can make it static.  What if
we make *all* functions static and only add weak aliases to those that
are supposed to be used outside the file?  That is after we deal with
the Darwin case, of course.

Also, this fails on OS X [1] and I don't really see why:

util/vircommand.c:988:1: error: only weak aliases are supported on darwin
VIR_MOCKABLE(void,
^
./internal.h:252:60: note: expanded from macro 'VIR_MOCKABLE'
     ret name(__VA_ARGS__) __attribute__((noinline, weak, __alias__(#name "Impl"))); \

It appears clang simply doesn't support 'alias' on Darwin.

Docs suggest that we need to use 'weakref(#name  "Impl")' so I'll have
to test whether that's viable for our usage scenario or not.

Regards,
Daniel
--
|: https://berrange.com      -o-    https://www.flickr.com/photos/dberrange :|
|: https://libvirt.org         -o-            https://fstop138.berrange.com :|
|: https://entangle-photo.org    -o-    https://www.instagram.com/dberrange :|

--
libvir-list mailing list
libvir-list redhat com
https://www.redhat.com/mailman/listinfo/libvir-list

Attachment: signature.asc
Description: Digital signature


[Date Prev][Date Next]   [Thread Prev][Thread Next]   [Thread Index] [Date Index] [Author Index]