Add missing `escapeForDot()` to labels for function names #564

NHDaly · 2020-10-03T04:07:40Z

In some programming languages, e.g. JuliaLang, function names can contain arbitrary characters. For example, in julia, functions with non-identifier characters 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 ".

Review Guidelines

All of the code changes are in the internal/graph/dotgraph.go file, and consist of escaping a few more callsites that produce text that will go in a string in the .dot file.
The rest of the files are modifications to the tests:
1. We added a new test to test string escaping in the newly escaped parts of the .dot file.
2. We fixed a bunch of windows tests, which were previously validating incorrect escaping of \, and had to be updated once we started correctly escaping "...\..." to "...\\..." on Windows.

Fixes JuliaPerf/PProf.jl#30.

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 `"`.

NHDaly · 2020-10-03T04:12:46Z

Hello! 👋 I believe this is my first contribution to pprof. Please excuse my sloppy go abilities. I learned a small bit of Go when I used to work at google a few years ago, but I never learned much.

I am one of the maintainers of PProf.jl, the julia bindings to pprof. We love this package, and make heavy use of it at my company, and in the wider julia community.

However, as described above, julia function names can be quite messy, and I encountered a case where pprof is failing to escape names that contain a string.

I've tested the above fix locally, and the tests pass, but it appears that some of the build tests have failed. Help here would be very much appreciated.

Thanks much!
Best,
~Nathan

…otentially harmful string labels. Remove mistaken `escapeStringForDot()` around tag names

NHDaly · 2020-10-03T05:02:16Z

Based on the ci output, it looks like the test-mac job is failing to build for unrelated reasons?

Otherwise, I think the tests should be passing now. :) Please take a look when you're available. Thanks!

internal/graph/dotgraph.go

codecov-commenter · 2020-10-03T06:04:26Z

Codecov Report

Merging #564 into master will decrease coverage by 1.30%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #564      +/-   ##
==========================================
- Coverage   68.46%   67.15%   -1.31%     
==========================================
  Files          78       78              
  Lines       16108    14076    -2032     
==========================================
- Hits        11028     9453    -1575     
+ Misses       4225     3788     -437     
+ Partials      855      835      -20

Impacted Files	Coverage Δ
internal/graph/dotgraph.go	`90.27% <100.00%> (-1.16%)`	⬇️
internal/driver/settings.go	`67.21% <0.00%> (-5.01%)`	⬇️
...ithub.com/google/pprof/internal/driver/settings.go	`67.21% <0.00%> (-5.01%)`	⬇️
internal/binutils/addr2liner_llvm.go	`64.51% <0.00%> (-4.06%)`	⬇️
profile/proto.go	`79.53% <0.00%> (-3.73%)`	⬇️
src/github.com/google/pprof/profile/proto.go	`79.53% <0.00%> (-3.73%)`	⬇️
internal/driver/flags.go	`15.38% <0.00%> (-3.67%)`	⬇️
...c/github.com/google/pprof/internal/driver/flags.go	`15.38% <0.00%> (-3.67%)`	⬇️
...b.com/google/pprof/internal/binutils/addr2liner.go	`65.43% <0.00%> (-3.46%)`	⬇️
.../google/pprof/internal/binutils/addr2liner_llvm.go	`70.96% <0.00%> (-3.32%)`	⬇️
... and 69 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update acf8798...845f55e. Read the comment docs.

internal/graph/dotgraph.go

aalexand

I am curious on what the code coverage dropped so substantially.

NHDaly · 2020-10-04T21:03:07Z

I am curious on what the code coverage dropped so substantially.

Yeah, it's really weird, right? I saw that too, and it led me to believe the coverage tool must be flaky. Do you find it to be usually quite reliable?

I added a test case that I believe covers the cases I've modified the code for. But maybe I messed something else up somehow? I would appreciate guidance here since I'm not a regular go programmer. 🙈 Thanks for your help so far! :)

…orDot

NHDaly

