Bug 1565766 - qemu failing to build with clang; __atomic_fetch_or_4
Summary: qemu failing to build with clang; __atomic_fetch_or_4
Keywords:
Status: CLOSED ERRATA
Alias: None
Product: Fedora
Classification: Fedora
Component: qemu
Version: 28
Hardware: Unspecified
OS: Unspecified
unspecified
unspecified
Target Milestone: ---
Assignee: Fedora Virtualization Maintainers
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2018-04-10 17:50 UTC by Dr. David Alan Gilbert
Modified: 2018-07-30 18:25 UTC (History)
15 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed: 2018-07-30 18:25:02 UTC
Type: Bug


Attachments (Terms of Use)

Description Dr. David Alan Gilbert 2018-04-10 17:50:01 UTC
Description of problem:
Current qemu doesn't build with clang on f28, it used to on f27, and I _think_ this is a clang change; I've got a smaller reproducer as well.
The error is:
hw/display/qxl.c:1883: undefined reference to `__atomic_fetch_or_4'

that line (which hasn't changed for ~5 years) is:

old_pending = atomic_fetch_or(&d->ram->int_pending, le_events);

some macros turn that atomic_fetch_or into __atomic_fetch_or
and the 'int_pending' is a uint32_t in a QXLRam struct, and the QXLRam struct is marked packed.

Version-Release number of selected component (if applicable):
clang-libs-6.0.0-5.fc28.x86_64
clang-tools-extra-6.0.0-5.fc28.x86_64
clang-analyzer-6.0.0-5.fc28.noarch
clang-6.0.0-5.fc28.x86_64
clang-devel-6.0.0-5.fc28.x86_64


How reproducible:
100%

Steps to Reproduce:
1. Grab QEMU source
2. configure with   configure --target-list=x86_64-softmmu --cc=clang
3. make -j anumber

Actual results:
hw/display/qxl.c:1883: undefined reference to `__atomic_fetch_or_4'

Expected results:
A happy linked qemu; either with a call to an __atomic_fetch_or_4 or a suitable instruction

Additional info:

a simpler test case 
----
#include <stdio.h>
#include <stddef.h>
#include <stdint.h>

struct __attribute__ ((__packed__)) baz {
  uint32_t x3;
};

struct foo {
  uint32_t x;
  uint32_t x2;
  struct baz *b;
} fx;


static inline uint32_t cpu_to_le32(uint32_t v){ return (v);}

uint32_t foor(struct foo *p, uint32_t y)
{
    uint32_t ly = cpu_to_le32(y);
    return __atomic_fetch_or(&p->b->x3, y, 5);
}

uint32_t bah(uint32_t *p)
{
  return foor(&fx, 8);
}

int main()
{
}
----

clang -O2  t.c 
/tmp/t-78af46.o: In function `foor':
t.c:(.text+0xa): undefined reference to `__atomic_fetch_or_4'
/tmp/t-78af46.o: In function `bah':
t.c:(.text+0x22): undefined reference to `__atomic_fetch_or_4'
clang-6.0: error: linker command failed with exit code 1 (use -v to see invocation)

Comment 1 Paolo Bonzini 2018-04-11 13:52:38 UTC
Hmm this might not be a clang bug after all, at least not on all architectures.  If the struct is packed, some architecture may need to use libatomic (which takes a spinlock around atomic access).  This is not the case for x86_64, but QEMU probably should not use atomics on packed members.

Comment 2 Dr. David Alan Gilbert 2018-04-11 14:02:22 UTC
yeh, linking my test with -latomic makes it link (although that's not necessary with gcc), and I thought x86 could do atomic's on any crazy alignment?
(I think in reality the data is actually aligned in this case)

Comment 3 Tom Stellard 2018-04-11 14:24:43 UTC
This 'regression' in clang was introduced by this commit: https://reviews.llvm.org/rL314145 see also llvm.org/PR34347.

Comment 4 Paolo Bonzini 2018-04-11 14:38:05 UTC
> I thought x86 could do atomic's on any crazy alignment?

Yes, that's why I said on x86_64 it *is* a clang bug.  But QEMU probably wants to fail compilation on all architectures when you do this; we already do this for atomics above pointer size, even though x86_32 supports them.

BTW the problematic struct here is in spice, so that's where we'd have to fix it.  https://pastebin.com/raw/qPRTjDBk is a tentative patch based on this working version of David's testcase:

#include <stdio.h>
#include <stddef.h>
#include <stdint.h>

typedef struct __attribute__ ((__packed__)) baz {
  uint32_t x3;
} baz_packed;

typedef __attribute__ ((__aligned__(4096))) baz_packed baz;
struct foo {
  uint32_t x;
  uint32_t x2;
  baz *b;
} fx;


static inline uint32_t cpu_to_le32(uint32_t v){ return (v);}

uint32_t foor(struct foo *p, uint32_t y)
{
    uint32_t ly = cpu_to_le32(y);
    return __atomic_fetch_or(&p->b->x3, y, 5);
}

uint32_t bah(uint32_t *p)
{
  return foor(&fx, 8);
}

int main()
{
}

Comment 5 Frediano Ziglio 2018-06-28 14:16:55 UTC
Yes, the QXLRam is always aligned to a page. Note however that alignment affects also the sizeof operator which would potentially break some ABI if we expect data after a QXLRam (like monitor configuration if I remember correctly).
Looking at the structure is safe to assume this structure is 8 byte aligned.
Note also that for similar reasons also the ring structures define in this header should be 4-byte aligned.
Is not hard to define a/some macro(s) to have packed and aligned attributes in gcc/clang, however this header is also used in some Visual Studio project and in VS packed attribute behave usually slightly differently.

Comment 6 Frediano Ziglio 2018-06-29 08:48:00 UTC
Proposed a patch at https://lists.freedesktop.org/archives/spice-devel/2018-June/044384.html.
I suppose would need to be backported if accepted.

Comment 7 Fedora Update System 2018-07-16 08:20:52 UTC
spice-protocol-0.12.14-2.fc28 has been submitted as an update to Fedora 28. https://bodhi.fedoraproject.org/updates/FEDORA-2018-fdadcf6c59

Comment 8 Fedora Update System 2018-07-16 20:29:37 UTC
spice-protocol-0.12.14-2.fc28 has been pushed to the Fedora 28 testing repository. If problems still persist, please make note of it in this bug report.
See https://fedoraproject.org/wiki/QA:Updates_Testing for
instructions on how to install test updates.
You can provide feedback for this update here: https://bodhi.fedoraproject.org/updates/FEDORA-2018-fdadcf6c59

Comment 9 Fedora Update System 2018-07-30 18:25:02 UTC
spice-protocol-0.12.14-2.fc28 has been pushed to the Fedora 28 stable repository. If problems still persist, please make note of it in this bug report.


Note You need to log in before you can comment on or make changes to this bug.