APValues and Constants and MaterializedTemporaryExpr need to have stable maps
ClosedPublic
Actions

Authored by majnemer on Jul 30 2015, 12:10 AM.

Details

Reviewers

Commits

rG2dcef9e0a413: Avoid iteration invalidation issues around MaterializedTemporaryExpr
rC244989: Avoid iteration invalidation issues around MaterializedTemporaryExpr
rL244989: Avoid iteration invalidation issues around MaterializedTemporaryExpr

Summary

We risk iterator invalidation issues if we use DenseMap structures for
MaterializedTemporaryExprs. Use a std::map to ensure that they don't
move around in memory.

This fixes PR24289.

Diff Detail

Repository: rL LLVM

Event Timeline

majnemer updated this revision to Diff 30999.Jul 30 2015, 12:10 AM

majnemer retitled this revision from to APValues and Constants and MaterializedTemporaryExpr need to have stable maps.

majnemer updated this object.

majnemer added a reviewer: rsmith.

majnemer added a subscriber: cfe-commits.

rsmith added inline comments.Aug 13 2015, 3:10 PM

include/clang/AST/ASTContext.h
180 ↗	(On Diff #30999)	I would prefer a `DenseMap<const MaterializedTemporaryExpr, APValue>`, with `APValue`s allocated by the `ASTContext`'s `BumpPtrAllocator`.

Address Justin's review comments.
Address Richard's review comments.

rsmith added inline comments.Aug 13 2015, 3:56 PM

lib/AST/ASTContext.cpp
8557 ↗	(On Diff #32100)	Maybe use a reference here rather than a pointer.
8559–8560 ↗	(On Diff #32100)	More succinctly expressed as: MTVI = new (*this) APValue;
8565–8566 ↗	(On Diff #32100)	This is now just `MaterializedTemporaryValues.lookup(E)`
lib/CodeGen/CodeGenModule.h
377 ↗	(On Diff #32100)	Can you fix this instead by not holding onto `Slot` in `CodeGenModule::GetAddrOfGlobalTemporary`? (Do the map lookup again at the end of the function.) Twice as many `DenseMap` lookups seems likely to be cheaper than switching to a `std::map`.