[Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Paolo Bonzini]

The following changes since commit 4b6eda626fdb8bf90472c6868d502a2ac09abeeb:

  Merge remote-tracking branch 'remotes/lalrae/tags/mips-20151124' into staging (2015-11-24 17:05:06 +0000)

are available in the git repository at:


  git://github.com/bonzini/qemu.git tags/for-upstream

for you to fetch changes up to 6ac504d20a0967da06caadf595388f753908328a:

  target-i386: kvm: Print warning when clearing mcg_cap bits (2015-11-25 17:48:46 +0100)

----------------------------------------------------------------
Small patches, most notably the introduction of -fwrapv.

----------------------------------------------------------------
Daniel P. Berrange (2):
      Revert "exec: silence hugetlbfs warning under qtest"
      exec: remove warning about mempath and hugetlbfs

Eduardo Habkost (3):
      target-i386: kvm: Abort if MCE bank count is not supported by host
      target-i386: kvm: Use env->mcg_cap when setting up MCE
      target-i386: kvm: Print warning when clearing mcg_cap bits

Paolo Bonzini (3):
      MAINTAINERS: Update TCG CPU cores section
      QEMU does not care about left shifts of signed negative values
      target-sparc: fix 32-bit truncation in fpackfix

Wen Congyang (1):
      call bdrv_drain_all() even if the vm is stopped

 HACKING                   |  6 ++++++
 MAINTAINERS               | 17 +++++++++++++----
 configure                 |  4 ++--
 cpus.c                    |  2 ++
 exec.c                    |  6 ------
 target-i386/cpu.h         |  2 ++
 target-i386/kvm.c         | 22 ++++++++++++++--------
 target-sparc/vis_helper.c |  2 +-
 vl.c                      | 28 ++++++++++++++--------------
 9 files changed, 54 insertions(+), 35 deletions(-)
-- 
1.8.3.1

[Qemu-devel] [PULL 1/9] MAINTAINERS: Update TCG CPU cores section

[Paolo Bonzini]

These are the people that I think have been touching it lately
or reviewing patches.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 MAINTAINERS | 17 +++++++++++++----
 1 file changed, 13 insertions(+), 4 deletions(-)

diff --git a/MAINTAINERS b/MAINTAINERS
index 28f0139..bb1f3e4 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -62,14 +62,22 @@ Guest CPU cores (TCG):
 ----------------------
 Overall
 L: qemu-devel@nongnu.org
-S: Odd fixes
+M: Paolo Bonzini <pbonzini@redhat.com>
+M: Peter Crosthwaite <crosthwaite.peter@gmail.com>
+M: Richard Henderson <rth@twiddle.net>
+S: Maintained
 F: cpu-exec.c
+F: cpu-exec-common.c
+F: cpus.c
 F: cputlb.c
+F: exec.c
 F: softmmu_template.h
-F: translate-all.c
-F: include/exec/cpu_ldst.h
-F: include/exec/cpu_ldst_template.h
+F: translate-all.*
+F: translate-common.c
+F: include/exec/cpu*.h
+F: include/exec/exec-all.h
 F: include/exec/helper*.h
+F: include/exec/tb-hash.h
 
 Alpha
 M: Richard Henderson <rth@twiddle.net>
@@ -1042,6 +1050,7 @@ S: Supported
 F: include/exec/ioport.h
 F: ioport.c
 F: include/exec/memory.h
+F: include/exec/ram_addr.h
 F: memory.c
 F: include/exec/memory-internal.h
 F: exec.c
-- 
1.8.3.1

[Qemu-devel] [PULL 2/9] QEMU does not care about left shifts of signed negative values

[Paolo Bonzini]

It seems like there's no good reason for the compiler to exploit the
undefinedness of left shifts.  GCC explicitly documents that they do not
use at all this possibility and, while they also say this is subject
to change, they have been saying this for 10 years (since the wording
appeared in the GCC 4.0 manual).

Any workaround for this particular case of undefined behavior uglifies the
code.  Using unsigned is unsafe (https://github.com/madler/zlib/pull/112
is the proof) because the value becomes positive when extended; -(a << b)
works but does not express that the intention is to compute -a * 2^N,
especially if "a" is a constant.

<rant>
The straw that broke the camel back is Clang's latest obnoxious,
pointless, unsafe---and did I mention *totally* useless---warning about
left shifting a negative integer.  It's obnoxious and pointless because
the compiler is not using the latitude that the standard gives it, so
the warning just adds noise.  It is useless and unsafe because it does
not catch the widely more common case where the LHS is a variable, and
thus gives a false sense of security.  The noisy nature of the warning
means that it should have never been added to -Wall.  The uselessness
means that it probably should not have even been added to -Wextra.

(It would have made sense to enable the warning for -fsanitize=shift,
as the program would always crash if the point is reached.  But this was
probably too sophisticated to come up with, when you're so busy giving
birth to gems such as -Wabsolute-value).
</rant>

Ubsan also has warnings for undefined behavior of left shifts.  Checks for
left shift overflow and left shift of negative numbers, unfortunately,
cannot be silenced without also silencing the useful ones about out-of-range
shift amounts. -fwrapv ought to shut them up, but doesn't yet
(https://llvm.org/bugs/show_bug.cgi?id=25552; I am taking care of fixing
the same issues in GCC).  Luckily ubsan is optional, and the easy
workaround is to use -fsanitize-recover.

Anyhow, this patch documents our assumptions explicitly, and shuts up the
stupid warning.  -fwrapv is a bit of a heavy hammer, but it is the safest
option and it ought to just work long term as the compilers improve.
Note that -fstrict-overflow does not silence ubsan's overflow warnings,
hence it's reasonable to assume that it won't silence the left shift
warnings either.  QEMU doesn't rely on pointer overflow anyway, and
that's the other major difference between -fwrapv (which only cares
about integer overflow) and -fstrict-overflow.

Thanks to everyone involved in the discussion!

Cc: Peter Maydell <peter.maydell@linaro.org>
Reviewed-by: Markus Armbruster <armbru@redhat.com>
Grudgingly-reviewed-by: Laszlo Ersek <lersek@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 HACKING   | 6 ++++++
 configure | 4 ++--
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/HACKING b/HACKING
index 12fbc8a..71ad23b 100644
--- a/HACKING
+++ b/HACKING
@@ -157,3 +157,9 @@ painful. These are:
  * you may assume that integers are 2s complement representation
  * you may assume that right shift of a signed integer duplicates
    the sign bit (ie it is an arithmetic shift, not a logical shift)
+
+In addition, QEMU assumes that the compiler does not use the latitude
+given in C99 and C11 to treat aspects of signed '<<' as undefined, as
+documented in the GNU Compiler Collection manual starting at version 4.0.
+If a compiler does not respect this when passed the -fwrapv option,
+it is not supported for compilation of QEMU.
diff --git a/configure b/configure
index 71d6cbc..5bb8187 100755
--- a/configure
+++ b/configure
@@ -413,7 +413,7 @@ sdl2_config="${SDL2_CONFIG-${cross_prefix}sdl2-config}"
 ARFLAGS="${ARFLAGS-rv}"
 
 # default flags for all hosts
-QEMU_CFLAGS="-fno-strict-aliasing -fno-common $QEMU_CFLAGS"
+QEMU_CFLAGS="-fno-strict-aliasing -fno-common -fwrapv $QEMU_CFLAGS"
 QEMU_CFLAGS="-Wall -Wundef -Wwrite-strings -Wmissing-prototypes $QEMU_CFLAGS"
 QEMU_CFLAGS="-Wstrict-prototypes -Wredundant-decls $QEMU_CFLAGS"
 QEMU_CFLAGS="-D_GNU_SOURCE -D_FILE_OFFSET_BITS=64 -D_LARGEFILE_SOURCE $QEMU_CFLAGS"
