[clang] Improve Dumping of APValues

[Tyker]

Summary

this is required for proper testing of D63640

[Tyker]

remove unintended code.

[riccibruno]

Thanks for finishing this.

I don't see why you are adding dumpPretty; the point of the dump function I added is to dump the APValue as the tree-like structure it is. But your dumpPretty doesn't do that at all. Instead it is just an alias for printPretty. So dump does one thing and dumpPretty does another completely different thing.

clang/include/clang/AST/PrettyPrinter.h

222 ↗

(On Diff #282687)

This seems to be unrelated. And anyway shouldn't this be inferred from the language mode?

clang/lib/AST/APValue.cpp

433–434

What I would do here is continue to take a const ASTContext & here, but in the LValue case in TextNodeDumper just print <ASTContext required> if the context is null (which should only happens when debugging since the parameter-less version of dump() is not used in the code-base).

Trying to do without the context here can result in confusing output. I think it is better to just give-up and tell the user to please pass the ASTContext (which again, should only happens in a debugger).

Additionally since the context will always be present outside of debugging, the code-path without the context will not be tested.

630

What's up with this inconsistency? One time OS and the other time Out...

clang/test/AST/ast-dump-APValue-MemPtr.cpp

5

Can you add a TODO to remember to add the serialization part when the serialization of APValue is fixed?

Also can you match full lines? Here the test would still pass if you had written
// CHECK-NEXT: value: MemberPointer &S.

You can use utils/make-ast-dump-check.sh + some manual editing to generate the test.

[riccibruno]

clang/lib/AST/APValue.cpp

542

And also I don't think that this change is safe since ASTContext::getAsArrayType applies qualifiers from the array type to the element type.

[riccibruno]

clang/include/clang/AST/PrettyPrinter.h

222 ↗

(On Diff #282687)

My bad. It is inferred from the language mode.

[Tyker]

this a patch i had to improve dumping before your improvement to dumping.

I think there is benefits to being able to dump APValues like LValues without an ASTContext even if the output will not be as good. but i agree that now that the dumper has an ASTContext during ast-dump it is hard to test.

In D85144#2191440, @riccibruno wrote:

Thanks for finishing this.

I don't see why you are adding dumpPretty; the point of the dump function I added is to dump the APValue as the tree-like structure it is. But your dumpPretty doesn't do that at all.

the intent was to reuse the same printing code that is already used for diagnostics (printPretty) since it can print Member pointers and LValues. it isn't being used for printing anything that isn't a leaf(as in no child APValues) when used in AST dump. where you planning for LValues or Membre pointer to have children and not being leaf nodes ?

So dump does one thing and dumpPretty does another completely different thing.

i removed dumpPretty and instead added a new overload to printPretty.

[riccibruno]

I agree with you that it's fine to use printPretty for leaves (and additionally it would be annoying to duplicate the LValue case); that's what I was planning to do anyway.

What I am not sure I agree with is the additional complexity to handle the (debugger-only and easy to avoid) case where no context is given.

clang/lib/AST/APValue.cpp

542

This works because we only use ElemTy to distinguish between the array case and the base-or-member case right?

[Tyker]

In D85144#2205461, @riccibruno wrote:

I agree with you that it's fine to use printPretty for leaves (and additionally it would be annoying to duplicate the LValue case); that's what I was planning to do anyway.

What I am not sure I agree with is the additional complexity to handle the (debugger-only and easy to avoid) case where no context is given.

i don't have a strong opinion on this.
it adds complexity, isn't testable and in many situation where the ASTContext is missing the QualType is missing too anyway.
soo i removed that code path.

[rsmith]

Ideally we should be tree-dumping the LValue representation rather than pretty-printing it (and similarly for member pointers and label address differences). The problem with doing so is that we can't interpret the LValuePathEntrys without knowing the type of the lvalue base. However, we could fix that: LValuePathEntry has spare bits. As a result of PR8256, we require the size *in bits* of an array type to fit into 64 bits, so the maximum possible array index is 2^61 (and the pointer representation is a Decl*, which has 3 spare low-order bits). So we could track which kind of path entry it is with no additional storage cost, which would remove the need to have an ASTContext and a QualType in order to be able to correctly dump an APValue in LValue form.

clang/include/clang/AST/APValue.h

public:
   bool isUnion() const { return Kind == Union; }
   bool isMemberPointer() const { return Kind == MemberPointer; }
   bool isAddrLabelDiff() const { return Kind == AddrLabelDiff; }
 
   void dump() const;
   void dump(raw_ostream &OS, const ASTContext &Context) const;
 
   void printPretty(raw_ostream &OS, const ASTContext &Ctx, QualType Ty) const;
+  void printPretty(raw_ostream &OS, const ASTContext *Ctx, QualType Ty) const;
   std::string getAsString(const ASTContext &Ctx, QualType Ty) const;
 
   APSInt &getInt() {
     assert(isInt() && "Invalid accessor");
     return *(APSInt*)(char*)Data.buffer;
   }
   const APSInt &getInt() const {
     return const_cast<APValue*>(this)->getInt();

clang/lib/AST/APValue.cpp

static double GetApproxValue(const llvm::APFloat &F) {
   bool ignored;
   V.convert(llvm::APFloat::IEEEdouble(), llvm::APFloat::rmNearestTiesToEven,
             &ignored);
   return V.convertToDouble();
 }
 
 void APValue::printPretty(raw_ostream &Out, const ASTContext &Ctx,
                           QualType Ty) const {
+  printPretty(Out, &Ctx, Ty);
+}
+
+void APValue::printPretty(raw_ostream &Out, const ASTContext *Ctx,
+                          QualType Ty) const {
   switch (getKind()) {
   case APValue::None:
     Out << "<out of lifetime>";
     return;
   case APValue::Indeterminate:
     Out << "<uninitialized>";
     return;
   case APValue::Int:
void APValue::printPretty(raw_ostream &Out, const ASTContext *Ctx,
   }
   case APValue::ComplexInt:
     Out << getComplexIntReal() << "+" << getComplexIntImag() << "i";
     return;
   case APValue::ComplexFloat:
     Out << GetApproxValue(getComplexFloatReal()) << "+"
         << GetApproxValue(getComplexFloatImag()) << "i";
     return;
   case APValue::LValue: {
+    assert(Ctx);
     bool IsReference = Ty->isReferenceType();
     QualType InnerTy
       = IsReference ? Ty.getNonReferenceType() : Ty->getPointeeType();
     if (InnerTy.isNull())
       InnerTy = Ty;
 
     LValueBase Base = getLValueBase();
     if (!Base) {
       if (isNullPointer()) {
-        Out << (Ctx.getLangOpts().CPlusPlus11 ? "nullptr" : "0");
+        Out << (Ctx->getLangOpts().CPlusPlus11 ? "nullptr" : "0");
       } else if (IsReference) {
-        Out << "*(" << InnerTy.stream(Ctx.getPrintingPolicy()) << "*)"
+        Out << "*(" << InnerTy.stream(Ctx->getPrintingPolicy()) << "*)"
             << getLValueOffset().getQuantity();
       } else {
-        Out << "(" << Ty.stream(Ctx.getPrintingPolicy()) << ")"
+        Out << "(" << Ty.stream(Ctx->getPrintingPolicy()) << ")"
             << getLValueOffset().getQuantity();
       }
       return;
     }
 
     if (!hasLValuePath()) {
       // No lvalue path: just print the offset.
       CharUnits O = getLValueOffset();
-      CharUnits S = Ctx.getTypeSizeInChars(InnerTy);
+      CharUnits S = Ctx->getTypeSizeInChars(InnerTy);
       if (!O.isZero()) {
         if (IsReference)
           Out << "*(";
         if (O % S) {
           Out << "(char*)";
           S = CharUnits::One();
         }
         Out << '&';
       } else if (!IsReference) {
         Out << '&';
       }
 
       if (const ValueDecl *VD = Base.dyn_cast<const ValueDecl*>())
         Out << *VD;
       else if (TypeInfoLValue TI = Base.dyn_cast<TypeInfoLValue>()) {
-        TI.print(Out, Ctx.getPrintingPolicy());
+        TI.print(Out, Ctx->getPrintingPolicy());
       } else if (DynamicAllocLValue DA = Base.dyn_cast<DynamicAllocLValue>()) {
         Out << "{*new "
-            << Base.getDynamicAllocType().stream(Ctx.getPrintingPolicy()) << "#"
-            << DA.getIndex() << "}";
+            << Base.getDynamicAllocType().stream(Ctx->getPrintingPolicy())
+            << "#" << DA.getIndex() << "}";
       } else {
         assert(Base.get<const Expr *>() != nullptr &&
                "Expecting non-null Expr");
-        Base.get<const Expr*>()->printPretty(Out, nullptr,
-                                             Ctx.getPrintingPolicy());
+        Base.get<const Expr *>()->printPretty(Out, nullptr,
+                                              Ctx->getPrintingPolicy());
       }
 
       if (!O.isZero()) {
         Out << " + " << (O / S);
         if (IsReference)
           Out << ')';
       }
       return;
     }
 
     // We have an lvalue path. Print it out nicely.
     if (!IsReference)
       Out << '&';
     else if (isLValueOnePastTheEnd())
       Out << "*(&";
 
     QualType ElemTy;
     if (const ValueDecl *VD = Base.dyn_cast<const ValueDecl*>()) {
       Out << *VD;
       ElemTy = VD->getType();
     } else if (TypeInfoLValue TI = Base.dyn_cast<TypeInfoLValue>()) {
-      TI.print(Out, Ctx.getPrintingPolicy());
+      TI.print(Out, Ctx->getPrintingPolicy());
       ElemTy = Base.getTypeInfoType();
     } else if (DynamicAllocLValue DA = Base.dyn_cast<DynamicAllocLValue>()) {
       Out << "{*new "
-          << Base.getDynamicAllocType().stream(Ctx.getPrintingPolicy()) << "#"
+          << Base.getDynamicAllocType().stream(Ctx->getPrintingPolicy()) << "#"
           << DA.getIndex() << "}";
       ElemTy = Base.getDynamicAllocType();
     } else {
       const Expr *E = Base.get<const Expr*>();
       assert(E != nullptr && "Expecting non-null Expr");
-      E->printPretty(Out, nullptr, Ctx.getPrintingPolicy());
+      E->printPretty(Out, nullptr, Ctx->getPrintingPolicy());
       // FIXME: This is wrong if E is a MaterializeTemporaryExpr with an lvalue
       // adjustment.
       ElemTy = E->getType();
     }
 
     ArrayRef<LValuePathEntry> Path = getLValuePath();
     const CXXRecordDecl *CastToBase = nullptr;
     for (unsigned I = 0, N = Path.size(); I != N; ++I) {
       if (ElemTy->getAs<RecordType>()) {
         // The lvalue refers to a class type, so the next path entry is a base
         // or member.
         const Decl *BaseOrMember = Path[I].getAsBaseOrMember().getPointer();
         if (const CXXRecordDecl *RD = dyn_cast<CXXRecordDecl>(BaseOrMember)) {
           CastToBase = RD;
-          ElemTy = Ctx.getRecordType(RD);
+          ElemTy = Ctx->getRecordType(RD);
         } else {
           const ValueDecl *VD = cast<ValueDecl>(BaseOrMember);
           Out << ".";
           if (CastToBase)
             Out << *CastToBase << "::";
           Out << *VD;
           ElemTy = VD->getType();
         }
       } else {
         // The lvalue must refer to an array.
         Out << '[' << Path[I].getAsArrayIndex() << ']';
-        ElemTy = Ctx.getAsArrayType(ElemTy)->getElementType();
+        ElemTy = Ctx->getAsArrayType(ElemTy)->getElementType();
       }
     }
 
     // Handle formatting of one-past-the-end lvalues.
     if (isLValueOnePastTheEnd()) {
       // FIXME: If CastToBase is non-0, we should prefix the output with
       // "(CastToBase*)".
       Out << " + 1";
       if (IsReference)
         Out << ')';
     }
     return;
   }
   case APValue::Array: {
-    const ArrayType *AT = Ctx.getAsArrayType(Ty);
+    assert(Ctx);
+    const ArrayType *AT = Ctx->getAsArrayType(Ty);
     QualType ElemTy = AT->getElementType();
     Out << '{';
     if (unsigned N = getArrayInitializedElts()) {
       getArrayInitializedElt(0).printPretty(Out, Ctx, ElemTy);
       for (unsigned I = 1; I != N; ++I) {
         Out << ", ";
         if (I == 10) {
           // Avoid printing out the entire contents of large arrays.
case APValue::AddrLabelDiff:
     Out << "&&" << getAddrLabelDiffRHS()->getLabel()->getName();
     return;
   }
   llvm_unreachable("Unknown APValue kind!");
 }
 
 std::string APValue::getAsString(const ASTContext &Ctx, QualType Ty) const {
   std::string Result;
   llvm::raw_string_ostream Out(Result);
   printPretty(Out, Ctx, Ty);
   Out.flush();
   return Result;
 }
 
 bool APValue::toIntegralConstant(APSInt &Result, QualType SrcTy,
                                  const ASTContext &Ctx) const {
   if (isInt()) {

clang/lib/AST/TextNodeDumper.cpp

case APValue::ComplexFloat:
     {
       ColorScope Color(OS, ShowColors, ValueColor);
       OS << GetApproxValue(Value.getComplexFloatReal()) << " + "
          << GetApproxValue(Value.getComplexFloatImag()) << 'i';
     }
     return;
   case APValue::LValue:
     (void)Context;
-    OS << "LValue <todo>";
+    OS << "LValue ";
+    if (Context)
+      Value.printPretty(OS, Context, Ty);
+    else
+      OS << "<ASTContext required>";
     return;
   case APValue::Array: {
     unsigned ArraySize = Value.getArraySize();
     unsigned NumInitializedElements = Value.getArrayInitializedElts();
     OS << "Array size=" << ArraySize;
 
     dumpAPValueChildren(
         Value, Ty,
if (isSimpleAPValue(UnionValue)) {
       Visit(UnionValue, Ty);
     } else {
       AddChild([=] { Visit(UnionValue, Ty); });
     }
 
     return;
   }
   case APValue::MemberPointer:
-    OS << "MemberPointer <todo>";
+    OS << "MemberPointer ";
+    Value.printPretty(OS, Context, Ty);
     return;
   case APValue::AddrLabelDiff:
-    OS << "AddrLabelDiff <todo>";
+    OS << "AddrLabelDiff ";
+    Value.printPretty(OS, Context, Ty);
     return;
   }
   llvm_unreachable("Unknown APValue kind!");
 }
 
 void TextNodeDumper::dumpPointer(const void *Ptr) {
   ColorScope Color(OS, ShowColors, AddressColor);
   OS << ' ' << Ptr;

clang/test/AST/ast-dump-APValue-LValue.cpp

+// Test without serialization:
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -Wno-unused-value -std=gnu++17 \
+// RUN:            -ast-dump %s -ast-dump-filter Test \
+// RUN: | FileCheck --strict-whitespace %s
+
+int i;
+
+void Test() {
+  constexpr int *pi = &i;
+  // CHECK:  VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} pi 'int *const' constexpr cinit
+  // CHECK-NEXT:  |-value: LValue &i
+}

clang/test/AST/ast-dump-APValue-MemPtr.cpp

+// Test without serialization:
+// RUN: %clang_cc1 -triple x86_64-unknown-unknown -Wno-unused-value -std=gnu++17 \
+// RUN:            -ast-dump %s -ast-dump-filter Test \
+// RUN: | FileCheck --strict-whitespace %s
+
+int i;
+struct S {
+  int i;
+};
+
+void Test() {
+  constexpr int(S::*pmi) = &S::i;
+  // CHECK:    VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} pmi 'int (S::*const)' constexpr cinit
+  // CHECK-NEXT:  value: MemberPointer &S::i
+}

clang/test/AST/ast-dump-APValue-todo.cpp

-// Test without serialization:
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown -Wno-unused-value -std=gnu++17 \
-// RUN:            -ast-dump %s -ast-dump-filter Test \
-// RUN: | FileCheck --strict-whitespace --match-full-lines %s
-//
-// Test with serialization:
-// RUN: %clang_cc1 -triple x86_64-unknown-unknown -Wno-unused-value -std=gnu++17 -emit-pch -o %t %s
-// RUN: %clang_cc1 -x c++ -triple x86_64-unknown-unknown -Wno-unused-value -std=gnu++17 \
-// RUN:           -include-pch %t -ast-dump-all -ast-dump-filter Test /dev/null \
-// RUN: | sed -e "s/ <undeserialized declarations>//" -e "s/ imported//" \
-// RUN: | FileCheck --strict-whitespace --match-full-lines %s
-
-int i;
-struct S {
-  int i;
-};
-
-void Test() {
-  constexpr int *pi = &i;
-  // CHECK:  | `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} pi 'int *const' constexpr cinit
-  // CHECK-NEXT:  |   |-value: LValue <todo>
-
-  constexpr int(S::*pmi) = &S::i;
-  // CHECK:    `-VarDecl {{.*}} <col:{{.*}}, col:{{.*}}> col:{{.*}} pmi 'int (S::*const)' constexpr cinit
-  // CHECK-NEXT:      |-value: MemberPointer <todo>
-}