[clang][Interp] Handle enums

[tbaeder]

Summary

Handle DeclRefExprs pointing to EnumConstantDecls.

[erichkeane]

I can't come up with a way to get a reference to one of these, so the logic in the rest of the DeclRefExpr handling is a little MORE awkward looking now. I think I'm OK with this, but I want to give a few other folks a chance to look.

[erichkeane]

Also note, the CI test is from your test!

[tbaeder]

In D134020#3795091, @erichkeane wrote:

I can't come up with a way to get a reference to one of these, so the logic in the rest of the DeclRefExpr handling is a little MORE awkward looking now. I think I'm OK with this, but I want to give a few other folks a chance to look.

Ah right, I could to through that code path as well and not immediately return. I actually tested references but the AST expresses that through a MaterializeTemporaryExpr, which don't work yet.

In D134020#3795092, @erichkeane wrote:

Also note, the CI test is from your test!

Yup, that needs one of the other open patches.

[shafik]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

867

If I check out IntExprEvaluator::CheckReferenceDecl(...) it is checking the sign and width match between the expression and the EnumConstantDecl. I am guessing we need to do that here as well?

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

867

I'm having a hard time trying to cause that code to trigger. e.g. in

enum Foo : unsigned int {
  A = -1,
};
static_assert(A == -1);

Both with the new and current interpreter, I get a`-Wc++11-narrowing` error by default, and the assertion works if I pass -Wno-c++11-narrowing.

[tbaeder]

clang/lib/AST/Interp/ByteCodeExprGen.cpp

867

Reading the code again, this seems to just compute a new value to not trigger assertions in Success(). Since those assertions don't exist in the new interpreter, the special casing shouldn't be needed.

[tbaeder]

Anything still missing here? (CI failure is because one of the other patches is missing)

[shafik]

LGTM

clang/test/AST/Interp/enums.cpp

26

Maybe some edge case values for enumerators like __INT_MAX__ *2U +1U (UINT_MAX)

and

enum E { // warning: enumeration values exceed range of largest integer [-Wenum-too-large]
  E1 = -__LONG_MAX__ -1L,
  E2 = __LONG_MAX__ *2UL+1UL 
};

[aaron.ballman]

LGTM!

[tbaeder]

clang/test/AST/Interp/enums.cpp

26

Hm, looks like that test broke one of the windows builders: https://lab.llvm.org/buildbot/#/builders/123/builds/13424 - are enums larger by default on Windows? What do you suggest to fix the test?

[aaron.ballman]

clang/test/AST/Interp/enums.cpp

26

If we're trying to be compatible with MSVC, we use their rules for picking the underlying type of an enumeration is which not fixed. One way to handle this is to add more RUN lines with explicit triples, but I think we lose too much interesting coverage that way. I'd probably use a #ifndef _MSC_VER block to control the expected diagnostics with a comment as to why the diagnostic is not expected on Windows.

[tbaeder]

clang/test/AST/Interp/enums.cpp

26

The second builder that broke was a hexagon builder: https://lab.llvm.org/buildbot/#/builders/38/builds/6231 - this would still fail with the _MSC_VER change, right?

[aaron.ballman]

clang/test/AST/Interp/enums.cpp

26

Nope, drat.

I would recommend breaking that specific test out into a separate file where we can add various RUN lines with triples, and the rest of the test can remain targetless.

[tbaeder]

clang/test/AST/Interp/enums.cpp

26

Shame, but alright. So, what triples should I test? :)

[aaron.ballman]

clang/test/AST/Interp/enums.cpp

26

The ones from the failing bots, and I'd say maybe x86_64-pc-linux and i686-pc-linux would be reasonable.

[tbaeder]

clang/test/AST/Interp/enums.cpp

26

Heh, the warning doesn't happen with the i686 triple either (with the old interpreter). Is that expected?

[aaron.ballman]

clang/test/AST/Interp/enums.cpp

26

Yeah, that sounds about like what I'd expect.

clang/lib/AST/Interp/ByteCodeExprGen.cpp

if (auto It = Locals.find(Decl); It != Locals.end()) {
     FoundDecl = true;
   } else if (const auto *PVD = dyn_cast<ParmVarDecl>(Decl)) {
     if (auto It = this->Params.find(PVD); It != this->Params.end()) {
       if (!this->emitGetPtrParam(It->second, E))
         return false;
 
       FoundDecl = true;
     }
+  } else if (const auto *ECD = dyn_cast<EnumConstantDecl>(Decl)) {
+    PrimType T = *classify(ECD->getType());
+
+    return this->emitConst(T, getIntWidth(ECD->getType()), ECD->getInitVal(),
+                           E);
   }
 
   // References are implemented using pointers, so when we get here,
   // we have a pointer to a pointer, which we need to de-reference once.
   if (FoundDecl) {
     if (IsReference) {
       if (!this->emitLoadPopPtr(E))
         return false;

clang/test/AST/Interp/enums.cpp

+// RUN: %clang_cc1 -fexperimental-new-constant-interpreter -verify %s
+// RUN: %clang_cc1 -verify=ref %s
+
+enum class EC : short {
+  A, B, C
+};
+static_assert(static_cast<int>(EC::A) == 0, "");
+static_assert(static_cast<int>(EC::B) == 1, "");
+static_assert(static_cast<int>(EC::C) == 2, "");
+static_assert(sizeof(EC) == sizeof(short), "");
+
+constexpr EC ec = EC::C;
+static_assert(static_cast<int>(ec) == 2, "");
+
+constexpr int N = 12;
+constexpr int M = 2;
+
+enum CE {
+  ONE = -1,
+  TWO = 2,
+  THREE,
+  FOUR = 4,
+  FIVE = N + M,
+  SIX = FIVE + 2,
+  MAX = __INT_MAX__ * 2U + 1U
+};
+static_assert(ONE == -1, "");
+static_assert(THREE == 3, "");
+static_assert(FIVE == 14, "");
+static_assert(SIX == 16, "");
+
+constexpr EC testEnums() {
+  EC e = EC::C;
+
+  e = EC::B;
+
+  EC::B = e; // expected-error{{expression is not assignable}} \
+             // ref-error{{expression is not assignable}}
+
+  return e;
+}
+
+constexpr EC getB() {
+  EC e = EC::C;
+  e = EC::B;
+  return e;
+}
+
+
+static_assert(getB() == EC::B, "");
+
+
+enum E { // expected-warning{{enumeration values exceed range of largest integer}} \
+         // ref-warning{{enumeration values exceed range of largest integer}}
+  E1 = -__LONG_MAX__ -1L,
+  E2 = __LONG_MAX__ *2UL+1UL
+};