[DebugInfo] Improvements to representation of enumeration types (PR36168)

[chill]

Summary

This is the clang counterpart to https://reviews.llvm.org/D42734

This patch:

• fixed an incorrect sign-extension of unsigned values, when emitting debug info metadata for enumerators
• the enumerators metadata is created with a flag, which determines interpretation of the value bits (signed or unsigned)
• the enumerations metadata contains the underlying integer type and a flag, indicating whether this is a C++ "fixed enum"

[dblaikie]

lib/CodeGen/CGDebugInfo.cpp

2491–2495

Move this "IsSigned" outside the loop, since it's loop invariant?

[chill]

Minor update, the parameter to createEnumerator is IsUnsigned now (instead of IsSigned), changed caller.

[dblaikie]

test/CodeGen/debug-info-enum.cpp

3

Could you summarize the purpose of each of these tests (possibly in a comment above the enum in each case) - there look to be more test cases than I'd imagine being necessary, but I haven't carefully analyzed them.

For example: I wouldn't expect to test every integer type, if the code handling them is general enough to be demonstrated by one or two cases?

7–11

Rather than relying on specific ordering of output, generally you should test that the actual references across different metadata records are correct (eg: check that the DICompositeType's member list elements are the DIEnumerators (use named matches, rather than hardcoding the metadata node numbers))

Though, admittedly, this is a lot easier to read as-is.

67

Probably drop the "flags: " part of this NOT check - so that if other flags are added, this won't be overly constrained (eg: if this type ended up with "flags: X | DIFlagFixedEnum" this NOT check as-is wouldn't catch the regression)

[chill]

Changes, relative to the previous revision:

• a few tweaks to the tests

[chill]

test/CodeGen/debug-info-enum.cpp

3

Yes, in hindsight, there are more tests that strictly necessary, I just enumerated all the different integer type sizes cross signed/usigned, for some there were no errors, for some there were always errors, for some there were sometimes errors only sometimes (i.e. depending on compiler target and/or options).

7–11

I've done that, to some extent, making sure each enumeration refers to the expected underlying type. Not idea how to do that completely independent of order.

[dblaikie]

Few things that could tidy up the tests a bit, but otherwise looks good :)

test/CodeGen/debug-info-enum.cpp

7–11

Thanks! Maybe add a comment (like I suggested in the LLVM review too, just now) in each case to describe each test case. You have one up the top for the enum class over signed char, but someting similar for each enum might be good.

Yeah, it's impossible to do it entirely independent of order.

You could do a bit more than what you've got here - the DIEnumerators could be checked:

match the list from the DICompositeType enumeration_type (the same way you match the baseType) then match the elements of the list, then match those elements to the DIEnumerators same as you do for the DIBasicType) - does that make sense?

[chill]

Changes since last revision:

• Add/update test to check that each enumerator belongs to the right enumeration

lib/CodeGen/CGDebugInfo.cpp

if (!ED->getTypeForDecl()->isIncompleteType()) {
     Align = getDeclAlignIfRequired(ED, CGM.getContext());
   }
 
   SmallString<256> FullName = getUniqueTagTypeName(Ty, CGM, TheCU);
 
   // Create elements for each enumerator.
   SmallVector<llvm::Metadata *, 16> Enumerators;
   ED = ED->getDefinition();
+  bool IsSigned = ED->getIntegerType()->isSignedIntegerType();
   for (const auto *Enum : ED->enumerators()) {
-    Enumerators.push_back(DBuilder.createEnumerator(
-        Enum->getName(), Enum->getInitVal().getSExtValue()));
+    const auto &InitVal = Enum->getInitVal();
+    auto Value = IsSigned ? InitVal.getSExtValue() : InitVal.getZExtValue();
+    Enumerators.push_back(
+        DBuilder.createEnumerator(Enum->getName(), Value, !IsSigned));
   }
 
   // Return a CompositeType for the enum itself.
   llvm::DINodeArray EltArray = DBuilder.getOrCreateArray(Enumerators);
 
   llvm::DIFile *DefUnit = getOrCreateFile(ED->getLocation());
   unsigned Line = getLineNumber(ED->getLocation());
   llvm::DIScope *EnumContext = getDeclContextDescriptor(ED);
