[PATCH] D23921: Remove va_start diagnostic false positive with enumerations

[Aaron Ballman via cfe-commits]

On Tue, Dec 20, 2016 at 7:58 AM, Attila Török <torokati44 at gmail.com> wrote:
> I did not see that it was reapplied later, sorry.
>
> With clang version 3.9.0 (tags/RELEASE_390/final) I get the warning in both
> c11 and c++11 mode.
> With clang version 4.0.0 (trunk 290146) (llvm/trunk 290118) it's gone in c11
> mode, but still there in c++11.
> The piece of code I tested it on:
> https://gist.github.com/torokati44/37e6aca2d516cb7c3cb31b7ccf8a519e
>
> In the part of the code affected by the patch, ED->getPromotionType() is
> BuiltinType 'int', and Type is EnumType 'enum E'.
> For these two types, Context.typesAreCompatible returns true in c11 mode,
> but false in c++11 mode (regardless of which underlying type - or if any -
> is specified).
> I presume this is the intended behavior. And if so, how could the example
> code be modified to make it warning-free in c++, while keeping the parameter
> an enum, and not making it a simple int?

Yes, that behavior is intended. The answer to how to modify the code
involves the C++ standards committee, though. [cstdarg.syn]p1 says, in
part,

The parameter parmN is the identifier of the rightmost parameter in
the variable parameter list of the function definition (the one just
before the ...). If the parameter parmN is of a reference type, or of
a type that is not compatible with the type that results when passing
an argument for which there is no parameter, the behavior is
undefined.

When typesAreCompatible() returns false, that means you are triggering
UB with your code.

The reason the C++ behavior differs from the C behavior when calling
typesAreCompaible() is because type compatibility (as a language
construct) is a C notion; in C++ this maps to "are the types the
same", which is not true for an enum type and an int type (even with
an explicit underlying type).

However, there are questions as to whether this should be UB or not
that I've raised with WG21 (I don't have a Core defect report for it
yet though). For right now, the safest answer is: change the parameter
to be an int, because that is definitely not UB. There may be a more
satisfactory answer in the future.

~Aaron

>
> Thank you,
> Attila
>
> 2016-12-19 18:36 GMT+01:00 Aaron Ballman <aaron.ballman at gmail.com>:
>>
>> On Fri, Dec 16, 2016 at 7:00 AM, Attila Török via Phabricator
>> <reviews at reviews.llvm.org> wrote:
>> > torokati44 added a comment.
>> >
>> > I see this has been reverted in r281612, but I can no longer access the
>> > build log serving as a reason linked in the message:
>> > https://www.mail-archive.com/cfe-commits@lists.llvm.org/msg35386.html
>> > We have a few false-positive warnings that (I think) would be silenced
>> > by this change. I'm just wondering if something like this, if not this,
>> > could be included anyway? Not critical of course, it just would be nice.
>>
>> This patch was reapplied in r281632:
>>
>>
>> http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20160912/170772.html
>>
>> Do you still have false positives even with that applied?
>>
>> Thanks!
>>
>> ~Aaron
>
>