This is an archive of the discontinued LLVM Phabricator instance.

[analyzer] MoveChecker Pt.3: Improve warning messages a bit.
ClosedPublic

Authored by NoQ on Nov 14 2018, 5:30 PM.

Details

Summary

The warning piece traditionally describes the bug itself, i.e., "The bug is a _____": "Attempt to delete released memory", "Resource leak", "Method call on a moved-from object".

The "moved-from" terminology we adopt here still feels a bit weird to me, but i don't have a better suggestion, so i just removed the single-quotes so that to at least feel proud about what we have.

Event pieces produced by the visitor are usually in a present tense, i.e., "At this moment _____": "Memory is released", "File is closed", "Object is moved", etc.

Additionally, type information is added into the event pieces for STL objects (in order to highlight that it is in fact an STL object), and the respective event piece now mentions that the object is left in an unspecified state after it was moved, which is a vital piece of information to understand the bug.

Diff Detail

Event Timeline

NoQ created this revision.Nov 14 2018, 5:30 PM
Szelethus accepted this revision.Nov 15 2018, 7:47 AM

Awesome! :D

lib/StaticAnalyzer/Checkers/MoveChecker.cpp
385–386

Maybe move this in-class?

This revision is now accepted and ready to land.Nov 15 2018, 7:47 AM
NoQ updated this revision to Diff 174477.Nov 16 2018, 4:25 PM

Write down full messages in tests. When the message was updated from 'x' is moved' to Object 'x' is moved, the tests were not updated because they kept passing because the former is still a sub-string of the latter.

lib/StaticAnalyzer/Checkers/MoveChecker.cpp
385–386

Mmm, what do you mean?

test/Analysis/use-after-move.cpp
789–791

Note how when an object is both a local and an STL object, the bug is reported as an STL bug. Which is good because it's more dangerous to use an STL object after move than to use your average local variable after move.

MTC added a subscriber: MTC.Nov 16 2018, 11:44 PM

The "moved-from" terminology we adopt here still feels a bit weird to me, but i don't have a better suggestion, so i just removed the single-quotes so that to at least feel proud about what we have.

I am personally fine with this terminology, this checker corresponds to the cert rule EXP63-CPP. Do not rely on the value of a moved-from object, and moved from is also used in many places in CppCoreGuidelines.

In D54560#1301870, @NoQ wrote:

Write down full messages in tests. When the message was updated from 'x' is moved' to Object 'x' is moved, the tests were not updated because they kept passing because the former is still a sub-string of the latter.

It would not be cool to rewrite this contains effect to equality like in the regex file-checks? I have ran in the same problem and I was very surprised (for an hour) why my error-dumps does not show up on rewriting bug-report messages.

Szelethus added inline comments.Nov 20 2018, 10:16 AM
lib/StaticAnalyzer/Checkers/MoveChecker.cpp
385–386

explain.* sounds like it either returns a string, or writes a stream object, but the return type isn't void nor string, maybe it'd be worth to put this comment in-class.

But yea, this is over the top nitpicking, I don't insist :)

Looks good! Some suggested minor tweaks to diagnostic text inline.

test/Analysis/use-after-move.cpp
146

"Moved-from object is copied 'a'" doesn't read quite right. I think the object name is in the wrong spot. Instead, I would suggest: "Moved-from object 'a' is copied"

789–791

I think the diagnostic text is missing an "a". It should be "... left in a valid but ... "

NoQ updated this revision to Diff 176504.Dec 3 2018, 3:49 PM
NoQ marked 4 inline comments as done.

Address comments!

In D54560#1301870, @NoQ wrote:

Write down full messages in tests. When the message was updated from 'x' is moved' to Object 'x' is moved, the tests were not updated because they kept passing because the former is still a sub-string of the latter.

It would not be cool to rewrite this contains effect to equality like in the regex file-checks? I have ran in the same problem and I was very surprised (for an hour) why my error-dumps does not show up on rewriting bug-report messages.

We already have // expected-warning-re{{{{^Message$}}}} for that purpose, it's just annoying to write and is rarely an issue, so nobody uses them until it's absolutely necessary (cf. test/Analysis/explain-svals.cpp). Same with FileCheck tests.

lib/StaticAnalyzer/Checkers/MoveChecker.cpp
385–386

Fxd!

test/Analysis/use-after-move.cpp
146

Whoops!!

NoQ added a comment.Dec 3 2018, 5:50 PM
In D54560#1302051, @MTC wrote:

The "moved-from" terminology we adopt here still feels a bit weird to me, but i don't have a better suggestion, so i just removed the single-quotes so that to at least feel proud about what we have.

I am personally fine with this terminology, this checker corresponds to the cert rule EXP63-CPP. Do not rely on the value of a moved-from object, and moved from is also used in many places in CppCoreGuidelines.

Double-checked - this terminology does indeed come from the Standard:

15.5.5.15 Moved-from state of library types [lib.types.movedfrom]
1 Objects of types defined in the C++ standard library may be moved from (10.3.4.2). Move operations may be explicitly specified or implicitly generated. Unless otherwise specified, such moved-from objects shall be placed in a valid but unspecified state.

This revision was automatically updated to reflect the committed changes.