Re: [Qemu-devel] [PATCH 0/4] target-arm: fix various clang UB sanitizer warnings

[Greg Bellows]

On Fri, Feb 6, 2015 at 8:34 AM, Peter Maydell <address@hidden> wrote:

This patchset fixes a collection of warnings emitted by the clang
undefined behaviour sanitizer in the course of booting an AArch64
Linux guest to a shell prompt. These are all various kinds of bad
shift (shifting into the sign bit, left shifting negative values,
shifting by more than the size of the data type).

There remains one set of warnings which I'm not sure how to
approach. We have an enum for the valid syndrome register EC
field values:

enum arm_exception_class {
    EC_UNCATEGORIZED          = 0x00,
    [...]
    EC_VECTORCATCH            = 0x3a,
    EC_AA64_BKPT              = 0x3c,
};

The EC field is a 6 bit field at the top of the 32 bit syndrome
register, so we define

#define ARM_EL_EC_SHIFT 26

and have utility functions that assemble syndrome values like:

static inline uint32_t syn_aa64_bkpt(uint32_t imm16)
{
    return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
}

Unfortunately this is UB, because C says that enums can be
signed types, and if the enum is signed then "EC_AA64_BKPT << 26"
is shifting into the sign bit of a signed type.

I can't see any nice way of avoiding this. Possible nasty ways:

(1) Drop the enum and just use old-fashioned
#define EC_AA64_BKPT 0x3cU
since then we can force them to be unsigned
(2) Cast the constant to unsigned at point of use
(3) Keep the enum and add defines wrapping actual use
#define EC_AA64_BKPT ((unsigned)EC_AA64_BKPT)
I guess there's also
(4) Ignore the problem and trust that the compiler developers will
never decide to use this bit of undefined behaviour.

My preference is (1) I suppose (we don't actually use the
enum for anything except to define the values, so if it's
not providing helpful definitions we should drop it).

Opinions?

​Similar to the #define approach, but possibly with the benefits of the enum.  What if you declare each enum separately as const unsigned int?

​

 

Peter Maydell (4):
  target-arm: A64: Fix shifts into sign bit
  target-arm: A64: Fix handling of rotate in logic_imm_decode_wmask
  target-arm: A64: Avoid left shifting negative integers in
    disas_pc_rel_addr
  target-arm: A64: Avoid signed shifts in disas_ldst_pair()

 target-arm/translate-a64.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

--
1.9.1