Remove Colours array in -print-changed=dot-cfg

[jamieschmeiser]

Summary

The Colours array is apparently the source of TSAN errors. It is
unnecessary and was there to ease readability of the code. Remove it to
clean up the TSAN errors.

[aeubanks]

llvm/lib/Passes/StandardInstrumentations.cpp

1272

should this be a std::string? it looks like it's referencing a std::string in DisplayEdge::DisplayEdge() that goes out of scope

[mehdi_amini]

Thanks for the fix!
It's best if @aeubanks approves, I'm unfamiliar with this code.

[jamieschmeiser]

llvm/lib/Passes/StandardInstrumentations.cpp

1272

I don't think so. This is referring to the member of this class and this class is a base of DisplayEdge. The Colour value is sent in during base construction and is a StringRef. All of the colour values that are stored and passed around are StringRefs and ultimately all refer back to the strings in the 3 options for specifying the colours so they should all have values and be in scope.

[jamieschmeiser]

I've updated the patch with some changes to same parameter names (V -> Value, etc) and corresponding comments. The only substantive change in the update is that I changed DotCfgDiffNode::getEdge to DotCfgDiffNode::getEdgeColour and had it return a StringRef to the colour instead of the std::pair it previously did since the only use just extracted the colour from the pair (lines 1496 and 1769).

llvm/lib/Passes/StandardInstrumentations.cpp

