From 0b75a10eb602852190f914f345fc5879bd966947 Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Fri, 2 Oct 2020 23:56:09 -0400 Subject: [PATCH 01/10] Add missing `escapeForDot()` to labels for function names In some programming languages, e.g. JuliaLang, function names can contain arbitrary characters. These are represented via the string macro `var"..."`, which allows constructing identifiers that wouldn't otherwise parse. These names are handled correctly by `pprof` in the FlameGraph view, but before this commit, they would produce an invalid dot file. This fixes the dot graph export for names that contain `"`. --- internal/graph/dotgraph.go | 4 ++-- internal/graph/dotgraph_test.go | 2 +- internal/graph/testdata/compose3.dot | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/graph/dotgraph.go b/internal/graph/dotgraph.go index cde648f20..5b851307c 100644 --- a/internal/graph/dotgraph.go +++ b/internal/graph/dotgraph.go @@ -187,7 +187,7 @@ func (b *builder) addNode(node *Node, nodeID int, maxFlat float64) { // Create DOT attribute for node. attr := fmt.Sprintf(`label="%s" id="node%d" fontsize=%d shape=%s tooltip="%s (%s)" color="%s" fillcolor="%s"`, - label, nodeID, fontSize, shape, node.Info.PrintableName(), cumValue, + escapeForDot(label), nodeID, fontSize, shape, node.Info.PrintableName(), cumValue, dotColor(float64(node.CumValue())/float64(abs64(b.config.Total)), false), dotColor(float64(node.CumValue())/float64(abs64(b.config.Total)), true)) @@ -247,7 +247,7 @@ func (b *builder) addNodelets(node *Node, nodeID int) bool { continue } weight := b.config.FormatValue(w) - nodelets += fmt.Sprintf(`N%d_%d [label = "%s" id="N%d_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", nodeID, i, t.Name, nodeID, i, weight) + nodelets += fmt.Sprintf(`N%d_%d [label = "%s" id="N%d_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", nodeID, i, escapeForDot(t.Name), nodeID, i, weight) nodelets += fmt.Sprintf(`N%d -> N%d_%d [label=" %s" weight=100 tooltip="%s" labeltooltip="%s"]`+"\n", nodeID, nodeID, i, weight, weight, weight) if nts := lnts[t.Name]; nts != nil { nodelets += b.numericNodelets(nts, maxNodelets, flatTags, fmt.Sprintf(`N%d_%d`, nodeID, i)) diff --git a/internal/graph/dotgraph_test.go b/internal/graph/dotgraph_test.go index 4232efaa5..bc05f95e8 100644 --- a/internal/graph/dotgraph_test.go +++ b/internal/graph/dotgraph_test.go @@ -76,7 +76,7 @@ func TestComposeWithTagsAndResidualEdge(t *testing.T) { } g.Nodes[0].NumericTags[""] = TagMap{ "b": &Tag{ - Name: "tag2", + Name: "var\"tag2\"", Cum: 20, Flat: 20, Unit: "ms", diff --git a/internal/graph/testdata/compose3.dot b/internal/graph/testdata/compose3.dot index f22ad9fe4..570f5de2f 100644 --- a/internal/graph/testdata/compose3.dot +++ b/internal/graph/testdata/compose3.dot @@ -4,7 +4,7 @@ subgraph cluster_L { "label1" [shape=box fontsize=16 label="label1\llabel2\llabe N1 [label="src\n10 (10.00%)\nof 25 (25.00%)" id="node1" fontsize=22 shape=box tooltip="src (25)" color="#b23c00" fillcolor="#edddd5"] N1_0 [label = "tag1" id="N1_0" fontsize=8 shape=box3d tooltip="10"] N1 -> N1_0 [label=" 10" weight=100 tooltip="10" labeltooltip="10"] -NN1_0 [label = "tag2" id="NN1_0" fontsize=8 shape=box3d tooltip="20"] +NN1_0 [label = "var\"tag2\"" id="NN1_0" fontsize=8 shape=box3d tooltip="20"] N1 -> NN1_0 [label=" 20" weight=100 tooltip="20" labeltooltip="20"] N2 [label="dest\n15 (15.00%)\nof 25 (25.00%)" id="node2" fontsize=24 shape=box tooltip="dest (25)" color="#b23c00" fillcolor="#edddd5"] N1 -> N2 [label=" 10" weight=11 color="#b28559" tooltip="src ... dest (10)" labeltooltip="src ... dest (10)" style="dotted" minlen=2] From 866a19c2a1084957f6bea6165f8f4a35ce88a2c1 Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Sat, 3 Oct 2020 00:37:41 -0400 Subject: [PATCH 02/10] Add separate test for name escaping --- internal/graph/dotgraph_test.go | 18 +++++++++++++++++- internal/graph/testdata/compose3.dot | 2 +- internal/graph/testdata/compose7.dot | 7 +++++++ 3 files changed, 25 insertions(+), 2 deletions(-) create mode 100644 internal/graph/testdata/compose7.dot diff --git a/internal/graph/dotgraph_test.go b/internal/graph/dotgraph_test.go index bc05f95e8..1bb30637f 100644 --- a/internal/graph/dotgraph_test.go +++ b/internal/graph/dotgraph_test.go @@ -76,7 +76,7 @@ func TestComposeWithTagsAndResidualEdge(t *testing.T) { } g.Nodes[0].NumericTags[""] = TagMap{ "b": &Tag{ - Name: "var\"tag2\"", + Name: "tag2", Cum: 20, Flat: 20, Unit: "ms", @@ -138,6 +138,22 @@ func TestComposeWithStandardGraphAndURL(t *testing.T) { compareGraphs(t, buf.Bytes(), "compose6.dot") } +func TestComposeWithNamesThatNeedEscaping(t *testing.T) { + g := baseGraph() + a, c := baseAttrsAndConfig() + + g.Nodes[0].Info = NodeInfo{Name: "var\"src\""}; + g.Nodes[1].Info = NodeInfo{Name: "var\"#dest#\""}; + + // Set edge to be Residual. + g.Nodes[0].Out[g.Nodes[1]].Residual = true + + var buf bytes.Buffer + ComposeDot(&buf, g, a, c) + + compareGraphs(t, buf.Bytes(), "compose7.dot") +} + func baseGraph() *Graph { src := &Node{ Info: NodeInfo{Name: "src"}, diff --git a/internal/graph/testdata/compose3.dot b/internal/graph/testdata/compose3.dot index 570f5de2f..f22ad9fe4 100644 --- a/internal/graph/testdata/compose3.dot +++ b/internal/graph/testdata/compose3.dot @@ -4,7 +4,7 @@ subgraph cluster_L { "label1" [shape=box fontsize=16 label="label1\llabel2\llabe N1 [label="src\n10 (10.00%)\nof 25 (25.00%)" id="node1" fontsize=22 shape=box tooltip="src (25)" color="#b23c00" fillcolor="#edddd5"] N1_0 [label = "tag1" id="N1_0" fontsize=8 shape=box3d tooltip="10"] N1 -> N1_0 [label=" 10" weight=100 tooltip="10" labeltooltip="10"] -NN1_0 [label = "var\"tag2\"" id="NN1_0" fontsize=8 shape=box3d tooltip="20"] +NN1_0 [label = "tag2" id="NN1_0" fontsize=8 shape=box3d tooltip="20"] N1 -> NN1_0 [label=" 20" weight=100 tooltip="20" labeltooltip="20"] N2 [label="dest\n15 (15.00%)\nof 25 (25.00%)" id="node2" fontsize=24 shape=box tooltip="dest (25)" color="#b23c00" fillcolor="#edddd5"] N1 -> N2 [label=" 10" weight=11 color="#b28559" tooltip="src ... dest (10)" labeltooltip="src ... dest (10)" style="dotted" minlen=2] diff --git a/internal/graph/testdata/compose7.dot b/internal/graph/testdata/compose7.dot new file mode 100644 index 000000000..9c998286b --- /dev/null +++ b/internal/graph/testdata/compose7.dot @@ -0,0 +1,7 @@ +digraph "testtitle" { +node [style=filled fillcolor="#f8f8f8"] +subgraph cluster_L { "label1" [shape=box fontsize=16 label="label1\llabel2\llabel3: \"foo\"\l" tooltip="testtitle"] } +N1 [label="var\"src\"\n10 (10.00%)\nof 25 (25.00%)" id="node1" fontsize=22 shape=box tooltip="var\"src\" (25)" color="#b23c00" fillcolor="#edddd5"] +N2 [label="var\"#dest#\"\n15 (15.00%)\nof 25 (25.00%)" id="node2" fontsize=24 shape=box tooltip="var\"#dest#\" (25)" color="#b23c00" fillcolor="#edddd5"] +N1 -> N2 [label=" 10" weight=11 color="#b28559" tooltip="var\"src\" ... var\"#dest#\" (10)" labeltooltip="var\"src\" ... var\"#dest#\" (10)" style="dotted" minlen=2] +} From 5d0860e39f8ff59959c7f106bd6e789d33ee3e40 Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Sat, 3 Oct 2020 00:45:36 -0400 Subject: [PATCH 03/10] Apply `escapeStringForDot()` in more places, to cover more cases of potentially harmful string labels. Remove mistaken `escapeStringForDot()` around tag names --- internal/graph/dotgraph.go | 15 ++++++++++----- internal/graph/testdata/compose7.dot | 2 +- 2 files changed, 11 insertions(+), 6 deletions(-) diff --git a/internal/graph/dotgraph.go b/internal/graph/dotgraph.go index 5b851307c..6fbf084e1 100644 --- a/internal/graph/dotgraph.go +++ b/internal/graph/dotgraph.go @@ -149,6 +149,7 @@ func (b *builder) addNode(node *Node, nodeID int, maxFlat float64) { } else { label = multilinePrintableName(&node.Info) } + // label = strings.Replace(label, `"`, `\"`, -1) flatValue := b.config.FormatValue(flat) if flat != 0 { @@ -187,7 +188,7 @@ func (b *builder) addNode(node *Node, nodeID int, maxFlat float64) { // Create DOT attribute for node. attr := fmt.Sprintf(`label="%s" id="node%d" fontsize=%d shape=%s tooltip="%s (%s)" color="%s" fillcolor="%s"`, - escapeForDot(label), nodeID, fontSize, shape, node.Info.PrintableName(), cumValue, + label, nodeID, fontSize, shape, escapeStringForDot(node.Info.PrintableName()), cumValue, dotColor(float64(node.CumValue())/float64(abs64(b.config.Total)), false), dotColor(float64(node.CumValue())/float64(abs64(b.config.Total)), true)) @@ -247,7 +248,7 @@ func (b *builder) addNodelets(node *Node, nodeID int) bool { continue } weight := b.config.FormatValue(w) - nodelets += fmt.Sprintf(`N%d_%d [label = "%s" id="N%d_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", nodeID, i, escapeForDot(t.Name), nodeID, i, weight) + nodelets += fmt.Sprintf(`N%d_%d [label = "%s" id="N%d_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", nodeID, i, t.Name, nodeID, i, weight) nodelets += fmt.Sprintf(`N%d -> N%d_%d [label=" %s" weight=100 tooltip="%s" labeltooltip="%s"]`+"\n", nodeID, nodeID, i, weight, weight, weight) if nts := lnts[t.Name]; nts != nil { nodelets += b.numericNodelets(nts, maxNodelets, flatTags, fmt.Sprintf(`N%d_%d`, nodeID, i)) @@ -305,7 +306,8 @@ func (b *builder) addEdge(edge *Edge, from, to int, hasNodelets bool) { arrow = "..." } tooltip := fmt.Sprintf(`"%s %s %s (%s)"`, - edge.Src.Info.PrintableName(), arrow, edge.Dest.Info.PrintableName(), w) + escapeStringForDot(edge.Src.Info.PrintableName()), arrow, + escapeStringForDot(edge.Dest.Info.PrintableName()), w) attr = fmt.Sprintf(`%s tooltip=%s labeltooltip=%s`, attr, tooltip, tooltip) if edge.Residual { @@ -382,7 +384,7 @@ func dotColor(score float64, isBackground bool) string { func multilinePrintableName(info *NodeInfo) string { infoCopy := *info - infoCopy.Name = ShortenFunctionName(infoCopy.Name) + infoCopy.Name = escapeStringForDot(ShortenFunctionName(infoCopy.Name)) infoCopy.Name = strings.Replace(infoCopy.Name, "::", `\n`, -1) infoCopy.Name = strings.Replace(infoCopy.Name, ".", `\n`, -1) if infoCopy.File != "" { @@ -479,7 +481,10 @@ func min64(a, b int64) int64 { func escapeForDot(in []string) []string { var out = make([]string, len(in)) for i := range in { - out[i] = strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(in[i], `\`, `\\`), `"`, `\"`), "\n", `\l`) + out[i] = escapeStringForDot(in[i]) } return out } +func escapeStringForDot(str string) string { + return strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(str, `\`, `\\`), `"`, `\"`), "\n", `\l`) +} diff --git a/internal/graph/testdata/compose7.dot b/internal/graph/testdata/compose7.dot index 9c998286b..2518663d2 100644 --- a/internal/graph/testdata/compose7.dot +++ b/internal/graph/testdata/compose7.dot @@ -3,5 +3,5 @@ node [style=filled fillcolor="#f8f8f8"] subgraph cluster_L { "label1" [shape=box fontsize=16 label="label1\llabel2\llabel3: \"foo\"\l" tooltip="testtitle"] } N1 [label="var\"src\"\n10 (10.00%)\nof 25 (25.00%)" id="node1" fontsize=22 shape=box tooltip="var\"src\" (25)" color="#b23c00" fillcolor="#edddd5"] N2 [label="var\"#dest#\"\n15 (15.00%)\nof 25 (25.00%)" id="node2" fontsize=24 shape=box tooltip="var\"#dest#\" (25)" color="#b23c00" fillcolor="#edddd5"] -N1 -> N2 [label=" 10" weight=11 color="#b28559" tooltip="var\"src\" ... var\"#dest#\" (10)" labeltooltip="var\"src\" ... var\"#dest#\" (10)" style="dotted" minlen=2] +N1 -> N2 [label=" 10" weight=11 color="#b28559" tooltip="var\"src\" ... var\"#dest#\" (10)" labeltooltip="var\"src\" ... var\"#dest#\" (10)" style="dotted"] } From 04c65fd1e93e6c6c1280704a20d4426cb862b278 Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Sat, 3 Oct 2020 00:58:14 -0400 Subject: [PATCH 04/10] gofmt cleanups --- internal/graph/dotgraph.go | 2 +- internal/graph/dotgraph_test.go | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/internal/graph/dotgraph.go b/internal/graph/dotgraph.go index 6fbf084e1..74d6c144f 100644 --- a/internal/graph/dotgraph.go +++ b/internal/graph/dotgraph.go @@ -486,5 +486,5 @@ func escapeForDot(in []string) []string { return out } func escapeStringForDot(str string) string { - return strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(str, `\`, `\\`), `"`, `\"`), "\n", `\l`) + return strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(str, `\`, `\\`), `"`, `\"`), "\n", `\l`) } diff --git a/internal/graph/dotgraph_test.go b/internal/graph/dotgraph_test.go index 1bb30637f..faa88a43c 100644 --- a/internal/graph/dotgraph_test.go +++ b/internal/graph/dotgraph_test.go @@ -142,8 +142,8 @@ func TestComposeWithNamesThatNeedEscaping(t *testing.T) { g := baseGraph() a, c := baseAttrsAndConfig() - g.Nodes[0].Info = NodeInfo{Name: "var\"src\""}; - g.Nodes[1].Info = NodeInfo{Name: "var\"#dest#\""}; + g.Nodes[0].Info = NodeInfo{Name: "var\"src\""} + g.Nodes[1].Info = NodeInfo{Name: "var\"#dest#\""} // Set edge to be Residual. g.Nodes[0].Out[g.Nodes[1]].Residual = true From 72389be747e8b00fd2f976e0ca512daed273752d Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Sat, 3 Oct 2020 01:04:26 -0400 Subject: [PATCH 05/10] Cleanup: Remove commented out line --- internal/graph/dotgraph.go | 1 - 1 file changed, 1 deletion(-) diff --git a/internal/graph/dotgraph.go b/internal/graph/dotgraph.go index 74d6c144f..a5592119c 100644 --- a/internal/graph/dotgraph.go +++ b/internal/graph/dotgraph.go @@ -149,7 +149,6 @@ func (b *builder) addNode(node *Node, nodeID int, maxFlat float64) { } else { label = multilinePrintableName(&node.Info) } - // label = strings.Replace(label, `"`, `\"`, -1) flatValue := b.config.FormatValue(flat) if flat != 0 { From dff1afcd898c3bc6799f02ca45da17ba49dea69c Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Mon, 5 Oct 2020 09:21:28 -0400 Subject: [PATCH 06/10] Rename escapeForDot => escapeAllForDot; escapeStringForDot => escapeForDot --- internal/graph/dotgraph.go | 24 +++++++++++++----------- internal/graph/dotgraph_test.go | 4 ++-- 2 files changed, 15 insertions(+), 13 deletions(-) diff --git a/internal/graph/dotgraph.go b/internal/graph/dotgraph.go index a5592119c..b52d4e73b 100644 --- a/internal/graph/dotgraph.go +++ b/internal/graph/dotgraph.go @@ -127,7 +127,7 @@ func (b *builder) addLegend() { } title := labels[0] fmt.Fprintf(b, `subgraph cluster_L { "%s" [shape=box fontsize=16`, title) - fmt.Fprintf(b, ` label="%s\l"`, strings.Join(escapeForDot(labels), `\l`)) + fmt.Fprintf(b, ` label="%s\l"`, strings.Join(escapeAllForDot(labels), `\l`)) if b.config.LegendURL != "" { fmt.Fprintf(b, ` URL="%s" target="_blank"`, b.config.LegendURL) } @@ -187,7 +187,7 @@ func (b *builder) addNode(node *Node, nodeID int, maxFlat float64) { // Create DOT attribute for node. attr := fmt.Sprintf(`label="%s" id="node%d" fontsize=%d shape=%s tooltip="%s (%s)" color="%s" fillcolor="%s"`, - label, nodeID, fontSize, shape, escapeStringForDot(node.Info.PrintableName()), cumValue, + label, nodeID, fontSize, shape, escapeForDot(node.Info.PrintableName()), cumValue, dotColor(float64(node.CumValue())/float64(abs64(b.config.Total)), false), dotColor(float64(node.CumValue())/float64(abs64(b.config.Total)), true)) @@ -305,8 +305,8 @@ func (b *builder) addEdge(edge *Edge, from, to int, hasNodelets bool) { arrow = "..." } tooltip := fmt.Sprintf(`"%s %s %s (%s)"`, - escapeStringForDot(edge.Src.Info.PrintableName()), arrow, - escapeStringForDot(edge.Dest.Info.PrintableName()), w) + escapeForDot(edge.Src.Info.PrintableName()), arrow, + escapeForDot(edge.Dest.Info.PrintableName()), w) attr = fmt.Sprintf(`%s tooltip=%s labeltooltip=%s`, attr, tooltip, tooltip) if edge.Residual { @@ -383,7 +383,7 @@ func dotColor(score float64, isBackground bool) string { func multilinePrintableName(info *NodeInfo) string { infoCopy := *info - infoCopy.Name = escapeStringForDot(ShortenFunctionName(infoCopy.Name)) + infoCopy.Name = escapeForDot(ShortenFunctionName(infoCopy.Name)) infoCopy.Name = strings.Replace(infoCopy.Name, "::", `\n`, -1) infoCopy.Name = strings.Replace(infoCopy.Name, ".", `\n`, -1) if infoCopy.File != "" { @@ -474,16 +474,18 @@ func min64(a, b int64) int64 { return b } -// escapeForDot escapes double quotes and backslashes, and replaces Graphviz's -// "center" character (\n) with a left-justified character. -// See https://graphviz.org/doc/info/attrs.html#k:escString for more info. -func escapeForDot(in []string) []string { +// Applies escapeForDot to all strings in the given slice. +func escapeAllForDot(in []string) []string { var out = make([]string, len(in)) for i := range in { - out[i] = escapeStringForDot(in[i]) + out[i] = escapeForDot(in[i]) } return out } -func escapeStringForDot(str string) string { + +// escapeForDot escapes double quotes and backslashes, and replaces Graphviz's +// "center" character (\n) with a left-justified character. +// See https://graphviz.org/doc/info/attrs.html#k:escString for more info. +func escapeForDot(str string) string { return strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(str, `\`, `\\`), `"`, `\"`), "\n", `\l`) } diff --git a/internal/graph/dotgraph_test.go b/internal/graph/dotgraph_test.go index faa88a43c..6da630835 100644 --- a/internal/graph/dotgraph_test.go +++ b/internal/graph/dotgraph_test.go @@ -375,8 +375,8 @@ func TestEscapeForDot(t *testing.T) { }, } { t.Run(tc.desc, func(t *testing.T) { - if got := escapeForDot(tc.input); !reflect.DeepEqual(got, tc.want) { - t.Errorf("escapeForDot(%s) = %s, want %s", tc.input, got, tc.want) + if got := escapeAllForDot(tc.input); !reflect.DeepEqual(got, tc.want) { + t.Errorf("escapeAllForDot(%s) = %s, want %s", tc.input, got, tc.want) } }) } From 956deb6b0fed78d6b5a7df776880621bdb170667 Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Mon, 5 Oct 2020 09:26:09 -0400 Subject: [PATCH 07/10] Add (failing) test of Tag with `"` in name --- internal/graph/dotgraph_test.go | 8 ++++++++ internal/graph/testdata/compose7.dot | 4 +++- 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/internal/graph/dotgraph_test.go b/internal/graph/dotgraph_test.go index 6da630835..be99dea1b 100644 --- a/internal/graph/dotgraph_test.go +++ b/internal/graph/dotgraph_test.go @@ -142,9 +142,17 @@ func TestComposeWithNamesThatNeedEscaping(t *testing.T) { g := baseGraph() a, c := baseAttrsAndConfig() + // Change node names to have `"` in them, which need to be escaped for dot. g.Nodes[0].Info = NodeInfo{Name: "var\"src\""} g.Nodes[1].Info = NodeInfo{Name: "var\"#dest#\""} + // Add tag to Node 1 with `"` in name. + g.Nodes[0].LabelTags["a"] = &Tag{ + Name: "var\"tag1\"", + Cum: 10, + Flat: 10, + } + // Set edge to be Residual. g.Nodes[0].Out[g.Nodes[1]].Residual = true diff --git a/internal/graph/testdata/compose7.dot b/internal/graph/testdata/compose7.dot index 2518663d2..f8e007a8d 100644 --- a/internal/graph/testdata/compose7.dot +++ b/internal/graph/testdata/compose7.dot @@ -2,6 +2,8 @@ digraph "testtitle" { node [style=filled fillcolor="#f8f8f8"] subgraph cluster_L { "label1" [shape=box fontsize=16 label="label1\llabel2\llabel3: \"foo\"\l" tooltip="testtitle"] } N1 [label="var\"src\"\n10 (10.00%)\nof 25 (25.00%)" id="node1" fontsize=22 shape=box tooltip="var\"src\" (25)" color="#b23c00" fillcolor="#edddd5"] +N1_0 [label = "var\"tag1\"" id="N1_0" fontsize=8 shape=box3d tooltip="10"] +N1 -> N1_0 [label=" 10" weight=100 tooltip="10" labeltooltip="10"] N2 [label="var\"#dest#\"\n15 (15.00%)\nof 25 (25.00%)" id="node2" fontsize=24 shape=box tooltip="var\"#dest#\" (25)" color="#b23c00" fillcolor="#edddd5"] -N1 -> N2 [label=" 10" weight=11 color="#b28559" tooltip="var\"src\" ... var\"#dest#\" (10)" labeltooltip="var\"src\" ... var\"#dest#\" (10)" style="dotted"] +N1 -> N2 [label=" 10" weight=11 color="#b28559" tooltip="var\"src\" ... var\"#dest#\" (10)" labeltooltip="var\"src\" ... var\"#dest#\" (10)" style="dotted" minlen=2] } From ce716bd88f56d0177437b4034bc3bf8703796289 Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Mon, 5 Oct 2020 09:53:27 -0400 Subject: [PATCH 08/10] Escape Nodelet/tag names for Dot as well. This escaping requirement was previously leaking into the implementation of `graph.go`, by writing `\n` directly instead of "\n". This meant that if it was displayed in a different program besides `dot`, it would have been potentially incorrectly escaped. Now, this commit moves all the escaping directly into dotgraph, which is more encapsulated, and also allows us to escape other characters in the name which need escaping. --- internal/graph/dotgraph.go | 6 +++--- internal/graph/graph.go | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/internal/graph/dotgraph.go b/internal/graph/dotgraph.go index b52d4e73b..47dd0cf44 100644 --- a/internal/graph/dotgraph.go +++ b/internal/graph/dotgraph.go @@ -247,7 +247,7 @@ func (b *builder) addNodelets(node *Node, nodeID int) bool { continue } weight := b.config.FormatValue(w) - nodelets += fmt.Sprintf(`N%d_%d [label = "%s" id="N%d_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", nodeID, i, t.Name, nodeID, i, weight) + nodelets += fmt.Sprintf(`N%d_%d [label = "%s" id="N%d_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", nodeID, i, escapeForDot(t.Name), nodeID, i, weight) nodelets += fmt.Sprintf(`N%d -> N%d_%d [label=" %s" weight=100 tooltip="%s" labeltooltip="%s"]`+"\n", nodeID, nodeID, i, weight, weight, weight) if nts := lnts[t.Name]; nts != nil { nodelets += b.numericNodelets(nts, maxNodelets, flatTags, fmt.Sprintf(`N%d_%d`, nodeID, i)) @@ -274,7 +274,7 @@ func (b *builder) numericNodelets(nts []*Tag, maxNumNodelets int, flatTags bool, } if w != 0 { weight := b.config.FormatValue(w) - nodelets += fmt.Sprintf(`N%s_%d [label = "%s" id="N%s_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", source, j, t.Name, source, j, weight) + nodelets += fmt.Sprintf(`N%s_%d [label = "%s" id="N%s_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", source, j, escapeForDot(t.Name), source, j, weight) nodelets += fmt.Sprintf(`%s -> N%s_%d [label=" %s" weight=100 tooltip="%s" labeltooltip="%s"%s]`+"\n", source, source, j, weight, weight, weight, attr) } } @@ -487,5 +487,5 @@ func escapeAllForDot(in []string) []string { // "center" character (\n) with a left-justified character. // See https://graphviz.org/doc/info/attrs.html#k:escString for more info. func escapeForDot(str string) string { - return strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(str, `\`, `\\`), `"`, `\"`), "\n", `\l`) + return strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(str, `\`, `\\`), `"`, `\"`), "\n", `\n`) } diff --git a/internal/graph/graph.go b/internal/graph/graph.go index d2397a93d..8ac089abd 100644 --- a/internal/graph/graph.go +++ b/internal/graph/graph.go @@ -522,7 +522,7 @@ func joinLabels(s *profile.Sample) string { } } sort.Strings(labels) - return strings.Join(labels, `\n`) + return strings.Join(labels, "\n") // This will be escaped downstream if needed. } // isNegative returns true if the node is considered as "negative" for the From e739e7e586b06d81d5f1757c280e2378cc12e1aa Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Mon, 5 Oct 2020 10:15:29 -0400 Subject: [PATCH 09/10] Add justify option to escapeForDot to preserve old behavior This allows tag newlines to stay center justified, and legend label newlines to stay left justified, as they were before we started escaping them for Dot. Also ran gofmt --- internal/graph/dotgraph.go | 39 +++++++++++++++++++++++---------- internal/graph/dotgraph_test.go | 21 +++++++++++++++++- internal/graph/graph.go | 2 +- 3 files changed, 49 insertions(+), 13 deletions(-) diff --git a/internal/graph/dotgraph.go b/internal/graph/dotgraph.go index 47dd0cf44..185e0c65d 100644 --- a/internal/graph/dotgraph.go +++ b/internal/graph/dotgraph.go @@ -127,7 +127,7 @@ func (b *builder) addLegend() { } title := labels[0] fmt.Fprintf(b, `subgraph cluster_L { "%s" [shape=box fontsize=16`, title) - fmt.Fprintf(b, ` label="%s\l"`, strings.Join(escapeAllForDot(labels), `\l`)) + fmt.Fprintf(b, ` label="%s\l"`, strings.Join(escapeAllForDot(labels, LeftJustify), `\l`)) if b.config.LegendURL != "" { fmt.Fprintf(b, ` URL="%s" target="_blank"`, b.config.LegendURL) } @@ -187,7 +187,7 @@ func (b *builder) addNode(node *Node, nodeID int, maxFlat float64) { // Create DOT attribute for node. attr := fmt.Sprintf(`label="%s" id="node%d" fontsize=%d shape=%s tooltip="%s (%s)" color="%s" fillcolor="%s"`, - label, nodeID, fontSize, shape, escapeForDot(node.Info.PrintableName()), cumValue, + label, nodeID, fontSize, shape, escapeForDot(node.Info.PrintableName(), CenterJustify), cumValue, dotColor(float64(node.CumValue())/float64(abs64(b.config.Total)), false), dotColor(float64(node.CumValue())/float64(abs64(b.config.Total)), true)) @@ -247,7 +247,7 @@ func (b *builder) addNodelets(node *Node, nodeID int) bool { continue } weight := b.config.FormatValue(w) - nodelets += fmt.Sprintf(`N%d_%d [label = "%s" id="N%d_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", nodeID, i, escapeForDot(t.Name), nodeID, i, weight) + nodelets += fmt.Sprintf(`N%d_%d [label = "%s" id="N%d_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", nodeID, i, escapeForDot(t.Name, CenterJustify), nodeID, i, weight) nodelets += fmt.Sprintf(`N%d -> N%d_%d [label=" %s" weight=100 tooltip="%s" labeltooltip="%s"]`+"\n", nodeID, nodeID, i, weight, weight, weight) if nts := lnts[t.Name]; nts != nil { nodelets += b.numericNodelets(nts, maxNodelets, flatTags, fmt.Sprintf(`N%d_%d`, nodeID, i)) @@ -274,7 +274,7 @@ func (b *builder) numericNodelets(nts []*Tag, maxNumNodelets int, flatTags bool, } if w != 0 { weight := b.config.FormatValue(w) - nodelets += fmt.Sprintf(`N%s_%d [label = "%s" id="N%s_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", source, j, escapeForDot(t.Name), source, j, weight) + nodelets += fmt.Sprintf(`N%s_%d [label = "%s" id="N%s_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", source, j, escapeForDot(t.Name, CenterJustify), source, j, weight) nodelets += fmt.Sprintf(`%s -> N%s_%d [label=" %s" weight=100 tooltip="%s" labeltooltip="%s"%s]`+"\n", source, source, j, weight, weight, weight, attr) } } @@ -305,8 +305,8 @@ func (b *builder) addEdge(edge *Edge, from, to int, hasNodelets bool) { arrow = "..." } tooltip := fmt.Sprintf(`"%s %s %s (%s)"`, - escapeForDot(edge.Src.Info.PrintableName()), arrow, - escapeForDot(edge.Dest.Info.PrintableName()), w) + escapeForDot(edge.Src.Info.PrintableName(), CenterJustify), arrow, + escapeForDot(edge.Dest.Info.PrintableName(), CenterJustify), w) attr = fmt.Sprintf(`%s tooltip=%s labeltooltip=%s`, attr, tooltip, tooltip) if edge.Residual { @@ -383,7 +383,7 @@ func dotColor(score float64, isBackground bool) string { func multilinePrintableName(info *NodeInfo) string { infoCopy := *info - infoCopy.Name = escapeForDot(ShortenFunctionName(infoCopy.Name)) + infoCopy.Name = escapeForDot(ShortenFunctionName(infoCopy.Name), CenterJustify) infoCopy.Name = strings.Replace(infoCopy.Name, "::", `\n`, -1) infoCopy.Name = strings.Replace(infoCopy.Name, ".", `\n`, -1) if infoCopy.File != "" { @@ -474,11 +474,19 @@ func min64(a, b int64) int64 { return b } +type DotJustifyType int64 + +const ( + LeftJustify DotJustifyType = 0 + CenterJustify DotJustifyType = 1 + RightJustify DotJustifyType = 2 +) + // Applies escapeForDot to all strings in the given slice. -func escapeAllForDot(in []string) []string { +func escapeAllForDot(in []string, justify DotJustifyType) []string { var out = make([]string, len(in)) for i := range in { - out[i] = escapeForDot(in[i]) + out[i] = escapeForDot(in[i], justify) } return out } @@ -486,6 +494,15 @@ func escapeAllForDot(in []string) []string { // escapeForDot escapes double quotes and backslashes, and replaces Graphviz's // "center" character (\n) with a left-justified character. // See https://graphviz.org/doc/info/attrs.html#k:escString for more info. -func escapeForDot(str string) string { - return strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(str, `\`, `\\`), `"`, `\"`), "\n", `\n`) +func escapeForDot(str string, justify DotJustifyType) string { + var newline string + switch justify { + case LeftJustify: + newline = `\l` + case CenterJustify: + newline = `\n` + case RightJustify: + newline = `\r` + } + return strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(str, `\`, `\\`), `"`, `\"`), "\n", newline) } diff --git a/internal/graph/dotgraph_test.go b/internal/graph/dotgraph_test.go index be99dea1b..751044067 100644 --- a/internal/graph/dotgraph_test.go +++ b/internal/graph/dotgraph_test.go @@ -383,11 +383,30 @@ func TestEscapeForDot(t *testing.T) { }, } { t.Run(tc.desc, func(t *testing.T) { - if got := escapeAllForDot(tc.input); !reflect.DeepEqual(got, tc.want) { + if got := escapeAllForDot(tc.input, LeftJustify); !reflect.DeepEqual(got, tc.want) { t.Errorf("escapeAllForDot(%s) = %s, want %s", tc.input, got, tc.want) } }) } + + // Test the different options for justifying text newlines in Dot + for _, justify := range []DotJustifyType{LeftJustify, CenterJustify, RightJustify} { + t.Run("Dot newline justification", func(t *testing.T) { + input := []string{"Line 1\nLine 2"} + var want []string + switch justify { + case LeftJustify: + want = []string{`Line 1\lLine 2`} + case CenterJustify: + want = []string{`Line 1\nLine 2`} + case RightJustify: + want = []string{`Line 1\rLine 2`} + } + if got := escapeAllForDot(input, justify); !reflect.DeepEqual(got, want) { + t.Errorf("escapeAllForDot(%s) = %s, want %s", input, got, want) + } + }) + } } func tagString(t []*Tag) string { diff --git a/internal/graph/graph.go b/internal/graph/graph.go index 8ac089abd..352830063 100644 --- a/internal/graph/graph.go +++ b/internal/graph/graph.go @@ -522,7 +522,7 @@ func joinLabels(s *profile.Sample) string { } } sort.Strings(labels) - return strings.Join(labels, "\n") // This will be escaped downstream if needed. + return strings.Join(labels, "\n") // This will be escaped downstream if needed. } // isNegative returns true if the node is considered as "negative" for the From 8c597aa2bd9181b599ab037ffc87c779d9eb03f7 Mon Sep 17 00:00:00 2001 From: Nathan Daly Date: Mon, 5 Oct 2020 10:33:36 -0400 Subject: [PATCH 10/10] Unexport dotJustifyType since it's only meant to be used for dotgraph. --- internal/graph/dotgraph.go | 32 ++++++++++++++++---------------- internal/graph/dotgraph_test.go | 10 +++++----- 2 files changed, 21 insertions(+), 21 deletions(-) diff --git a/internal/graph/dotgraph.go b/internal/graph/dotgraph.go index 185e0c65d..26c1c9e4d 100644 --- a/internal/graph/dotgraph.go +++ b/internal/graph/dotgraph.go @@ -127,7 +127,7 @@ func (b *builder) addLegend() { } title := labels[0] fmt.Fprintf(b, `subgraph cluster_L { "%s" [shape=box fontsize=16`, title) - fmt.Fprintf(b, ` label="%s\l"`, strings.Join(escapeAllForDot(labels, LeftJustify), `\l`)) + fmt.Fprintf(b, ` label="%s\l"`, strings.Join(escapeAllForDot(labels, leftJustify), `\l`)) if b.config.LegendURL != "" { fmt.Fprintf(b, ` URL="%s" target="_blank"`, b.config.LegendURL) } @@ -187,7 +187,7 @@ func (b *builder) addNode(node *Node, nodeID int, maxFlat float64) { // Create DOT attribute for node. attr := fmt.Sprintf(`label="%s" id="node%d" fontsize=%d shape=%s tooltip="%s (%s)" color="%s" fillcolor="%s"`, - label, nodeID, fontSize, shape, escapeForDot(node.Info.PrintableName(), CenterJustify), cumValue, + label, nodeID, fontSize, shape, escapeForDot(node.Info.PrintableName(), centerJustify), cumValue, dotColor(float64(node.CumValue())/float64(abs64(b.config.Total)), false), dotColor(float64(node.CumValue())/float64(abs64(b.config.Total)), true)) @@ -247,7 +247,7 @@ func (b *builder) addNodelets(node *Node, nodeID int) bool { continue } weight := b.config.FormatValue(w) - nodelets += fmt.Sprintf(`N%d_%d [label = "%s" id="N%d_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", nodeID, i, escapeForDot(t.Name, CenterJustify), nodeID, i, weight) + nodelets += fmt.Sprintf(`N%d_%d [label = "%s" id="N%d_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", nodeID, i, escapeForDot(t.Name, centerJustify), nodeID, i, weight) nodelets += fmt.Sprintf(`N%d -> N%d_%d [label=" %s" weight=100 tooltip="%s" labeltooltip="%s"]`+"\n", nodeID, nodeID, i, weight, weight, weight) if nts := lnts[t.Name]; nts != nil { nodelets += b.numericNodelets(nts, maxNodelets, flatTags, fmt.Sprintf(`N%d_%d`, nodeID, i)) @@ -274,7 +274,7 @@ func (b *builder) numericNodelets(nts []*Tag, maxNumNodelets int, flatTags bool, } if w != 0 { weight := b.config.FormatValue(w) - nodelets += fmt.Sprintf(`N%s_%d [label = "%s" id="N%s_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", source, j, escapeForDot(t.Name, CenterJustify), source, j, weight) + nodelets += fmt.Sprintf(`N%s_%d [label = "%s" id="N%s_%d" fontsize=8 shape=box3d tooltip="%s"]`+"\n", source, j, escapeForDot(t.Name, centerJustify), source, j, weight) nodelets += fmt.Sprintf(`%s -> N%s_%d [label=" %s" weight=100 tooltip="%s" labeltooltip="%s"%s]`+"\n", source, source, j, weight, weight, weight, attr) } } @@ -305,8 +305,8 @@ func (b *builder) addEdge(edge *Edge, from, to int, hasNodelets bool) { arrow = "..." } tooltip := fmt.Sprintf(`"%s %s %s (%s)"`, - escapeForDot(edge.Src.Info.PrintableName(), CenterJustify), arrow, - escapeForDot(edge.Dest.Info.PrintableName(), CenterJustify), w) + escapeForDot(edge.Src.Info.PrintableName(), centerJustify), arrow, + escapeForDot(edge.Dest.Info.PrintableName(), centerJustify), w) attr = fmt.Sprintf(`%s tooltip=%s labeltooltip=%s`, attr, tooltip, tooltip) if edge.Residual { @@ -383,7 +383,7 @@ func dotColor(score float64, isBackground bool) string { func multilinePrintableName(info *NodeInfo) string { infoCopy := *info - infoCopy.Name = escapeForDot(ShortenFunctionName(infoCopy.Name), CenterJustify) + infoCopy.Name = escapeForDot(ShortenFunctionName(infoCopy.Name), centerJustify) infoCopy.Name = strings.Replace(infoCopy.Name, "::", `\n`, -1) infoCopy.Name = strings.Replace(infoCopy.Name, ".", `\n`, -1) if infoCopy.File != "" { @@ -474,16 +474,16 @@ func min64(a, b int64) int64 { return b } -type DotJustifyType int64 +type dotJustifyType int64 const ( - LeftJustify DotJustifyType = 0 - CenterJustify DotJustifyType = 1 - RightJustify DotJustifyType = 2 + leftJustify dotJustifyType = 0 + centerJustify dotJustifyType = 1 + rightJustify dotJustifyType = 2 ) // Applies escapeForDot to all strings in the given slice. -func escapeAllForDot(in []string, justify DotJustifyType) []string { +func escapeAllForDot(in []string, justify dotJustifyType) []string { var out = make([]string, len(in)) for i := range in { out[i] = escapeForDot(in[i], justify) @@ -494,14 +494,14 @@ func escapeAllForDot(in []string, justify DotJustifyType) []string { // escapeForDot escapes double quotes and backslashes, and replaces Graphviz's // "center" character (\n) with a left-justified character. // See https://graphviz.org/doc/info/attrs.html#k:escString for more info. -func escapeForDot(str string, justify DotJustifyType) string { +func escapeForDot(str string, justify dotJustifyType) string { var newline string switch justify { - case LeftJustify: + case leftJustify: newline = `\l` - case CenterJustify: + case centerJustify: newline = `\n` - case RightJustify: + case rightJustify: newline = `\r` } return strings.ReplaceAll(strings.ReplaceAll(strings.ReplaceAll(str, `\`, `\\`), `"`, `\"`), "\n", newline) diff --git a/internal/graph/dotgraph_test.go b/internal/graph/dotgraph_test.go index 751044067..bd345a6f1 100644 --- a/internal/graph/dotgraph_test.go +++ b/internal/graph/dotgraph_test.go @@ -383,23 +383,23 @@ func TestEscapeForDot(t *testing.T) { }, } { t.Run(tc.desc, func(t *testing.T) { - if got := escapeAllForDot(tc.input, LeftJustify); !reflect.DeepEqual(got, tc.want) { + if got := escapeAllForDot(tc.input, leftJustify); !reflect.DeepEqual(got, tc.want) { t.Errorf("escapeAllForDot(%s) = %s, want %s", tc.input, got, tc.want) } }) } // Test the different options for justifying text newlines in Dot - for _, justify := range []DotJustifyType{LeftJustify, CenterJustify, RightJustify} { + for _, justify := range []dotJustifyType{leftJustify, centerJustify, rightJustify} { t.Run("Dot newline justification", func(t *testing.T) { input := []string{"Line 1\nLine 2"} var want []string switch justify { - case LeftJustify: + case leftJustify: want = []string{`Line 1\lLine 2`} - case CenterJustify: + case centerJustify: want = []string{`Line 1\nLine 2`} - case RightJustify: + case rightJustify: want = []string{`Line 1\rLine 2`} } if got := escapeAllForDot(input, justify); !reflect.DeepEqual(got, want) {