[clang][Interp] Record item types in InterpStack

[tbaeder]

Summary

Since I've run into this a few times now...

When push()ing something on the stack, we need to later pop() the correct type of value, otherwise we run into problems.

This s just an idea and I've disabled it if NDEBUG is defined. The current tests all still work though, which is good.

[erichkeane]

Can you clarify what the intent of this patch is? Perhaps I'm just being slow today, but I don't really get the intent here.

clang/lib/AST/Interp/InterpStack.h

133

Each of these ALSO need to be 'if constexpr' I think.

159

llvm_unreachable?

[tbaeder]

In D133941#3792330, @erichkeane wrote:

Can you clarify what the intent of this patch is? Perhaps I'm just being slow today, but I don't really get the intent here.

Consider:

push<Pointer>(...);
(lots of stuff)
pop<int>();

currently this would just work and give you an integer, but they value wouldn't make any sense. This patch would assert here since the value on the stack is not an integer, it's a pointer.
It basically adds a bit of type-safety back.

[junaire]

Do you think std::is_same_v will be even better?

[tbaeder]

In D133941#3794441, @junaire wrote:

Do you think std::is_same_v will be even better?

I don't really have a preference.

[erichkeane]

is_same_v would be an improvement, but otherwise I think this is fine.

clang/lib/AST/Interp/InterpStack.h

137

Side note: Having Integral's 2nd template parameter be a bool here is a bad design decision IMO. It should probably be an enum, so that we can just do:

Integral<8, Signed> or Integral<8, Unsigned> kinda thing.

clang/lib/AST/Interp/InterpStack.h

 //===--- InterpStack.h - Stack implementation for the VM --------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //
 // Defines the upwards-growing stack used by the interpreter.
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_CLANG_AST_INTERP_INTERPSTACK_H
 #define LLVM_CLANG_AST_INTERP_INTERPSTACK_H
 
+#include "PrimType.h"
 #include <memory>
+#include <vector>
 
 namespace clang {
 namespace interp {
 
 /// Stack frame storing temporaries and parameters.
 class InterpStack final {
 public:
   InterpStack() {}
 
   /// Destroys the stack, freeing up storage.
   ~InterpStack();
 
   /// Constructs a value in place on the top of the stack.
   template <typename T, typename... Tys> void push(Tys &&... Args) {
     new (grow(aligned_size<T>())) T(std::forward<Tys>(Args)...);
+#ifndef NDEBUG
+    ItemTypes.push_back(toPrimType<T>());
+#endif
   }
 
   /// Returns the value from the top of the stack and removes it.
   template <typename T> T pop() {
+#ifndef NDEBUG
+    assert(!ItemTypes.empty());
+    assert(ItemTypes.back() == toPrimType<T>());
+    ItemTypes.pop_back();
+#endif
     auto *Ptr = &peek<T>();
     auto Value = std::move(*Ptr);
     Ptr->~T();
     shrink(aligned_size<T>());
     return Value;
   }
 
   /// Discards the top value from the stack.
   template <typename T> void discard() {
+#ifndef NDEBUG
+    assert(ItemTypes.back() == toPrimType<T>());
+    ItemTypes.pop_back();
+#endif
     auto *Ptr = &peek<T>();
     Ptr->~T();
     shrink(aligned_size<T>());
   }
 
   /// Returns a reference to the value on the top of the stack.
   template <typename T> T &peek() const {
     return *reinterpret_cast<T *>(peek(aligned_size<T>()));
struct StackChunk {
     }
   };
   static_assert(sizeof(StackChunk) < ChunkSize, "Invalid chunk size");
 
   /// First chunk on the stack.
   StackChunk *Chunk = nullptr;
   /// Total size of the stack.
   size_t StackSize = 0;
+
+#ifndef NDEBUG
+  /// vector recording the type of data we pushed into the stack.
+  std::vector<PrimType> ItemTypes;
+
+  template <typename T> static constexpr PrimType toPrimType() {
+    if constexpr (std::is_same_v<T, Pointer>)
+      return PT_Ptr;
+    else if constexpr (std::is_same_v<T, bool> ||
+                       std::is_same_v<T, Boolean>)
+      return PT_Bool;
+    else if constexpr (std::is_same_v<T, int8_t> ||
+                       std::is_same_v<T, Integral<8, true>>)
+      return PT_Sint8;
+    else if constexpr (std::is_same_v<T, uint8_t> ||
+                       std::is_same_v<T, Integral<8, false>>)
+      return PT_Uint8;
+    else if constexpr (std::is_same_v<T, int16_t> ||
+                       std::is_same_v<T, Integral<16, true>>)
+      return PT_Sint16;
+    else if constexpr (std::is_same_v<T, uint16_t> ||
+                       std::is_same_v<T, Integral<16, false>>)
+      return PT_Uint16;
+    else if constexpr (std::is_same_v<T, int32_t> ||
+                       std::is_same_v<T, Integral<32, true>>)
+      return PT_Sint32;
+    else if constexpr (std::is_same_v<T, uint32_t> ||
+                       std::is_same_v<T, Integral<32, false>>)
+      return PT_Uint32;
+    else if constexpr (std::is_same_v<T, int64_t> ||
+                       std::is_same_v<T, Integral<64, true>>)
+      return PT_Sint64;
+    else if constexpr (std::is_same_v<T, uint64_t> ||
+                       std::is_same_v<T, Integral<64, false>>)
+      return PT_Uint64;
+
+    llvm_unreachable("unknown type push()'ed into InterpStack");
+  }
+#endif
 };
 
 } // namespace interp
 } // namespace clang
 
 #endif