On Fri, Feb 6, 2015 at 8:34 AM, Peter Maydell <peter.mayd...@linaro.org>
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
>
>
>
