This is an archive of the discontinued LLVM Phabricator instance.

[clang] Rework how and when APValues are dumped
ClosedPublic

Authored by riccibruno on Jul 5 2020, 3:21 PM.

Details

Summary

Currently APValues are dumped as a single string. This becomes quickly completely
unreadable since APValue is a tree-like structure. Even a simple example is not pretty:

struct S { int arr[4]; float f; };
constexpr S s = { .arr = {1,2}, .f = 3.1415f };
// Struct  fields: Array: Int: 1, Int: 2, 2 x Int: 0, Float: 3.141500e+00

With this patch this becomes:

-Struct
 |-field: Array size=4
 | |-elements: Int 1, Int 2
 | `-filler: 2 x Int 0
 `-field: Float 3.141500e+00

Additionally APValues are currently only dumped as part of visiting a ConstantExpr.
This patch also dump the value of the initializer of constexpr variable declarations:

constexpr int foo(int a, int b) { return a + b - 42; }
constexpr int a = 1, b = 2;
constexpr int c = foo(a, b) > 0 ? foo(a, b) : foo(b, a);
// VarDecl 0x62100008aec8 <col:3, col:57> col:17 c 'const int' constexpr cinit
// |-value: Int -39
// `-ConditionalOperator 0x62100008b4d0 <col:21, col:57> 'int'
// <snip>

Do the above by moving the dump functions to TextNodeDumper which already has
the machinery to display trees. The cases APValue::LValue, APValue::MemberPointer
and APValue::AddrLabelDiff are left as they were before (unimplemented).

We try to display multiple elements on the same line if they are considered to be "simple".
This is to avoid wasting large amounts of vertical space in an example like:

constexpr int arr[8] = {0,1,2,3,4,5,6,7};
// VarDecl 0x62100008bb78 <col:3, col:42> col:17 arr 'int const[8]' constexpr cinit
// |-value: Array size=8
// | |-elements: Int 0, Int 1, Int 2, Int 3
// | `-elements: Int 4, Int 5, Int 6, Int 7

Diff Detail

Event Timeline

riccibruno created this revision.Jul 5 2020, 3:21 PM
riccibruno edited the summary of this revision. (Show Details)Jul 5 2020, 4:42 PM

Do none of the JSON tests break from this change?

clang/include/clang/AST/TextNodeDumper.h
163

This is a pretty strange public method; any way to limit its visibility?

clang/test/Import/switch-stmt/test.cpp
8

I sort of wonder whether we want both the text and the JSON dumpers to dump these as: value(type): <val>, as that seems like it produces results that are a bit more well-structured. WDYT?

riccibruno updated this revision to Diff 275713.Jul 6 2020, 7:50 AM

Format the tests and make them more relevant.

riccibruno marked 2 inline comments as done.Jul 6 2020, 8:24 AM

Thanks for your comments!

Do none of the JSON tests break from this change?

No, but only because I am not modifying the JSON output at all (the JSON output is still from APValue::printPretty).

clang/include/clang/AST/TextNodeDumper.h
163

I just need it for dumpAPValueChildren, but I can make dumpAPValueChildren a private method of TextNodeDumper instead.

clang/test/Import/switch-stmt/test.cpp
8

I'm not sure I follow. The value is just a label for the child of the VarDecl.
If you look at a more complex example such as:

VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} s4 'const S4'
|-value: Struct
| |-base: Struct
| | `-fields: Int 0, Union .j Int 0
| |-fields: Int 1, Int 2, Int 3
| |-field: Struct
| `-fields: Int 4, Int 5, Int 6

There is no other value label.

Make dumpAPValueChildren a private member function and remove TextNodeDumper::getOS.

riccibruno marked an inline comment as done.Jul 6 2020, 12:08 PM
aaron.ballman accepted this revision.Jul 6 2020, 12:45 PM

LGTM!

clang/test/Import/switch-stmt/test.cpp
8

Ah, I was misunderstanding the ouput.

This revision is now accepted and ready to land.Jul 6 2020, 12:45 PM
This revision was automatically updated to reflect the committed changes.