@@ -1461,7 +1461,7 @@ fi
 gcc_flags="-Wold-style-declaration -Wold-style-definition -Wtype-limits"
 gcc_flags="-Wformat-security -Wformat-y2k -Winit-self -Wignored-qualifiers $gcc_flags"
 gcc_flags="-Wmissing-include-dirs -Wempty-body -Wnested-externs $gcc_flags"
-gcc_flags="-Wendif-labels $gcc_flags"
+gcc_flags="-Wendif-labels -Wno-shift-negative-value $gcc_flags"
 gcc_flags="-Wno-initializer-overrides $gcc_flags"
 gcc_flags="-Wno-string-plus-int $gcc_flags"
 # Note that we do not add -Werror to gcc_flags here, because that would
-- 
1.8.3.1

[Qemu-devel] [PULL 3/9] call bdrv_drain_all() even if the vm is stopped

[Paolo Bonzini]

From: Wen Congyang <wency@cn.fujitsu.com>

There are still I/O operations when the vm is stopped. For example,
stop the vm, and do block migration. In this case, we don't drain all
I/O operation, and may meet the following problem:

qemu-system-x86_64: migration/block.c:731: block_save_complete: Assertion `block_mig_state.submitted == 0' failed.

Signed-off-by: Wen Congyang <wency@cn.fujitsu.com>
Message-Id: <564EE92E.4070701@cn.fujitsu.com>
Cc: qemu-stable@nongnu.org
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 cpus.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/cpus.c b/cpus.c
index 877bd70..43676fa 100644
--- a/cpus.c
+++ b/cpus.c
@@ -1415,6 +1415,8 @@ int vm_stop_force_state(RunState state)
         return vm_stop(state);
     } else {
         runstate_set(state);
+
+        bdrv_drain_all();
         /* Make sure to return an error if the flush in a previous vm_stop()
          * failed. */
         return bdrv_flush_all();
-- 
1.8.3.1

[Qemu-devel] [PULL 4/9] Revert "exec: silence hugetlbfs warning under qtest"

[Paolo Bonzini]

From: "Daniel P. Berrange" <berrange@redhat.com>

This reverts commit 1c7ba94a184df1eddd589d5400d879568d3e5d08.

That commit changed QEMU initialization order from

 - object-initial, chardev, qtest, object-late

to

 - chardev, qtest, object-initial, object-late

This breaks chardev setups which need to rely on objects
having been created. For example, when chardevs use TLS
encryption in the future, they need to have tls credential
objects created first.

This revert, restores the ordering introduced in

  commit f08f9271bfe3f19a5eb3d7a2f48532065304d5c8
  Author: Daniel P. Berrange <berrange@redhat.com>
  Date:   Wed May 13 17:14:04 2015 +0100

    vl: Create (most) objects before creating chardev backends

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1448448749-1332-2-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c |  5 +----
 vl.c   | 28 ++++++++++++++--------------
 2 files changed, 15 insertions(+), 18 deletions(-)

diff --git a/exec.c b/exec.c
index acbd4a2..b09f18b 100644
--- a/exec.c
+++ b/exec.c
@@ -51,7 +51,6 @@
 #include "qemu/main-loop.h"
 #include "translate-all.h"
 #include "sysemu/replay.h"
-#include "sysemu/qtest.h"
 
 #include "exec/memory-internal.h"
 #include "exec/ram_addr.h"
@@ -1197,10 +1196,8 @@ static long gethugepagesize(const char *path, Error **errp)
         return 0;
     }
 
-    if (!qtest_driver() &&
-        fs.f_type != HUGETLBFS_MAGIC) {
+    if (fs.f_type != HUGETLBFS_MAGIC)
         fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path);
-    }
 
     return fs.f_bsize;
 }
diff --git a/vl.c b/vl.c
index 525929b..4211ff1 100644
--- a/vl.c
+++ b/vl.c
@@ -4291,26 +4291,17 @@ int main(int argc, char **argv, char **envp)
     page_size_init();
     socket_init();
 
-    if (qemu_opts_foreach(qemu_find_opts("chardev"),
-                          chardev_init_func, NULL, NULL)) {
-        exit(1);
-    }
-
-    if (qtest_chrdev) {
-        Error *local_err = NULL;
-        qtest_init(qtest_chrdev, qtest_log, &local_err);
-        if (local_err) {
-            error_report_err(local_err);
-            exit(1);
-        }
-    }
-
     if (qemu_opts_foreach(qemu_find_opts("object"),
                           object_create,
                           object_create_initial, NULL)) {
         exit(1);
     }
 
+    if (qemu_opts_foreach(qemu_find_opts("chardev"),
+                          chardev_init_func, NULL, NULL)) {
+        exit(1);
+    }
+
 #ifdef CONFIG_VIRTFS
     if (qemu_opts_foreach(qemu_find_opts("fsdev"),
                           fsdev_init_func, NULL, NULL)) {
@@ -4337,6 +4328,15 @@ int main(int argc, char **argv, char **envp)
 
     configure_accelerator(current_machine);
 
+    if (qtest_chrdev) {
+        Error *local_err = NULL;
+        qtest_init(qtest_chrdev, qtest_log, &local_err);
+        if (local_err) {
+            error_report_err(local_err);
+            exit(1);
+        }
+    }
+
     machine_opts = qemu_get_machine_opts();
     kernel_filename = qemu_opt_get(machine_opts, "kernel");
     initrd_filename = qemu_opt_get(machine_opts, "initrd");
-- 
1.8.3.1

[Qemu-devel] [PULL 5/9] exec: remove warning about mempath and hugetlbfs

[Paolo Bonzini]

From: "Daniel P. Berrange" <berrange@redhat.com>

The gethugepagesize() method in exec.c printed a warning if
the file path for "-mem-path" or "-object memory-backend-file"
was not on a hugetlbfs filesystem. This warning is bogus, because
QEMU functions perfectly well with the path on a regular tmpfs
filesystem. Use of hugetlbfs vs tmpfs is a choice for the management
application or end user to make as best fits their needs. As such it
is inappropriate for QEMU to have an opinion on whether the user's
choice is right or wrong in this case.

Signed-off-by: Daniel P. Berrange <berrange@redhat.com>
Message-Id: <1448448749-1332-3-git-send-email-berrange@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 exec.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/exec.c b/exec.c
index b09f18b..de1cf19 100644
--- a/exec.c
+++ b/exec.c
@@ -1196,9 +1196,6 @@ static long gethugepagesize(const char *path, Error **errp)
         return 0;
     }
 
-    if (fs.f_type != HUGETLBFS_MAGIC)
-        fprintf(stderr, "Warning: path not on HugeTLBFS: %s\n", path);
-
     return fs.f_bsize;
 }
 
-- 
1.8.3.1

[Qemu-devel] [PULL 6/9] target-sparc: fix 32-bit truncation in fpackfix

[Paolo Bonzini]

This is reported by Coverity.  The algorithm description at
ftp://ftp.icm.edu.pl/packages/ggi/doc/hw/sparc/Sparc.pdf suggests
that the 32-bit parts of rs2, after the left shift, is treated
as a 64-bit integer.  Bits 32 and above are used to do the
saturating truncation.

Message-Id: <1446473134-4330-1-git-send-email-pbonzini@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-sparc/vis_helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-sparc/vis_helper.c b/target-sparc/vis_helper.c
index 383cc8b..45fc7db 100644
--- a/target-sparc/vis_helper.c
+++ b/target-sparc/vis_helper.c
@@ -447,7 +447,7 @@ uint32_t helper_fpackfix(uint64_t gsr, uint64_t rs2)
     for (word = 0; word < 2; word++) {
         uint32_t val;
         int32_t src = rs2 >> (word * 32);
-        int64_t scaled = src << scale;
+        int64_t scaled = (int64_t)src << scale;
         int64_t from_fixed = scaled >> 16;
 
         val = (from_fixed < -32768 ? -32768 :
-- 
1.8.3.1

[Qemu-devel] [PULL 7/9] target-i386: kvm: Abort if MCE bank count is not supported by host

[Paolo Bonzini]

From: Eduardo Habkost <ehabkost@redhat.com>

Instead of silently changing the number of banks in mcg_cap based
on kvm_get_mce_cap_supported(), abort initialization if the host
doesn't support MCE_BANKS_DEF banks.

Note that MCE_BANKS_DEF was always 10 since it was introduced in
QEMU, and Linux always returned 32 at KVM_CAP_MCE since
KVM_CAP_MCE was introduced, so no behavior is being changed and
the error can't be triggered by any Linux version. The point of
the new check is to ensure we won't silently change the bank
count if we change MCE_BANKS_DEF or make the bank count
configurable in the future.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
[Avoid Yoda condition and \n at end of error_report. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/kvm.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 2a9953b..93d1f5e 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -784,11 +784,14 @@ int kvm_arch_init_vcpu(CPUState *cs)
             return ret;
         }
 
-        if (banks > MCE_BANKS_DEF) {
-            banks = MCE_BANKS_DEF;
+        if (banks < MCE_BANKS_DEF) {
+            error_report("kvm: Unsupported MCE bank count (QEMU = %d, KVM = %d)",
+                         MCE_BANKS_DEF, banks);
+            return -ENOTSUP;
         }
+
         mcg_cap &= MCE_CAP_DEF;
-        mcg_cap |= banks;
+        mcg_cap |= MCE_BANKS_DEF;
         ret = kvm_vcpu_ioctl(cs, KVM_X86_SETUP_MCE, &mcg_cap);
         if (ret < 0) {
             fprintf(stderr, "KVM_X86_SETUP_MCE: %s", strerror(-ret));
-- 
1.8.3.1

[Qemu-devel] [PULL 8/9] target-i386: kvm: Use env->mcg_cap when setting up MCE

[Paolo Bonzini]

From: Eduardo Habkost <ehabkost@redhat.com>

When setting up MCE, instead of using the MCE_*_DEF macros
directly, just filter the existing env->mcg_cap value.

As env->mcg_cap is already initialized as
MCE_CAP_DEF|MCE_BANKS_DEF at target-i386/cpu.c:mce_init(), this
doesn't change any behavior. But it will allow us to change
mce_init() in the future, to implement different defaults
depending on CPU model, machine-type or command-line parameters.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/cpu.h |  2 ++
 target-i386/kvm.c | 11 ++++-------
 2 files changed, 6 insertions(+), 7 deletions(-)

diff --git a/target-i386/cpu.h b/target-i386/cpu.h
index fc4a605..84edfd0 100644
--- a/target-i386/cpu.h
+++ b/target-i386/cpu.h
@@ -286,6 +286,8 @@
 #define MCE_CAP_DEF     (MCG_CTL_P|MCG_SER_P)
 #define MCE_BANKS_DEF   10
 
+#define MCG_CAP_BANKS_MASK 0xff
+
 #define MCG_STATUS_RIPV (1ULL<<0)   /* restart ip valid */
 #define MCG_STATUS_EIPV (1ULL<<1)   /* ip points to correct instruction */
 #define MCG_STATUS_MCIP (1ULL<<2)   /* machine check in progress */
diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 93d1f5e..90bd447 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -784,21 +784,18 @@ int kvm_arch_init_vcpu(CPUState *cs)
             return ret;
         }
 
-        if (banks < MCE_BANKS_DEF) {
+        if (banks < (env->mcg_cap & MCG_CAP_BANKS_MASK)) {
             error_report("kvm: Unsupported MCE bank count (QEMU = %d, KVM = %d)",
-                         MCE_BANKS_DEF, banks);
+                         (int)(env->mcg_cap & MCG_CAP_BANKS_MASK), banks);
             return -ENOTSUP;
         }
 
-        mcg_cap &= MCE_CAP_DEF;
-        mcg_cap |= MCE_BANKS_DEF;
-        ret = kvm_vcpu_ioctl(cs, KVM_X86_SETUP_MCE, &mcg_cap);
+        env->mcg_cap &= mcg_cap | MCG_CAP_BANKS_MASK;
+        ret = kvm_vcpu_ioctl(cs, KVM_X86_SETUP_MCE, &env->mcg_cap);
         if (ret < 0) {
             fprintf(stderr, "KVM_X86_SETUP_MCE: %s", strerror(-ret));
             return ret;
         }
-
-        env->mcg_cap = mcg_cap;
     }
 
     qemu_add_vm_change_state_handler(cpu_update_state, env);
-- 
1.8.3.1

[Qemu-devel] [PULL 9/9] target-i386: kvm: Print warning when clearing mcg_cap bits

[Paolo Bonzini]

From: Eduardo Habkost <ehabkost@redhat.com>

Instead of silently clearing mcg_cap bits when the host doesn't
support them, print a warning when doing that.

Signed-off-by: Eduardo Habkost <ehabkost@redhat.com>
[Avoid \n at end of error_report. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 target-i386/kvm.c | 8 +++++++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/target-i386/kvm.c b/target-i386/kvm.c
index 90bd447..6dc9846 100644
--- a/target-i386/kvm.c
+++ b/target-i386/kvm.c
@@ -774,7 +774,7 @@ int kvm_arch_init_vcpu(CPUState *cs)
         && (env->features[FEAT_1_EDX] & (CPUID_MCE | CPUID_MCA)) ==
            (CPUID_MCE | CPUID_MCA)
         && kvm_check_extension(cs->kvm_state, KVM_CAP_MCE) > 0) {
-        uint64_t mcg_cap;
+        uint64_t mcg_cap, unsupported_caps;
         int banks;
         int ret;
 
@@ -790,6 +790,12 @@ int kvm_arch_init_vcpu(CPUState *cs)
             return -ENOTSUP;
         }
 
+        unsupported_caps = env->mcg_cap & ~(mcg_cap | MCG_CAP_BANKS_MASK);
+        if (unsupported_caps) {
+            error_report("warning: Unsupported MCG_CAP bits: 0x%" PRIx64,
+                         unsupported_caps);
+        }
+
         env->mcg_cap &= mcg_cap | MCG_CAP_BANKS_MASK;
         ret = kvm_vcpu_ioctl(cs, KVM_X86_SETUP_MCE, &env->mcg_cap);
         if (ret < 0) {
-- 
1.8.3.1

Re: [Qemu-devel] [PULL 2/9] QEMU does not care about left shifts of signed negative values

[Peter Maydell]

On 25 November 2015 at 17:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
> Ubsan also has warnings for undefined behavior of left shifts.  Checks for
> left shift overflow and left shift of negative numbers, unfortunately,
> cannot be silenced without also silencing the useful ones about out-of-range
> shift amounts. -fwrapv ought to shut them up, but doesn't yet
> (https://llvm.org/bugs/show_bug.cgi?id=25552; I am taking care of fixing
> the same issues in GCC).  Luckily ubsan is optional, and the easy
> workaround is to use -fsanitize-recover.

We still haven't had any response from the LLVM/clang folks that
this interpretation of the meaning of -fwrapv is their interpretation
of it, have we? (I can't see any comments on the referenced bug.)

thanks
-- PMM

Re: [Qemu-devel] [PULL 2/9] QEMU does not care about left shifts of signed negative values

[Paolo Bonzini]

On 25/11/2015 18:44, Peter Maydell wrote:
> > Ubsan also has warnings for undefined behavior of left shifts.  Checks for
> > left shift overflow and left shift of negative numbers, unfortunately,
> > cannot be silenced without also silencing the useful ones about out-of-range
> > shift amounts. -fwrapv ought to shut them up, but doesn't yet
> > (https://llvm.org/bugs/show_bug.cgi?id=25552; I am taking care of fixing
> > the same issues in GCC).  Luckily ubsan is optional, and the easy
> > workaround is to use -fsanitize-recover.
>
> We still haven't had any response from the LLVM/clang folks that
> this interpretation of the meaning of -fwrapv is their interpretation
> of it, have we? (I can't see any comments on the referenced bug.)

Reasonably, they will have to follow what GCC does, independent of
-fwrapv.  GCC has now promised to not exploit << undefined behavior,
even without -fwrapv.

So at this point, -fwrapv is only required to placate ubsan---which it
will do for GCC as soon as my other patch is approved (I talked on IRC
with one of the GCC-ubsan authors and he said he was okay).  clang with
ubsan remains broken, but that's no worse than before.

Paolo

Re: [Qemu-devel] [PULL 2/9] QEMU does not care about left shifts of signed negative values

[Peter Maydell]

On 25 November 2015 at 17:50, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 25/11/2015 18:44, Peter Maydell wrote:
>> We still haven't had any response from the LLVM/clang folks that
>> this interpretation of the meaning of -fwrapv is their interpretation
>> of it, have we? (I can't see any comments on the referenced bug.)
>
> Reasonably, they will have to follow what GCC does, independent of
> -fwrapv.  GCC has now promised to not exploit << undefined behavior,
> even without -fwrapv.

I don't think that follows. If -fwrapv is still documented by gcc
as only affecting arithmetic and not shifts, I don't see any
reason why the llvm people will expect it do to anything else.
And LLVM is its own project and its developers don't always exactly
follow gcc behaviour.

Your latest patch at https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03055.html
doesn't seem to touch the documentation of -fwrapv at all, so I
don't think it is sufficient to allow users of the compiler
to say "-fwrapv means signed behaviour for shifts".

> So at this point, -fwrapv is only required to placate ubsan---which it
> will do for GCC as soon as my other patch is approved (I talked on IRC
> with one of the GCC-ubsan authors and he said he was okay).  clang with
> ubsan remains broken, but that's no worse than before.

I would rather see GCC's documentation explicitly state that
-fwrapv means a dialect where [among other things] shifts of
signed integers have 2s-complement behaviour, and ditto
clang, before we accept this patch. (Or at least have those
documentation fixes in the works.)

I don't mind if there are still unsuppressed warnings with
older compilers. What I want is a clear statement in the
docs for both compilers that -fwrapv gives us the semantics
we're trying to rely on.

thanks
-- PMM

Re: [Qemu-devel] [PULL 2/9] QEMU does not care about left shifts of signed negative values

[Paolo Bonzini]

On 25/11/2015 20:18, Peter Maydell wrote:
> And LLVM is its own project and its developers don't always exactly
> follow gcc behaviour.

True.  The patch docuuments that if LLVM will not respect 2s complement
for signed shifts when passed the -fwrapv option, it will not be
supported for compilation of QEMU.  But let;s cross that bridge when we
reach it.  So far, -Wshift-negative-value seems to be a misguided
attempt to copy GCC's warning without understanding it.

> Your latest patch at https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03055.html
> doesn't seem to touch the documentation of -fwrapv at all, so I
> don't think it is sufficient to allow users of the compiler
> to say "-fwrapv means signed behaviour for shifts".

GCC *always* does signed behavior for shifts, even without -fwrapv.
I'll commit tomorrow the patch that promises that for the future.

GCC does not need -fwrapv at all.

Paolo

Re: [Qemu-devel] [PULL 2/9] QEMU does not care about left shifts of signed negative values

[Peter Maydell]

On 25 November 2015 at 19:30, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 25/11/2015 20:18, Peter Maydell wrote:
>> And LLVM is its own project and its developers don't always exactly
>> follow gcc behaviour.
>
> True.  The patch documents that if LLVM will not respect 2s complement
> for signed shifts when passed the -fwrapv option, it will not be
> supported for compilation of QEMU.  But let;s cross that bridge when we
> reach it.  So far, -Wshift-negative-value seems to be a misguided
> attempt to copy GCC's warning without understanding it.

If we don't have documented support for a "signed negative shifts
are valid and have these semantics" option from both our compilers,
then we definitely should not be committing a patch which silences
warnings about them. Instead we need to fix all the places in our
code which rely on those semantics.

>> Your latest patch at https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03055.html
>> doesn't seem to touch the documentation of -fwrapv at all, so I
>> don't think it is sufficient to allow users of the compiler
>> to say "-fwrapv means signed behaviour for shifts".
>
> GCC *always* does signed behavior for shifts, even without -fwrapv.
> I'll commit tomorrow the patch that promises that for the future.
>
> GCC does not need -fwrapv at all.

Yes it does, because without -fwrapv it still wants to warn
about them. We need to tell the compiler that we really do
want a C dialect where they have specific behaviour and so no
warnings are ever correct. By default (as documented, even
with your patch) GCC just promises that it won't actually
do undefined behaviour for signed negative shifts. (It doesn't
even actually say what impdef semantics it does provide,
and in practice the impdef semantics include "warn about
this", which we don't want.)

thanks
-- PMM

Re: [Qemu-devel] [PULL 2/9] QEMU does not care about left shifts of signed negative values

[Paolo Bonzini]

On 25/11/2015 20:54, Peter Maydell wrote:
> > > Your latest patch at https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03055.html
> > > doesn't seem to touch the documentation of -fwrapv at all, so I
> > > don't think it is sufficient to allow users of the compiler
> > > to say "-fwrapv means signed behaviour for shifts".
> >
> > GCC *always* does signed behavior for shifts, even without -fwrapv.
> > I'll commit tomorrow the patch that promises that for the future.
> >
> > GCC does not need -fwrapv at all.
>
> Yes it does, because without -fwrapv it still wants to warn
> about them. We need to tell the compiler that we really do
> want a C dialect where they have specific behaviour and so no
> warnings are ever correct. By default (as documented, even
> with your patch) GCC just promises that it won't actually
> do undefined behaviour for signed negative shifts. (It doesn't
> even actually say what impdef semantics it does provide,

It says it above the text I changed:

     GCC supports only two's complement integer types, and all bit
     patterns are ordinary values.
     [...]
     Bitwise operators act on the representation of the value including
     both the sign and value bits, where the sign bit is considered
     immediately above the highest-value value bit.

> and in practice the impdef semantics include "warn about
> this", which we don't want.)

No, it doesn't warn with the commonly used options.  Only with 
-pedantic, which is documented as

    Issue all the warnings demanded by strict ISO C and ISO C++;
    reject all programs that use forbidden extensions, and some
    other programs that do not follow ISO C and ISO C++.

So the combination of -fwrapv and -pedantic is not particularly 
interesting.  Even then the warning is "initializer element is not
a constant expression"; nothing to do with overflow.  For example:

    #define INT_MIN ((int)-0x80000000)
    int y = INT_MIN - 1;
    int z = -1 << 2;
    int w = INT_MIN << 1;
    int u = 1 << 31;

$ gcc f.c -std=c11 -Wall -pedantic
f.c:2:1: warning: overflow in constant expression [-Woverflow]
f.c:3:9: warning: initializer element is not a constant expression [-Wpedantic]
f.c:4:9: warning: initializer element is not a constant expression [-Wpedantic]
f.c:5:9: warning: initializer element is not a constant expression [-Wpedantic]

The first warning is activated by -Wall, the others aren't.

Paolo

Re: [Qemu-devel] [PULL 2/9] QEMU does not care about left shifts of signed negative values

[Peter Maydell]

On 25 November 2015 at 21:05, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 25/11/2015 20:54, Peter Maydell wrote:
>> > > Your latest patch at https://gcc.gnu.org/ml/gcc-patches/2015-11/msg03055.html
>> > > doesn't seem to touch the documentation of -fwrapv at all, so I
>> > > don't think it is sufficient to allow users of the compiler
>> > > to say "-fwrapv means signed behaviour for shifts".
>> >
>> > GCC *always* does signed behavior for shifts, even without -fwrapv.
>> > I'll commit tomorrow the patch that promises that for the future.
>> >
>> > GCC does not need -fwrapv at all.
>>
>> Yes it does, because without -fwrapv it still wants to warn
>> about them. We need to tell the compiler that we really do
>> want a C dialect where they have specific behaviour and so no
>> warnings are ever correct. By default (as documented, even
>> with your patch) GCC just promises that it won't actually
>> do undefined behaviour for signed negative shifts. (It doesn't
>> even actually say what impdef semantics it does provide,
>
> It says it above the text I changed:
>
>      GCC supports only two's complement integer types, and all bit
>      patterns are ordinary values.
>      [...]
>      Bitwise operators act on the representation of the value including
>      both the sign and value bits, where the sign bit is considered
>      immediately above the highest-value value bit.

Oops, yes. I misread the doc there.

>> and in practice the impdef semantics include "warn about
>> this", which we don't want.)
>
> No, it doesn't warn with the commonly used options.

"They are also diagnosed where constant expressions are required."
strongly implies "we feel free to warn about this usage". Constant
expressions are required all over the place...

>  Only with -pedantic, which is documented as
>
>     Issue all the warnings demanded by strict ISO C and ISO C++;
>     reject all programs that use forbidden extensions, and some
>     other programs that do not follow ISO C and ISO C++.
>
> So the combination of -fwrapv and -pedantic is not particularly
> interesting.  Even then the warning is "initializer element is not
> a constant expression"; nothing to do with overflow.  For example:
>
>     #define INT_MIN ((int)-0x80000000)
>     int y = INT_MIN - 1;
>     int z = -1 << 2;
>     int w = INT_MIN << 1;
>     int u = 1 << 31;
>
> $ gcc f.c -std=c11 -Wall -pedantic
> f.c:2:1: warning: overflow in constant expression [-Woverflow]
> f.c:3:9: warning: initializer element is not a constant expression [-Wpedantic]
> f.c:4:9: warning: initializer element is not a constant expression [-Wpedantic]
> f.c:5:9: warning: initializer element is not a constant expression [-Wpedantic]
>
> The first warning is activated by -Wall, the others aren't.

Unless the C standard specifically requires the warning text
as written, I'd say these are pretty terrible warnings, since
they don't actually diagnose the problem with the code. But
we don't use -pedantic so I don't care about them specifically.
I just care that the documentation should say clearly
"we guarantee these semantics; we won't warn unless you
specifically ask us to warn about their use".

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Peter Maydell]

On 25 November 2015 at 17:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
> The following changes since commit 4b6eda626fdb8bf90472c6868d502a2ac09abeeb:
>
>   Merge remote-tracking branch 'remotes/lalrae/tags/mips-20151124' into staging (2015-11-24 17:05:06 +0000)
>
> are available in the git repository at:
>
>
>   git://github.com/bonzini/qemu.git tags/for-upstream
>
> for you to fetch changes up to 6ac504d20a0967da06caadf595388f753908328a:
>
>   target-i386: kvm: Print warning when clearing mcg_cap bits (2015-11-25 17:48:46 +0100)
>
> ----------------------------------------------------------------
> Small patches, most notably the introduction of -fwrapv.
>
> ----------------------------------------------------------------
> Daniel P. Berrange (2):
>       Revert "exec: silence hugetlbfs warning under qtest"
>       exec: remove warning about mempath and hugetlbfs
>
> Eduardo Habkost (3):
>       target-i386: kvm: Abort if MCE bank count is not supported by host
>       target-i386: kvm: Use env->mcg_cap when setting up MCE
>       target-i386: kvm: Print warning when clearing mcg_cap bits
>
> Paolo Bonzini (3):
>       MAINTAINERS: Update TCG CPU cores section
>       QEMU does not care about left shifts of signed negative values
>       target-sparc: fix 32-bit truncation in fpackfix
>
> Wen Congyang (1):
>       call bdrv_drain_all() even if the vm is stopped

I definitely don't think we should apply the -fwrapv patch yet;
would you mind respinning this pullrequest without it?

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Paolo Bonzini]

On 26/11/2015 10:46, Peter Maydell wrote:
> On 25 November 2015 at 17:19, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> The following changes since commit 4b6eda626fdb8bf90472c6868d502a2ac09abeeb:
>>
>>   Merge remote-tracking branch 'remotes/lalrae/tags/mips-20151124' into staging (2015-11-24 17:05:06 +0000)
>>
>> are available in the git repository at:
>>
>>
>>   git://github.com/bonzini/qemu.git tags/for-upstream
>>
>> for you to fetch changes up to 6ac504d20a0967da06caadf595388f753908328a:
>>
>>   target-i386: kvm: Print warning when clearing mcg_cap bits (2015-11-25 17:48:46 +0100)
>>
>> ----------------------------------------------------------------
>> Small patches, most notably the introduction of -fwrapv.
>>
>> ----------------------------------------------------------------
>> Daniel P. Berrange (2):
>>       Revert "exec: silence hugetlbfs warning under qtest"
>>       exec: remove warning about mempath and hugetlbfs
>>
>> Eduardo Habkost (3):
>>       target-i386: kvm: Abort if MCE bank count is not supported by host
>>       target-i386: kvm: Use env->mcg_cap when setting up MCE
>>       target-i386: kvm: Print warning when clearing mcg_cap bits
>>
>> Paolo Bonzini (3):
>>       MAINTAINERS: Update TCG CPU cores section
>>       QEMU does not care about left shifts of signed negative values
>>       target-sparc: fix 32-bit truncation in fpackfix
>>
>> Wen Congyang (1):
>>       call bdrv_drain_all() even if the vm is stopped
> 
> I definitely don't think we should apply the -fwrapv patch yet;
> would you mind respinning this pullrequest without it?

In what way does that patch make that thing worse?

Paolo

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Peter Maydell]

On 26 November 2015 at 10:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 26/11/2015 10:46, Peter Maydell wrote:
>> I definitely don't think we should apply the -fwrapv patch yet;
>> would you mind respinning this pullrequest without it?
>
> In what way does that patch make that thing worse?

It makes a claim about the semantics that the compiler
guarantees us which isn't currently valid. (Or
alternatively, it's implicitly claiming that clang isn't
a supported compiler, which isn't true.) I don't think
we should document or rely on signed-shift semantics
until we have the relevant documented promises from the
compiler developers that that is what they are providing.
(I'm happy that the gcc folks have provided those promises, they
just need to actually document them in the -fwrapv option
docs. The clang folks haven't replied yet so we don't know.)

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Paolo Bonzini]

On 26/11/2015 11:56, Peter Maydell wrote:
> On 26 November 2015 at 10:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>>
>> On 26/11/2015 10:46, Peter Maydell wrote:
>>> I definitely don't think we should apply the -fwrapv patch yet;
>>> would you mind respinning this pullrequest without it?
>>
>> In what way does that patch make that thing worse?
> 
> It makes a claim about the semantics that the compiler
> guarantees us which isn't currently valid. (Or
> alternatively, it's implicitly claiming that clang isn't
> a supported compiler, which isn't true.) I don't think
> we should document or rely on signed-shift semantics

But we are relying on them, and thus we should document them.  Witness
the number of patches fixing so called "undefined" behavior.  And those
patches are _dangerous_.

I can certainly remove the "as documented by the GCC manual" part and
the -fwrapv setting, but silencing -Wshift-negative-value and
documenting what we rely on should go in.

Paolo

> until we have the relevant documented promises from the
> compiler developers that that is what they are providing.
> (I'm happy that the gcc folks have provided those promises, they
> just need to actually document them in the -fwrapv option
> docs. The clang folks haven't replied yet so we don't know.)

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Peter Maydell]

On 26 November 2015 at 11:23, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 26/11/2015 11:56, Peter Maydell wrote:
>> It makes a claim about the semantics that the compiler
>> guarantees us which isn't currently valid. (Or
>> alternatively, it's implicitly claiming that clang isn't
>> a supported compiler, which isn't true.) I don't think
>> we should document or rely on signed-shift semantics
>
> But we are relying on them, and thus we should document them.  Witness
> the number of patches fixing so called "undefined" behavior.  And those
> patches are _dangerous_.

Until and unless the compiler guarantees us the semantics that
we want, then silently hoping we get something we're not getting
is even more dangerous, and avoiding the UB is better than
just crossing our fingers and hoping.

> I can certainly remove the "as documented by the GCC manual" part and
> the -fwrapv setting, but silencing -Wshift-negative-value and
> documenting what we rely on should go in.

I don't see much point in documenting what we rely on
if we can't rely on it and need to stop relying on it.

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 26 November 2015 at 11:23, Paolo Bonzini <pbonzini@redhat.com> wrote:
>> On 26/11/2015 11:56, Peter Maydell wrote:
>>> It makes a claim about the semantics that the compiler
>>> guarantees us which isn't currently valid. (Or
>>> alternatively, it's implicitly claiming that clang isn't
>>> a supported compiler, which isn't true.) I don't think
>>> we should document or rely on signed-shift semantics
>>
>> But we are relying on them, and thus we should document them.  Witness
>> the number of patches fixing so called "undefined" behavior.  And those
>> patches are _dangerous_.

I'm pretty sure us screwing some of them up is a much larger risk than
gcc suddenly starting to screw up our signed shifts without anybody
noticing.

If gcc ever started to mess up signed shifts, I'd fully expect the
kernel and many applications to go down in flames.  Hopefully quickly
enough to avoid real damage.  I don't think gcc has become *that*
reckless.  Clang neither.

> Until and unless the compiler guarantees us the semantics that
> we want, then silently hoping we get something we're not getting
> is even more dangerous, and avoiding the UB is better than
> just crossing our fingers and hoping.

The cost and risk of proactively fixing a hypothetical^Wlatent problem
needs to be weighed against the probability of it ever becoming real.

>> I can certainly remove the "as documented by the GCC manual" part and
>> the -fwrapv setting, but silencing -Wshift-negative-value and
>> documenting what we rely on should go in.
>
> I don't see much point in documenting what we rely on
> if we can't rely on it and need to stop relying on it.

"Can't" and "need" are too strong.  The kernel can, and I fail to see
what makes us so special that we absolutely cannot.

For what it's worth, I'm sick and tired of patches "fixing" signed
shifts, and the unnecessary risk that comes with them.

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Peter Maydell]

On 26 November 2015 at 12:15, Markus Armbruster <armbru@redhat.com> wrote:
> Peter Maydell <peter.maydell@linaro.org> writes:
>> I don't see much point in documenting what we rely on
>> if we can't rely on it and need to stop relying on it.
>
> "Can't" and "need" are too strong.  The kernel can, and I fail to see
> what makes us so special that we absolutely cannot.

The kernel has the luxury of being able to say "we only compile
with gcc".

> For what it's worth, I'm sick and tired of patches "fixing" signed
> shifts, and the unnecessary risk that comes with them.

Me too. I just want us to fix this by getting the compiler authors
to document that we can rely on this stuff, not just by silencing
warnings in QEMU's makefiles.

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Paolo Bonzini]

On 26/11/2015 12:28, Peter Maydell wrote:
>> But we are relying on them, and thus we should document them.  Witness
>> the number of patches fixing so called "undefined" behavior.  And those
>> patches are _dangerous_.
> 
> Until and unless the compiler guarantees us the semantics that
> we want, then silently hoping we get something we're not getting
> is even more dangerous, and avoiding the UB is better than
> just crossing our fingers and hoping.
> 
>> I can certainly remove the "as documented by the GCC manual" part and
>> the -fwrapv setting, but silencing -Wshift-negative-value and
>> documenting what we rely on should go in.
> 
> I don't see much point in documenting what we rely on
> if we can't rely on it and need to stop relying on it.

I'm having a hard, hard time believing that we can't rely on it.  And if
we can rely on it, we don't need to stop relying on it.

To sum up:

- GCC promises that signed shift of << is implementation-defined except
for overflow in constant expressions, and defines it as operating on bit
values.  This was approved.  For GCC, -fwrapv does not even apply except
again for constant expressions.

- signed shift of negative values in constant expressions _are_ covered
by GCC's promise.  The implementation-defined behavior of signed <<
gives a meaning to all signed shifts, and all that the C standard
requires is "Each constant expression shall evaluate to a constant that
is in the range of representable values for its type" (6.6p4).
Therefore we should at least disable -Wshift-negative-value, because it
doesn't buy us anything.

- regarding overflow, in addition to the weird -Wpedantic warning, GCC 6
adds a new -Wshift-overflow flag which is enabled by default in C99 and
C11 modes, and which only applies to constant expressions.  So the
remaining case where the compiler may change its behavior on overflow,
i.e. constant expressions, will thus be caught by our usage of -Werror
(because -Wshift-overflow is enabled by default).  So, independent of
the limited scope of GCC's promise, with GCC 6 we will never have
constant expressions that overflow because of left shifts.

- if a compiler actually treated signed << overflow undefined behavior,
a possible fix would be to use C89 mode (-std=gnu89), since signed << is
unspecified there rather than undefined.  With C89, GCC's promise is
complete.  We do use C89 with GCC <= 4.9 anyway, it makes sense to be
homogeneous across all supported compilers.

Now, -fwrapv was really included in my patch only to silence ubsan in
the future.  I think it should, and I will work on patches to fix that.
 However, an advantage of -std=gnu89 is that it silences ubsan _now_ at
least for GCC.  So let's just drop -fwrapv and use -std=gnu89 instead.
This buys us time, and in the meanwhile I'll gett -fwrapv complete in GCC.

If this is okay, I'll remove the patch, respin the pull request, and
post a new configure change to use -std=gnu89.

Yes, we haven't heard anything from clang developers.  But clang does
not even document implementation-defined behavior
(https://llvm.org/bugs/show_bug.cgi?id=11272).  The bug says:

> The clang documentation should specify how clang behaves in cases
> specified to be implementation-defined in the relevant standards.
> Perhaps simply saying that our behavior is the same as GCC's would suffice?

This is about implementation-defined rather than undefined behavior, but
I think it's enough to not care about clang developer's silence.

Paolo

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Paolo Bonzini]

On 26/11/2015 13:19, Peter Maydell wrote:
> On 26 November 2015 at 12:15, Markus Armbruster <armbru@redhat.com> wrote:
>> Peter Maydell <peter.maydell@linaro.org> writes:
>>> I don't see much point in documenting what we rely on
>>> if we can't rely on it and need to stop relying on it.
>>
>> "Can't" and "need" are too strong.  The kernel can, and I fail to see
>> what makes us so special that we absolutely cannot.
> 
> The kernel has the luxury of being able to say "we only compile
> with gcc".

Actually no, there are people interested in compiling it with clang
(mostly because of GPL FUD and LLVM koolaid, but that's secondary in
this context).

>> For what it's worth, I'm sick and tired of patches "fixing" signed
>> shifts, and the unnecessary risk that comes with them.
> 
> Me too.

Great, that's a start.  And I'm totally not sarcastic about this.

Paolo

> I just want us to fix this by getting the compiler authors
> to document that we can rely on this stuff, not just by silencing
> warnings in QEMU's makefiles.

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Peter Maydell]

On 26 November 2015 at 13:04, Paolo Bonzini <pbonzini@redhat.com> wrote:
>
>
> On 26/11/2015 12:28, Peter Maydell wrote:
>>> But we are relying on them, and thus we should document them.  Witness
>>> the number of patches fixing so called "undefined" behavior.  And those
>>> patches are _dangerous_.
>>
>> Until and unless the compiler guarantees us the semantics that
>> we want, then silently hoping we get something we're not getting
>> is even more dangerous, and avoiding the UB is better than
>> just crossing our fingers and hoping.
>>
>>> I can certainly remove the "as documented by the GCC manual" part and
>>> the -fwrapv setting, but silencing -Wshift-negative-value and
>>> documenting what we rely on should go in.
>>
>> I don't see much point in documenting what we rely on
>> if we can't rely on it and need to stop relying on it.
>
> I'm having a hard, hard time believing that we can't rely on it.  And if
> we can rely on it, we don't need to stop relying on it.

Really my concern here is simply an ordering one: we should
(1) confirm with the clang and gcc developers that what we think
-fwrapv means is what they agree (and will document) as what it means
(2) update our makefiles/docs/etc appropriately

and also I think that (1) is the significant part of the exercise,
whereas (2) is just a cleanup/tidyup that we can do afterwards.
This patch is doing (2) before we've done (1).

> To sum up:
>
> - GCC promises that signed shift of << is implementation-defined except
> for overflow in constant expressions, and defines it as operating on bit
> values.  This was approved.  For GCC, -fwrapv does not even apply except
> again for constant expressions.
>
> - signed shift of negative values in constant expressions _are_ covered
> by GCC's promise.  The implementation-defined behavior of signed <<
> gives a meaning to all signed shifts, and all that the C standard
> requires is "Each constant expression shall evaluate to a constant that
> is in the range of representable values for its type" (6.6p4).
> Therefore we should at least disable -Wshift-negative-value, because it
> doesn't buy us anything.
>
> - regarding overflow, in addition to the weird -Wpedantic warning, GCC 6
> adds a new -Wshift-overflow flag which is enabled by default in C99 and
> C11 modes, and which only applies to constant expressions.  So the
> remaining case where the compiler may change its behavior on overflow,
> i.e. constant expressions, will thus be caught by our usage of -Werror
> (because -Wshift-overflow is enabled by default).  So, independent of
> the limited scope of GCC's promise, with GCC 6 we will never have
> constant expressions that overflow because of left shifts.

I'm confused by all this text about constant expressions. Does
-fwrapv guarantee that signed shift of << behaves as we want
in all situations (including constant expressions) or doesn't it?
If it doesn't do that for constant expressions I don't think we should
rely on it, because it's way too confusing to have "this is OK except
when it isn't OK". (And a lot of our uses of "-1 << 8" are indeed
in constant expressions.)

> - if a compiler actually treated signed << overflow undefined behavior,
> a possible fix would be to use C89 mode (-std=gnu89), since signed << is
> unspecified there rather than undefined.  With C89, GCC's promise is
> complete.  We do use C89 with GCC <= 4.9 anyway, it makes sense to be
> homogeneous across all supported compilers.

"unspecified" isn't a great deal better than "undefined" really.

> Now, -fwrapv was really included in my patch only to silence ubsan in
> the future.  I think it should, and I will work on patches to fix that.
>  However, an advantage of -std=gnu89 is that it silences ubsan _now_ at
> least for GCC.  So let's just drop -fwrapv and use -std=gnu89 instead.
> This buys us time, and in the meanwhile I'll gett -fwrapv complete in GCC.
>
> If this is okay, I'll remove the patch, respin the pull request, and
> post a new configure change to use -std=gnu89.

I don't think this gains us much as a different approach, and it's
still trying to do cleanup (2) before we have dealt with the main
issue (1).

> Yes, we haven't heard anything from clang developers.  But clang does
> not even document implementation-defined behavior
> (https://llvm.org/bugs/show_bug.cgi?id=11272).  The bug says:
>
>> The clang documentation should specify how clang behaves in cases
>> specified to be implementation-defined in the relevant standards.
>> Perhaps simply saying that our behavior is the same as GCC's would suffice?
>
> This is about implementation-defined rather than undefined behavior, but
> I think it's enough to not care about clang developer's silence.

I disagree here. I think it's important to get the clang developers
to tell us what they mean by -fwrapv and what they want to guarantee
about signed shifts. That's because if they decide to say "no, we
don't guarantee behaviour here because the C spec says it's UB" then
we need to change how we deal with these in QEMU.

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Paolo Bonzini]

On 26/11/2015 16:01, Peter Maydell wrote:
> > - regarding overflow, in addition to the weird -Wpedantic warning, GCC 6
> > adds a new -Wshift-overflow flag which is enabled by default in C99 and
> > C11 modes, and which only applies to constant expressions.  So the
> > remaining case where the compiler may change its behavior on overflow,
> > i.e. constant expressions, will thus be caught by our usage of -Werror
> > (because -Wshift-overflow is enabled by default).  So, independent of
> > the limited scope of GCC's promise, with GCC 6 we will never have
> > constant expressions that overflow because of left shifts.
> 
> I'm confused by all this text about constant expressions. Does
> -fwrapv guarantee that signed shift of << behaves as we want
> in all situations (including constant expressions) or doesn't it?

Until GCC 5, it does even without -fwrapv.

Until GCC 6, the generated code is OK but you get warnings.  For (-1 <<
8) you have to enable them manually, for (0xFF << 28) you always get
them.  I am working on a patch to make GCC 6 shut up if you specify
-fwrapv, but my point is that -fwrapv is _not_ necessary because:

- GCC very sensibly does not make -Wall enable -Wshift-negative-value

- warnings such as 0xFF << 28 are much less contentious, and even shifts
into the sign bit (e.g. 1 << 31 or 0xFF << 24) are not enabled by
default either.

> (And a lot of our uses of "-1 << 8" are indeed in constant expressions.)

Those are okay as long as you do not use -Wextra.

Again, the _value_ is perfectly specified by the GCC documentation (and
as of this morning, it's promised to remain that way).  GCC leaves the
door open for warning in constant expressions, and indeed GCC 6 warns
more than GCC 5 in this regard.

> "unspecified" isn't a great deal better than "undefined" really.

It is better inasmuch as it shuts up ubsan.

>> If this is okay, I'll remove the patch, respin the pull request, and
>> post a new configure change to use -std=gnu89.
> 
> I don't think this gains us much as a different approach, and it's
> still trying to do cleanup (2) before we have dealt with the main
> issue (1).

What I'm saying is that:

- you were (rightly) worried about the compiler's behavior for constant
expressions

- but since we use -Werror, we do not have to worry about them.  With
-Werror, anything that GCC 6 can compile is perfectly specified by the
GCC documentation, including left shifts of negative values.

So GCC does not even need -fwrapv, and -std=gnu89 is a better way to
shut up ubsan because it already works.

Regarding clang, we cannot be hostage of their silence.  And recalling
that they were the ones who f***ed up their warning levels in the first
place, and are making us waste so much time, I couldn't care less about
their opinions.

> > This is about implementation-defined rather than undefined behavior, but
> > I think it's enough to not care about clang developer's silence.
>
> I disagree here. I think it's important to get the clang developers
> to tell us what they mean by -fwrapv and what they want to guarantee
> about signed shifts. That's because if they decide to say "no, we
> don't guarantee behaviour here because the C spec says it's UB" then
> we need to change how we deal with these in QEMU.

No, we need to change our list of supported compilers (if that happens).

Paolo

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Peter Maydell]

On 26 November 2015 at 15:40, Paolo Bonzini <pbonzini@redhat.com> wrote:
> On 26/11/2015 16:01, Peter Maydell wrote:
>> I'm confused by all this text about constant expressions. Does
>> -fwrapv guarantee that signed shift of << behaves as we want
>> in all situations (including constant expressions) or doesn't it?
>
> Until GCC 5, it does even without -fwrapv.
>
> Until GCC 6, the generated code is OK but you get warnings.  For (-1 <<
> 8) you have to enable them manually, for (0xFF << 28) you always get
> them.  I am working on a patch to make GCC 6 shut up if you specify
> -fwrapv, but my point is that -fwrapv is _not_ necessary because:
>
> - GCC very sensibly does not make -Wall enable -Wshift-negative-value
>
> - warnings such as 0xFF << 28 are much less contentious, and even shifts
> into the sign bit (e.g. 1 << 31 or 0xFF << 24) are not enabled by
> default either.
>
>> (And a lot of our uses of "-1 << 8" are indeed in constant expressions.)
>
> Those are okay as long as you do not use -Wextra.

This is all talking about in-practice behaviour of the compiler.
What I'm interested in is what the documented guarantees are.

> Again, the _value_ is perfectly specified by the GCC documentation (and
> as of this morning, it's promised to remain that way).  GCC leaves the
> door open for warning in constant expressions, and indeed GCC 6 warns
> more than GCC 5 in this regard.

I still don't understand why the GCC documentation can't straightforwardly
say "-fwrapv means you get 2s complement behaviour for all operations
including shifts and arithmetic, in all contexts, and we will not warn
or otherwise complain about it". If that's what they're in practice
agreeing to then why not just say so in a comprehensible way, rather
than splitting the information across two different sections of the
documentation and including a confusing bit of text about constant
expressions?

>> I don't think this gains us much as a different approach, and it's
>> still trying to do cleanup (2) before we have dealt with the main
>> issue (1).
>
> What I'm saying is that:
>
> - you were (rightly) worried about the compiler's behavior for constant
> expressions
>
> - but since we use -Werror, we do not have to worry about them.  With
> -Werror, anything that GCC 6 can compile is perfectly specified by the
> GCC documentation, including left shifts of negative values.
>
> So GCC does not even need -fwrapv, and -std=gnu89 is a better way to
> shut up ubsan because it already works.
>
> Regarding clang, we cannot be hostage of their silence.  And recalling
> that they were the ones who f***ed up their warning levels in the first
> place, and are making us waste so much time, I couldn't care less about
> their opinions.

It's been less than 10 days since you filed a bug report with them.
Why are we in such a hurry? I would much rather we took the time to
hear from the clang developers as well as the gcc developers, and then
made our decision about what the right compiler flags are. There doesn't
seem to be any sudden change here that means we need to adjust our
compiler flags for the 2.5 release.

>> > This is about implementation-defined rather than undefined behavior, but
>> > I think it's enough to not care about clang developer's silence.
>>
>> I disagree here. I think it's important to get the clang developers
>> to tell us what they mean by -fwrapv and what they want to guarantee
>> about signed shifts. That's because if they decide to say "no, we
>> don't guarantee behaviour here because the C spec says it's UB" then
>> we need to change how we deal with these in QEMU.
>
> No, we need to change our list of supported compilers (if that happens).

I would strongly favour avoiding this UB rather than dropping clang
as a supported compiler,and implicitly dropping OSX as a supported
platform. (But it doesn't seem to me very likely we'd end up having
to make that awkward choice.)

thanks
-- PMM

Re: [Qemu-devel] [PULL 0/9] Misc patches for QEMU 2.5-rc2 (2015-11-25)

[Paolo Bonzini]

On 26/11/2015 16:55, Peter Maydell wrote:
> This is all talking about in-practice behaviour of the compiler.
> What I'm interested in is what the documented guarantees are.

They are these:

>> Again, the _value_ is perfectly specified by the GCC documentation (and
>> as of this morning, it's promised to remain that way).  GCC leaves the
>> door open for warning in constant expressions, and indeed GCC 6 warns
>> more than GCC 5 in this regard.

and they don't require -fwrapv at all.  I only added -fwrapv for ubsan,
but I now believe that C89 is a better way to shut it up.

> I still don't understand why the GCC documentation can't straightforwardly
> say "-fwrapv means you get 2s complement behaviour for all operations
> including shifts and arithmetic, in all contexts, and we will not warn
> or otherwise complain about it". If that's what they're in practice
> agreeing to then why not just say so in a comprehensible way, rather
> than splitting the information across two different sections of the
> documentation and including a confusing bit of text about constant
> expressions?

Because for example -fwrapv still gives you a SIGFPE for INT_MIN / -1.

>>>> This is about implementation-defined rather than undefined behavior, but
>>>> I think it's enough to not care about clang developer's silence.
>>>
>>> I disagree here. I think it's important to get the clang developers
>>> to tell us what they mean by -fwrapv and what they want to guarantee
>>> about signed shifts. That's because if they decide to say "no, we
>>> don't guarantee behaviour here because the C spec says it's UB" then
>>> we need to change how we deal with these in QEMU.
>>
>> No, we need to change our list of supported compilers (if that happens).
> 
> I would strongly favour avoiding this UB rather than dropping clang
> as a supported compiler,and implicitly dropping OSX as a supported
> platform.

gcc supports OS X, but...

> (But it doesn't seem to me very likely we'd end up having
> to make that awkward choice.)

... to me neither.   Also, if we had to make the choice, it'd probably
be a good idea anyway. :)

Paolo