This is an archive of the discontinued LLVM Phabricator instance.

[clang][Checkers] Extend PthreadLockChecker state dump (NFC).
ClosedPublic

Authored by balazske on Mar 12 2021, 6:32 AM.

Diff Detail

Event Timeline

balazske created this revision.Mar 12 2021, 6:32 AM
balazske requested review of this revision.Mar 12 2021, 6:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 12 2021, 6:32 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

This patch makes one TODO less and it is possible to debug cases like in the next (in stack) change.

NoQ added a comment.Mar 16 2021, 1:15 PM

That's what i like to see!

You can test this via clang_analyzer_printState().

clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
344

I think this should be passive. The mutex doesn't actively destroy anybody.

balazske added inline comments.Mar 17 2021, 1:31 AM
clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
344

Maybe "Mutex destroy calls with unknown result:"? This is a title of a list, not about a single mutex.

NoQ added inline comments.Mar 17 2021, 2:04 AM
clang/lib/StaticAnalyzer/Checkers/PthreadLockChecker.cpp
344

Oh that's what you meant, I see.

"Mutexes in unresolved possibly destroyed state:"?

balazske updated this revision to Diff 331484.Mar 18 2021, 1:36 AM

Change of text in dump, add a test.

balazske marked an inline comment as done.Mar 18 2021, 1:37 AM
balazske updated this revision to Diff 331485.Mar 18 2021, 1:39 AM

Add accidentally removed empty line.

steakhal added inline comments.Mar 20 2021, 9:56 AM
clang/test/Analysis/pthreadlock_state.c
56

Out of curiosity, what is the purpose of this 'empty' line? I've seen it many times in other tests but I still don't know.

71

I recommend not hardcoding symbol identifier numbers. reg$NN, conj_$NN, LCNN, SNNNN
It would result in a less fragile test.
I'm pretty sure you can use some regexp to achieve this.

balazske updated this revision to Diff 332250.Mar 22 2021, 4:56 AM

Add regular expression to test, removed space characters from line endings.

balazske marked an inline comment as done.Mar 22 2021, 4:58 AM
balazske added inline comments.
clang/test/Analysis/pthreadlock_state.c
56

This is an issue with the state dump, the checker does not print the empty line but the state dump shows it always.

steakhal added inline comments.Mar 22 2021, 5:00 AM
clang/test/Analysis/pthreadlock_state.c
71

AFAIK only the line prefix is checked.
So you could use simply "SymRegion{reg_${{[0-9]+}}<pthread_mutex_t * mtx1>}: conj_$"

Make sure you mask the number out of the reg_$12 as well, etc.

balazske updated this revision to Diff 332293.Mar 22 2021, 8:16 AM

Simplified the test somewhat.

steakhal accepted this revision.Mar 30 2021, 4:06 AM

Everything looks fine.
I like that regexp matcher so much. <3

This revision is now accepted and ready to land.Mar 30 2021, 4:06 AM
balazske added inline comments.Mar 31 2021, 5:36 AM
clang/test/Analysis/pthreadlock_state.c
59

This test fails at some buildbots because ordering of the lines ("mtx" and "SymRegion") is reversed.

steakhal added inline comments.Mar 31 2021, 5:41 AM
clang/test/Analysis/pthreadlock_state.c
59

Uh yea. That's a mapping over pointer values - which is nondeterministic.

You have two options. Either rework the test to have a single entry in that mapping at most, or sort them somehow.
TBH I think the first is the easier :D

Test failures occurred, fix of the problem is not trivial, probably the test file should be split.

balazske reopened this revision.Apr 1 2021, 12:50 AM

Test should be updated.

This revision is now accepted and ready to land.Apr 1 2021, 12:50 AM
balazske updated this revision to Diff 334609.Apr 1 2021, 12:51 AM

Split the test file.

steakhal accepted this revision.Apr 1 2021, 12:56 AM
steakhal added inline comments.
clang/test/Analysis/pthreadlock_state.c
2

AFAIK core should be enabled for all checks. Same for the other file.

balazske updated this revision to Diff 334619.Apr 1 2021, 1:56 AM

Add 'core' to enabled checks in test.