Allow enabling and disabling tracking of ObjC/CF objects
separately from tracking of OS objects.
ObjC and CF are still grouped together, as we can't rename existing checkers.
Differential D55400
[analyzer] Move out tracking retain count for OSObjects into a separate checker george.karpenkov on Dec 6 2018, 4:30 PM. Authored by
Details
Allow enabling and disabling tracking of ObjC/CF objects ObjC and CF are still grouped together, as we can't rename existing checkers.
Diff Detail Event Timeline
Comment Actions I'm working on rebasing my dependency branches on top of trunk, and I'm somewhat stuck on this patch, could you lend a hand please? So, I'm seeing in essence 3 checkers: the class RetainCountChecker, that does the modeling, and two checkers that are exposed to the users, osx.cocoa.RetainCount and osx.OSObjectRetainCount. While generally the solution would be to register checker called RetainCountBase, that is a common base of both of the already registered checkers, it raises a serious problem: all of the plist files that say "this error originates from checker osx.cocoa.RetainCount" will now instead refer to osx.cocoa.RetainCountBase. Unfortunately, I can't just make both checkers have the same name, as how would the user disable one and not the other? There were other checkers that suffered from this, but those affected very few test files, and this one hits around 9, and I know that very serious work is being put into it from your part. I'd like to propose 3 solutions, and I'm very much open to new ones:
Comment Actions
What does it mean?
I'm not sure what do you propose exactly, but sounds pretty bad.
I don't think we can rename the old checker, as older Xcode versions have "osx.cocoa.RetainCount" hardcoded in them. TBH I'm not really sold on the way we "bundle" multiple checkers (from the tablegen) into a single class. Essentially, we have a single checker class with two checks, which should be toggle-able orthogonally from each other. Comment Actions Poor wording on my end, sorry about that. Let me clarify. When a report is emitted in the plist output, it'll also that which checker is "responsible" for that warning. I vaguely remember reading that this isn't used that much in your application, but we use this information extensively here. Now, what I'd propose, is register a new, osx.RetainCountBase checker, and make both of the already existing checkers depend on that. Long story short, this will make the name of the checker that is listed in that plist entry osx.RetainCountBase, as opposed to osx.cocoa.RetainCount, but this doesn't sound that illogical, considering that both of those checkers emit the same checker name anyways (meaning, that if one report originates from osx.OSObjectRetainCount and one from osx.cocoa.RetainCount, both reports will be listed as they originated from osx.cocoa.RetainCount).
It does. I think IvarInvalidation uses something like that, and it's both unsustainable and very ugly.
Yea, this one makes little sense. Should've just left this one out really.
Do I understand correctly that you refer that the way checker names are generated is very poor?
I think the current system would be fine as long as there would be a way to hide these implementation detail checkers, which could also be defined in tblgen. Having a dependency tree would be neat, and also somewhat unavoidable in order to move forward with some very important changes I'm planning, like listing all checker options.
Hmmm, does this mess with options that bad? Could you please clarify? Comment Actions
registerChecker gets-or-creates a checker object. A checker name (used for getting the options) is set the first time it's created. I think the current situation is a mess - ideally I would prefer to be able to access the options in the constructor, but we can't even do that, Comment Actions Sure, in fact its already implemented and got accepted by @NoQ, I just didnt have the time to commit and fine tune :) The solution is essentially to create CheckerManager::getChecker alongside registerChecker, and making them in order assert if the checker is unregistered, and assert when the checked is already registered. In an earlier patch, I implement handling of dependencies on a higher level, essentially making CheckerRegistry take care of this, instead of making checker registry functions register multiple checkers, or, like in this case, register a common base. Before the registry function for either RetainCound or OSObjectRetainCount is called (which will now invoke getChecker), the registry function for RetainCountBase will be called, that will register the actual checker object. You can take a look at how this would look like in D54438. Would love to hear your thoughts on this! :)
The proposed solution solves this issue as well. |
This comment needs to go up a bit.