-  llvm::DIType *ClassTy =
-      ED->isFixed() ? getOrCreateType(ED->getIntegerType(), DefUnit) : nullptr;
+  llvm::DIType *ClassTy = getOrCreateType(ED->getIntegerType(), DefUnit);
   return DBuilder.createEnumerationType(EnumContext, ED->getName(), DefUnit,
                                         Line, Size, Align, EltArray, ClassTy,
-                                        FullName);
+                                        FullName, ED->isFixed());
 }
 
 llvm::DIMacro *CGDebugInfo::CreateMacro(llvm::DIMacroFile *Parent,
                                         unsigned MType, SourceLocation LineLoc,
                                         StringRef Name, StringRef Value) {
   unsigned Line = LineLoc.isInvalid() ? 0 : getLineNumber(LineLoc);
   return DBuilder.createMacro(Parent, Line, MType, Name, Value);
 }

test/CodeGen/debug-info-enum.cpp

+// RUN: %clang -target x86_64-linux -g -S -emit-llvm -o - %s | FileCheck %s
+
+// Test enumerator value representation for each possible underlying integer
+// type; test the integer type and FixedEnum flags are as expected.
+enum class E0 : signed char {
+  A0 = -128,
+  B0 = 127,
+} x0;
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E0"
+// CHECK-SAME: baseType: ![[SCHAR:[0-9]+]]
+// CHECK-SAME: DIFlagFixedEnum
+// CHECK: ![[SCHAR]] = !DIBasicType(name: "signed char", size: 8, encoding: DW_ATE_signed_char)
+// CHECK: !DIEnumerator(name: "A0", value: -128)
+// CHECK: !DIEnumerator(name: "B0", value: 127)
+
+enum class E1 : unsigned char { A1 = 255 } x1;
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E1"
+// CHECK-SAME: baseType: ![[UCHAR:[0-9]+]]
+// CHECK-SAME: DIFlagFixedEnum
+// CHECK: ![[UCHAR]] = !DIBasicType(name: "unsigned char", size: 8, encoding: DW_ATE_unsigned_char)
+// CHECK: !DIEnumerator(name: "A1", value: 255, isUnsigned: true)
+
+enum class E2 : signed short {
+  A2 = -32768,
+  B2 = 32767,
+} x2;
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E2"
+// CHECK-SAME: baseType: ![[SHORT:[0-9]+]]
+// CHECK-SAME: DIFlagFixedEnum
+// CHECK: ![[SHORT]] = !DIBasicType(name: "short", size: 16, encoding: DW_ATE_signed)
+// CHECK: !DIEnumerator(name: "A2", value: -32768)
+// CHECK: !DIEnumerator(name: "B2", value: 32767)
+
+enum class E3 : unsigned short { A3 = 65535 } x3;
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E3"
+// CHECK-SAME: baseType: ![[USHORT:[0-9]+]]
+// CHECK-SAME: DIFlagFixedEnum
+// CHECK: ![[USHORT]] = !DIBasicType(name: "unsigned short", size: 16, encoding: DW_ATE_unsigned)
+// CHECK: !DIEnumerator(name: "A3", value: 65535, isUnsigned: true)
+
+enum class E4 : signed int { A4 = -2147483648, B4 = 2147483647 } x4;
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E4"
+// CHECK-SAME: baseType: ![[INT:[0-9]+]]
+// CHECK-SAME: DIFlagFixedEnum
+// CHECK: ![[INT]] = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+// CHECK: !DIEnumerator(name: "A4", value: -2147483648)
+// CHECK: !DIEnumerator(name: "B4", value: 2147483647)
+
+enum class E5 : unsigned int { A5 = 4294967295 } x5;
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E5"
+// CHECK-SAME: baseType: ![[UINT:[0-9]+]]
+// CHECK-SAME: DIFlagFixedEnum
+// CHECK: ![[UINT]] = !DIBasicType(name: "unsigned int", size: 32, encoding: DW_ATE_unsigned)
+// CHECK: !DIEnumerator(name: "A5", value: 4294967295, isUnsigned: true)
+
+enum class E6 : signed long long {
+  A6 = -9223372036854775807LL - 1,
+  B6 = 9223372036854775807LL
+} x6;
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E6"
+// CHECK-SAME: baseType: ![[LONG:[0-9]+]]
+// CHECK-SAME: DIFlagFixedEnum
+// CHECK: ![[LONG]] = !DIBasicType(name: "long long int", size: 64, encoding: DW_ATE_signed)
+// CHECK: !DIEnumerator(name: "A6", value: -9223372036854775808)
+// CHECK: !DIEnumerator(name: "B6", value: 9223372036854775807)
+
+enum class E7 : unsigned long long { A7 = 18446744073709551615ULL } x7;
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E7"
+// CHECK-SAME: baseType: ![[ULONG:[0-9]+]]
+// CHECK-SAME: DIFlagFixedEnum
+// CHECK: ![[ULONG]] = !DIBasicType(name: "long long unsigned int", size: 64, encoding: DW_ATE_unsigned)
+// CHECK: !DIEnumerator(name: "A7", value: 18446744073709551615, isUnsigned: true)
+
+// Also test the FixedEnum flag is not present for old-style enumerations.
+enum E8 { A8 = -128, B8 = 127 } x8;
+// CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "E8"
+// CHECK-SAME: baseType: ![[INT]]
+// CHECK-NOT: DIFlagFixedEnum
+// CHECK: !DIEnumerator(name: "A8", value: -128)
+

