On 3/15/23 20:10, Michal Privoznik wrote: Forgot to mention: huge thanks to Martin Kletzander, who helped me debug all of this. Michal

On Wed, Mar 15, 2023 at 08:10:26PM +0100, Michal Privoznik wrote: snip snip Clang's behaviour is not broken or wrong. The code it generates is functionally correct when used, because according to the C stndard it is forbidden to define the same function more than once. Apps linked aginst libvirt.so or our daemons all operate perfectly. The problem is confined to our test suite where we're trying to selectively replace code at runtime and in doing so, we're making assumptions about code optimization that are not valid. In doing this we're violating the language standard. Of course this is a feature that ELF explicitly supports, despite being outside the language stndards. What this means is that we can't assume the compiler default behaviour will do what we want. The sepecific problem we're facing here is that the compiler is doing inter-procedural analysis because it can see the body of both functions. We've already tried to suppress the optimization by using the 'noinline' annotation but unfortunately is possible to keep a function as non-inlined, but still be wrong. Whuat we're seeing here is that a separate optimization is looking at the return value and determining that it is a compile time constant and thus elide the conditional, while still calling the orginal function ! I can demonstrate this a little more clearly with an isolated example: $ cat inline.h int IsAvailable(int i) __attribute__((noinline)); int IsSomethingOK(int i); $ cat inline.c #include <stdbool.h> int IsAvailable(int i) { fprintf(stderr, "Bad hit\n"); return false; } #endif int IsSomethingOK(int i) { while (i--) { if (IsAvailable(i)) continue; fprintf(stderr, "Bad stuff\n"); return -1; } fprintf(stderr, "OK\n"); return 0; } $ cat mock.c #include <stdbool.h> #include <stdio.h> #include "inline.h" int IsAvailable(int i) { fprintf(stderr, "Good hit\n"); return true; } $ LD_LIBRARY_PATH=. ./something Bad hit Bad stuff Here we see 'Bad hit' so it must be running the original IsAvailable() code body, and we see 'Bad stuff' so it must be seeing the 'return false' return value. Now running with the override $ LD_PRELOAD=mock.so LD_LIBRARY_PATH=. ./something Good hit Bad stuff we see 'Good hit' so it is running our replacement mocked impl of IsAvailable(), but we still see 'Bad stuff' so it must be still using the old 'return value' value. This proves that the "inlining" optimization is separate from the "constant return value" optimization. We're only disabling the former optimization, so our mocks are not safe. We've hit this about 6 years ago which lead to using 'weak' annotation too: commit e4b980c853d2114b25fa805a84ea288384416221 Author: Daniel P. Berrangé <berrange(a)redhat.com&gt; Date: Wed Jul 5 11:46:28 2017 +0100 Prevent more compiler optimization of mockable functions Currently all mockable functions are annotated with the 'noinline' attribute. This is insufficient to guarantee that a function can be reliably mocked with an LD_PRELOAD. The C language spec allows the compiler to assume there is only a single implementation of each function. It can thus do things like propagating constant return values into the caller at compile time, or creating multiple specialized copies of the function body each optimized for a different caller. To prevent these optimizations we must also set the 'noclone' and 'weak' attributes. This fixes the test suite when libvirt.so is built with CLang with optimization enabled. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; but this caused unwanted side effects: commit 407a281a8e2b6c5078ba1148535663ea64fd9314 Author: Daniel P. Berrangé <berrange(a)redhat.com&gt; Date: Wed Jul 12 11:07:17 2017 +0100 Revert "Prevent more compiler optimization of mockable functions" 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. This risk is silent bad linkage that affects runtime behaviour is not acceptable for a fix that was merely trying to fix the test suite. So stop using __weak__ again. Signed-off-by: Daniel P. Berrange <berrange(a)redhat.com&gt; There's a bit of a thread here: https://listman.redhat.com/archives/libvir-list/2017-July/150772.html I tied another approach here: https://listman.redhat.com/archives/libvir-list/2017-July/150809.html which didn't work well enough and then IIRC we abandoned attempts at that time. Meanwhile though GCC gained a 'noipa' attribute (no inter procedural analysis) which can suppress these return value optimizations, amongst other things. CLang though doesn't support it yet https://reviews.llvm.org/D101011 Looking at the compiler flags though there is another option which is to pass '-fsemantic-interposition'. With CLang this defaults to off, which means it is free to assume replacement functions have the same semantics. Passing -fsemantic-interposition forces it to assume that replacements might have different semantics, and thus cannot do the return value optimizations. The downside is that it would have a impact on performance across everything we compile with that arg, but I don't think libvirt stuff is generally that performance sensitive, so we can probably just go with -fsemantic-interposition on clang unconditionally. With 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 :|

