Hey Ted, Took a while to find the time, attached is the latest patch. Notes below.
On 04 Dec 2013, at 00:24, Ted Kremenek <[email protected]> wrote: > > To start off, this file is missing the standard LLVM boilerplate, which > mentions the name of the file and what its purpose is. In this case, a brief > description of the purpose of the checker seems warranted. > Comments and boilerplate added, methods renamed. >> VersionTuple deploymentTarget(ProgramStateRef State) const; > > I don’t think this method is even needed. More comments below. > There are still a couple of places where this is needed. For example, checking if the introduced version of a function is later than the deployment target, or to output the deployment target in the bug report. >> VersionTuple getCheckedForVersion(ProgramStateRef State) const; >> ProgramStateRef setCheckedForVersion(ProgramStateRef State, >> VersionTuple V) const; >> }; >> } >> >> REGISTER_TRAIT_WITH_PROGRAMSTATE(CheckedVersionMajor, unsigned) >> REGISTER_TRAIT_WITH_PROGRAMSTATE(CheckedVersionMinor, unsigned) >> REGISTER_TRAIT_WITH_PROGRAMSTATE(CheckedVersionSubminor, unsigned) > > Having three maps is far less efficient than having one map for VersionTuple. > It’s not just the size of the individual pieces of data; its the metadata to > represent the map itself. All of these seemed to be changed together in > lockstep, it is is far more efficient to both retrieve and set data from one > map instead of 3. To understand the efficiency, each map is represented by a > functional AVL binary tree. Having three trees requires balancing and > querying three trees, as well as the memory associated with three trees. > Yes, this was supposed to be temporary while I worked out how to stuff a VersionTuple into the program state. After spending a bit of time on this, I am still none the wiser. What is the easiest way to achieve this? To use a single element list of VersionTuple as the type, or is there some other way to extend the PartialProgramState to handle a VersionTuple? >> >> VersionTuple Introduced = versionIntroduced(D); >> >> // If we have an ObjC method with no introduced version, check the class's >> or >> // the protocol's introduced version. >> const NamedDecl *VersionDecl = cast<NamedDecl>(D); >> if (Introduced.empty()) { >> if (const ObjCMethodDecl *MD = dyn_cast<ObjCMethodDecl>(D)) { >> const DeclContext *DC = MD->getDeclContext(); >> if (const ObjCProtocolDecl *PD = dyn_cast<ObjCProtocolDecl>(DC)) { >> Introduced = versionIntroduced(PD); >> VersionDecl = PD; >> } else { >> const ObjCInterfaceDecl *ID = MD->getClassInterface(); >> Introduced = versionIntroduced(ID); >> VersionDecl = ID; >> } >> } >> } > > This looks like this can be hoisted directly into “versionIntroduced” (or > rather, “getVersionIntroduced”). That way getVersionIntroduced() computes > the truth as a self-encapsulated information, regardless of whether it > consults the map or has to compute some of the information lazily. Also, it > doesn’t look like all of the callers to getVersionIntroduced() compute this > information consistently; hoisting it into getVersionIntroduced() would solve > that problem. > Done, although it is a little bit messy as we need both the version tuple and the decl from which it came, so the getVersionIntroduced function now returns a pair. >> >> if (TargetMinVersion >= Introduced) >> return; >> >> // We have a method being called that is available later than the minimum >> // deployment target, so we need to check for any availability checks. >> VersionTuple CheckedVersion = getCheckedForVersion(State); >> if (CheckedVersion >= Introduced) >> return; >> >> if (ExplodedNode *N = Ctx.addTransition(State)) { >> displayError(State, Introduced, VersionDecl, N, &Ctx.getBugReporter()); >> } > > This is good, but maybe we should only emit a warning once along a path. I’m > concerned about a cascade of warnings. For example, say I had 10 function > calls, one after the other, that were used unsafely, but would be safely > guarded with a single check. We really don’t need 10 warnings; we only need > one. If we stopped tracking version information along a path, we could > perhaps only emit the warning once? One way to do that would be to pump up > the checked version to something really high and store that in the state. A > call to ‘addTransition()’ doesn’t stop the path; the analyzer will keep > exploring a path. > Is it not sufficient to just change this to a generateSink instead of addTransition? >> void ento::registerUnavailableMethodChecker(CheckerManager &mgr) { >> mgr.registerChecker<UnavailableMethodChecker>(); > > As mentioned earlier, you can put some checking for deployment information > right here. If the context doesn’t support running the checker, just don’t > run it at all. > This is a nice idea, but how can we get hold of the context here? >> } > > For the test cases, they look like a good start. One thing I’d like to see > is a test case that shows two unsafe calls one after the other. For example, > you currently have: > >> - (void)doVersion30InstanceStuff { >> [super doVersion30InstanceStuff]; // no warning >> [self doVersion40InstanceStuff]; // expected-warning {{Calling method >> introduced in OS X 40.0 (deployment target is 10.5.0)}} >> } > > > Try adding a third call here, maybe just repeating the second line: > >> - (void)doVersion30InstanceStuff_B { >> [super doVersion30InstanceStuff]; // no warning >> [self doVersion40InstanceStuff]; // expected-warning {{Calling method >> introduced in OS X 40.0 (deployment target is 10.5.0)}} >> [self doVersion40InstanceStuff]; // expected-warning {{Calling method >> introduced in OS X 40.0 (deployment target is 10.5.0)}} >> } > > > Right now I’d expect to see two warnings. It would be great to get them down > to one (just the first one). > Done. > Overall this is looking really great. Thanks so much for working on this! > > Cheers, > Ted Other than the state map and the checker registration, I think I have everything else covered. Let me know what you think. Ta, Richard. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
