Page MenuHomePhabricator

Add pretty printing of Clang "bitfield" enums
ClosedPublic

Authored by friss on Sep 12 2019, 1:59 PM.

Details

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.

Diff Detail

Event Timeline

friss created this revision.Sep 12 2019, 1:59 PM

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...

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.

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...

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 added inline comments.
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 added a comment.Mon, Sep 16, 8:23 AM

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 updated this revision to Diff 223702.Mon, Oct 7, 5:16 PM

Added a couple more tests

friss marked 3 inline comments as done.Mon, Oct 7, 5:21 PM

(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 updated this revision to Diff 223705.Mon, Oct 7, 5:27 PM
friss marked an inline comment as done.

Add comments to some tests.

Harbormaster completed remote builds in B39131: Diff 223705.
labath accepted this revision.Tue, Oct 8, 5:14 AM

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.

This revision is now accepted and ready to land.Tue, Oct 8, 5:14 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptTue, Oct 8, 8:36 AM

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.