[libvirt] [PATCH] Do not inline virNumaNodeIsAvailable

[Ján Tomko]

On Mon, Mar 09, 2015 at 02:03:16PM +0100, Martin Kletzander wrote:
> On Mon, Mar 09, 2015 at 10:09:17AM +0000, Daniel P. Berrange wrote:
> >On Mon, Mar 09, 2015 at 11:01:31AM +0100, Martin Kletzander wrote:
> >> On Thu, Mar 05, 2015 at 11:09:41AM +0000, Daniel P. Berrange wrote:
> >> >On Thu, Mar 05, 2015 at 12:05:52PM +0100, Ján Tomko wrote:
> >> >>Explicitly request that virNumaNodeIsAvailable not be inlined.
> >> >>This fixes the test suite when building with clang (3.5.1).
> >> >
> >> >Huh, so clang will inline functions, even if they are exported
> >> >in the .so library ?  Is there some clang compiler flag we can
> >> >use to stop that ?  I'd only expect it to inline stuff which
> >> >was declared static, or whose impl body was in the header file
> >> >
> >>
> >> If I understand it correctly, that means that clang is not
> >> "compatible" with gcc.
> >>
> >> Excerpt from gcc online docs [1]:
> >>
> >>  When a function is both inline and static, if all calls to the
> >>  function are integrated into the caller, and the function's address
> >>  is never used, then the function's own assembler code is never
> >>  referenced.
> >>
> >> Excerpt from gcc online docs [1]:
> >>
> >>  By default, Clang builds C code in GNU C11 mode, so it uses standard
> >>  C99 semantics for the inline keyword. These semantics are different
> >>  from those in GNU C89 mode, which is the default mode in versions of
> >>  GCC prior to 5.0.
> >>
> >> However further reading of the second documentation and c89 semantics
> >> it doesn't say anything about the fact that such function should be
> >> inlined.
> >
> >But we haven't added the 'inline' keyword to this function at
> >all - it is just a normal function marked for export in the
> >.so file, so I'm puzzelled why it is getting inlined.
> >
> 
> Exactly, that's what I'm trying to find out as well.
> 
> >>
> >> Anyway, is this clang 3.6 specific?  I don't have this problem when
> >> compiling with 3.5.  Nor does this show with gcc -std=gnu11.  I'm
> >> getting 3.6 to check whether that's the difference.
> >>
> 
> After updating clang and llvm from 3.5 to 3.6, I still don't get this
> error.  And I have only 4 (fake) nodes available, so it _is_ rewriting
> that function.

I'm getting the error with 3.5.1, as I said in the commit message.

These are the failing qemuxml2argvtest cases:
60) QEMU XML-2-ARGV hugepages-pages
... libvirt:  error : internal error: NUMA node 1 is unavailable
63) QEMU XML-2-ARGV hugepages-shared
... libvirt:  error : internal error: NUMA node 1 is unavailable
324) QEMU XML-2-ARGV numatune-memnode
... libvirt:  error : internal error: NUMA node 1 is unavailable
326) QEMU XML-2-ARGV numatune-memnode-no-memory
... libvirt:  error : internal error: NUMA node 3 is unavailable
329) QEMU XML-2-ARGV numatune-auto-prefer
... libvirt:  error : internal error: NUMA node 1 is unavailable

So with 4 fake nodes, the tests could still pass even if the function is
not mocked. Try changing the nodeset in #326 to 4 if it fails.

> 
> >> [1] https://gcc.gnu.org/onlinedocs/gcc/Inline.html
> >> [2] http://clang.llvm.org/compatibility.html
> >>
> >> >>---
> >> >>This only leaves the mysterious check-protocol failure.
> >>
> >> That's not that mysterious, it's just that we check the order and
> >> clang sorts enums before structs, but gcc doesn't.  Also clang adds
> >> "public:" to structs, so it probably treats it as a C++ or C# structs
> >> or something?
> >>
> 
> By the way if I compile with clang with -std=gnu11 or -std=gnu99, the
> "public:" stuff is gone :)
> 

It is mysterious, because it doesn't fail consistently.
It was working for me after I tried it again after
'git clean -fxd', today it failed again (though I don't remember if I
ran autogen again).

Jan
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: Digital signature
URL: <http://listman.redhat.com/archives/libvir-list/attachments/20150310/3e34e588/attachment-0001.sig>