[PATCH] [ELF] Use llvm ADT's instead of std.

Rui Ueyama ruiu at google.com
Wed Feb 25 12:27:00 PST 2015


I usually don't apply someone else's patch to test Chromium build, but this should not affect the Windows port anyway, because this only changes the ELF port.

But this change may be riskier than you might think because of the subtle difference of semantics between std::map and llvm::DenseMap regarding references returned by operator[] and iterators. For std::map, it is guaranteed that inserting a new element does not invalidate existing references to a std::map. It doesn't invalidate iterators unless you remove an element and an iterator is pointing to the element.

These properties are not guaranteed by DenseMap. So, if you add a new item to a map while you are iterating over elements of the map, it may crash. It actually crashes only when the hash table is resized to add a new element for LHS, so it could produce a nasty flaky bug. For example the bug in the LayoutPass was there for more than 1 year until I fixed that.

Even

m[x] = m[y]

is not guaranteed to be safe for a DenseMap m, because a reference returned by m[y] can be invalidated by m[x]. This particular one is the bug in LayoutPass.cpp that Reid mentioned above (fix is r213969).

I checked the code quickly, and it looks safe to me, but please review your patch again with the above thing in your mind.


http://reviews.llvm.org/D7885

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/






More information about the llvm-commits mailing list