This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] Handle `\l` symbol in string literals in exploded-graph-rewriter
ClosedPublic

Authored by ASDenysPetrov on Jun 18 2020, 6:44 AM.

Details

Summary

Handle \l separately because a string literal can be in code like "string\\literal" with the \l inside. Also on Windows macros FILE produces specific delimiters \ and a directory or file may starts with the letter l.

Fix: Use regex on for replacing all \l (like ,\l, }\l, [\l) except \\l, because a literal as a rule containes multiple \ before \l.

Diff Detail

Event Timeline

ASDenysPetrov created this revision.Jun 18 2020, 6:44 AM

Even though it doesn't seem that necessary, I would still vote to comply with clang-format in tests as well.
Other then that looks good to me!

Additionally, I want to make a note that we are not really working hard on keeping all the scripts and the analyzer itself to be fully functional on Windows. It can limit the user base and also prevent new developers from participating in the project. So, thank you for cleaning that up and for making it easier for other people to join our community :)

NoQ added a comment.Jun 18 2020, 7:38 AM

Nice catch!

clang/utils/analyzer/exploded-graph-rewriter.py
383

Behavior of this script should not depend on the host platform. Neither should it depend on the target platform if the graph is obtained via cross-compilation. It should only depend on the graph itself, as it's a simple dot file converter. The graph itself may, of course, depend on the target platform (maybe even on the host platform).

So it sounds like this fix should be on the C++ side.

NoQ added a comment.Jun 18 2020, 7:40 AM

Additionally, I want to make a note that we are not really working hard on keeping all the scripts and the analyzer itself to be fully functional on Windows.

Yup. See also D76768 - i.e., well, i guess we tried.

NoQ added inline comments.Jun 18 2020, 7:41 AM
clang/utils/analyzer/exploded-graph-rewriter.py
383

Like, i mean, i see no reason why it should be impossible to copy the original dot file from one machine to another and still be able to run the script. It's probably not a super-important use case but i don't see why would we consciously break that.

Fixed clang-format.

ASDenysPetrov marked an inline comment as done.Jun 19 2020, 3:51 AM

Even though it doesn't seem that necessary, I would still vote to comply with clang-format in tests as well.

Yes, I just forgot to format it and was going to load it after.

Additionally, I want to make a note that we are not really working hard on keeping all the scripts and the analyzer itself to be fully functional on Windows.

I think it depends on the platform you prefer, so that's natural. I also try to use Ubuntu on VM to check any differences.

clang/utils/analyzer/exploded-graph-rewriter.py
383

So it sounds like this fix should be on the C++ side.

The first I did was C++ side. BTW an explicit literal "string\\literal" can also cause this issue.
But then I found that I am not sure what the solution could be. How can we let the script skip this particular case without modifiing the string? How can we modify the string to keep it logically valid? The only solution I see is to make the replacement selection more accurate.

Like, i mean, i see no reason why it should be impossible to copy the original dot file from one machine to another and still be able to run the script.

Correct! I'll keep the regex part for all platforms.

I've just checked the old code on Ubuntu using the sample. char text[] = "string\\literal"; And it also fails as on Windows.
When I used the patch It passed. So this is not only a Windows related issue.

ASDenysPetrov retitled this revision from [analyzer] Handle `\l` symbol in Windows specific file paths in exploded-graph-rewriter to [analyzer] Handle `\l` symbol in string literals in exploded-graph-rewriter.Jun 19 2020, 4:18 AM
ASDenysPetrov edited the summary of this revision. (Show Details)

Expanded the patch for all platforms.

ASDenysPetrov marked an inline comment as done.Jun 19 2020, 4:28 AM
ASDenysPetrov added inline comments.
clang/utils/analyzer/exploded-graph-rewriter.py
432

This makes the graph data being look from

to
.

NoQ accepted this revision.Jun 19 2020, 7:35 AM

I see what you did here! Great!

clang/test/Analysis/exploded-graph-rewriter/l_name_starts_with_l.cpp
25

I still wouldn't mind testing the actual output.

This revision is now accepted and ready to land.Jun 19 2020, 7:35 AM
ASDenysPetrov marked an inline comment as done.Jun 19 2020, 3:45 PM

Thanks for the quick review, guys!

clang/test/Analysis/exploded-graph-rewriter/l_name_starts_with_l.cpp
25

I'll add more checks before the load.

This revision was automatically updated to reflect the committed changes.