PTAL. This is again ready for review.
I have applied the renaming we suggested.

Also, as long as I was looking at this, I've also added escaping for Tags, which actually proved to be a bit more difficult, since graph.go was already doing some manual escaping of \n, which wound up double escaped.

I've sorted that all out, but since it's a fairly sizeable PR as well, I've opened it as a separate PR against this branch, here:
NHDaly#1

Please take a look at that PR as well if you get a chance, and we can either merge it directly into this branch first if you prefer merging a single PR, or we can take them one at a time. It's up to you. Thanks!

(I tried opening it directly against this repo, but then i couldn't base it on top of this PR. GitHub still has some rough edges...)

internal/graph/dotgraph.go

internal/graph/dotgraph_test.go

aalexand · 2020-10-07T02:38:29Z

Two Windows tests are failing.

codecov-io · 2020-10-07T03:22:23Z

Codecov Report

Merging #564 (064b590) into master (3e6fc7f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #564   +/-   ##
=======================================
  Coverage   67.14%   67.14%           
=======================================
  Files          78       78           
  Lines       14078    14082    +4     
=======================================
+ Hits         9452     9455    +3     
- Misses       3791     3792    +1     
  Partials      835      835

Impacted Files	Coverage Δ
internal/graph/dotgraph.go	`90.27% <100.00%> (+0.09%)`	⬆️
internal/binutils/binutils.go	`59.11% <0.00%> (-0.45%)`	⬇️
...github.com/google/pprof/internal/graph/dotgraph.go	`90.27% <0.00%> (+0.09%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3e6fc7f...064b590. Read the comment docs.

NHDaly · 2020-10-07T03:34:22Z

Two Windows tests are failing.

Ugh, thanks.

It appears that the failure is because now we're escaping the backslashes in windows paths, e.g. from testdata\source1 to testdata\\source1.

I'm very unsure what to do about this.
Is the original \ even correct? Is it okay to have a single standalone \ in a string like that, even on windows? Maybe this is actually fixing a bug?

If so, I guess we can just fix the test to expect the new behavior instead of finding some way to disable it on windows.

Also, it looks like the coverage report is back to normal, so that's good! :)

aalexand · 2020-10-07T05:15:59Z

Two Windows tests are failing.

Ugh, thanks.

It appears that the failure is because now we're escaping the backslashes in windows paths, e.g. from testdata\source1 to testdata\\source1.

I'm very unsure what to do about this.
Is the original \ even correct? Is it okay to have a single standalone \ in a string like that, even on windows? Maybe this is actually fixing a bug?

If so, I guess we can just fix the test to expect the new behavior instead of finding some way to disable it on windows.

Also, it looks like the coverage report is back to normal, so that's good! :)

I would expect that double slashes should be escaped in the *.dot file strings, yes.

I had unwittingly copied a test that was _specifically tesing_ Residual edges (the name of the test set I copied was `TestComposeWithTagsAndResidualEdge`). In my test, I'm simply testing the printing of the _names_ of the edges, and it's not relevant whether the edges are residual or not, so I've removed it just to simplify the test.

NHDaly · 2020-10-20T02:39:17Z

I would expect that double slashes should be escaped in the *.dot file strings, yes.

Awesome, thanks!

… in paths This PR adds proper escaping to dot strings, so that the backslash (`\`) in windows paths will now print correctly in dot output. Previously, the tests were incorreclty checking for an unescaped single `\` in the output, which isn't a valid `dot` string, so this commit updates the dot tests to expect the newly correct output. Updates the path testing assertions in various test files

NHDaly · 2020-10-24T21:32:22Z

@aalexand - I have (finally) fixed all the remaining windows test failures! :)

If you could, would you please take another look when you get the chance? Thanks very much!
Best,
~Nathan

NHDaly · 2020-10-28T15:29:56Z

Friendly ping: this is now ready for final review :) Thanks again for all the help so far!

aalexand · 2020-10-28T18:31:11Z

@NHDaly I'll take a look at it by EOW, sorry for the delay.

NHDaly · 2020-10-28T22:14:19Z

:) Thanks, no worries at all! :) I just wanted to be sure this stays on your radar 👍

internal/graph/dotgraph.go

google-cla · 2020-11-06T19:34:42Z

We found a Contributor License Agreement for you (the sender of this pull request), but were unable to determine that you authored the commits in this PR. Maybe you used a different email address in the git commits than was used to sign the CLA? If someone else authored these commits, then please add them to this pull request and have them confirm that they're okay with them being contributed to Google. If there are co-authors, make sure they're formatted properly.

In order to pass this check, please resolve this problem and then comment@googlebot I fixed it... If the bot doesn't comment, it means it doesn't think anything has changed.

ℹ️ Googlers: Go here for more info.

NHDaly · 2020-11-06T19:38:39Z

@googlebot I fixed it.

NHDaly · 2020-11-09T22:23:47Z

Thanks for the review, @Segflow! :)

Friendly ping @aalexand - when you get the chance this should be all set to merge. I think it's a simple review now; we're just escaping a couple new strings that weren't being escaped before 😁

Thanks!

NHDaly · 2020-11-09T22:24:33Z

(oh, whoops, sorry, I hadn't re-requested a Review from you through the GitHub UI. I've done that now! 👍)

i've also updated the top-level comment with Review Guidelines that explain the changes in this PR and will hopefully make it easier to review. Thanks!! :)

NHDaly · 2020-11-09T23:19:21Z

Awesome. Thank you very much for your help and your time! :)

Bump from 20191205061153 => 20201109224723 My personal interest is to pull in google/pprof#564, which adds support for displaying names with `"` in them, which julia functions sometimes have (e.g. `var"#foo#23"`) Includes: - google/pprof#564 - google/pprof#575 - google/pprof#574 - google/pprof#571 - google/pprof#572 - google/pprof#570 - google/pprof#562 - google/pprof#561 - google/pprof#565 - google/pprof#560 - google/pprof#563 - google/pprof#557 - google/pprof#554 - google/pprof#552 - google/pprof#545 - google/pprof#549 - google/pprof#547 - google/pprof#541 - google/pprof#534 - google/pprof#542 - google/pprof#535 - google/pprof#531 - google/pprof#530 - google/pprof#528 - google/pprof#522 - google/pprof#525 - google/pprof#527 - google/pprof#519 - google/pprof#520 - google/pprof#517 - google/pprof#518 - google/pprof#514 - google/pprof#513 - google/pprof#510 - google/pprof#508 - google/pprof#506 - google/pprof#509 - google/pprof#504

* Update pprof to latest revision Bump from 20191205061153 => 20201109224723 My personal interest is to pull in google/pprof#564, which adds support for displaying names with `"` in them, which julia functions sometimes have (e.g. `var"#foo#23"`) Includes: - google/pprof#564 - google/pprof#575 - google/pprof#574 - google/pprof#571 - google/pprof#572 - google/pprof#570 - google/pprof#562 - google/pprof#561 - google/pprof#565 - google/pprof#560 - google/pprof#563 - google/pprof#557 - google/pprof#554 - google/pprof#552 - google/pprof#545 - google/pprof#549 - google/pprof#547 - google/pprof#541 - google/pprof#534 - google/pprof#542 - google/pprof#535 - google/pprof#531 - google/pprof#530 - google/pprof#528 - google/pprof#522 - google/pprof#525 - google/pprof#527 - google/pprof#519 - google/pprof#520 - google/pprof#517 - google/pprof#518 - google/pprof#514 - google/pprof#513 - google/pprof#510 - google/pprof#508 - google/pprof#506 - google/pprof#509 - google/pprof#504 * Update P/pprof/build_tarballs.jl - use a real version number Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com> * Remove now unused `timestamp` * [pprof] Use `GitSource` Co-authored-by: Mosè Giordano <giordano@users.noreply.github.com>

* 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 `"`. * Add separate test for name escaping * Apply `escapeStringForDot()` in more places, to cover more cases of potentially harmful string labels. Remove mistaken `escapeStringForDot()` around tag names * gofmt cleanups * Cleanup: Remove commented out line * Rename escapeForDot => escapeAllForDot; escapeStringForDot => escapeForDot * Apply formatting suggestions from code review * Remove unneeded `Residual` from dotgraph test I had unwittingly copied a test that was _specifically tesing_ Residual edges (the name of the test set I copied was `TestComposeWithTagsAndResidualEdge`). In my test, I'm simply testing the printing of the _names_ of the edges, and it's not relevant whether the edges are residual or not, so I've removed it just to simplify the test. * Fix Windows test (after fixing Windows printing): properly escape `\` in paths This PR adds proper escaping to dot strings, so that the backslash (`\`) in windows paths will now print correctly in dot output. Previously, the tests were incorreclty checking for an unescaped single `\` in the output, which isn't a valid `dot` string, so this commit updates the dot tests to expect the newly correct output. Updates the path testing assertions in various test files * Fix unexported comment in internal/graph/dotgraph.go Co-authored-by: Alexey Alexandrov <aalexand@users.noreply.github.com>

google-cla bot added the cla: yes label Oct 3, 2020

NHDaly added 2 commits October 3, 2020 00:37

Add separate test for name escaping

866a19c

Apply escapeStringForDot() in more places, to cover more cases of p…

5d0860e

…otentially harmful string labels. Remove mistaken `escapeStringForDot()` around tag names

NHDaly force-pushed the nhd-dotgraph-escape-labels branch from 78c9e82 to 5d0860e Compare October 3, 2020 04:50

gofmt cleanups

04c65fd

Cleanup: Remove commented out line

72389be

NHDaly commented Oct 3, 2020

View reviewed changes

internal/graph/dotgraph.go Outdated Show resolved Hide resolved

aalexand reviewed Oct 3, 2020

View reviewed changes

internal/graph/dotgraph.go Outdated Show resolved Hide resolved

aalexand reviewed Oct 3, 2020

View reviewed changes

Rename escapeForDot => escapeAllForDot; escapeStringForDot => escapeF…

dff1afc

…orDot

NHDaly force-pushed the nhd-dotgraph-escape-labels branch from 1be6a7b to dff1afc Compare October 5, 2020 14:19

NHDaly mentioned this pull request Oct 5, 2020

Escape Nodelet/tag names for Dot as well #568

Open

NHDaly commented Oct 5, 2020

View reviewed changes

internal/graph/dotgraph.go Outdated Show resolved Hide resolved

NHDaly mentioned this pull request Oct 5, 2020

Escape Nodelet/tag names for Dot as well. NHDaly/pprof#1

Closed

aalexand reviewed Oct 6, 2020

View reviewed changes

internal/graph/dotgraph_test.go Outdated Show resolved Hide resolved

internal/graph/dotgraph_test.go Outdated Show resolved Hide resolved

internal/graph/dotgraph_test.go Outdated Show resolved Hide resolved

internal/graph/dotgraph_test.go Outdated Show resolved Hide resolved

NHDaly commented Oct 6, 2020

View reviewed changes

internal/graph/dotgraph_test.go Outdated Show resolved Hide resolved

internal/graph/dotgraph_test.go Outdated Show resolved Hide resolved

internal/graph/dotgraph_test.go Outdated Show resolved Hide resolved

internal/graph/dotgraph_test.go Outdated Show resolved Hide resolved

Apply formatting suggestions from code review

845f55e

Merge branch 'master' into nhd-dotgraph-escape-labels

f07deb8

Merge branch 'master' into nhd-dotgraph-escape-labels

02ed730

NHDaly force-pushed the nhd-dotgraph-escape-labels branch from 4beaf0b to 28a8f81 Compare October 24, 2020 20:51

NHDaly force-pushed the nhd-dotgraph-escape-labels branch from 28a8f81 to 9061b08 Compare October 24, 2020 20:54

Merge branch 'master' into nhd-dotgraph-escape-labels

884cc2f

Segflow reviewed Nov 6, 2020

View reviewed changes

internal/graph/dotgraph.go Show resolved Hide resolved

NHDaly commented Nov 6, 2020

View reviewed changes

internal/graph/dotgraph.go Outdated Show resolved Hide resolved

google-cla bot added cla: no and removed cla: yes labels Nov 6, 2020

Fix unexported comment in internal/graph/dotgraph.go

064b590

NHDaly force-pushed the nhd-dotgraph-escape-labels branch from fc32837 to 064b590 Compare November 6, 2020 19:37

google-cla bot added cla: yes and removed cla: no labels Nov 6, 2020

Segflow approved these changes Nov 6, 2020

View reviewed changes

NHDaly requested a review from aalexand November 9, 2020 22:24

aalexand approved these changes Nov 9, 2020

View reviewed changes

aalexand merged commit 20978b5 into google:master Nov 9, 2020

NHDaly mentioned this pull request Nov 13, 2020

Update pprof to latest revision JuliaPackaging/Yggdrasil#2096

Merged

NHDaly deleted the nhd-dotgraph-escape-labels branch March 4, 2022 15:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add missing `escapeForDot()` to labels for function names #564

Add missing `escapeForDot()` to labels for function names #564

NHDaly commented Oct 3, 2020 •

edited

Loading

NHDaly commented Oct 3, 2020

NHDaly commented Oct 3, 2020

codecov-commenter commented Oct 3, 2020 •

edited

Loading

aalexand left a comment

NHDaly commented Oct 4, 2020

NHDaly left a comment •

edited

Loading

aalexand commented Oct 7, 2020

codecov-io commented Oct 7, 2020 •

edited

Loading

NHDaly commented Oct 7, 2020

aalexand commented Oct 7, 2020

NHDaly commented Oct 20, 2020

NHDaly commented Oct 24, 2020

NHDaly commented Oct 28, 2020

aalexand commented Oct 28, 2020

NHDaly commented Oct 28, 2020

google-cla bot commented Nov 6, 2020

NHDaly commented Nov 6, 2020

NHDaly commented Nov 9, 2020

NHDaly commented Nov 9, 2020 •

edited

Loading

NHDaly commented Nov 9, 2020

Add missing escapeForDot() to labels for function names #564

Add missing escapeForDot() to labels for function names #564

Conversation

NHDaly commented Oct 3, 2020 • edited Loading

Review Guidelines

NHDaly commented Oct 3, 2020

NHDaly commented Oct 3, 2020

codecov-commenter commented Oct 3, 2020 • edited Loading

Codecov Report

aalexand left a comment

Choose a reason for hiding this comment

NHDaly commented Oct 4, 2020

NHDaly left a comment • edited Loading

Choose a reason for hiding this comment

aalexand commented Oct 7, 2020

codecov-io commented Oct 7, 2020 • edited Loading

Codecov Report

NHDaly commented Oct 7, 2020

aalexand commented Oct 7, 2020

NHDaly commented Oct 20, 2020

NHDaly commented Oct 24, 2020

NHDaly commented Oct 28, 2020

aalexand commented Oct 28, 2020

NHDaly commented Oct 28, 2020

google-cla bot commented Nov 6, 2020

NHDaly commented Nov 6, 2020

NHDaly commented Nov 9, 2020

NHDaly commented Nov 9, 2020 • edited Loading

NHDaly commented Nov 9, 2020

Add missing `escapeForDot()` to labels for function names #564

Add missing `escapeForDot()` to labels for function names #564

NHDaly commented Oct 3, 2020 •

edited

Loading

codecov-commenter commented Oct 3, 2020 •

edited

Loading

NHDaly left a comment •

edited

Loading

codecov-io commented Oct 7, 2020 •

edited

Loading

NHDaly commented Nov 9, 2020 •

edited

Loading