if (PrintChanged == ChangePrinter::PrintChangedDiffVerbose ||
       PrintChanged == ChangePrinter::PrintChangedDiffQuiet ||
       PrintChanged == ChangePrinter::PrintChangedColourDiffVerbose ||
       PrintChanged == ChangePrinter::PrintChangedColourDiffQuiet)
     TextChangeReporter<IRDataT<EmptyData>>::registerRequiredCallbacks(PIC);
 }
 
 namespace {
 
-enum IRChangeDiffType { InBefore, InAfter, IsCommon, NumIRChangeDiffTypes };
-
-// Describe where a given element exists.
-std::string Colours[NumIRChangeDiffTypes];
-
 class DisplayNode;
 class DotCfgDiffDisplayGraph;
 
 // Base class for a node or edge in the dot-cfg-changes graph.
 class DisplayElement {
 public:
   // Is this in before, after, or both?
-  IRChangeDiffType getType() const { return Type; }
+  StringRef getColour() const { return Colour; }
 
 protected:
-  DisplayElement(IRChangeDiffType T) : Type(T) {}
-  const IRChangeDiffType Type;
+  DisplayElement(StringRef Colour) : Colour(Colour) {}
+  const StringRef Colour;
 };
 
 // An edge representing a transition between basic blocks in the
 // dot-cfg-changes graph.
 class DisplayEdge : public DisplayElement {
 public:
-  DisplayEdge(std::string V, DisplayNode &Node, IRChangeDiffType T)
-      : DisplayElement(T), Value(V), Node(Node) {}
+  DisplayEdge(std::string Value, DisplayNode &Node, StringRef Colour)
+      : DisplayElement(Colour), Value(Value), Node(Node) {}
   // The value on which the transition is made.
   std::string getValue() const { return Value; }
   // The node (representing a basic block) reached by this transition.
   const DisplayNode &getDestinationNode() const { return Node; }
 
 protected:
   std::string Value;
   const DisplayNode &Node;
 };
 
 // A node in the dot-cfg-changes graph which represents a basic block.
 class DisplayNode : public DisplayElement {
 public:
   // \p C is the content for the node, \p T indicates the colour for the
   // outline of the node
-  DisplayNode(std::string C, IRChangeDiffType T)
-      : DisplayElement(T), Content(C) {}
+  DisplayNode(std::string Content, StringRef Colour)
+      : DisplayElement(Colour), Content(Content) {}
 
   // Iterator to the child nodes.  Required by GraphWriter.
   using ChildIterator = std::unordered_set<DisplayNode *>::const_iterator;
   ChildIterator children_begin() const { return Children.cbegin(); }
   ChildIterator children_end() const { return Children.cend(); }
 
   // Iterator for the edges.  Required by GraphWriter.
   using EdgeIterator = std::vector<DisplayEdge *>::const_iterator;
   EdgeIterator edges_begin() const { return EdgePtrs.cbegin(); }
   EdgeIterator edges_end() const { return EdgePtrs.cend(); }
 
-  // Create an edge to \p Node on value \p V, with type \p T.
-  void createEdge(StringRef V, DisplayNode &Node, IRChangeDiffType T);
+  // Create an edge to \p Node on value \p Value, with colour \p Colour.
+  void createEdge(StringRef Value, DisplayNode &Node, StringRef Colour);
 
   // Return the content of this node.
   std::string getContent() const { return Content; }
 
-  // Return the type of the edge to node \p S.
+  // Return the edge to node \p S.
   const DisplayEdge &getEdge(const DisplayNode &To) const {
     assert(EdgeMap.find(&To) != EdgeMap.end() && "Expected to find edge.");
     return *EdgeMap.find(&To)->second;
   }
 
   // Return the value for the transition to basic block \p S.
   // Required by GraphWriter.
   std::string getEdgeSourceLabel(const DisplayNode &Sink) const {
void setEntryNode(unsigned N) {
     NodeGenerationComplete = true;
     for (auto &N : Nodes)
       NodePtrs.emplace_back(&N);
 
     EntryNode = NodePtrs[N];
   }
 
   // Create a node.
-  void createNode(std::string C, IRChangeDiffType T) {
+  void createNode(std::string C, StringRef Colour) {
     assert(!NodeGenerationComplete && "Unexpected node creation");
-    Nodes.emplace_back(C, T);
+    Nodes.emplace_back(C, Colour);
   }
   // Return the node at index \p N to avoid problems with vectors reallocating.
   DisplayNode &getNode(unsigned N) {
     assert(N < Nodes.size() && "Node is out of bounds");
     return Nodes[N];
   }
   unsigned size() const {
     assert(NodeGenerationComplete && "Unexpected children iterator creation");
     return Nodes.size();
   }
 
   // Return the name of the graph.  Required by GraphWriter.
   std::string getGraphName() const { return GraphName; }
 
   // Return the string representing the differences for basic block \p Node.
   // Required by GraphWriter.
   std::string getNodeLabel(const DisplayNode &Node) const {
     return Node.getContent();
   }
 
   // Return a string with colour information for Dot.  Required by GraphWriter.
   std::string getNodeAttributes(const DisplayNode &Node) const {
-    return attribute(Node.getType());
+    return attribute(Node.getColour());
   }
 
   // Return a string with colour information for Dot.  Required by GraphWriter.
   std::string getEdgeColorAttr(const DisplayNode &From,
                                const DisplayNode &To) const {
-    return attribute(From.getEdge(To).getType());
+    return attribute(From.getEdge(To).getColour());
   }
 
   // Get the starting basic block.  Required by GraphWriter.
   DisplayNode *getEntryNode() const {
     assert(NodeGenerationComplete && "Unexpected children iterator creation");
     return EntryNode;
   }
 
 protected:
   // Return the string containing the colour to use as a Dot attribute.
-  std::string attribute(IRChangeDiffType T) const;
+  std::string attribute(StringRef Colour) const {
+    return "color=" + Colour.str();
+  }
 
   bool NodeGenerationComplete = false;
   const std::string GraphName;
   std::vector<DisplayNode> Nodes;
   std::vector<DisplayNode *> NodePtrs;
   DisplayNode *EntryNode = nullptr;
 };
 
-void DisplayNode::createEdge(StringRef V, DisplayNode &Node,
-                             IRChangeDiffType T) {
+void DisplayNode::createEdge(StringRef Value, DisplayNode &Node,
+                             StringRef Colour) {
   assert(!AllEdgesCreated && "Expected to be able to still create edges.");
-  Edges.emplace_back(V.str(), Node, T);
+  Edges.emplace_back(Value.str(), Node, Colour);
   Children.insert(&Node);
 }
 
 void DisplayNode::createEdgeMap() {
   // No more edges will be added so we can now use pointers to the edges
   // as the vector will not grow and reallocate.
   AllEdgesCreated = true;
   for (auto &E : Edges)
     EdgeMap.insert({&E.getDestinationNode(), &E});
 }
 
 class DotCfgDiffNode;
 class DotCfgDiff;
 
 // A class representing a basic block in the Dot difference graph.
 class DotCfgDiffNode {
 public:
   DotCfgDiffNode() = delete;
 
   // Create a node in Dot difference graph \p G representing the basic block
-  // represented by \p BD with type \p T (where it exists).
+  // represented by \p BD with colour \p Colour (where it exists).
   DotCfgDiffNode(DotCfgDiff &G, unsigned N, const BlockDataT<DCData> &BD,
-                 IRChangeDiffType T)
-      : Graph(G), N(N), Data{&BD, nullptr}, Type(T) {}
+                 StringRef Colour)
+      : Graph(G), N(N), Data{&BD, nullptr}, Colour(Colour) {}
   DotCfgDiffNode(const DotCfgDiffNode &DN)
-      : Graph(DN.Graph), N(DN.N), Data{DN.Data[0], DN.Data[1]}, Type(DN.Type),
-        EdgesMap(DN.EdgesMap), Children(DN.Children), Edges(DN.Edges) {}
+      : Graph(DN.Graph), N(DN.N), Data{DN.Data[0], DN.Data[1]},
+        Colour(DN.Colour), EdgesMap(DN.EdgesMap), Children(DN.Children),
+        Edges(DN.Edges) {}
 
   unsigned getIndex() const { return N; }
 
   // The label of the basic block
   StringRef getLabel() const {
     assert(Data[0] && "Expected Data[0] to be set.");
     return Data[0]->getLabel();
   }
-  // Return where this block exists.
-  IRChangeDiffType getType() const { return Type; }
+  // Return the colour for this block
+  StringRef getColour() const { return Colour; }
   // Change this basic block from being only in before to being common.
   // Save the pointer to \p Other.
   void setCommon(const BlockDataT<DCData> &Other) {
     assert(!Data[1] && "Expected only one block datum");
     Data[1] = &Other;
-    Type = IsCommon;
+    Colour = CommonColour;
   }
-  // Add an edge to \p E of type {\p Value, \p T}.
-  void addEdge(unsigned E, StringRef Value, IRChangeDiffType T) {
+  // Add an edge to \p E of colour {\p Value, \p Colour}.
+  void addEdge(unsigned E, StringRef Value, StringRef Colour) {
     // This is a new edge or it is an edge being made common.
-    assert((EdgesMap.count(E) == 0 || T == IsCommon) &&
-           "Unexpected edge count and type.");
-    EdgesMap[E] = {Value.str(), T};
+    assert((EdgesMap.count(E) == 0 || Colour == CommonColour) &&
+           "Unexpected edge count and color.");
+    EdgesMap[E] = {Value.str(), Colour};
   }
   // Record the children and create edges.
   void finalize(DotCfgDiff &G);
 
-  // Return the type of the edge to node \p S.
-  std::pair<std::string, IRChangeDiffType> getEdge(const unsigned S) const {
+  // Return the colour of the edge to node \p S.
+  StringRef getEdgeColour(const unsigned S) const {
     assert(EdgesMap.count(S) == 1 && "Expected to find edge.");
-    return EdgesMap.at(S);
+    return EdgesMap.at(S).second;
   }
 
   // Return the string representing the basic block.
   std::string getBodyContent() const;
 
   void createDisplayEdges(DotCfgDiffDisplayGraph &Graph, unsigned DisplayNode,
                           std::map<const unsigned, unsigned> &NodeMap) const;
 
 protected:
   DotCfgDiff &Graph;
   const unsigned N;
   const BlockDataT<DCData> *Data[2];
-  IRChangeDiffType Type;
-  std::map<const unsigned, std::pair<std::string, IRChangeDiffType>> EdgesMap;
+  StringRef Colour;
+  std::map<const unsigned, std::pair<std::string, StringRef>> EdgesMap;
   std::vector<unsigned> Children;
   std::vector<unsigned> Edges;
 };
 
 // Class representing the difference graph between two functions.
 class DotCfgDiff {
 public:
   // \p Title is the title given to the graph.  \p EntryNodeName is the
public:
 
   const DotCfgDiffNode &getNode(unsigned N) const {
     assert(N < Nodes.size() && "Unexpected index for node reference");
     return Nodes[N];
   }
 
 protected:
   // Return the string surrounded by HTML to make it the appropriate colour.
-  std::string colourize(std::string S, IRChangeDiffType T) const;
+  std::string colourize(std::string S, StringRef Colour) const;
 
-  void createNode(StringRef Label, const BlockDataT<DCData> &BD,
-                  IRChangeDiffType T) {
+  void createNode(StringRef Label, const BlockDataT<DCData> &BD, StringRef C) {
     unsigned Pos = Nodes.size();
-    Nodes.emplace_back(*this, Pos, BD, T);
+    Nodes.emplace_back(*this, Pos, BD, C);
     NodePosition.insert({Label, Pos});
   }
 
   // TODO Nodes should probably be a StringMap<DotCfgDiffNode> after the
   // display graph is separated out, which would remove the need for
   // NodePosition.
   std::vector<DotCfgDiffNode> Nodes;
   StringMap<unsigned> NodePosition;
   const std::string GraphName;
 
   StringMap<std::string> EdgeLabels;
 };
 
 std::string DotCfgDiffNode::getBodyContent() const {
-  if (Type == IsCommon) {
+  if (Colour == CommonColour) {
     assert(Data[1] && "Expected Data[1] to be set.");
 
     StringRef SR[2];
     for (unsigned I = 0; I < 2; ++I) {
       SR[I] = Data[I]->getBody();
       // drop initial '\n' if present
       if (SR[I][0] == '\n')
         SR[I] = SR[I].drop_front();
       // drop predecessors as they can be big and are redundant
       SR[I] = SR[I].drop_until([](char C) { return C == '\n'; }).drop_front();
     }
 
     SmallString<80> OldLineFormat = formatv(
-        "<FONT COLOR=\"{0}\">%l</FONT><BR align=\"left\"/>", Colours[InBefore]);
+        "<FONT COLOR=\"{0}\">%l</FONT><BR align=\"left\"/>", BeforeColour);
     SmallString<80> NewLineFormat = formatv(
-        "<FONT COLOR=\"{0}\">%l</FONT><BR align=\"left\"/>", Colours[InAfter]);
+        "<FONT COLOR=\"{0}\">%l</FONT><BR align=\"left\"/>", AfterColour);
     SmallString<80> UnchangedLineFormat = formatv(
-        "<FONT COLOR=\"{0}\">%l</FONT><BR align=\"left\"/>", Colours[IsCommon]);
+        "<FONT COLOR=\"{0}\">%l</FONT><BR align=\"left\"/>", CommonColour);
     std::string Diff = Data[0]->getLabel().str();
     Diff += ":\n<BR align=\"left\"/>" +
             doSystemDiff(makeHTMLReady(SR[0]), makeHTMLReady(SR[1]),
                          OldLineFormat, NewLineFormat, UnchangedLineFormat);
 
     // Diff adds in some empty colour changes which are not valid HTML
     // so remove them.  Colours are all lowercase alpha characters (as
     // listed in https://graphviz.org/pdf/dotguide.pdf).
std::string DotCfgDiffNode::getBodyContent() const {
   // Drop leading newline, if present.
   if (BS.front() == '\n')
     BS1 = BS1.drop_front(1);
   // Get label.
   StringRef Label = BS1.take_until([](char C) { return C == ':'; });
   // drop predecessors as they can be big and are redundant
   BS1 = BS1.drop_until([](char C) { return C == '\n'; }).drop_front();
 
-  std::string S = "<FONT COLOR=\"" + Colours[Type] + "\">" + Label.str() + ":";
+  std::string S = "<FONT COLOR=\"" + Colour.str() + "\">" + Label.str() + ":";
 
   // align each line to the left.
   while (BS1.size()) {
     S.append("<BR align=\"left\"/>");
     StringRef Line = BS1.take_until([](char C) { return C == '\n'; });
     S.append(Line.str());
     BS1 = BS1.drop_front(Line.size() + 1);
   }
   S.append("<BR align=\"left\"/></FONT>");
   return S;
 }
 
-std::string DotCfgDiff::colourize(std::string S, IRChangeDiffType T) const {
+std::string DotCfgDiff::colourize(std::string S, StringRef Colour) const {
   if (S.length() == 0)
     return S;
-  return "<FONT COLOR=\"" + Colours[T] + "\">" + S + "</FONT>";
-}
-
-std::string DotCfgDiffDisplayGraph::attribute(IRChangeDiffType T) const {
-  return "color=" + Colours[T];
+  return "<FONT COLOR=\"" + Colour.str() + "\">" + S + "</FONT>";
 }
 
 DotCfgDiff::DotCfgDiff(StringRef Title, const FuncDataT<DCData> &Before,
                        const FuncDataT<DCData> &After)
     : GraphName(Title.str()) {
-  StringMap<IRChangeDiffType> EdgesMap;
+  StringMap<StringRef> EdgesMap;
 
   // Handle each basic block in the before IR.
   for (auto &B : Before.getData()) {
     StringRef Label = B.getKey();
     const BlockDataT<DCData> &BD = B.getValue();
-    createNode(Label, BD, InBefore);
+    createNode(Label, BD, BeforeColour);
 
     // Create transitions with names made up of the from block label, the value
     // on which the transition is made and the to block label.
     for (StringMap<std::string>::const_iterator Sink = BD.getData().begin(),
                                                 E = BD.getData().end();
          Sink != E; ++Sink) {
       std::string Key = (Label + " " + Sink->getKey().str()).str() + " " +
                         BD.getData().getSuccessorLabel(Sink->getKey()).str();
-      EdgesMap.insert({Key, InBefore});
+      EdgesMap.insert({Key, BeforeColour});
     }
   }
 
   // Handle each basic block in the after IR
   for (auto &A : After.getData()) {
     StringRef Label = A.getKey();
     const BlockDataT<DCData> &BD = A.getValue();
     unsigned C = NodePosition.count(Label);
     if (C == 0)
       // This only exists in the after IR.  Create the node.
-      createNode(Label, BD, InAfter);
+      createNode(Label, BD, AfterColour);
     else {
       assert(C == 1 && "Unexpected multiple nodes.");
       Nodes[NodePosition[Label]].setCommon(BD);
     }
     // Add in the edges between the nodes (as common or only in after).
     for (StringMap<std::string>::const_iterator Sink = BD.getData().begin(),
                                                 E = BD.getData().end();
          Sink != E; ++Sink) {
       std::string Key = (Label + " " + Sink->getKey().str()).str() + " " +
                         BD.getData().getSuccessorLabel(Sink->getKey()).str();
       unsigned C = EdgesMap.count(Key);
       if (C == 0)
-        EdgesMap.insert({Key, InAfter});
+        EdgesMap.insert({Key, AfterColour});
       else {
-        EdgesMap[Key] = IsCommon;
+        EdgesMap[Key] = CommonColour;
       }
     }
   }
 
   // Now go through the map of edges and add them to the node.
   for (auto &E : EdgesMap) {
     // Extract the source, sink and value from the edge key.
     StringRef S = E.getKey();
     auto SP1 = S.rsplit(' ');
     auto &SourceSink = SP1.first;
     auto SP2 = SourceSink.split(' ');
     StringRef Source = SP2.first;
     StringRef Sink = SP2.second;
     StringRef Value = SP1.second;
 
     assert(NodePosition.count(Source) == 1 && "Expected to find node.");
     DotCfgDiffNode &SourceNode = Nodes[NodePosition[Source]];
     assert(NodePosition.count(Sink) == 1 && "Expected to find node.");
     unsigned SinkNode = NodePosition[Sink];
-    IRChangeDiffType T = E.second;
+    StringRef Colour = E.second;
 
     // Look for an edge from Source to Sink
     if (EdgeLabels.count(SourceSink) == 0)
-      EdgeLabels.insert({SourceSink, colourize(Value.str(), T)});
+      EdgeLabels.insert({SourceSink, colourize(Value.str(), Colour)});
     else {
       StringRef V = EdgeLabels.find(SourceSink)->getValue();
-      std::string NV = colourize(V.str() + " " + Value.str(), T);
-      T = IsCommon;
+      std::string NV = colourize(V.str() + " " + Value.str(), Colour);
+      Colour = CommonColour;
       EdgeLabels[SourceSink] = NV;
     }
-    SourceNode.addEdge(SinkNode, Value, T);
+    SourceNode.addEdge(SinkNode, Value, Colour);
   }
   for (auto &I : Nodes)
     I.finalize(*this);
 }
 
 DotCfgDiffDisplayGraph DotCfgDiff::createDisplayGraph(StringRef Title,
                                                       StringRef EntryNodeName) {
   assert(NodePosition.count(EntryNodeName) == 1 &&
          "Expected to find entry block in map.");
   unsigned Entry = NodePosition[EntryNodeName];
   assert(Entry < Nodes.size() && "Expected to find entry node");
   DotCfgDiffDisplayGraph G(Title.str());
 
   std::map<const unsigned, unsigned> NodeMap;
 
   int EntryIndex = -1;
   unsigned Index = 0;
   for (auto &I : Nodes) {
     if (I.getIndex() == Entry)
       EntryIndex = Index;
-    G.createNode(I.getBodyContent(), I.getType());
+    G.createNode(I.getBodyContent(), I.getColour());
     NodeMap.insert({I.getIndex(), Index++});
   }
   assert(EntryIndex >= 0 && "Expected entry node index to be set.");
   G.setEntryNode(EntryIndex);
 
   for (auto &I : NodeMap) {
     unsigned SourceNode = I.first;
     unsigned DisplayNode = I.second;
     getNode(SourceNode).createDisplayEdges(G, DisplayNode, NodeMap);
   }
   return G;
 }
 
 void DotCfgDiffNode::createDisplayEdges(
     DotCfgDiffDisplayGraph &DisplayGraph, unsigned DisplayNodeIndex,
     std::map<const unsigned, unsigned> &NodeMap) const {
 
   DisplayNode &SourceDisplayNode = DisplayGraph.getNode(DisplayNodeIndex);
 
   for (auto I : Edges) {
     unsigned SinkNodeIndex = I;
-    IRChangeDiffType Type = getEdge(SinkNodeIndex).second;
+    StringRef Colour = getEdgeColour(SinkNodeIndex);
     const DotCfgDiffNode *SinkNode = &Graph.getNode(SinkNodeIndex);
 
     StringRef Label = Graph.getEdgeSourceLabel(getIndex(), SinkNodeIndex);
     DisplayNode &SinkDisplayNode = DisplayGraph.getNode(SinkNode->getIndex());
-    SourceDisplayNode.createEdge(Label, SinkDisplayNode, Type);
+    SourceDisplayNode.createEdge(Label, SinkDisplayNode, Colour);
   }
   SourceDisplayNode.createEdgeMap();
 }
 
 void DotCfgDiffNode::finalize(DotCfgDiff &G) {
   for (auto E : EdgesMap) {
     Children.emplace_back(E.first);
     Edges.emplace_back(E.first);
for (auto &C : Sw->cases()) {
       addSuccessorLabel(C.getCaseSuccessor()->getName().str(), Value);
     }
   } else
     for (const_succ_iterator I = succ_begin(&B), E = succ_end(&B); I != E; ++I)
       addSuccessorLabel((*I)->getName().str(), "");
 }
 
 DotCfgChangeReporter::DotCfgChangeReporter(bool Verbose)
-    : ChangeReporter<IRDataT<DCData>>(Verbose) {
-  // Set up the colours based on the hidden options.
-  Colours[InBefore] = BeforeColour;
-  Colours[InAfter] = AfterColour;
-  Colours[IsCommon] = CommonColour;
-}
+    : ChangeReporter<IRDataT<DCData>>(Verbose) {}
 
 void DotCfgChangeReporter::handleFunctionCompare(
     StringRef Name, StringRef Prefix, StringRef PassID, StringRef Divider,
     bool InModule, unsigned Minor, const FuncDataT<DCData> &Before,
     const FuncDataT<DCData> &After) {
   assert(HTML && "Expected outstream to be set");
   SmallString<8> Extender;
   SmallString<8> Number;