Re: [PULL v2 25/31] osdep: Make MIN/MAX evaluate arguments only once

[Markus Armbruster]

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

> On Fri, 26 Jun 2020 at 14:57, Paolo Bonzini <pbonzini@redhat.com> wrote:
>>
>> From: Eric Blake <eblake@redhat.com>
>>
>> I'm not aware of any immediate bugs in qemu where a second runtime
>> evaluation of the arguments to MIN() or MAX() causes a problem, but
>> proactively preventing such abuse is easier than falling prey to an
>> unintended case down the road.  At any rate, here's the conversation
>> that sparked the current patch:
>> https://lists.gnu.org/archive/html/qemu-devel/2018-12/msg05718.html
>
> Hi; the changes in this patch seem to confuse Coverity.
>
>> +#undef MIN
>> +#define MIN(a, b)                                       \
>> +    ({                                                  \
>> +        typeof(1 ? (a) : (b)) _a = (a), _b = (b);       \
>> +        _a < _b ? _a : _b;                              \
>> +    })
>> +#define MIN_CONST(a, b)                                         \
>> +    __builtin_choose_expr(                                      \
>> +        __builtin_constant_p(a) && __builtin_constant_p(b),     \
>> +        (a) < (b) ? (a) : (b),                                  \
>> +        ((void)0))
>> +#undef MAX
>> +#define MAX(a, b)                                       \
>> +    ({                                                  \
>> +        typeof(1 ? (a) : (b)) _a = (a), _b = (b);       \
>> +        _a > _b ? _a : _b;                              \
>> +    })
>> +#define MAX_CONST(a, b)                                         \
>> +    __builtin_choose_expr(                                      \
>> +        __builtin_constant_p(a) && __builtin_constant_p(b),     \
>> +        (a) > (b) ? (a) : (b),                                  \
>> +        ((void)0))
>
> In particular, where MIN_CONST or MAX_CONST are used to
> define values that must be const, eg in qemu-file.c:
>  50    DECLARE_BITMAP(may_free, MAX_IOV_SIZE);
> or in hcd-xhci.h:
> 217    USBPort  uports[MAX_CONST(MAXPORTS_2, MAXPORTS_3)];
>
> Coverity reports:
>
> CID 1429992 (#1 of 1): Unrecoverable parse warning (PARSE_ERROR)1.
> expr_not_constant: expression must have a constant value
>
> Can we do something (eg providing fallback less-intelligent
> versions of the macro ifdef __COVERITY__) to help it?

Perhaps we can solve the issue with scripts/coverity-model.c.
Unfortunately, I can't spare the time to try right now.

> (This is the cause of CID 1429992, 1429995, 1429997,
> 1429999. Parse errors are unfortunate because Coverity
> abandons analysis of the affected function entirely,
> and analysis of its callers is also limited.)

Bummer.  I recommend to revert until we figure out how not to break
Coverity.