Page MenuHomePhabricator

[analyzer] pr41335: NoStoreFuncVisitor: Fix crash when no-store event is in a body-farmed function.
ClosedPublic

Authored by NoQ on Apr 16 2019, 5:48 PM.

Diff Detail

Repository
rC Clang

Event Timeline

NoQ created this revision.Apr 16 2019, 5:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2019, 5:48 PM
NoQ updated this revision to Diff 195497.Apr 16 2019, 5:51 PM

Fix comments.

NoQ added a comment.EditedApr 16 2019, 10:17 PM

Hmm, i think i'd love to know why doesn't the uninitialized variable checker fire on the if-statement as farmed by the body farm:

592   // Signature:
593   // _Bool OSAtomicCompareAndSwapPtr(void *__oldValue,
594   //                                 void *__newValue,
595   //                                 void * volatile *__theValue)
596   // Generate body:
597   //   if (oldValue == *theValue) {
598   //    *theValue = newValue;
599   //    return YES;
600   //   }
601   //   else return NO;

(closing brace accidentally omitted in the original comment as well)

alexfh accepted this revision.Apr 17 2019, 2:58 PM
alexfh added a subscriber: alexfh.

LG with a couple of nits.

clang/lib/StaticAnalyzer/Core/BugReporterVisitors.cpp
582 ↗(On Diff #195497)

Should this be marked with a FIXME?

clang/test/Analysis/diagnostics/body-farm-crashes.c
1 ↗(On Diff #195497)

Add a space before the backslash.

This revision is now accepted and ready to land.Apr 17 2019, 2:58 PM
NoQ planned changes to this revision.Apr 17 2019, 5:35 PM
In D60808#1469734, @NoQ wrote:

Hmm, i think i'd love to know why doesn't the uninitialized variable checker fire on the if-statement as farmed by the body farm:

Passing arguments to this whole body farm thing doesn't work. It builds the body for the declaration on line 4 but then calls the declaration on line 5, and parameter variables in the synthesized body don't match parameter variables of the call, so it cannot read argument values :/

NoQ updated this revision to Diff 195810.Apr 18 2019, 1:52 PM

Don't canonicalize the decl in the body farm. The decl supplied by the AnalysisDeclContext is already the correct one (and not necessarily the canonical one).

Keep the defensive behavior for NoStoreFuncVisitor because it's generally the right thing to do for future-proofness.

This revision is now accepted and ready to land.Apr 18 2019, 1:52 PM
NoQ added a comment.Apr 18 2019, 1:52 PM

(also fix typo in the function name)

NoQ updated this revision to Diff 195850.Apr 18 2019, 5:59 PM

Separate the canonicalization change that unbreaks body farms into a separate patch.

NoQ updated this revision to Diff 195851.Apr 18 2019, 6:01 PM
NoQ marked an inline comment as done.

Add space before \.

This revision was automatically updated to reflect the committed changes.