We need to start using the -Woverloaded-virtual option in the makefiles to catch some inheritance bugs. Patches for this bug should first rid all of LLVM of warnings produced by -Woverloaded-virtual option and then patch the makefiles to turn the option on. This idea submitted by Vladimir Prus where he made the following comments on LLVMdev list: I've just had some fun, because I wanted to override FunctionPass::addAnalysisUsage, but forgot "const" after the method name -- so instead of overriding I've just created a new unrelated method. After spending some time on this, I've decided to add -Woverloaded-virtual option to compiler to catch such cases. However, it also gives some warnings on LLVM code: ../../../include/llvm/Pass.h:264: warning: `virtual bool llvm::FunctionPass::run(llvm::Module&)' was hidden ../../../include/llvm/Pass.h:326: warning: by `bool llvm::BasicBlockPass::run(llvm::BasicBlock&)' ../../../include/llvm/Pass.h:275: warning: `virtual void llvm::FunctionPass::addToPassManager(llvm::PassManagerT<llvm::Module>*, llvm::AnalysisUsage&)' was hidden ../../../include/llvm/Pass.h:332: warning: by `virtual void llvm::BasicBlockPass::addToPassManager(llvm::PassManagerT<llvm::BasicBlock>*, llvm::AnalysisUsage&)' The problem is that "run" method is virtual in Path (with Module& as argument), but another version (non-virtual), which takes BasicBlock& is defined in BasicBlockPath. Do you think this warning is worth fixing? One possible approach is to rename virtual run(Module&) to runOnModule, but that would require updating all derived classes :-(
Created attachment 145 [details] List Of 158 Overload Hiding Instances This file shows all 158 occurrences where -Woverloaded-virtual generates a warning about overloaded virtuals. Fortunately, the damage is contained to a few files with numerous overloaded virtuals in each file. These all need to be investigated and fixed before we can turn on -Woverloaded-virtual.
The attached file was generated from my work on the bytecode analyzer which has a large number of virtual functions. Several output bugs were caused by interface changes that caused unintentional overloading to occur. To combat this, I turned on -Woverloaded-virtual to detect the errors. The attachment shows all the overloading as of the date of the file. There is likely some bugs lurking in there.
Okay, well the first biggest step is to rectify the ones in the Pass class: virtual void print(std::ostream &O, const Module *M) const { print(O); } virtual void print(std::ostream &O) const; There is no reason for the second one. We should just change any who implement it to implement the first one. -Chris
Mine
Schedule for 1.5
This patch fixes 1/2 of the warnings, those coming from utils/TableGen/Record.h: http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041206/021811.html
These patches fix Pass::print http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041206/021812.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041206/021813.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041206/021814.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041206/021815.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041206/021816.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041206/021817.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041206/021818.html
Some patches to fix some Pass class related virtual overloading: http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041206/021830.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041206/021831.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041206/021832.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041206/021833.html http://mail.cs.uiuc.edu/pipermail/llvm-commits/Week-of-Mon-20041206/021834.html
At this point, the only remaining problem is: /proj/work/llvm/build/../llvm/include/llvm/Pass.h:292: warning: `virtual void llvm::FunctionPass::addToPassManager(llvm::PassManagerT<llvm::Module>*, llvm::AnalysisUsage&)' was hidden /proj/work/llvm/build/../llvm/include/llvm/Pass.h:350: warning: by `virtual void llvm::BasicBlockPass::addToPassManager(llvm::PassManagerT<llvm::Function>*, llvm::AnalysisUsage&)' However, my attempts to fix this one have not succeeded. Doing the obvious thing and making both overloadings available in both classes doesn't help. The problem comes with PassManagerT and what kinds of things can call "addPass" on it. Perhaps someone who's cozy and intimate with the PassManager can take a look at this?
A recitation of the problem: The -Woverloaded-virtual flag is in GCC for a really good reason; becase the language requires that when a sub-class does not provide ALL overloadings of a virtual function, that the base class versions are completely hidden from the subclass. If you have this: class X { pubic: virtual void nada(int); }; class Y : public X { public: virtual void nada(double); }; then there is no way to call nada(int) from a Y*. The language requires that nada(double) "hides" X::nada(int). The -Woverloaded-virtual option exposes these situations and this bug is about making LLVM compile without warning when -Woverloaded-virtual is used. Using our example above, there are two ways to solve this problem: (a) change Y::nada to a new name or (b) change X to have a nada(double) method. Unfortunately, the patches so far have chosen (b) as the solution so that the method names don't have to change and there is no need to visit all the call sites. This is unfortunate because method (b) is leading to a bloated API for LLVM and requiring "not implemented" implementations of super-class methods. Consequently, we're going to switch to option (a) for solution. Its more changes, but will lead to a better API in the end run.
Note that there's yet another method: add using X::nada; in the declaration of class Y. This is actually pretty clean approach. However, it does not work when X::nada is private, as is the case with those AddToPassManager functions.
With the recent modifications to the PassManager and a few other places, this bug is (finally!) resolved. We now generate warnings on overloaded virtuals by turning on the -Woverloaded-virtual option. If you get this warning, please resolve the overload before committing the code.