[PATCH 0/2] Work around broken clang. Again.

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

 



We ran into so many clang bugs recently, that I'm strongly in favor of
making it optional and aim on gcc only. Debugging these issues burns our
time needlessly. Just consider what's happening here.

I've pushed patches recently that call some virNuma*() APIs during QEMU
cmd line generation. And while at it, I removed virNuma*() mocks from
qemuxml2argvmock.c (v9.1.0-213-g95ae91fdd4). Andrea told me in review to
make sure our CI works. So I've fired up my freebsd VM and compiled my
code there. It worked. Except, I'm compiling with -O0 and our CI
compiles with the default (-O2). And of course this is the source of the
current build breakage.

Long story short, clang is so eager to produce the fastest code that it
simply ignores written code and generates what it *assumes* behaves the
same. Well, it doesn't and this breaks our mocking and thus our tests.
And of course it's happening only with optimizations.

In this particular case, I've noticed that while
virNumaNodesetIsAvailable() calls virNumaNodeIsAvailable(), it
disregards the return value and virReportError()-s immediately.
This is because the !WITH_NUMACTL variant of virNumaNodeIsAvailable()
calls virNumaGetMaxNode() which fails and thus false is returned.

WORSE, then I stepped in gdb into the mock, I've seen random numbers as
integers. This is because the function from virnumamock.c wasn't
optimized as much and followed standard calling convention (it grabbed
the integer argument from the stack). But the caller
(virNumaNodesetIsAvailable()) was so optimized that it did not even
bothered to push anything onto stack.

After these patches, there is still one qemuxml2argvtest case failing,
and it's simply because no matter how hard I try, I can't stop clang
from optimizing the function. Consider the following code:

  virNumaCPUSetToNodeset(virBitmap *cpuset,
                         virBitmap **nodeset)
  {
      g_autoptr(virBitmap) nodes = virBitmapNew(0);
      ssize_t pos = -1;

      while ((pos = virBitmapNextSetBit(cpuset, pos)) >= 0) {
          int node = virNumaGetNodeOfCPU(pos);

          if (node < 0) {
              virReportSystemError(errno,
                                   _("Unable to get NUMA node of cpu %zd"),
                                   pos);
              return -1;
          }
          ...
      }
      ...
  }

And this is the assembly code that clang generates (even after
VIR_OPTNONE treatment):


  116f5f:       e8 0c b7 f9 ff          call   b2670 <virBitmapNew@plt>
  116f64:       48 89 44 24 40          mov    %rax,0x40(%rsp)
  116f69:       48 c7 44 24 18 ff ff    movq   $0xffffffffffffffff,0x18(%rsp)
  116f70:       ff ff
  116f72:       48 8b 7c 24 38          mov    0x38(%rsp),%rdi
  116f77:       48 8b 74 24 18          mov    0x18(%rsp),%rsi
  116f7c:       e8 0f 74 f9 ff          call   ae390 <virBitmapNextSetBit@plt>
  116f81:       eb 00                   jmp    116f83 <virNumaCPUSetToNodeset+0x43>
  116f83:       48 89 44 24 18          mov    %rax,0x18(%rsp)
  116f88:       48 83 f8 00             cmp    $0x0,%rax
  116f8c:       0f 8c b2 00 00 00       jl     117044 <virNumaCPUSetToNodeset+0x104>
  116f92:       48 8b 7c 24 18          mov    0x18(%rsp),%rdi
  116f97:       e8 d4 a1 f9 ff          call   b1170 <virNumaGetNodeOfCPU@plt>
  116f9c:       c7 44 24 10 ff ff ff    movl   $0xffffffff,0x10(%rsp)
  116fa3:       ff
  116fa4:       83 7c 24 10 00          cmpl   $0x0,0x10(%rsp)
  116fa9:       7d 74                   jge    11701f <virNumaCPUSetToNodeset+0xdf>
  116fab:       e8 80 72 f9 ff          call   ae230 <__errno_location@plt>
  116fb0:       8b 18                   mov    (%rax),%ebx
  116fb2:       48 8d 3d 27 35 2b 00    lea    0x2b3527(%rip),%rdi        # 3ca4e0 <vmdk4GetBackingStore.prefix+0xf260>
  116fb9:       48 8d 35 8c e3 25 00    lea    0x25e38c(%rip),%rsi        # 37534c <modeMap+0x215c>
  116fc0:       ba 05 00 00 00          mov    $0x5,%edx
  116fc5:       e8 e6 a7 f9 ff          call   b17b0 <dcgettext@plt>
  116fca:       48 8b 4c 24 18          mov    0x18(%rsp),%rcx
  116fcf:       48 89 0c 24             mov    %rcx,(%rsp)
  116fd3:       48 8d 15 ea e0 25 00    lea    0x25e0ea(%rip),%rdx        # 3750c4 <modeMap+0x1ed4>
  116fda:       48 8d 0d 54 e3 25 00    lea    0x25e354(%rip),%rcx        # 375335 <modeMap+0x2145>
  116fe1:       31 ff                   xor    %edi,%edi
  116fe3:       89 de                   mov    %ebx,%esi
  116fe5:       41 b8 10 04 00 00       mov    $0x410,%r8d
  116feb:       49 89 c1                mov    %rax,%r9
  116fee:       31 c0                   xor    %eax,%eax
  116ff0:       e8 8b c1 f9 ff          call   b3180 <virReportSystemErrorFull@plt>


My assembler is a bit rusty, but at virNumaGetNodeOfCPU@plt is called
116f97, after which, -1 is stored on a stack (which corresponds to @pos
variable), after which comparison against 0 is made and the jump happens
iff the value (-1) is greater or equal than zero. Otherwise
virReportSystemError() is called.

To reproduce this issue even on Linux, configure with:

  export CC=clang
  meson setup -Dsystem=true -Dexpensive_tests=enabled -Dnumactl=disabled _build


At this point, I think we can call clang broken and focus our time on
developing libvirt. Not coming up with hacks around broken compilers.

Michal Prívozník (2):
  internal.h: Invent VIR_OPTNONE
  virnuma: Annotate some functions as VIR_OPTNONE

 src/internal.h     | 20 ++++++++++++++++++++
 src/util/virnuma.c | 20 ++++++++++----------
 2 files changed, 30 insertions(+), 10 deletions(-)

-- 
2.39.2




[Index of Archives]     [Virt Tools]     [Libvirt Users]     [Lib OS Info]     [Fedora Users]     [Fedora Desktop]     [Fedora SELinux]     [Big List of Linux Books]     [Yosemite News]     [KDE Users]     [Fedora Tools]

  Powered by Linux