Page MenuHomePhabricator

Emit -Wmicrosoft-enum-value warning instead of error in MS ABI
ClosedPublic

Authored by rnk on Fri, Sep 6, 3:31 PM.

Details

Summary

The first NFC change is to replace a getCXXABI().isMicrosoft() check
with getTriple().isWindowsMSVCEnvironment(). This code takes effect in
non-C++ compilations, so it doesn't make sense to check the C++ ABI. In
the MS ABI, enums are always considered to be "complete" because the
underlying type of an unfixed enum will always be 'int'. This behavior
was moved from -fms-compatibility to MS ABI back in r249656.

The second change is functional, and it downgrades an error to a warning
when the MS ABI is used rather than only under -fms-compatibility. The
reasoning is that it's unreasonable for the following code to reject the
following code for all MS ABI targets with -fno-ms-compatibility:

enum Foo { Foo_Val = 0xDEADBEEF };

This is valid code for any other target, but in the MS ABI, Foo_Val just
happens to be negative. With this change, clang emits a
-Wmicrosoft-enum-value warning on this code, but compiles it without
error.

Fixes PR38478

Diff Detail

Repository
rL LLVM

Event Timeline

rnk created this revision.Fri, Sep 6, 3:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptFri, Sep 6, 3:31 PM
hans accepted this revision.Mon, Sep 9, 6:11 AM

lgtm with comment

The main functional change is that -fno-ms-compatibility now no longer
sends us down the hard error diagnostic code path for ObjC fixed enums.
Instead, complete-but-not-fixed MS enums go down the C99 codepath which
issues a -Wmicrosoft-enum-value warning about the enum value not being
representable as an 'int'. This was highlighted as the single largest
blocker to compiling windows.h with -fno-ms-compatibility, so this
should make that feasible.

Should there be a test exercising this part? The updated tests both have -fms-compatibility, so were already just warning.

This revision is now accepted and ready to land.Mon, Sep 9, 6:11 AM
rnk updated this revision to Diff 219425.Mon, Sep 9, 1:38 PM
  • rewrite, abandon unification
rnk added a comment.Mon, Sep 9, 1:40 PM

Should there be a test exercising this part? The updated tests both have -fms-compatibility, so were already just warning.

Good point, we aren't testing that this unfixed enum behavior is a part of the ABI, not -fms-compatibility.

After testing this some more, I think I need to back off on my misguided refactoring. I didn't notice that the C99 warning was an off-by-default pedantic warning, and it didn't insert the implicit cast in the case where the value was not representable in int.

rnk requested review of this revision.Mon, Sep 9, 1:40 PM
rnk retitled this revision from Unify checking enumerator values in ObjC, C, and MSVC modes to Emit -Wmicrosoft-enum-value warning instead of error in MS ABI.
rnk edited the summary of this revision. (Show Details)

Ptal, new patch

hans accepted this revision.Tue, Sep 10, 1:29 AM

lgtm

This revision is now accepted and ready to land.Tue, Sep 10, 1:29 AM

We have the old TODO of changing this warning to be emitted at enum use time (e.g. when Foo_Val is compared to 0) instead of declaration time. Maybe that's a better fix. Or is implementing that very involved?

rnk added a comment.Tue, Sep 10, 4:33 PM

We have the old TODO of changing this warning to be emitted at enum use time (e.g. when Foo_Val is compared to 0) instead of declaration time. Maybe that's a better fix. Or is implementing that very involved?

I guess it would depend on the logic of the point-of-use warning. We could make it very simple. We could:

  • mark all enum types with coerced enumerators (maybe we already know this)
  • warn on all ordering comparisons that use that enum type

If the user explicitly casts to any other integer type to do the comparison, we can expect them to know the rules.

I think it can't allow false negatives, though. -Wmicrosoft is kind of like our -pedantic mode for standard C. It basically says, if your code is -Wmicrosoft clean, we promise that it is not relying on Microsoft-specific behavior. The simplest way to do that is to soften the existing error to a warning, which is what this does.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptTue, Sep 10, 6:00 PM