This is an archive of the discontinued LLVM Phabricator instance.

[XRay] Sanitize DOT labels in graph output
ClosedPublic

Authored by tetsuo-cpp on Oct 26 2019, 12:35 AM.

Details

Summary

Bugzilla: https://bugs.llvm.org/show_bug.cgi?id=39701

This patch is to convert certain characters to their XML escape sequences when generating labels for a DOT graph.

I had trouble reproducing the exact issue described on the tracker. I ran llvm-xray graph on a log from a test program that included function templates but wasn't able to get the dot tool to complain about the < and > characters. The documentation also suggests that the escape sequences should only be necessary when using HTML string labels which XRay doesn't use (label=<...> as opposed to label="..."). Perhaps newer versions of Graphviz silently handle this in the case of quoted-string labels.

In any case, the generated labels still look correct after this patch and should also fix the reporter's issue.

I was a bit unsure how to add a test for this since the existing tests seem to only care about func-id rather than giving an actual name. If you could give me a hint on the best way to go about this, that'd be much appreciated!

Diff Detail

Event Timeline

tetsuo-cpp created this revision.Oct 26 2019, 12:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 26 2019, 12:35 AM

I think you just want to call llvm::DOT::EscapeString().

@lebedev.ri
Thanks for taking a look. I did come across that function earlier but didn't really understand it since I don't see any escaping rules in the docs that look like that. It also doesn't do anything with the & character which will appear in function names too.
But if that solves the problem you described then I'll just use that instead.

I don't actually know if that one fixes my issue, but i would think they are solving the same issue (? i think ?),
so it is possible that one is simply not complete.

Understood. If you're still running the same setup, could you please post your dot -V output? I can try testing with the same one.

Understood. If you're still running the same setup, could you please post your dot -V output? I can try testing with the same one.

Hm, the bug was Reported: 2018-11-18, so as per https://tracker.debian.org/pkg/graphviz i suspect back then i was using [[ https://tracker.debian.org/news/992788/accepted-graphviz-2401-5-source-amd64-all-into-unstable/ | 2.40.1-5 ]]

dberris accepted this revision.Oct 27 2019, 3:05 PM

LGTM with one nit. Thanks!

llvm/tools/llvm-xray/xray-graph.cpp
175

nit: Consider using StringRef instead?

This revision is now accepted and ready to land.Oct 27 2019, 3:05 PM
tetsuo-cpp updated this revision to Diff 227562.Nov 1 2019, 7:52 PM

Use StringRef.

tetsuo-cpp marked an inline comment as done.Nov 1 2019, 8:06 PM

I was running 2.40.1-6 which is identical (plus a CVE fix) but still couldn't get dot to error with a test program with templates. The output of dot looks correct for different formats (I was looking mainly at SVG and JSON) before and after this change.
So I'm still not 100% sure that you need this change since I haven't seen the problem first hand. But it doesn't hurt either way.

I compiled multiple older versions of dot and gave them a try (I went back to 2.30.1) but still didn't see this problem. I've been testing like this (mainly testing SVG and JSON):

tetsuo@garland-1572667608860-c-16-sgp1-01:~/dev/llvm-project$ build/bin/llvm-xray graph ./xray-log.a.out.uRHPKt -instr_map ./a.out | dot -Tsvg

@dberris Since I can't reproduce this, should I abandon this revision? Or would you still like the patch.

dberris accepted this revision.Nov 10 2019, 2:59 PM

I compiled multiple older versions of dot and gave them a try (I went back to 2.30.1) but still didn't see this problem. I've been testing like this (mainly testing SVG and JSON):

tetsuo@garland-1572667608860-c-16-sgp1-01:~/dev/llvm-project$ build/bin/llvm-xray graph ./xray-log.a.out.uRHPKt -instr_map ./a.out | dot -Tsvg

@dberris Since I can't reproduce this, should I abandon this revision? Or would you still like the patch.

I would still like the patch, because it's still the right thing to do. :)

Thank you. check-llvm looks healthy with this patch.
Could you please commit this for me?

Friendly ping!

This revision was automatically updated to reflect the committed changes.