Page MenuHomePhabricator

ADT: Fix reference invalidation in SmallVector::resize
ClosedPublic

Authored by dexonsmith on Wed, Dec 23, 2:05 PM.

Details

Summary

For small enough, trivially copyable T, take the parameter by-value in
SmallVector::resize. Otherwise, when growing, update the arugment
appropriately.

Depends on https://reviews.llvm.org/D93780.
Split out from https://reviews.llvm.org/D91837.

Diff Detail

Event Timeline

dexonsmith created this revision.Wed, Dec 23, 2:05 PM
dexonsmith requested review of this revision.Wed, Dec 23, 2:05 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Dec 23, 2:05 PM
dblaikie accepted this revision.Wed, Dec 23, 3:48 PM

Looks good (did you happen to try this with sanitizers without the fix applied & observe a sanitizer failure when running the updated test? Would be nice to know that's actually working as intended)

This revision is now accepted and ready to land.Wed, Dec 23, 3:48 PM

Looks good (did you happen to try this with sanitizers without the fix applied & observe a sanitizer failure when running the updated test? Would be nice to know that's actually working as intended)

No, but just now I ran with the SmallVector.h part of this patch mostly reverted (didn't add back the assertion), and the test fails:

/Users/dexonsmith/data/github/llvm-project/working/llvm/unittests/ADT/SmallVectorTest.cpp:1139: Failure
      Expected: N
      Which is: 3
To be equal to: V.back()
      Which is: 8-byte object <01-00 00-00 00-00 00-00>
/Users/dexonsmith/data/github/llvm-project/working/llvm/unittests/ADT/SmallVectorTest.cpp:1145: Failure
      Expected: 1
To be equal to: V.back()
      Which is: 8-byte object <01-04 00-00 00-00 00-00>

The line numbers are different vs. the patch posted here because of a rebase, but this seems to confirm the test is working even without sanitizers.

About to commit (check-llvm just finished on the rebase); I can follow up with ASan post-commit if you'd like (let me know if you would).

This revision was landed with ongoing or failed builds.Wed, Jan 13, 8:48 PM
This revision was automatically updated to reflect the committed changes.

Looks good (did you happen to try this with sanitizers without the fix applied & observe a sanitizer failure when running the updated test? Would be nice to know that's actually working as intended)

No, but just now I ran with the SmallVector.h part of this patch mostly reverted (didn't add back the assertion), and the test fails:

/Users/dexonsmith/data/github/llvm-project/working/llvm/unittests/ADT/SmallVectorTest.cpp:1139: Failure
      Expected: N
      Which is: 3
To be equal to: V.back()
      Which is: 8-byte object <01-00 00-00 00-00 00-00>
/Users/dexonsmith/data/github/llvm-project/working/llvm/unittests/ADT/SmallVectorTest.cpp:1145: Failure
      Expected: 1
To be equal to: V.back()
      Which is: 8-byte object <01-04 00-00 00-00 00-00>

The line numbers are different vs. the patch posted here because of a rebase, but this seems to confirm the test is working even without sanitizers.

About to commit (check-llvm just finished on the rebase); I can follow up with ASan post-commit if you'd like (let me know if you would).

Looks good to me as-is - thanks!