| Message ID | 20210712155918.1422519-1-richard.henderson@linaro.org |
|---|---|
| Headers | show |
| Series | Atomic cleanup + clang-12 build fix | expand |
On 7/12/21 11:59 AM, Richard Henderson wrote: > The first two patches are not strictly required, but they > were useful in tracking down the root problem here. > > I understand the logic behind the clang-12 warning, but I think > it's a clear mistake that it should be enabled by default for a > target where alignment is not enforced by default. > > I found over a dozen places where we would have to manually add > QEMU_ALIGNED(8) to uint64_t declarations in order to suppress > all of the instances. IMO there's no point fighting this. > I tested your patches, they seem to get rid of the warnings. The errors persist. FWIW here's my reproduce starting from fedora 34 x86_64 host: $ sudo mock --root fedora-35-i386 --install dnf --install dnf-utils --install fedora-packager --install clang $ sudo mock --root fedora-35-i386 --shell --enable-network # dnf builddep -y qemu # git clone https://github.com/qemu/qemu # cd qemu # CC=clang CXX=clang++ ./configure --disable-werror # make V=1 Thanks, Cole
On 7/12/21 2:30 PM, Cole Robinson wrote: > On 7/12/21 11:59 AM, Richard Henderson wrote: >> The first two patches are not strictly required, but they >> were useful in tracking down the root problem here. >> >> I understand the logic behind the clang-12 warning, but I think >> it's a clear mistake that it should be enabled by default for a >> target where alignment is not enforced by default. >> >> I found over a dozen places where we would have to manually add >> QEMU_ALIGNED(8) to uint64_t declarations in order to suppress >> all of the instances. IMO there's no point fighting this. >> > > I tested your patches, they seem to get rid of the warnings. The errors > persist. > > FWIW here's my reproduce starting from fedora 34 x86_64 host: > > $ sudo mock --root fedora-35-i386 --install dnf --install dnf-utils > --install fedora-packager --install clang > $ sudo mock --root fedora-35-i386 --shell --enable-network > # dnf builddep -y qemu > # git clone https://github.com/qemu/qemu > # cd qemu > # CC=clang CXX=clang++ ./configure --disable-werror > # make V=1 Ho hum. So, the warnings are where clang has decided to insert calls to libatomic. So we either have to (1) work around all of the places, which, unless we set up an i386 clang-12 builder will quickly bitrot, or (2) write our own routines, compatible with libatomic, using cmpxchg8b directly. which requires no (extra) locking, and so is compatible with the tcg jit output, or (3) file a bug with clang, and document "use clang-11 and not clang-12". Thoughts? r~
On 7/12/21 5:37 PM, Richard Henderson wrote: > On 7/12/21 2:30 PM, Cole Robinson wrote: >> On 7/12/21 11:59 AM, Richard Henderson wrote: >>> The first two patches are not strictly required, but they >>> were useful in tracking down the root problem here. >>> >>> I understand the logic behind the clang-12 warning, but I think >>> it's a clear mistake that it should be enabled by default for a >>> target where alignment is not enforced by default. >>> >>> I found over a dozen places where we would have to manually add >>> QEMU_ALIGNED(8) to uint64_t declarations in order to suppress >>> all of the instances. IMO there's no point fighting this. >>> >> >> I tested your patches, they seem to get rid of the warnings. The errors >> persist. >> >> FWIW here's my reproduce starting from fedora 34 x86_64 host: >> >> $ sudo mock --root fedora-35-i386 --install dnf --install dnf-utils >> --install fedora-packager --install clang >> $ sudo mock --root fedora-35-i386 --shell --enable-network >> # dnf builddep -y qemu >> # git clone https://github.com/qemu/qemu >> # cd qemu >> # CC=clang CXX=clang++ ./configure --disable-werror >> # make V=1 > > Ho hum. So, the warnings are where clang has decided to insert calls to libatomic. > > So we either have to > > (1) work around all of the places, which, unless we set up an i386 clang-12 builder will > quickly bitrot, or Update: (1) is out. There's a warning in cputlb.c vs a pointer that's known to be aligned, and it still fires. I have filed a bug: https://bugs.llvm.org/show_bug.cgi?id=51076 > > (2) write our own routines, compatible with libatomic, using cmpxchg8b directly. which > requires no (extra) locking, and so is compatible with the tcg jit output, or > > (3) file a bug with clang, and document "use clang-11 and not clang-12". So, Cole, with respect to (3), is this just general regression testing that discovered this (in which case, yay) or is there some other reason clang is required? Assuming that (3) isn't really viable long term, I guess (2) is the only viable option. Thoughts? r~
On 7/13/21 10:43 AM, Richard Henderson wrote: > On 7/12/21 5:37 PM, Richard Henderson wrote: >> On 7/12/21 2:30 PM, Cole Robinson wrote: >>> On 7/12/21 11:59 AM, Richard Henderson wrote: >>>> The first two patches are not strictly required, but they >>>> were useful in tracking down the root problem here. >>>> >>>> I understand the logic behind the clang-12 warning, but I think >>>> it's a clear mistake that it should be enabled by default for a >>>> target where alignment is not enforced by default. >>>> >>>> I found over a dozen places where we would have to manually add >>>> QEMU_ALIGNED(8) to uint64_t declarations in order to suppress >>>> all of the instances. IMO there's no point fighting this. >>>> >>> >>> I tested your patches, they seem to get rid of the warnings. The errors >>> persist. >>> >>> FWIW here's my reproduce starting from fedora 34 x86_64 host: >>> >>> $ sudo mock --root fedora-35-i386 --install dnf --install dnf-utils >>> --install fedora-packager --install clang >>> $ sudo mock --root fedora-35-i386 --shell --enable-network >>> # dnf builddep -y qemu >>> # git clone https://github.com/qemu/qemu >>> # cd qemu >>> # CC=clang CXX=clang++ ./configure --disable-werror >>> # make V=1 >> >> Ho hum. So, the warnings are where clang has decided to insert calls >> to libatomic. >> >> So we either have to >> >> (1) work around all of the places, which, unless we set up an i386 >> clang-12 builder will quickly bitrot, or > > Update: (1) is out. There's a warning in cputlb.c vs a pointer that's > known to be aligned, and it still fires. I have filed a bug: > > https://bugs.llvm.org/show_bug.cgi?id=51076 > >> >> (2) write our own routines, compatible with libatomic, using cmpxchg8b >> directly. which requires no (extra) locking, and so is compatible >> with the tcg jit output, or >> >> (3) file a bug with clang, and document "use clang-11 and not clang-12". > > So, Cole, with respect to (3), is this just general regression testing > that discovered this (in which case, yay) or is there some other reason > clang is required? > > Assuming that (3) isn't really viable long term, I guess (2) is the only > viable option. > I never tested building qemu with clang prior to this so no idea if it's a regression. There's some interest in using clang (eventually with cfi) to build the Fedora qemu package, so I gave it a test run. If this case is problematic we could keep using gcc for it and clang for every other arch, in the short/medium term. Richard can you clarify, do you think the errors are a clang bug as well, or strictly a qemu issue? If it's clang maybe I can get Red Hat llvm devs to help Thanks, Cole
On 7/13/21 8:18 AM, Cole Robinson wrote: >> https://bugs.llvm.org/show_bug.cgi?id=51076 ... > Richard can you clarify, do you think the errors are a clang bug as > well, or strictly a qemu issue? If it's clang maybe I can get Red Hat > llvm devs to help There are minor adjustments that can (and perhaps should be) be made to qemu, but the major portion seems to me to be a clang bug, reported above. Assistance with clang would be quite welcome. r~
On 7/13/21 12:56 PM, Richard Henderson wrote: > On 7/13/21 8:18 AM, Cole Robinson wrote: >>> https://bugs.llvm.org/show_bug.cgi?id=51076 > ... >> Richard can you clarify, do you think the errors are a clang bug as >> well, or strictly a qemu issue? If it's clang maybe I can get Red Hat >> llvm devs to help > > There are minor adjustments that can (and perhaps should be) be made to > qemu, but the major portion seems to me to be a clang bug, reported > above. Assistance with clang would be quite welcome. > I tried to summarize the discussion and filed a bug against fedora rawhide clang: https://bugzilla.redhat.com/show_bug.cgi?id=1982740 Thanks, Cole