Bug in LocationContextManager

Hi,

there seems to be a bug in LocationContextManager preventing it from doing what it is IMHO supposed to do: the member variable Contexts holds LocationContext (and profile them accordingly), but the two insert functions profile the actual types. Thus if getStackFrame is called, always a new LocationContext is created. Making the Profile method in LocationContext virtual would solve the issue. Patch attached.

Best
Olaf Krzikalla

analysisctx.patch (488 Bytes)

Hi Olaf,

Other than adding clarity, this patch doesn't change program semantics:

- void Profile(llvm::FoldingSetNodeID &ID) {
+ virtual void Profile(llvm::FoldingSetNodeID &ID) {
      Profile(ID, Kind, Ctx, Parent);
    }

Since this version of 'Profile' is defined as 'virtual' in llvm::FoldingSetNode, it is implicit defined as virtual in LocationContext.

For clarity, I've gone ahead and added the virtual keywords to these methods.

Ted

Hi Ted,

Ted Kremenek schrieb:

Since this version of 'Profile' is defined as 'virtual' in llvm::FoldingSetNode, it is implicit defined as virtual in LocationContext.

No, that's IMHO not right. FoldingSetImpl::Node, to which llvm::FoldingSetNode is typedefed to, has no Profile method at all. Instead there is a static dispatch in FoldingSetTrait which normally redirects to a Profile method of the instantiated class. Thus the virtual keyword is needed in LocationContext (believe me, it solved the bug ;-).

Best Olaf

Hi Olaf,

I apologize, you're absolutely right. For some reason I remember it being the other way, but now realize that I was completely mistaken. Thanks for pointing this out!

Fortunately it was not causing bug because only AddPointer() and
AddInteger() are used in the profiling. But anyway they should be
virtual.

Zhongxing Xu schrieb:

Fortunately it was not causing bug because only AddPointer() and
AddInteger() are used in the profiling.

Indeed it did. In LocationContextManager::getStackFrame a static call to StackFrameContext::Profile profiles the (maybe) new element.
The elements in the container however were profiled using LocationContext::Profile only.

Best Olaf

Zhongxing Xu schrieb:

Fortunately it was not causing bug because only AddPointer() and
AddInteger() are used in the profiling.

Indeed it did. In LocationContextManager::getStackFrame a static call to
StackFrameContext::Profile profiles the (maybe) new element.
The elements in the container however were profiled using
LocationContext::Profile only.

Ah, right. FoldingSetImpl::FindNodeOrInsertPos() uses the Profile() in
trait. The old implementation was not virtual. So only the method in
the base class was used. Thanks Olaf.