Hey Ted, any feedback on this one?
Ta, Richard On 15 Dec 2013, at 22:28, Richard <[email protected]> wrote: > Oops, this time with the patch attached. > > <unavailable-1512.patch> > > On 15 Dec 2013, at 22:27, Richard <[email protected]> wrote: > >> 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
