[clang][Interp] Fix crash during InterpStack printing

[isuckatcs]

InterpStack is using an std::vector<> to track the ItemTypes. As a result, the new types are inserted
to the back of the std::vector<>, however dump() was reading the types from the front (the bottom
of the stack) and printing the value on the top of the stack.

This lead to a crash if the type on the bottom had a different type from the type on the top. E.g.:

Items: 2. Size: 40
0/8: 0
1/40: 0x5590cddc0460 {16, 16, 32}

The same method also miscalculated the offsets during printing the stack, which was a source of
incorrect stack dumps and future crashes.

This patch changes the order of iteration of the types and fixes the offset calculation.

As for testing the change, the issue is that it needs to be done as a unittest, however from
clang/unittests we don't have access to clang/lib, where Interp resides. Although the previous
implementation didn't have unittests either, so I'm not sure if we actually care that much or not.

[llvmbot]

@llvm/pr-subscribers-clang

Changes

InterpStack is using an std::vector<> to track the ItemTypes. As a result, the new types are inserted
to the back of the std::vector<>, however dump() was reading the types from the front (the bottom
of the stack) and printing the value on the top of the stack.

This lead to a crash if the type on the bottom had a different type from the type on the top. E.g.:

Items: 2. Size: 40
0/8: 0
1/40: 0x5590cddc0460 {16, 16, 32}

The same method also miscalculated the offsets during printing the stack, which was a source of
incorrect stack dumps and future crashes.

This patch changes the order of iteration of the types and fixes the offset calculation.

As for testing the change, the issue is that it needs to be done as a unittest, however from
clang/unittests we don't have access to clang/lib, where Interp resides. Although the previous
implementation didn't have unittests either, so I'm not sure if we actually care that much or not.

---

Full diff: https://github.com/llvm/llvm-project/pull/68246.diff

1 Files Affected:

• (modified) clang/lib/AST/Interp/InterpStack.cpp (+12-7)

diff --git a/clang/lib/AST/Interp/InterpStack.cpp b/clang/lib/AST/Interp/InterpStack.cpp
index da4b36f8c1bf351..b1e042c967d4241 100644
--- a/clang/lib/AST/Interp/InterpStack.cpp
+++ b/clang/lib/AST/Interp/InterpStack.cpp
@@ -85,20 +85,25 @@ void InterpStack::shrink(size_t Size) {
 
 void InterpStack::dump() const {
 #ifndef NDEBUG
-  llvm::errs() << "Items: " << ItemTypes.size() << ". Size: " << size() << "\n";
+  llvm::errs() << "Items: " << ItemTypes.size() << ". Size: " << size() << '\n';
   if (ItemTypes.empty())
     return;
 
   size_t Index = 0;
-  size_t Offset = align(primSize(ItemTypes[0]));
-  for (PrimType Ty : ItemTypes) {
-    llvm::errs() << Index << "/" << Offset << ": ";
-    TYPE_SWITCH(Ty, {
+  size_t Offset = 0;
+
+  // The type of the item on the top of the stack is inserted to the back
+  // of the vector, so the iteration has to happen backwards.
+  for (auto TyIt = ItemTypes.rbegin(); TyIt != ItemTypes.rend(); ++TyIt) {
+    Offset += align(primSize(*TyIt));
+
+    llvm::errs() << Index << '/' << Offset << ": ";
+    TYPE_SWITCH(*TyIt, {
       const T &V = peek<T>(Offset);
       llvm::errs() << V;
     });
-    llvm::errs() << "\n";
-    Offset += align(primSize(Ty));
+    llvm::errs() << '\n';
+
     ++Index;
   }
 #endif

[tbaederr]

Although the previous
implementation didn't have unittests either, so I'm not sure if we actually care that much or not.

We don't, I think I've used this function once. But I don't want to delete it either because it could come in handy in the future.

LGTM

[isuckatcs]

it could come in handy in the future

For me it was helpful to check what is on the stack at the moment, so it's definitely handy I think.