Add pretty printing of Clang "bitfield" enums

[friss]

Summary

Using enumerators as flags is standard practice. This patch adds
support to LLDB to display such enum values symbolically, eg:

(E) e1 = A | B

If enumerators don't cover the whole value, the remaining bits are
displayed as hexadecimal:

(E) e4 = A | 0x10

Detecting whether an enum is used as a bitfield or not is
complicated. This patch implements a heuristic that assumes that such
enumerators will either have only 1 bit set or will be a combination
of previous values.

This patch doesn't change the way we currently display enums which the
above heuristic would not consider as bitfields.

[jingham]

Your description says that if there are bits not belonging to the enum, you will print them after the enum values are listed. But you don't have a test for that. I thought you were going to use the "nonsense" var for that, but then you didn't...

[jingham]

Also, since in C++ you can't do:

enum bitfield ac = A | C;

it's fairly common practice to pass or'ed elements from the enum as an int of some sort.

This feature would be really handy in that case, but the way you would get at it is not through frame var (since you can't actually have a variable of bitfield type that has more than one value) but by casting in the expression parser:

int whatever = A | B

And then to reveal this, you would want to do:

(lldb) expr (enum bitfield) whatever

Be nice to see a test of that to make sure that works through the expression parser as well.

[jingham]

The code looks fine to me.

source/Symbol/ClangASTContext.cpp

9502 ↗

(On Diff #219993)

Can you put a comment here like:

// TADA - we found the exact value, print it.

It took me a bit to figure out where the normal path of printing an enum that had a simple enum value went...

[labath]

This looks like a really useful feature. The code seems fine, but I am wondering if we should really bail out when encountering a zero enumerator. It is not uncommon to use a special enumerator to mean "none of the above". Lldb does that occasionally (eEmulateInstructionOptionNone), and other APIs do that too (PROT_NONE, PROT_READ, PROT_WRITE, PROT_EXEC in mmap(2) for instance). I am guessing this practice is even more common for "class" enums, as those can't be implicitly constructed from integer constants.

I think it would be useful to add one or two tests with enum types where this heuristic does not kick in. Like a type which has a two-bit enumerator which is not covered by previous enumerators, or (if you decide to keep the current behavior) a type with a zero enumerator.

[MaskRay]

source/Symbol/ClangASTContext.cpp

9529 ↗

(On Diff #219993)

You can simply sort by magnitude and iterating the elements from the largest to the smallest.

[friss]

In D67520#1670880, @labath wrote:

This looks like a really useful feature. The code seems fine, but I am wondering if we should really bail out when encountering a zero enumerator. It is not uncommon to use a special enumerator to mean "none of the above". Lldb does that occasionally (eEmulateInstructionOptionNone), and other APIs do that too (PROT_NONE, PROT_READ, PROT_WRITE, PROT_EXEC in mmap(2) for instance). I am guessing this practice is even more common for "class" enums, as those can't be implicitly constructed from integer constants.

I think it would be useful to add one or two tests with enum types where this heuristic does not kick in. Like a type which has a two-bit enumerator which is not covered by previous enumerators, or (if you decide to keep the current behavior) a type with a zero enumerator.

I had looked at a few enums in LLVM before sending out the patch and decided to remove the "0" heuristic. I wrote the patch description with the new heuristic and then forgot to update the patch itself... I'm currently tracking down another regression that prevents Jim's use case from working, I'll refresh the patch with your comments addressed after I'm done with this.

[friss]

Added a couple more tests

[friss]

In D67520#1668494, @jingham wrote:

(lldb) expr (enum bitfield) whatever

Be nice to see a test of that to make sure that works through the expression parser as well.

This works now and I added a test for it.

source/Symbol/ClangASTContext.cpp

9529 ↗

(On Diff #219993)

After reading your comment, I thought we didn't need any sorting at all given the heuristic above guarantees some ordering in the elements we get here.

The error that ensued from replying the stable_sort by a reverse reminded me why I used a stable_sort in the first place. In the added test, ac would be printed C | A instead of A | C. Granted both of them are correct, but for some reason it bothers me a lot that the elements are not printed in declaration order, so I left the code as-is.

[friss]

Add comments to some tests.

[labath]

looks good to me

packages/Python/lldbsuite/test/lang/c/enum_types/TestEnumTypes.py

44 ↗

(On Diff #223705)

s/clabg/clang

source/Symbol/ClangASTContext.cpp

9428 ↗

(On Diff #223705)

I guess this really means "where A is declared before C", because A can be numerically higher even if it's declared first.

[stella.stamenova]

It looks like this broke one of the tests on the Windows LLDB bot:

http://lab.llvm.org:8011/builders/lldb-x64-windows-ninja/builds/9628/steps/test/logs/stdio

The bot was already red because of a change from yesterday, so you probably didn't get a notification.

[shafik]

See the same break on green dragon as well: http://lab.llvm.org:8080/green/view/LLDB/job/lldb-cmake/2360/testReport/LLDB/SymbolFile_NativePDB/s_constant_cpp/

lldb/lit/SymbolFile/DWARF/debug-types-missing-signature.test

 RUN: %lldb %t -b -o "print (E) 1" 2>&1 | FileCheck --check-prefix=PRINTE %s
 PRINTE: use of undeclared identifier 'E'
 
 RUN: %lldb %t -b -o "print (EC) 1" 2>&1 | FileCheck --check-prefix=PRINTEC %s
 PRINTEC: use of undeclared identifier 'EC'
 
 RUN: %lldb %t -b -o "target variable a e ec" | FileCheck --check-prefix=VARS %s
 VARS: (const (anonymous struct)) a = {}
-VARS: (const (anonymous enum)) e = 1
-VARS: (const (anonymous enum)) ec = 1
+VARS: (const (anonymous enum)) e = 0x1
+VARS: (const (anonymous enum)) ec = 0x1

lldb/packages/Python/lldbsuite/test/lang/c/enum_types/TestEnumTypes.py

def setUp(self):
         self.line = line_number('main.c', '// Set break point at this line.')
 
     def test(self):
         """Test 'image lookup -t days' and check for correct display and enum value printing."""
         self.build()
         exe = self.getBuildArtifact("a.out")
         self.runCmd("file " + exe, CURRENT_EXECUTABLE_SET)
 
+        lldbutil.run_to_source_breakpoint(
+            self, '// Breakpoint for bitfield', lldb.SBFileSpec("main.c"))
+
+        self.expect("fr var a", DATA_TYPES_DISPLAYED_CORRECTLY,
+                    patterns=[' = A$'])
+        self.expect("fr var b", DATA_TYPES_DISPLAYED_CORRECTLY,
+                    patterns=[' = B$'])
+        self.expect("fr var c", DATA_TYPES_DISPLAYED_CORRECTLY,
+                    patterns=[' = C$'])
+        self.expect("fr var ab", DATA_TYPES_DISPLAYED_CORRECTLY,
+                    patterns=[' = AB$'])
+        self.expect("fr var ac", DATA_TYPES_DISPLAYED_CORRECTLY,
+                    patterns=[' = A | C$'])
+        self.expect("fr var all", DATA_TYPES_DISPLAYED_CORRECTLY,
+                    patterns=[' = ALL$'])
+        # Test that an enum that doesn't match the heuristic we use in
+        # ClangASTContext::DumpEnumValue, gets printed as a raw integer.
+        self.expect("fr var omega", DATA_TYPES_DISPLAYED_CORRECTLY,
+                    patterns=[' = 7$'])
+        # Test the behavior in case have a variable of a type considered
+        # 'bitfield' by the heuristic, but the value isn't actually fully
+        # covered by the enumerators.
+        self.expect("p (enum bitfield)nonsense", DATA_TYPES_DISPLAYED_CORRECTLY,
+                    patterns=[' = B | C | 0x10$'])
+
         # Break inside the main.
         bkpt_id = lldbutil.run_break_set_by_file_and_line(
             self, "main.c", self.line, num_expected_locations=1, loc_exact=True)
-
-        self.runCmd("run", RUN_SUCCEEDED)
+        self.runCmd("c", RUN_SUCCEEDED)
 
         # The stop reason of the thread should be breakpoint.
         self.expect("thread list", STOPPED_DUE_TO_BREAKPOINT,
                     substrs=['stopped',
                              'stop reason = breakpoint'])
 
         # The breakpoint should have a hit count of 1.
         self.expect("breakpoint list -f", BREAKPOINT_HIT_ONCE,

lldb/packages/Python/lldbsuite/test/lang/c/enum_types/main.c

 typedef enum ops ops;
 
 struct foo {
   ops *op;
 };
 
 int main (int argc, char const *argv[])
 {
+    enum bitfield {
+        None = 0,
+        A = 1 << 0,
+        B = 1 << 1,
+        C = 1 << 2,
+        AB = A | B,
+        ALL = A | B | C,
+    };
+
+    enum non_bitfield {
+        Alpha = 3,
+        Beta = 4
+    };
+
     enum days {
         Monday = -3,
         Tuesday,
         Wednesday,
         Thursday,
         Friday,
         Saturday,
         Sunday,
         kNumDays
     };
+
+    enum bitfield a = A, b = B, c = C, ab = AB, ac = A | C, all = ALL;
+    int nonsense = a + b + c + ab + ac + all;
+    enum non_bitfield omega = Alpha | Beta;
+
     enum days day;
     struct foo f;
-    f.op = NULL;
+    f.op = NULL; // Breakpoint for bitfield
     for (day = Monday - 1; day <= kNumDays + 1; day++)
     {
         printf("day as int is %i\n", (int)day); // Set break point at this line.
     }
     return 0;
 }

lldb/source/Symbol/ClangASTContext.cpp

 static bool DumpEnumValue(const clang::QualType &qual_type, Stream *s,
                           const DataExtractor &data, lldb::offset_t byte_offset,
                           size_t byte_size, uint32_t bitfield_bit_offset,
                           uint32_t bitfield_bit_size) {
   const clang::EnumType *enutype =
       llvm::cast<clang::EnumType>(qual_type.getTypePtr());
   const clang::EnumDecl *enum_decl = enutype->getDecl();
   assert(enum_decl);
-  clang::EnumDecl::enumerator_iterator enum_pos, enum_end_pos;
-  const bool is_signed = qual_type->isSignedIntegerOrEnumerationType();
   lldb::offset_t offset = byte_offset;
-  const int64_t enum_value = data.GetMaxS64Bitfield(
+  const uint64_t enum_svalue = data.GetMaxS64Bitfield(
       &offset, byte_size, bitfield_bit_size, bitfield_bit_offset);
-
+  bool can_be_bitfield = true;
+  uint64_t covered_bits = 0;
+  int num_enumerators = 0;
+
+  // Try to find an exact match for the value.
+  // At the same time, we're applying a heuristic to determine whether we want
+  // to print this enum as a bitfield. We're likely dealing with a bitfield if
+  // every enumrator is either a one bit value or a superset of the previous
+  // enumerators. Also 0 doesn't make sense when the enumerators are used as
+  // flags.
   for (auto enumerator : enum_decl->enumerators()) {
-    if (enumerator->getInitVal().getSExtValue() == enum_value) {
+    uint64_t val = enumerator->getInitVal().getSExtValue();
+    if (llvm::countPopulation(val) != 1 && (val & ~covered_bits) != 0)
+      can_be_bitfield = false;
+    covered_bits |= val;
+    ++num_enumerators;
+    if (val == enum_svalue) {
+      // Found an exact match, that's all we need to do.
       s->PutCString(enumerator->getNameAsString());
       return true;
     }
   }
-  // If we have gotten here we didn't get find the enumerator in the
-  // enum decl, so just print the integer.
-  s->Printf("%" PRIi64, enum_value);
+
+  // No exact match, but we don't think this is a bitfield. Print the value as
+  // decimal.
+  if (!can_be_bitfield) {
+    s->Printf("%" PRIi64, enum_svalue);
+    return true;
+  }
+
+  // Unsigned values make more sense for flags.
+  offset = byte_offset;
+  const uint64_t enum_uvalue = data.GetMaxU64Bitfield(
+      &offset, byte_size, bitfield_bit_size, bitfield_bit_offset);
+
+  uint64_t remaining_value = enum_uvalue;
+  std::vector<std::pair<uint64_t, llvm::StringRef>> values;
+  values.reserve(num_enumerators);
+  for (auto enumerator : enum_decl->enumerators())
+    if (auto val = enumerator->getInitVal().getZExtValue())
+      values.emplace_back(val, enumerator->getName());
+
+  // Sort in reverse order of the number of the population count,  so that in
+  // `enum {A, B, ALL = A|B }` we visit ALL first. Use a stable sort so that
+  // A | C where A is declared before C is displayed in this order.
+  std::stable_sort(values.begin(), values.end(), [](const auto &a, const auto &b) {
+        return llvm::countPopulation(a.first) > llvm::countPopulation(b.first);
+      });
+
+  for (const auto &val : values) {
+    if ((remaining_value & val.first) != val.first)
+      continue;
+    remaining_value &= ~val.first;
+    s->PutCString(val.second);
+    if (remaining_value)
+      s->PutCString(" | ");
+  }
+
+  // If there is a remainder that is not covered by the value, print it as hex.
+  if (remaining_value)
+    s->Printf("0x%" PRIx64, remaining_value);
+
   return true;
 }
 
 bool ClangASTContext::DumpTypeValue(
     lldb::opaque_compiler_type_t type, Stream *s, lldb::Format format,
     const DataExtractor &data, lldb::offset_t byte_offset, size_t byte_size,
     uint32_t bitfield_bit_size, uint32_t bitfield_bit_offset,
     ExecutionContextScope *exe_scope) {
   if (!type)
     return false;
   if (IsAggregateType(type)) {
     return false;
   } else {
     clang::QualType qual_type(GetQualType(type));
 
     const clang::Type::TypeClass type_class = qual_type->getTypeClass();
+
+    if (type_class == clang::Type::Elaborated) {
+      qual_type = llvm::cast<clang::ElaboratedType>(qual_type)->getNamedType();
+      return DumpTypeValue(qual_type.getAsOpaquePtr(), s, format, data, byte_offset, byte_size,
+                           bitfield_bit_size, bitfield_bit_offset, exe_scope);
+    }
+
     switch (type_class) {
     case clang::Type::Typedef: {
       clang::QualType typedef_qual_type =
           llvm::cast<clang::TypedefType>(qual_type)
               ->getDecl()
               ->getUnderlyingType();
       CompilerType typedef_clang_type(this, typedef_qual_type.getAsOpaquePtr());
       if (format == eFormatDefault)