test/CodeGenCXX/debug-info-enum-class.cpp

 C c;
 D d;
 
 // CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "A"
 // CHECK-SAME:             line: 3
 // CHECK-SAME:             baseType: ![[INT:[0-9]+]]
 // CHECK-SAME:             size: 32
 // CHECK-NOT:              offset:
-// CHECK-NOT:              flags:
+// CHECK-SAME:             flags: DIFlagFixedEnum
 // CHECK-SAME:             ){{$}}
 // CHECK: ![[INT]] = !DIBasicType(name: "int"
 // CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "B"
 // CHECK-SAME:             line: 4
 // CHECK-SAME:             baseType: ![[ULONG:[0-9]+]]
 // CHECK-SAME:             size: 64
 // CHECK-NOT:              offset:
-// CHECK-NOT:              flags:
+// CHECK-SAME:             flags: DIFlagFixedEnum
 // CHECK-SAME:             ){{$}}
 // CHECK: ![[ULONG]] = !DIBasicType(name: "long unsigned int"
 // CHECK: !DICompositeType(tag: DW_TAG_enumeration_type, name: "C"
 // CHECK-SAME:             line: 5
-// CHECK-NOT:              baseType:
+// CHECK-SAME:             baseType: ![[ULONG:[0-9]+]]
 // CHECK-SAME:             size: 32
 // CHECK-NOT:              offset:
 // CHECK-NOT:              flags:
 // CHECK-SAME:             ){{$}}
 
 namespace PR14029 {
   // Make sure this doesn't crash/assert.
   template <typename T> struct Test {

test/CodeGenCXX/debug-info-enum.cpp

 // RUN: %clang_cc1 -triple %itanium_abi_triple -emit-llvm -debug-info-kind=limited %s -o - | FileCheck %s
 
 // CHECK: !DICompileUnit(
 // CHECK-SAME:           enums: [[ENUMS:![0-9]*]]
 // CHECK: [[ENUMS]] = !{[[E1:![0-9]*]], [[E2:![0-9]*]], [[E3:![0-9]*]]}
 
 namespace test1 {
 // CHECK: [[E1]] = !DICompositeType(tag: DW_TAG_enumeration_type, name: "e"
 // CHECK-SAME:                      scope: [[TEST1:![0-9]*]]
 // CHECK-SAME:                      elements: [[TEST1_ENUMS:![0-9]*]]
 // CHECK-SAME:                      identifier: "_ZTSN5test11eE"
 // CHECK: [[TEST1]] = !DINamespace(name: "test1"
 // CHECK: [[TEST1_ENUMS]] = !{[[TEST1_E:![0-9]*]]}
-// CHECK: [[TEST1_E]] = !DIEnumerator(name: "E", value: 0)
+// CHECK: [[TEST1_E]] = !DIEnumerator(name: "E", value: 0, isUnsigned: true)
 enum e { E };
 void foo() {
   int v = E;
 }
 }
 
 namespace test2 {
 // rdar://8195980

test/Modules/ModuleDebugInfo.cpp

 
 // CHECK: !DICompositeType(tag: DW_TAG_enumeration_type,
 // CHECK-NOT:              name:
 // CHECK-SAME:             identifier: "_ZTS11TypedefEnum")
 
 // CHECK: !DICompositeType(tag: DW_TAG_enumeration_type,
 // CHECK-NOT:              name:
 // CHECK-SAME:             )
-// CHECK: !DIEnumerator(name: "e5", value: 5)
+// CHECK: !DIEnumerator(name: "e5", value: 5, isUnsigned: true)
 
 // CHECK: !DIDerivedType(tag: DW_TAG_typedef, name: "B",
 // no mangled name here yet.
 
 // This type is anchored by a function parameter.
 // CHECK: !DICompositeType(tag: DW_TAG_class_type, name: "A<void>"
 // CHECK-SAME:             elements:
 // CHECK-SAME:             templateParams: