________________________________________ From: Jordan Rose [[email protected]]
> Seeing this code is really making me think we need to expose a way to > look up macros in the analyzer, even if it only handles simple ones... Agreed. I have one quick follow-up question, inline below. > On Mar 14, 2014, at 9:53 , Meyer, Conrad <[email protected]> wrote: >> + if (!Val_M_ZERO.hasValue()) { >> + if (C.getASTContext().getTargetInfo().getTriple().getOS() >> + == >> llvm::Triple::FreeBSD) >> + Val_M_ZERO = 0x0100; >> + else if (C.getASTContext().getTargetInfo().getTriple().getOS() >> + == >> llvm::Triple::NetBSD) >> + Val_M_ZERO = 0x0002; >> + else if (C.getASTContext().getTargetInfo().getTriple().getOS() >> + == >> llvm::Triple::OpenBSD) >> + Val_M_ZERO = 0x0008; > > Let's factor that common call out of the if-statement, so we don't > have to wrap it. (The LLVM way to wrap it would be to put the == at > the end of the line, then break, then indent to just past the open > paren.) Sure, I can fix that. > >> + else >> + // FIXME: We need a more general way of getting the M_ZERO >> value. >> + // See also: O_CREAT in UnixAPIChecker.cpp. >> + >> + // Fall back to normal malloc behavior on platforms where we >> don't >> + // know M_ZERO. >> + State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), >> State); >> + } >> + >> + if (Val_M_ZERO.hasValue()) { >> + const Expr *FlagsEx = CE->getArg(2); >> + const SVal V = State->getSVal(FlagsEx, C.getLocationContext()); >> + if (!V.getAs<NonLoc>()) { >> + // The case where 'V' can be a location can only be due to a >> bad header, >> + // so in this case bail out. >> + return; >> + } >> + >> + NonLoc Flags = V.castAs<NonLoc>(); >> + NonLoc ZeroFlag = C.getSValBuilder() >> + .makeIntVal(Val_M_ZERO.getValue(), >> FlagsEx->getType()).castAs<NonLoc>(); >> + SVal MaskedFlagsUC = C.getSValBuilder().evalBinOpNN(State, BO_And, >> + Flags, >> ZeroFlag, >> + >> FlagsEx->getType()); >> + if (MaskedFlagsUC.isUnknownOrUndef()) >> + return; > > This could be unknown if the argument value is unknown (i.e. the > analyzer has failed to symbolize it for some reason). In that case, we > should probably still treat this as a regular malloc. Agreed. It's probably obvious, but I'm not super familiar with Clang internals — should we also consider the previous case (!V.getAs<NonLoc>()) as regular malloc as well? > >> + DefinedSVal MaskedFlags = MaskedFlagsUC.castAs<DefinedSVal>(); >> + >> + // Check if maskedFlags is non-zero. >> + ProgramStateRef TrueState, FalseState; >> + std::tie(TrueState, FalseState) = State->assume(MaskedFlags); >> + >> + // If M_ZERO is set, treat this like calloc (initialized). >> + if (TrueState && !FalseState) >> + State = CallocMem(C, CE); >> + else >> + State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), >> State); >> + } >> + } > > The "fall-back" case has been duplicated twice already, and I just > suggested adding it a third time. How about adding a helper function > for three-argument malloc that returns an Optional<ProgramStateRef>, > and if it returns None, we just do the normal malloc thing? That way > we can use early returns to simplify all this. > > (Why Optional<ProgramStateRef>? In case there's ever a reason for > three-argument malloc to return a null state, i.e. it detects some > state that shouldn't be reached.) This sounds like the right approach. Okay. > >> + } else if (FunI == II_valloc) { >> if (CE->getNumArgs() < 1) >> return; >> State = MallocMemAux(C, CE, CE->getArg(0), UndefinedVal(), State); > > This is generally looking good. Thanks for bearing with me on these > change requests! > > Jordan Thanks! I appreciate your patience with me — it is looking a lot better than when we started. Conrad _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