On 3/16/23 15:51, Andrea Bolognani wrote: I'm fine with doing that change. FreeBSD offers option to install gcc. And for macOS - the fact we compile there says absolutely nothing. Anybody willing to make libvirt work on macOS is more than welcome to post patches. Yep. That won't do any good as it's not the root cause of this problem. The problem is (was until Dan fixed it) that even though I've specifically marked some functions to be not optimized, clang ignored that and optimized them anyway. And in this particular case it was virNumaCPUSetToNodeset() and virNumaGetNodeOfCPU() which are new. They were not implemented in the commit you're referencing. The problem is, __attribute__((optnone)) works only when passed at function definition, while noinline can be passed at function definition. At any rate, this is not solved (for new enough clang, i.e. 11.0+) and for even newer clang, where completely unrelated issue is happening, I've posted another fix [1]. Which brings me back to the question from the cover letter - how many clang related problems are acceptable for us? 1: https://listman.redhat.com/archives/libvir-list/2023-March/238923.html Michal

On 3/20/23 15:24, Daniel P. Berrangé wrote: The -fsemantic-interposition is not documented in clang. At least not in its manpage or --help output. I had to go to gcc's manpage to learn what the option is doing. But okay, CLang itself is not broken. But the defaults they chose are weird and we are investing non-negligible amount of time trying to fix them. Not to mention, that apparently the optimization was enabled way before users were able to turn it off. Speaking of which, the option was initially available for x86_64 only: https://github.com/llvm/llvm-project/commit/68a20c7f36d1d51cc46c0bd17384c... Hence, FreeBSD 12 (clang 10.0.1) is foobared. And macOS very likely did not pick up aforementioned commit hence it claims it doesn't have the option. For some reason, I am not able to reproduce this issue with no combination of -ON and -fno-semantic-interposition/-fsemantic interposition with GCC. So CLang must be doing something more than GCC and semantic-interposition stops it. Michal

On Mon, Mar 20, 2023 at 03:17:24PM +0100, Michal Prívozník wrote: clang is the system compiler for those platforms, and IMHO we should be compatible with it. Just as Fedora was long unhappy with certain apps trying to force use of clang, when Fedora's system compiler was GCC, the opposite is reasonable for other platforms. Is that a problem in fact ? We only need to suppress inlining when the calling function is in the same compilation unit as the called function. Likewise for this return value optimization. So if 'noinline' annotation was in the definition, rather than the declaration, it would be fine for our purposes. I think it would make sense to have a VIR_MOCKABLE annotation that covered 'noline', 'optnone' and 'noipa' attributes (the precise set varying according to what compiler we target). If anything this is more appealing that having it in the .h file, since this annotations are not intended to influence callers from external compilation units. We would need to update our test program that validates the existance of noinline for mocked functions to look in the .c instead of .h With 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 :|

On 3/21/23 11:46, Daniel P. Berrangé wrote: The problem we're facing is not that functions we mock are optimized, but their callers are optimized in such way that our mocking is ineffective. IOW: int mockThisFunction() { ... } int someRegularNotMockedFunction() { ... mockThisFunction(); ... } we would need to annotate someRegularNotMockedFunction() and leave mockThisFunction() be. This will get hairy pretty soon. And just for the record: the correct set of attributes that work for me are: optnone and used. Michal

On 3/21/23 15:19, Daniel P. Berrangé wrote: D'oh. Spoke too early. What I did also was rewrite virNumaGetNodeOfCPU() as follows: int VIR_MOCKABLE virNumaGetNodeOfCPU(int cpu) { errno = ENOSYS; if (cpu < 0) return -1; return -1; } (just don't make that a ternary operator, as it stops working) Where VIR_MOCKABLE is: #if defined(__clang__) # define VIR_OPTNONE __attribute__((optnone)) #else # define VIR_OPTNONE #endif #if defined(__clang__) # define VIR_NOIPA #else # define VIR_NOIPA __attribute__((noipa)) #endif #define VIR_MOCKABLE VIR_OPTNONE VIR_NOIPA __attribute__((noinline)) __attribute__((used)) BUT, I also had to annotate virNumaCPUSetToNodeset() which is NOT mocked. And this is the most annoying thing about whole approach. Since we already have a fix and now are fighting two failed CI builds, I wonder whether it makes sense to just forcibly disable all optimizations IF the compiler is clang and it doesn't have -fsemantic-interposition option. Michal