Page MenuHomePhabricator

APValues and Constants and MaterializedTemporaryExpr need to have stable maps
ClosedPublic

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

Details

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 APValues allocated by the ASTContext's BumpPtrAllocator.

majnemer updated this revision to Diff 32100.Aug 13 2015, 3:32 PM
  • 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.

majnemer updated this revision to Diff 32106.Aug 13 2015, 4:20 PM
  • Address Richard's latest review comments.
rsmith accepted this revision.Aug 13 2015, 4:22 PM
rsmith edited edge metadata.

LGTM

This revision is now accepted and ready to land.Aug 13 2015, 4:22 PM
This revision was automatically updated to reflect the committed changes.