This is an archive of the discontinued LLVM Phabricator instance.

Fix UBSan error reports in ValueMapCallbackVH and AssertingVH<T> empty/tombstone keys generation.
ClosedPublic

Authored by samsonov on Jan 9 2015, 12:50 PM.

Details

Summary

One more attempt to fix UBSan reports: make sure DenseMapInfo::getEmptyKey()
and DenseMapInfo::getTombstoneKey() doesn't do any upcasts/downcasts to/from Value*.

Diff Detail

Event Timeline

samsonov updated this revision to Diff 17941.Jan 9 2015, 12:50 PM
samsonov retitled this revision from to Fix UBSan error reports in ValueMapCallbackVH and AssertingVH<T> empty/tombstone keys generation..
samsonov updated this object.
samsonov edited the test plan for this revision. (Show Details)
samsonov added reviewers: chandlerc, dexonsmith.
samsonov added subscribers: majnemer, Unknown Object (MLST).
chandlerc added inline comments.Jan 9 2015, 1:02 PM
include/llvm/IR/ValueHandle.h
239–248

I think you need to use DenseMapInfo<Value *> here?

samsonov updated this revision to Diff 17945.Jan 9 2015, 1:30 PM

Address Chandler's comment.

include/llvm/IR/ValueHandle.h
239–248

Sure. Fixed.

chandlerc added inline comments.Jan 9 2015, 1:52 PM
include/llvm/IR/ValueHandle.h
192–203

I would define helpers here for getRawValPtr and setRawValPtr which do the indirection between NDEBUG and debug builds. Then use them below (see comments).

240–275

All of these NDEBUG vs. not can go away by using helpers defined once in the class above to get the raw Value* and set the raw Value*.

I also think you want to have both isEqual and getHashValue use the raw Value* and delegate to DenseMapInfo<Value *>.

samsonov updated this revision to Diff 17949.Jan 9 2015, 2:26 PM

Address comments.

include/llvm/IR/ValueHandle.h
192–203

Done

240–275

Done. However, what if some subclass MyValue of Value provides its own DenseMapInfo<MyValue*>::isEqual or DenseMapInfo<MyValue*>::getHashValue ?

chandlerc accepted this revision.Jan 9 2015, 2:36 PM
chandlerc edited edge metadata.

LGTM as well, thanks!!! Sorry for the circles this round around in!

include/llvm/IR/ValueHandle.h
240–275

As Duncan said, I think that would be weird. I might go further and say I think using such an AssertingVH<MyValue> would be a bug. We can't both test for equality using their delegated routine *and* not use their type's pointers. I think it is reasonable to insist for simplicity that Value subclasses that want DenseMaps of AssertingVHs must be happy with DenseMapInfo<Value*> or generally DenseMapInfo<T*>.

This revision is now accepted and ready to land.Jan 9 2015, 2:36 PM

Thanks, submitting this now!

include/llvm/IR/ValueHandle.h
240–275

Right, I agree with that, just wanted to double-check my understanding.

This revision was automatically updated to reflect the committed changes.