[Lldb-commits] [PATCH] Force iteration over FieldDecls and RecordDecls in deterministic order.

[Zachary Turner]

Hi clayborg, majnemer, jingham, amccarth, ovyalov,

Previously we were, in many places in the code, using a DenseMap<> to hold FieldDecls representing class members, and RecordDecls representing bases and virtual bases.

In some instances, we would try to copy records by iterating over their fields and bases and adding each one to a new AST.  This yields a non-deterministic ordering of the results, and leads to invalid record layout.  Specifically, clang expects field index order to be in offset order, and the field index order is determined by the order you add items to a record.  So when synthesizing a new AST, you *must* add items in field offset order.  Therefore, copying by iterating over a DenseMap of FieldDecls or RecordDecls for bases and inserting them this way yields invalid record layout.

This bug is present on all platforms and all ABIs, but was only surfaced on Windows with the MS ABI record layout because of extra checks and asserts present in MicrosoftRecordLayoutBuilder::layoutField.  Also, this requirement of importing in a deterministic order applies to bases and virtual bases too, not just fields.

The fix employed here is to change all occurrences of DenseMap<A,B> as it pertains to ASTs to std::vector<std::pair<A, B>>.  There were only a very few instances of anyone using the DenseMaps for lookup, and given the typical number of fields a class has, it seems like a reasonable tradeoff to ensure that this kind of problem can never happen again.  A std::vector<> should also require less memory than a DenseMap.  I'm not sure if there are tons of SymbolFileDWARF::LayoutInfos lying around in memory, but if there are this could have an impact on memory footprint too.

Since this problem was present everywhere it's unclear how -- if at all -- it was manifesting or what types of issues this might fix for everyone else.  The problem may have been masked if ASLR was disabled or depending on what allocator was used.  On Windows it definitely fixes a whole slew of test failures.  I know Linux has been seeing some non-deterministic failures as well, so one of the Linux people shoudl see if this patch makes a difference.

http://reviews.llvm.org/D8512

Files:
  include/lldb/Expression/ClangASTSource.h
  include/lldb/Symbol/ClangExternalASTSourceCallbacks.h
  source/Expression/ClangASTSource.cpp
  source/Plugins/LanguageRuntime/ObjC/AppleObjCRuntime/AppleObjCDeclVendor.cpp
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.h
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.cpp
  source/Plugins/SymbolFile/DWARF/SymbolFileDWARFDebugMap.h
  source/Symbol/ClangExternalASTSourceCallbacks.cpp

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/
-------------- next part --------------
A non-text attachment was scrubbed...
Name: D8512.22407.patch
Type: text/x-patch
Size: 28868 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/lldb-commits/attachments/20150321/d251e302/attachment.bin>