Page MenuHomePhabricator

[analyzer] Fix crash on spaceship operator (PR47511)
ClosedPublic

Authored by vsavchenko on Mar 23 2021, 5:41 AM.

Diff Detail

Event Timeline

vsavchenko created this revision.Mar 23 2021, 5:41 AM
vsavchenko requested review of this revision.Mar 23 2021, 5:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 23 2021, 5:41 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

I'm always in favor of bugfixes. However, I have some concerns about this one.

clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
169–171 ↗(On Diff #332640)

What a madness, we deal with every binary operator kind here O.O

Why do we get Undefined here? If we don't model something - theoretically - we should get Unknown.
I'm confused.

clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
331 ↗(On Diff #332640)

So, there are some corner cases already.
I can't see any way of implementing this reasonably here, as you pointed out the return value of the function is not a good fit.

Shouldn't put your mock 'implementation' to a different place?
I'm just nitpicking though.

vsavchenko added inline comments.Mar 24 2021, 7:48 AM
clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
331 ↗(On Diff #332640)

evalAPSInt is called in a lot of different contexts, I don't know how else to ensure that it won't crash on BO_Cmp in all of those contexts.

steakhal accepted this revision.Mar 31 2021, 12:32 AM
steakhal added inline comments.
clang/lib/StaticAnalyzer/Core/BasicValueFactory.cpp
331 ↗(On Diff #332640)

I don't know. It doesn't feel like a strong argument. I still don't feel that this is the right direction.
The code already looks convoluted and unnecessarily complex, your change wouldn't worsen that too much.
Considering that it actually fixes something, eh, so be it.

This revision is now accepted and ready to land.Mar 31 2021, 12:32 AM
NoQ added inline comments.Apr 5 2021, 9:36 AM
clang/lib/StaticAnalyzer/Checkers/UndefResultChecker.cpp
169–171 ↗(On Diff #332640)

Yes we should get Unknown. We should only return Undefined when there's undefined behavior in the program. Sounds like returning nullptr from BasicValueFactory causes SValBuilder to produce Undefined instead. I believe this needs to be fixed in SValBuilder instead.

loic-joly-sonarsource added inline comments.
clang/test/Analysis/PR47511.cpp
6

For such a simple test, there is no need to use the full-fledged header, the following would suffice:

namespace std { 
struct strong_ordering {
  int n;
  constexpr operator int() const { return n; }
  static const strong_ordering equal, greater, less;
};
constexpr strong_ordering strong_ordering::equal = { 0 };
constexpr strong_ordering strong_ordering::greater = { 1 };
constexpr strong_ordering strong_ordering::less = { -1 };
} // namespace std
vsavchenko updated this revision to Diff 336108.Apr 8 2021, 7:55 AM
vsavchenko marked 4 inline comments as done.

Change produced SVal for the spaceship operator from Undefined to Unknown

NoQ accepted this revision.Apr 8 2021, 9:15 AM

Looks great now!

This revision was automatically updated to reflect the committed changes.