martong marked an inline comment as done.
martong added inline comments.
================
Comment at: clang/lib/StaticAnalyzer/Checkers/StdLibraryFunctionsChecker.cpp:409
- Optional<Summary> FoundSummary = findFunctionSummary(FD, CE, C);
+ for (const ValueConstraintPtr& VC : Summary.ArgConstraints) {
+ ProgramStateRef SuccessSt = VC->apply(State, Call, Summary);
----------------
NoQ wrote:
> NoQ wrote:
> > martong wrote:
> > > NoQ wrote:
> > > > martong wrote:
> > > > > NoQ wrote:
> > > > > > Maybe we should add an assertion that the same argument isn't
> > > > > > specified multiple times.
> > > > > I think there could be cases when we want to have e.g. a not-null
> > > > > constraint on the 1st argument, but also we want to express that the
> > > > > 1st argument's size is described by the 2nd argument. I am planning
> > > > > to implement such a constraints in the future. In that case we would
> > > > > have two constraints on the 1st argument and the assert would fire.
> > > > Wait, i misunderstood the code. It's even worse than that: you're
> > > > adding transitions in a loop, so it'll cause state splits for every
> > > > constraint. Because you do not intend to create multiple branches here,
> > > > there needs to be exactly one `addTransition` performed every time
> > > > `checkPreCall` is called. I.e., for now this code is breaking
> > > > everything whenever there's more than one constraint, regardless of
> > > > whether it's on the same argument.
> > > Yeah, that's a very good catch, thanks! I am going to prepare a patch to
> > > fix this soon. My idea is to store the `SuccessSt` and apply the next
> > > argument constraint on that. And once the loop is finished I'll have call
> > > the `addTransition()`.
> > Yup, that's the common thing to do in such cases.
> While we're at it, could you try to come up with a runtime assertion that'll
> help us prevent these mistakes?
>
> Like, dunno, make `CheckerContext` crash whenever there's more than one
> branch being added, and then add a method to opt out when it's actually
> necessary to add more transitions (i.e., the user would say
> `C.setMaxTransitions(2)` at the beginning of their checker callback whenever
> they need to make a state split, defaulting to 1). It's a bit tricky because
> i still want to allow multiple transitions when they allow one branch (i.e.,
> transitions chained together) but i think it'll take a lot of review anxiety
> from me because it's a very dangerous mistake to make and for now code review
> is the only way to catch it. So, yay, faster code reviews.
Hmm I see your point and I agree this would be a valuable sanity check. But if
you don't mind I'd like to address this in a different and stand-alone patch
(independently from the quick-fix https://reviews.llvm.org/D76790) because it
does not seem to be trivial for me.
My first concern is this: if we have `1` as the default value for
`maxTranisitions` then we should add an extra `C.setMaxTransitions(N)` in every
checker callback that does a state split, is that right?
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D73898/new/
https://reviews.llvm.org/D73898
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
