Validate that Symbols are all part of the same program

Assert in each AST constructor that symbols belong to the program of the parent.

Bug: tint:709
Change-Id: I82ae9b23c88e89714a44e057a0272f0293385aaf
Reviewed-on: https://dawn-review.googlesource.com/c/tint/+/47624
Commit-Queue: Ben Clayton <bclayton@chromium.org>
Kokoro: Kokoro <noreply+kokoro@google.com>
Reviewed-by: James Price <jrprice@google.com>
Reviewed-by: Antonio Maiorano <amaiorano@google.com>

src/ast/function.cc

@@ -38,6 +38,7 @@ Function::Function(ProgramID program_id,
       body_(body),
       decorations_(std::move(decorations)),
       return_type_decorations_(std::move(return_type_decorations)) {
+  TINT_ASSERT_PROGRAM_IDS_EQUAL_IF_VALID(symbol_, program_id);
   TINT_ASSERT_PROGRAM_IDS_EQUAL_IF_VALID(body, program_id);
   for (auto* param : params_) {
     TINT_ASSERT(param && param->is_const());

src/ast/function_test.cc

@@ -79,6 +79,16 @@ TEST_F(FunctionTest, Assert_Null_Param) {
       "internal compiler error");
 }
 
+TEST_F(FunctionTest, Assert_DifferentProgramID_Symbol) {
+  EXPECT_FATAL_FAILURE(
+      {
+        ProgramBuilder b1;
+        ProgramBuilder b2;
+        b1.Func(b2.Sym("func"), VariableList{}, b1.ty.void_(), StatementList{});
+      },
+      "internal compiler error");
+}
+
 TEST_F(FunctionTest, Assert_DifferentProgramID_Param) {
   EXPECT_FATAL_FAILURE(
       {

src/ast/identifier_expression.cc

@@ -25,6 +25,7 @@ IdentifierExpression::IdentifierExpression(ProgramID program_id,
                                            const Source& source,
                                            Symbol sym)
     : Base(program_id, source), sym_(sym) {
+  TINT_ASSERT_PROGRAM_IDS_EQUAL_IF_VALID(sym_, program_id);
   TINT_ASSERT(sym_.IsValid());
 }
 

src/ast/identifier_expression_test.cc

@@ -23,12 +23,12 @@ using IdentifierExpressionTest = TestHelper;
 
 TEST_F(IdentifierExpressionTest, Creation) {
   auto* i = Expr("ident");
-  EXPECT_EQ(i->symbol(), Symbol(1));
+  EXPECT_EQ(i->symbol(), Symbol(1, ID()));
 }
 
 TEST_F(IdentifierExpressionTest, Creation_WithSource) {
   auto* i = Expr(Source{Source::Location{20, 2}}, "ident");
-  EXPECT_EQ(i->symbol(), Symbol(1));
+  EXPECT_EQ(i->symbol(), Symbol(1, ID()));
 
   auto src = i->source();
   EXPECT_EQ(src.range.begin.line, 20u);
@@ -49,6 +49,16 @@ TEST_F(IdentifierExpressionTest, Assert_InvalidSymbol) {
       "internal compiler error");
 }
 
+TEST_F(IdentifierExpressionTest, Assert_DifferentProgramID_Symbol) {
+  EXPECT_FATAL_FAILURE(
+      {
+        ProgramBuilder b1;
+        ProgramBuilder b2;
+        b1.Expr(b2.Sym(""));
+      },
+      "internal compiler error");
+}
+
 TEST_F(IdentifierExpressionTest, ToStr) {
   auto* i = Expr("ident");
   EXPECT_EQ(str(i), R"(Identifier[not set]{ident}

src/ast/struct_member.cc

@@ -32,6 +32,7 @@ StructMember::StructMember(ProgramID program_id,
       decorations_(std::move(decorations)) {
   TINT_ASSERT(type);
   TINT_ASSERT(symbol_.IsValid());
+  TINT_ASSERT_PROGRAM_IDS_EQUAL_IF_VALID(symbol_, program_id);
   for (auto* deco : decorations_) {
     TINT_ASSERT(deco);
     TINT_ASSERT_PROGRAM_IDS_EQUAL_IF_VALID(deco, program_id);

src/ast/struct_member_test.cc

@@ -23,7 +23,7 @@ using StructMemberTest = TestHelper;
 
 TEST_F(StructMemberTest, Creation) {
   auto* st = Member("a", ty.i32(), {MemberSize(4)});
-  EXPECT_EQ(st->symbol(), Symbol(1));
+  EXPECT_EQ(st->symbol(), Symbol(1, ID()));
   EXPECT_EQ(st->type(), ty.i32());
   EXPECT_EQ(st->decorations().size(), 1u);
   EXPECT_TRUE(st->decorations()[0]->Is<StructMemberSizeDecoration>());
@@ -37,7 +37,7 @@ TEST_F(StructMemberTest, CreationWithSource) {
   auto* st = Member(
       Source{Source::Range{Source::Location{27, 4}, Source::Location{27, 8}}},
       "a", ty.i32());
-  EXPECT_EQ(st->symbol(), Symbol(1));
+  EXPECT_EQ(st->symbol(), Symbol(1, ID()));
   EXPECT_EQ(st->type(), ty.i32());
   EXPECT_EQ(st->decorations().size(), 0u);
   EXPECT_EQ(st->source().range.begin.line, 27u);
@@ -73,6 +73,16 @@ TEST_F(StructMemberTest, Assert_Null_Decoration) {
       "internal compiler error");
 }
 
+TEST_F(StructMemberTest, Assert_DifferentProgramID_Symbol) {
+  EXPECT_FATAL_FAILURE(
+      {
+        ProgramBuilder b1;
+        ProgramBuilder b2;
+        b1.Member(b2.Sym("a"), b1.ty.i32(), {b1.MemberSize(4)});
+      },
+      "internal compiler error");
+}
+
 TEST_F(StructMemberTest, Assert_DifferentProgramID_Decoration) {
   EXPECT_FATAL_FAILURE(
       {

src/ast/variable.cc

@@ -39,6 +39,7 @@ Variable::Variable(ProgramID program_id,
       decorations_(std::move(decorations)),
       declared_storage_class_(declared_storage_class) {
   TINT_ASSERT(symbol_.IsValid());
+  TINT_ASSERT_PROGRAM_IDS_EQUAL_IF_VALID(symbol_, program_id);
   // no type means we must have a constructor to infer it
   TINT_ASSERT(declared_type_ || constructor);
   TINT_ASSERT_PROGRAM_IDS_EQUAL_IF_VALID(constructor, program_id);

src/ast/variable_test.cc

@@ -25,7 +25,7 @@ using VariableTest = TestHelper;
 TEST_F(VariableTest, Creation) {
   auto* v = Var("my_var", ty.i32(), StorageClass::kFunction);
 
-  EXPECT_EQ(v->symbol(), Symbol(1));
+  EXPECT_EQ(v->symbol(), Symbol(1, ID()));
   EXPECT_EQ(v->declared_storage_class(), StorageClass::kFunction);
   EXPECT_EQ(v->declared_type(), ty.i32());
   EXPECT_EQ(v->source().range.begin.line, 0u);
@@ -39,7 +39,7 @@ TEST_F(VariableTest, CreationWithSource) {
       Source{Source::Range{Source::Location{27, 4}, Source::Location{27, 5}}},
       "i", ty.f32(), StorageClass::kPrivate, nullptr, DecorationList{});
 
-  EXPECT_EQ(v->symbol(), Symbol(1));
+  EXPECT_EQ(v->symbol(), Symbol(1, ID()));
   EXPECT_EQ(v->declared_storage_class(), StorageClass::kPrivate);
   EXPECT_EQ(v->declared_type(), ty.f32());
   EXPECT_EQ(v->source().range.begin.line, 27u);
@@ -53,7 +53,7 @@ TEST_F(VariableTest, CreationEmpty) {
       Source{Source::Range{Source::Location{27, 4}, Source::Location{27, 7}}},
       "a_var", ty.i32(), StorageClass::kWorkgroup, nullptr, DecorationList{});
 
-  EXPECT_EQ(v->symbol(), Symbol(1));
+  EXPECT_EQ(v->symbol(), Symbol(1, ID()));
   EXPECT_EQ(v->declared_storage_class(), StorageClass::kWorkgroup);
   EXPECT_EQ(v->declared_type(), ty.i32());
   EXPECT_EQ(v->source().range.begin.line, 27u);
@@ -80,6 +80,16 @@ TEST_F(VariableTest, Assert_Null_Type) {
       "internal compiler error");
 }
 
+TEST_F(VariableTest, Assert_DifferentProgramID_Symbol) {
+  EXPECT_FATAL_FAILURE(
+      {
+        ProgramBuilder b1;
+        ProgramBuilder b2;
+        b1.Var(b2.Sym("x"), b1.ty.f32(), StorageClass::kNone);
+      },
+      "internal compiler error");
+}
+
 TEST_F(VariableTest, Assert_DifferentProgramID_Constructor) {
   EXPECT_FATAL_FAILURE(
       {

src/demangler.cc

@@ -50,7 +50,7 @@ std::string Demangler::Demangle(const SymbolTable& symbols,
     auto len = end_idx - start_idx;
 
     auto id = str.substr(start_idx, len);
-    Symbol sym(std::stoi(id));
+    Symbol sym(std::stoi(id), symbols.ProgramID());
     out << symbols.NameFor(sym);
 
     pos = end_idx;

src/demangler_test.cc

@@ -23,7 +23,7 @@ namespace {
 using DemanglerTest = testing::Test;
 
 TEST_F(DemanglerTest, NoSymbols) {
-  SymbolTable t;
+  SymbolTable t{ProgramID::New()};
   t.Register("sym1");
 
   Demangler d;
@@ -31,7 +31,7 @@ TEST_F(DemanglerTest, NoSymbols) {
 }
 
 TEST_F(DemanglerTest, Symbol) {
-  SymbolTable t;
+  SymbolTable t{ProgramID::New()};
   t.Register("sym1");
 
   Demangler d;
@@ -39,7 +39,7 @@ TEST_F(DemanglerTest, Symbol) {
 }
 
 TEST_F(DemanglerTest, MultipleSymbols) {
-  SymbolTable t;
+  SymbolTable t{ProgramID::New()};
   t.Register("sym1");
   t.Register("sym2");
 

src/program.h

@@ -165,7 +165,7 @@ class Program {
   SemNodeAllocator sem_nodes_;
   ast::Module* ast_ = nullptr;
   semantic::Info sem_;
-  SymbolTable symbols_;
+  SymbolTable symbols_{id_};
   diag::List diagnostics_;
   bool is_valid_ = false;  // Not valid until it is built
   bool moved_ = false;

src/program_builder.h

@@ -1435,7 +1435,7 @@ class ProgramBuilder {
   SemNodeAllocator sem_nodes_;
   ast::Module* ast_;
   semantic::Info sem_;
-  SymbolTable symbols_;
+  SymbolTable symbols_{id_};
   diag::List diagnostics_;
 
   /// The source to use when creating AST nodes without providing a Source as

src/scope_stack_test.cc

@@ -23,7 +23,7 @@ class ScopeStackTest : public ProgramBuilder, public testing::Test {};
 
 TEST_F(ScopeStackTest, Global) {
   ScopeStack<uint32_t> s;
-  Symbol sym(1);
+  Symbol sym(1, ID());
   s.set_global(sym, 5);
 
   uint32_t val = 0;
@@ -43,7 +43,7 @@ TEST_F(ScopeStackTest, Global_SetWithPointer) {
 
 TEST_F(ScopeStackTest, Global_CanNotPop) {
   ScopeStack<uint32_t> s;
-  Symbol sym(1);
+  Symbol sym(1, ID());
   s.set_global(sym, 5);
   s.pop_scope();
 
@@ -54,7 +54,7 @@ TEST_F(ScopeStackTest, Global_CanNotPop) {
 
 TEST_F(ScopeStackTest, Scope) {
   ScopeStack<uint32_t> s;
-  Symbol sym(1);
+  Symbol sym(1, ID());
   s.push_scope();
   s.set(sym, 5);
 
@@ -65,7 +65,7 @@ TEST_F(ScopeStackTest, Scope) {
 
 TEST_F(ScopeStackTest, Get_MissingSymbol) {
   ScopeStack<uint32_t> s;
-  Symbol sym(1);
+  Symbol sym(1, ID());
   uint32_t ret = 0;
   EXPECT_FALSE(s.get(sym, &ret));
   EXPECT_EQ(ret, 0u);
@@ -73,8 +73,8 @@ TEST_F(ScopeStackTest, Get_MissingSymbol) {
 
 TEST_F(ScopeStackTest, Has) {
   ScopeStack<uint32_t> s;
-  Symbol sym(1);
-  Symbol sym2(2);
+  Symbol sym(1, ID());
+  Symbol sym2(2, ID());
   s.set_global(sym2, 3);
   s.push_scope();
   s.set(sym, 5);
@@ -85,7 +85,7 @@ TEST_F(ScopeStackTest, Has) {
 
 TEST_F(ScopeStackTest, ReturnsScopeBeforeGlobalFirst) {
   ScopeStack<uint32_t> s;
-  Symbol sym(1);
+  Symbol sym(1, ID());
   s.set_global(sym, 3);
   s.push_scope();
   s.set(sym, 5);

src/semantic/sem_call_target.cc

@@ -67,7 +67,7 @@ const char* str(Parameter::Usage usage) {
 }
 
 std::ostream& operator<<(std::ostream& out, Parameter parameter) {
-  out << "[type: " << parameter.type->FriendlyName(SymbolTable{})
+  out << "[type: " << parameter.type->FriendlyName(SymbolTable{ProgramID{}})
       << ", usage: " << str(parameter.usage) << "]";
   return out;
 }

src/symbol.cc

@@ -18,7 +18,8 @@ namespace tint {
 
 Symbol::Symbol() = default;
 
-Symbol::Symbol(uint32_t val) : val_(val) {}
+Symbol::Symbol(uint32_t val, tint::ProgramID program_id)
+    : val_(val), program_id_(program_id) {}
 
 Symbol::Symbol(const Symbol& o) = default;
 
@@ -31,6 +32,7 @@ Symbol& Symbol::operator=(const Symbol& o) = default;
 Symbol& Symbol::operator=(Symbol&& o) = default;
 
 bool Symbol::operator==(const Symbol& other) const {
+  TINT_ASSERT_PROGRAM_IDS_EQUAL_IF_VALID(program_id_, other.program_id_);
   return val_ == other.val_;
 }
 

src/symbol.h

@@ -17,6 +17,8 @@
 
 #include <string>
 
+#include "src/program_id.h"
+
 namespace tint {
 
 /// A symbol representing a string in the system
@@ -27,7 +29,8 @@ class Symbol {
   Symbol();
   /// Constructor
   /// @param val the symbol value
-  explicit Symbol(uint32_t val);
+  /// @param program_id the identifier of the program that owns this Symbol
+  Symbol(uint32_t val, tint::ProgramID program_id);
   /// Copy constructor
   /// @param o the symbol to copy
   Symbol(const Symbol& o);
@@ -61,10 +64,20 @@ class Symbol {
   /// @return the string representation of the symbol
   std::string to_str() const;
 
+  /// @returns the identifier of the Program that owns this symbol.
+  tint::ProgramID ProgramID() const { return program_id_; }
+
  private:
   uint32_t val_ = static_cast<uint32_t>(-1);
+  tint::ProgramID program_id_;
 };
 
+/// @param sym the Symbol
+/// @returns the ProgramID that owns the given Symbol
+inline ProgramID ProgramIDOf(Symbol sym) {
+  return sym.IsValid() ? sym.ProgramID() : ProgramID();
+}
+
 }  // namespace tint
 
 namespace std {

src/symbol_table.cc

@@ -14,9 +14,12 @@
 
 #include "src/symbol_table.h"
 
+#include "src/debug.h"
+
 namespace tint {
 
-SymbolTable::SymbolTable() = default;
+SymbolTable::SymbolTable(tint::ProgramID program_id)
+    : program_id_(program_id) {}
 
 SymbolTable::SymbolTable(const SymbolTable&) = default;
 
@@ -36,7 +39,7 @@ Symbol SymbolTable::Register(const std::string& name) {
   if (it != name_to_symbol_.end())
     return it->second;
 
-  Symbol sym(next_symbol_);
+  Symbol sym(next_symbol_, program_id_);
   ++next_symbol_;
 
   name_to_symbol_[name] = sym;
@@ -51,6 +54,7 @@ Symbol SymbolTable::Get(const std::string& name) const {
 }
 
 std::string SymbolTable::NameFor(const Symbol symbol) const {
+  TINT_ASSERT_PROGRAM_IDS_EQUAL(program_id_, symbol);
   auto it = symbol_to_name_.find(symbol);
   if (it == symbol_to_name_.end()) {
     return symbol.to_str();

src/symbol_table.h

@@ -26,7 +26,9 @@ namespace tint {
 class SymbolTable {
  public:
   /// Constructor
-  SymbolTable();
+  /// @param program_id the identifier of the program that owns this symbol
+  /// table
+  explicit SymbolTable(tint::ProgramID program_id);
   /// Copy constructor
   SymbolTable(const SymbolTable&);
   /// Move Constructor
@@ -76,14 +78,24 @@ class SymbolTable {
     }
   }
 
+  /// @returns the identifier of the Program that owns this symbol table.
+  tint::ProgramID ProgramID() const { return program_id_; }
+
  private:
   // The value to be associated to the next registered symbol table entry.
   uint32_t next_symbol_ = 1;
 
   std::unordered_map<Symbol, std::string> symbol_to_name_;
   std::unordered_map<std::string, Symbol> name_to_symbol_;
+  tint::ProgramID program_id_;
 };
 
+/// @param symbol_table the SymbolTable
+/// @returns the ProgramID that owns the given SymbolTable
+inline ProgramID ProgramIDOf(const SymbolTable& symbol_table) {
+  return symbol_table.ProgramID();
+}
+
 }  // namespace tint
 
 #endif  // SRC_SYMBOL_TABLE_H_

src/symbol_table_test.cc

@@ -22,31 +22,36 @@ namespace {
 using SymbolTableTest = testing::Test;
 
 TEST_F(SymbolTableTest, GeneratesSymbolForName) {
-  SymbolTable s;
-  EXPECT_EQ(Symbol(1), s.Register("name"));
-  EXPECT_EQ(Symbol(2), s.Register("another_name"));
+  auto program_id = ProgramID::New();
+  SymbolTable s{program_id};
+  EXPECT_EQ(Symbol(1, program_id), s.Register("name"));
+  EXPECT_EQ(Symbol(2, program_id), s.Register("another_name"));
 }
 
 TEST_F(SymbolTableTest, DeduplicatesNames) {
-  SymbolTable s;
-  EXPECT_EQ(Symbol(1), s.Register("name"));
-  EXPECT_EQ(Symbol(2), s.Register("another_name"));
-  EXPECT_EQ(Symbol(1), s.Register("name"));
+  auto program_id = ProgramID::New();
+  SymbolTable s{program_id};
+  EXPECT_EQ(Symbol(1, program_id), s.Register("name"));
+  EXPECT_EQ(Symbol(2, program_id), s.Register("another_name"));
+  EXPECT_EQ(Symbol(1, program_id), s.Register("name"));
 }
 
 TEST_F(SymbolTableTest, ReturnsNameForSymbol) {
-  SymbolTable s;
+  auto program_id = ProgramID::New();
+  SymbolTable s{program_id};
   auto sym = s.Register("name");
   EXPECT_EQ("name", s.NameFor(sym));
 }
 
 TEST_F(SymbolTableTest, ReturnsBlankForMissingSymbol) {
-  SymbolTable s;
-  EXPECT_EQ("$2", s.NameFor(Symbol(2)));
+  auto program_id = ProgramID::New();
+  SymbolTable s{program_id};
+  EXPECT_EQ("$2", s.NameFor(Symbol(2, program_id)));
 }
 
 TEST_F(SymbolTableTest, ReturnsInvalidForBlankString) {
-  SymbolTable s;
+  auto program_id = ProgramID::New();
+  SymbolTable s{program_id};
   EXPECT_FALSE(s.Register("").IsValid());
 }
 

src/symbol_test.cc

@@ -22,12 +22,12 @@ namespace {
 using SymbolTest = testing::Test;
 
 TEST_F(SymbolTest, ToStr) {
-  Symbol sym(1);
+  Symbol sym(1, ProgramID::New());
   EXPECT_EQ("$1", sym.to_str());
 }
 
 TEST_F(SymbolTest, CopyAssign) {
-  Symbol sym1(1);
+  Symbol sym1(1, ProgramID::New());
   Symbol sym2;
 
   EXPECT_FALSE(sym2.IsValid());
@@ -37,9 +37,10 @@ TEST_F(SymbolTest, CopyAssign) {
 }
 
 TEST_F(SymbolTest, Comparison) {
-  Symbol sym1(1);
-  Symbol sym2(2);
-  Symbol sym3(1);
+  auto program_id = ProgramID::New();
+  Symbol sym1(1, program_id);
+  Symbol sym2(2, program_id);
+  Symbol sym3(1, program_id);
 
   EXPECT_TRUE(sym1 == sym3);
   EXPECT_FALSE(sym1 == sym2);

src/type/alias_type_test.cc

@@ -24,7 +24,7 @@ using AliasTest = TestHelper;
 
 TEST_F(AliasTest, Create) {
   auto* a = ty.alias("a_type", ty.u32());
-  EXPECT_EQ(a->symbol(), Symbol(1));
+  EXPECT_EQ(a->symbol(), Symbol(1, ID()));
   EXPECT_EQ(a->type(), ty.u32());
 }
 
@@ -58,7 +58,7 @@ TEST_F(AliasTest, FriendlyName) {
 
 TEST_F(AliasTest, UnwrapIfNeeded_Alias) {
   auto* a = ty.alias("a_type", ty.u32());
-  EXPECT_EQ(a->symbol(), Symbol(1));
+  EXPECT_EQ(a->symbol(), Symbol(1, ID()));
   EXPECT_EQ(a->type(), ty.u32());
   EXPECT_EQ(a->UnwrapIfNeeded(), ty.u32());
   EXPECT_EQ(ty.u32()->UnwrapIfNeeded(), ty.u32());
@@ -74,7 +74,7 @@ TEST_F(AliasTest, UnwrapIfNeeded_MultiLevel) {
   auto* a = ty.alias("a_type", ty.u32());
   auto* aa = ty.alias("aa_type", a);
 
-  EXPECT_EQ(aa->symbol(), Symbol(2));
+  EXPECT_EQ(aa->symbol(), Symbol(2, ID()));
   EXPECT_EQ(aa->type(), a);
   EXPECT_EQ(aa->UnwrapIfNeeded(), ty.u32());
 }
@@ -94,7 +94,7 @@ TEST_F(AliasTest, UnwrapAll_TwiceAliasPointerTwiceAlias) {
   auto* apaa = ty.alias("paa_type", &paa);
   auto* aapaa = ty.alias("aapaa_type", apaa);
 
-  EXPECT_EQ(aapaa->symbol(), Symbol(4));
+  EXPECT_EQ(aapaa->symbol(), Symbol(4, ID()));
   EXPECT_EQ(aapaa->type(), apaa);
   EXPECT_EQ(aapaa->UnwrapAll(), ty.u32());
 }