This is an archive of the discontinued LLVM Phabricator instance.

[Sema] SequenceChecker: Handle references, members and structured bindings.
Changes PlannedPublic

Authored by riccibruno on Feb 3 2019, 11:20 AM.

Details

Summary

Currently the checker for unsequenced operations is not able to see through members and references. For example the following code is not detected:

struct S { unsigned x; } s;
s.x + s.x++;

Improve this by teaching the unsequenced operation checker about references, members (including static members) and structured bindings. To do this introduce a class MemoryLocation which will approximate C++ memory locations. Then modify the method getObject (renamed to getMemoryLocation) to follow references, handle members and look through bindings. Also update the diagnostic messages.

With this patch the sequence checker do not handle bit-fields in any particular way, but ideally we should take into account that two distinct bit-fields can be in the same memory location. This is marked as TODO.

Diff Detail

Event Timeline

riccibruno created this revision.Feb 3 2019, 11:20 AM
riccibruno updated this revision to Diff 185048.Feb 4 2019, 7:01 AM

Added tests for nested/anonymous unions and local structs.

Rebased. Does this implementation make sense ?

riccibruno planned changes to this revision.Feb 19 2019, 8:28 AM

This needs more work.

riccibruno retitled this revision from [Sema] SequenceChecker: Handle references and members to [Sema] SequenceChecker: Handle references, members and structured bindings..
riccibruno edited the summary of this revision. (Show Details)
riccibruno edited the summary of this revision. (Show Details)Mar 14 2019, 10:38 AM

Added some context in the description of the patch. Ping !

Friendly ping. One thing I am wondering about is whether MemoryLocation and getMemoryLocation is duplicating something that is already present somewhere else. It feels like something similar should already exist but I can't find anything (but that is not saying much).

Friendly ping.

Added some reviewers

Patch improves suboptimal diagnostic, which misses bugs like:
https://bugs.llvm.org/show_bug.cgi?id=43052

Added some reviewers

Patch improves suboptimal diagnostic, which misses bugs like:
https://bugs.llvm.org/show_bug.cgi?id=43052

Thanks for taking a look !

I like this improvement. However I'm not a reviewer.

clang/lib/Sema/SemaChecking.cpp
12775

Please use \p here since it's not a parameter definition.

xbolva00 added inline comments.Dec 9 2019, 3:32 AM
clang/lib/Sema/SemaChecking.cpp
13059–13060

You can land some NFC changes in separate commit.

I like this improvement. However I'm not a reviewer.

Thanks for looking at the patch!

You can land some NFC changes in separate commit.

Yep, indeed. Will do when I rebase it.

xbolva00 added a comment.EditedDec 17 2019, 9:24 AM

Hello @aaron.ballman / @rsmith, can you please take a look on this patch?

riccibruno edited the summary of this revision. (Show Details)

I have factored out various NFCs which were present in this patch. This should make review easier.
Also addressed some inline comments.

Note that I am not entirely sure that the implementation in getMemoryLocation/MemoryLocation is the right way to do this; I think that this patch should be considered a work-in-progress.

riccibruno marked an inline comment as done.Dec 18 2019, 6:42 AM

Add missing patch context

rsmith added inline comments.Jun 2 2020, 5:56 PM
clang/include/clang/Basic/DiagnosticSemaKinds.td
2031–2033

I wonder if we could just print out a simple access path here (eg x.a[3]). APValue has support for representing and pretty-printing such things already, and you could try passing arbitrary lvalue expressions to the constant evaluator to try to form them rather than special-casing a few things in getMemoryLocationImpl.

clang/lib/Sema/SemaChecking.cpp
12782–12783

Per the above, I think this can be replaced by calling EvaluateAsLValue on the expression.

riccibruno marked an inline comment as done.Jun 6 2020, 5:15 AM
riccibruno added inline comments.
clang/include/clang/Basic/DiagnosticSemaKinds.td
2031–2033

Ah yes perhaps this can work. I will take a look at that. Thanks for the pointer!

riccibruno planned changes to this revision.Jun 8 2020, 1:42 PM