Check that symbol names in symbol tables are unique.

src/PNaClTranslator.cpp

 //===- subzero/src/PNaClTranslator.cpp - ICE from bitcode -----------------===//
 //
 //                        The Subzero Code Generator
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 ///
 /// \file
@@ skipping 18 matching lines @@
 #pragma clang diagnostic ignored "-Wunused-parameter"
 #include "llvm/ADT/SmallString.h"
 #include "llvm/Bitcode/NaCl/NaClBitcodeDecoders.h"
 #include "llvm/Bitcode/NaCl/NaClBitcodeDefs.h"
 #include "llvm/Bitcode/NaCl/NaClBitcodeHeader.h"
 #include "llvm/Bitcode/NaCl/NaClBitcodeParser.h"
 #include "llvm/Bitcode/NaCl/NaClReaderWriter.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/raw_ostream.h"
+#include <set>

[Jim Stichnoth, 2015/09/15 21:05:29]

Can you use unordered_set instead?

[Karl, 2015/09/16 17:54:25]

After figuring out how to define an appropriate hash function, changed to use
unordered set.

 #pragma clang diagnostic pop
 
 namespace {
 using namespace llvm;
 
 // Models elements in the list of types defined in the types block.
 // These elements can be undefined, a (simple) type, or a function type
 // signature. Note that an extended type is undefined on construction.
 // Use methods setAsSimpleType and setAsFuncSigType to define
 // the extended type.
@@ skipping 1104 matching lines @@
   ValuesymtabParser(unsigned BlockID, BlockParserBaseClass *EnclosingParser)
       : BlockParserBaseClass(BlockID, EnclosingParser) {}
 
   ~ValuesymtabParser() override = default;
 
   const char *getBlockName() const override { return "valuesymtab"; }
 
 protected:
   using StringType = SmallString<128>;
 
+  // Returns the name to identify the kind of symbol table this is
+  // in error messages.
+  virtual const char *getTableKind() const = 0;
+
   // Associates Name with the value defined by the given Index.
   virtual void setValueName(NaClBcIndexSize_t Index, StringType &Name) = 0;
 
   // Associates Name with the value defined by the given Index;
   virtual void setBbName(NaClBcIndexSize_t Index, StringType &Name) = 0;
 
+  // Reports that the assignment of Name to the value associated with
+  // index is not possible, for the given Context.
+  void reportUnableToAssign(const char *Context, NaClBcIndexSize_t Index,
+                            StringType &Name);
+
 private:
+  typedef std::set<StringType> NamesSetType;

[Jim Stichnoth, 2015/09/15 21:05:29]

using NamesSetType = std::set<StringType>;

(we're using "using" now instead of typedef)

[Karl, 2015/09/16 17:54:25]

Done.

+  NamesSetType ValueNames;
+  NamesSetType BlockNames;
+
   void ProcessRecord() override;
 
-  void convertToString(StringType &ConvertedName) {
+  // Extracts out ConvertedName. Returns true if unique wrt to Names.
+  bool convertToString(NamesSetType &Names, StringType &ConvertedName) {
     const NaClBitcodeRecord::RecordVector &Values = Record.GetValues();
     for (size_t i = 1, e = Values.size(); i != e; ++i) {
       ConvertedName += static_cast<char>(Values[i]);
     }
+    auto pair = Names.insert(ConvertedName);

[Jim Stichnoth, 2015/09/15 21:05:29]

capitalize Pair

[Karl, 2015/09/16 17:54:25]

Done.

+    return pair.second;
   }
+
+  void ReportDuplicateName(const char *NameCat, StringType &Name);
 };
 
+void ValuesymtabParser::reportUnableToAssign(const char *Context,
+                                             NaClBcIndexSize_t Index,
+                                             StringType &Name) {
+  std::string Buffer;
+  raw_string_ostream StrBuf(Buffer);
+  StrBuf << getTableKind() << " " << getBlockName() << ": " << Context
+         << " name '" << Name << "' can't be associated with index " << Index;
+  Error(StrBuf.str());
+}
+
+void ValuesymtabParser::ReportDuplicateName(const char *NameCat,
+                                            StringType &Name) {
+  std::string Buffer;
+  raw_string_ostream StrBuf(Buffer);
+  StrBuf << getTableKind() << " " << getBlockName() << " defines duplicate "
+         << NameCat << " name: '" << Name << "'";
+  Error(StrBuf.str());
+}
+
 void ValuesymtabParser::ProcessRecord() {
   const NaClBitcodeRecord::RecordVector &Values = Record.GetValues();
   StringType ConvertedName;
   switch (Record.GetCode()) {
   case naclbitc::VST_CODE_ENTRY: {
     // VST_ENTRY: [ValueId, namechar x N]
     if (!isValidRecordSizeAtLeast(2, "value entry"))
       return;
-    convertToString(ConvertedName);
-    setValueName(Values[0], ConvertedName);
+    if (convertToString(ValueNames, ConvertedName))
+      setValueName(Values[0], ConvertedName);
+    else
+      ReportDuplicateName("value", ConvertedName);
     return;
   }
   case naclbitc::VST_CODE_BBENTRY: {
     // VST_BBENTRY: [BbId, namechar x N]
     if (!isValidRecordSizeAtLeast(2, "basic block entry"))
       return;
-    convertToString(ConvertedName);
-    setBbName(Values[0], ConvertedName);
+    if (convertToString(BlockNames, ConvertedName))
+      setBbName(Values[0], ConvertedName);
+    else
+      ReportDuplicateName("block", ConvertedName);
     return;
   }
   default:
     break;
   }
   // If reached, don't know how to handle record.
   BlockParserBaseClass::ProcessRecord();
   return;
 }
 
@@ skipping 1669 matching lines @@
         Timer(Ice::TimerStack::TT_parseFunctionValuesymtabs,
               getTranslator().getContext()) {}
 
 private:
   Ice::TimerMarker Timer;
   // Returns the enclosing function parser.
   FunctionParser *getFunctionParser() const {
     return reinterpret_cast<FunctionParser *>(GetEnclosingParser());
   }
 
-  void setValueName(NaClBcIndexSize_t Index, StringType &Name) override;
-  void setBbName(NaClBcIndexSize_t Index, StringType &Name) override;
+  const char *getTableKind() const final { return "Function"; }
 
-  // Reports that the assignment of Name to the value associated with
-  // index is not possible, for the given Context.
-  void reportUnableToAssign(const char *Context, NaClBcIndexSize_t Index,
-                            StringType &Name) {
-    std::string Buffer;
-    raw_string_ostream StrBuf(Buffer);
-    StrBuf << "Function-local " << Context << " name '" << Name
-           << "' can't be associated with index " << Index;
-    Error(StrBuf.str());
-  }
+  void setValueName(NaClBcIndexSize_t Index, StringType &Name) final;
+  void setBbName(NaClBcIndexSize_t Index, StringType &Name) final;
 };
 
 void FunctionValuesymtabParser::setValueName(NaClBcIndexSize_t Index,
                                              StringType &Name) {
   // Note: We check when Index is too small, so that we can error recover
   // (FP->getOperand will create fatal error).
   if (Index < getFunctionParser()->getNumGlobalIDs()) {
-    reportUnableToAssign("instruction", Index, Name);
+    reportUnableToAssign("Global value", Index, Name);
     return;
   }
   if (isIRGenerationDisabled())
     return;
   Ice::Operand *Op = getFunctionParser()->getOperand(Index);
   if (Ice::Variable *V = dyn_cast<Ice::Variable>(Op)) {
     if (Ice::BuildDefs::dump()) {
       std::string Nm(Name.data(), Name.size());
       V->setName(getFunctionParser()->getFunc(), Nm);
     }
   } else {
-    reportUnableToAssign("variable", Index, Name);
+    reportUnableToAssign("Local value", Index, Name);
   }
 }
 
 void FunctionValuesymtabParser::setBbName(NaClBcIndexSize_t Index,
                                           StringType &Name) {
   if (!Ice::BuildDefs::dump())
     return;
   if (isIRGenerationDisabled())
     return;
   if (Index >= getFunctionParser()->getFunc()->getNumNodes()) {
-    reportUnableToAssign("block", Index, Name);
+    reportUnableToAssign("Basic block", Index, Name);
     return;
   }
   std::string Nm(Name.data(), Name.size());
   if (Ice::BuildDefs::dump())
     getFunctionParser()->getFunc()->getNodes()[Index]->setName(Nm);
 }
 
 bool FunctionParser::ParseBlock(unsigned BlockID) {
   switch (BlockID) {
   case naclbitc::CONSTANTS_BLOCK_ID: {
@@ skipping 62 matching lines @@
 public:
   ModuleValuesymtabParser(unsigned BlockID, ModuleParser *MP)
       : ValuesymtabParser(BlockID, MP),
         Timer(Ice::TimerStack::TT_parseModuleValuesymtabs,
               getTranslator().getContext()) {}
 
   ~ModuleValuesymtabParser() override = default;
 
 private:
   Ice::TimerMarker Timer;
-  void setValueName(NaClBcIndexSize_t Index, StringType &Name) override;
-  void setBbName(NaClBcIndexSize_t Index, StringType &Name) override;
+  const char *getTableKind() const final { return "Module"; }
+  void setValueName(NaClBcIndexSize_t Index, StringType &Name) final;
+  void setBbName(NaClBcIndexSize_t Index, StringType &Name) final;
 };
 
 void ModuleValuesymtabParser::setValueName(NaClBcIndexSize_t Index,
                                            StringType &Name) {
   Context->getGlobalDeclarationByID(Index)
       ->setName(StringRef(Name.data(), Name.size()));
 }
 
 void ModuleValuesymtabParser::setBbName(NaClBcIndexSize_t Index,
                                         StringType &Name) {
-  std::string Buffer;
-  raw_string_ostream StrBuf(Buffer);
-  StrBuf << "Can't define basic block name at global level: '" << Name
-         << "' -> " << Index;
-  Error(StrBuf.str());
+  reportUnableToAssign("Basic block", Index, Name);
 }
 
 bool ModuleParser::ParseBlock(unsigned BlockID) {
   switch (BlockID) {
   case naclbitc::BLOCKINFO_BLOCK_ID:
     return NaClBitcodeParser::ParseBlock(BlockID);
   case naclbitc::TYPE_BLOCK_ID_NEW: {
     TypesParser Parser(BlockID, this);
     return Parser.ParseThisBlock();
   }
@@ skipping 131 matching lines @@
   }
   if (InputStreamFile.getBitcodeBytes().getExtent() % 4 != 0) {
     ErrStream
         << IRFilename
         << ": Bitcode stream should be a multiple of 4 bytes in length.\n";
     llvm::report_fatal_error("Bitcode stream should be a multiple of 4 bytes");
   }
 }
 
 } // end of namespace Ice