This is an archive of the discontinued LLVM Phabricator instance.

ADT: Add assertions to SmallVector::insert, etc., for reference invalidation
ClosedPublic

Authored by dexonsmith on Nov 18 2020, 2:33 PM.

Details

Summary

2c196bbc6bd897b3dcc1d87a3baac28e1e88df41 asserted that
SmallVector::push_back doesn't invalidate the parameter when it needs
to grow. Do the same for resize, append, assign, insert, and
emplace_back.

Diff Detail

Event Timeline

dexonsmith created this revision.Nov 18 2020, 2:33 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 18 2020, 2:33 PM
Herald added a subscriber: ributzka. · View Herald Transcript
dexonsmith requested review of this revision.Nov 18 2020, 2:33 PM
mehdi_amini added inline comments.Nov 18 2020, 2:54 PM
llvm/include/llvm/ADT/SmallVector.h
774

Do you need the SFINAE and the two implementation?
Wouldn't it always be valid to call this->assertSafeToAdd(&Arg);?

dexonsmith edited the summary of this revision. (Show Details)

Updates:

  • Per @mehdi_amini's suggestion, check all arguments of emplace_back.
  • Check ranges in the iterator range versions of assign, append, and insert.
dexonsmith marked an inline comment as done.Nov 18 2020, 4:00 PM
dexonsmith added inline comments.
llvm/include/llvm/ADT/SmallVector.h
774

In fact we can also check for internal references in the multi-argument versions of emplace_back. The new patch does this... thanks!

mehdi_amini accepted this revision.Nov 18 2020, 4:30 PM
This revision is now accepted and ready to land.Nov 18 2020, 4:30 PM
dexonsmith marked an inline comment as done.

Running check-all for llvm/clang/clang-tools-extra hit three more assertions, now fixed (since splitting the patch out from https://reviews.llvm.org/D91467 I'd just run ADTTests until now):

  • Two MCInst::insert calls.
  • One emplace_back call with an internal reference.

I'll commit in a minute.

I committed a follow-up for Windows at 90966daac3dd633b9621426e6621a706807d7c0d.