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

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

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

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

s/clabg/clang

source/Symbol/ClangASTContext.cpp

9529

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/

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

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$'])
+
         # 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,

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 {
+        A = 1 << 0,
+        B = 1 << 1,
+        C = 1 << 2,
+        AB = A | B,
+        ALL = A | B | C,
+    };
+
+    enum bitfield a = A, b = B, c = C, ab = AB, ac = A | C, all = ALL;
+    int nonsense = a + b + c + ab + ac + all; // Breakpoint for bitfield
+
     enum days {
         Monday = -3,
         Tuesday,
         Wednesday,
         Thursday,
         Friday,
         Saturday,
         Sunday,

source/Symbol/ClangASTContext.cpp

DumpDataExtractor(data, s, data_byte_offset, format, data_byte_size, 1,
                       bitfield_bit_offset);
 
     if (show_summary)
       DumpSummary(type, exe_ctx, s, data, data_byte_offset, data_byte_size);
     break;
   }
 }
 
+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);
+  lldb::offset_t offset = byte_offset;
+  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()) {
+    uint64_t val = enumerator->getInitVal().getSExtValue();
+    if ((llvm::countPopulation(val) != 1 && (val & ~covered_bits) != 0) ||
+        val == 0)
+      can_be_bitfield = false;
+    covered_bits |= val;
+    ++num_enumerators;
+    if (val == enum_svalue) {
+      s->PutCString(enumerator->getNameAsString());
+      return true;
+    }
+  }
+
+  // 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())
+    values.emplace_back(enumerator->getInitVal().getZExtValue(),
+                        enumerator->getName());
+
+  // Sort in reverse order of the number of set bits. This is so that
+  // in `enum {A, B, ALL = A|B }` we visit ALL first.
+  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)) {
case clang::Type::Typedef: {
                                // bitfield_bit_size != 0
           exe_scope);
     } break;
 
     case clang::Type::Enum:
       // If our format is enum or default, show the enumeration value as its
       // enumeration string value, else just display it as requested.
       if ((format == eFormatEnum || format == eFormatDefault) &&
-          GetCompleteType(type)) {
-        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;
-        if (is_signed) {
-          const int64_t enum_svalue = data.GetMaxS64Bitfield(
-              &offset, byte_size, bitfield_bit_size, bitfield_bit_offset);
-          for (enum_pos = enum_decl->enumerator_begin(),
-              enum_end_pos = enum_decl->enumerator_end();
-               enum_pos != enum_end_pos; ++enum_pos) {
-            if (enum_pos->getInitVal().getSExtValue() == enum_svalue) {
-              s->PutCString(enum_pos->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_svalue);
-        } else {
-          const uint64_t enum_uvalue = data.GetMaxU64Bitfield(
-              &offset, byte_size, bitfield_bit_size, bitfield_bit_offset);
-          for (enum_pos = enum_decl->enumerator_begin(),
-              enum_end_pos = enum_decl->enumerator_end();
-               enum_pos != enum_end_pos; ++enum_pos) {
-            if (enum_pos->getInitVal().getZExtValue() == enum_uvalue) {
-              s->PutCString(enum_pos->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("%" PRIu64, enum_uvalue);
-        }
-        return true;
-      }
+          GetCompleteType(type))
+        return DumpEnumValue(qual_type, s, data, byte_offset, byte_size,
+                             bitfield_bit_offset, bitfield_bit_size);
       // format was not enum, just fall through and dump the value as
       // requested....
       LLVM_FALLTHROUGH;
 
     default:
       // We are down to a scalar type that we just need to display.
       {
         uint32_t item_count = 1;