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

[Eric Blake]

On 02/06/2015 07:34 AM, Peter Maydell 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)

There's also:

(5) change the usage of the shifting macro:
#define ARM_EL_EC_SHIFT(x) (((x)+0U) << 26)
...
    return ARM_EL_EC_SHIFT(EC_AA64_BKPT) | ARM_EL_IL | (imm16 & 0xffff);

> 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.

HACKING already implies we assume sane 2's complement behavior of shifts
(maybe it's worth another line for this particular case of shifting into
the signed bit of a signed result, and figuring out how to shut up clang):

>> The C language specification defines regions of undefined behavior and
>> implementation defined behavior (to give compiler authors enough leeway to
>> produce better code).  In general, code in QEMU should follow the language
>> specification and avoid both undefined and implementation defined
>> constructs. ("It works fine on the gcc I tested it with" is not a valid
>> argument...) However there are a few areas where we allow ourselves to
>> assume certain behaviors because in practice all the platforms we care about
>> behave in the same way and writing strictly conformant code would be
>> 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)


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org

signature.asc
Description: OpenPGP digital signature