[PATCH] D19556: [ThinLTO] Emit individual index files for distributed backends

Fri May 6 17:11:08 PDT 2016

tejohnson added a comment.

I'm at a loss as to what is going on with the msan failures. Maybe it will be obvious to someone else, so posting what I found so far here.

Unfortunately, I can make the msan failure go away any number of ways (including building at a lower level of optimization or adding any print statements, which is making debugging challenging). I also put a log from a debugging session below to show what info I can get.

With my own msan built compiler, I can reproduce most but not all of the failures from the bot. The case I have been looking at is with the distributed_indexes.ll test added with this patch. In that case when we create the IndexBitcodeWriter we have a non-null ModuleToSummariesForIndex pointer, so we don't initialize the IndexSummaries* pointers. When I step through in the debugger, the range based for loop in the IndexBitcodeWriter constructor executes the expected paths in the begin() and end() iterator constructors (not shown in the below trace). After the first for loop body executes, I can see where we execute the iterator's operator++() and operator==() paths for the expected ModuleToSummariesForIndex!=null case. But then oddly we seem to jump back to operator++() handling on the wrong path (that we would execute if ModuleToSummariesForIndex==null), and is where we attempt to access and get the msan error on the IndexSummaries* iterator accesses.

A few things that make it go away, but I don't understand why (may be luck):

1. Initializing the IndexSummariesBack and IndexSummariesIter unconditionally in the constructor (we can do this safely since the Index is always passed in).
2. Re-initializing IndexSummariesBack in the operator++() code right before we access it (although that access shouldn't happen as noted above).

For both 1) and 2) I don't understand why these would help except luck, since we shouldn't be executing the path that accesses these iterators. Is there something about the way they are declared that would make the compiler think it can hoist the accesses so that they are unconditionally executed?

3. Change the range based for loop to be a non-range for loop: for (IndexBitcodeWriter::iterator I = begin(); I != end(); ++I) GUIDToValueIdMap[(*I).first] = ++GlobalValueId;
4. Change the range based for loop to make a copy of the value instead of assigning to a const reference: for (auto I : *this) GUIDToValueIdMap[I.first] = ++GlobalValueId;

For both 3) and 4) this avoids binding the return of the iterator::operator* (which is a std::pair) to a const reference. However, looking at operator* which invokes make_pair on a GUID and summary pointer to create the return value, I think the const reference should extend the lifetime of the returned temporary through the loop body shouldn't it? In any case, this doesn't explain why I am seeing the wrong path of the operator++() being executed after we appear to already have done the operator++ and operator==.

Here's the debugging log, with some comments sprinkled in:

Starting program: /usr/local/google/home/tejohnson/llvm/llvm_msan/bin/llvm-lto -thinlto-action=distributedindexes -thinlto-index /usr/local/google/home/tejohnson/llvm/llvm_msan/test/ThinLTO/X86/Output/distributed_indexes.ll.tmp.index.bc /usr/local/google/home/tejohnson/llvm/llvm_msan/test/ThinLTO/X86/Output/distributed_indexes.ll.tmp1.bc /usr/local/google/home/tejohnson/llvm/llvm_msan/test/ThinLTO/X86/Output/distributed_indexes.ll.tmp2.bc

Breakpoint 3, IndexBitcodeWriter () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:285
285	      : BitcodeWriter(Buffer), Index(Index),

// entered the IndexBitcodeWriter constructor, use next to get to the body of the first iteration:

(gdb) n
286	        ModuleToSummariesForIndex(ModuleToSummariesForIndex) {
(gdb) 
285	      : BitcodeWriter(Buffer), Index(Index),
(gdb) 
286	        ModuleToSummariesForIndex(ModuleToSummariesForIndex) {
(gdb) 
281	  IndexBitcodeWriter(SmallVectorImpl<char> &Buffer,
(gdb) 
275	  unsigned GlobalValueId = 0;
(gdb) 
291	    for (const auto &I : *this)
(gdb) 
292	      GUIDToValueIdMap[I.first] = ++GlobalValueId;

// single step from here. First set of instructions are the emplace into map from line 292:

(gdb) s
operator[] () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:292
292	      GUIDToValueIdMap[I.first] = ++GlobalValueId;
(gdb) 
__emplace_unique_key_args<unsigned long, const std::__1::piecewise_construct_t &, std::__1::tuple<const unsigned long &>, std::__1::tuple<> > () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:292
292	      GUIDToValueIdMap[I.first] = ++GlobalValueId;
(gdb) 
__insert_node_at () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:292
292	      GUIDToValueIdMap[I.first] = ++GlobalValueId;
(gdb) 
size () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:292
292	      GUIDToValueIdMap[I.first] = ++GlobalValueId;
(gdb) 
first () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:292
292	      GUIDToValueIdMap[I.first] = ++GlobalValueId;
(gdb) 
first () at /usr/local/google/home/tejohnson/llvm/llvm_msan/build-libcxx-msan/include/c++/v1/memory:2312
2312	    _LIBCPP_INLINE_VISIBILITY _T1_reference       first() _NOEXCEPT       {return __first_;}
(gdb) 
__emplace_unique_key_args<unsigned long, const std::__1::piecewise_construct_t &, std::__1::tuple<const unsigned long &>, std::__1::tuple<> > () at /usr/local/google/home/tejohnson/llvm/llvm_msan/build-libcxx-msan/include/c++/v1/__tree:2013
2013	    __node_base_pointer& __child = __find_equal(__parent, __k);
(gdb) 
__find_equal<unsigned long> () at /usr/local/google/home/tejohnson/llvm/llvm_msan/build-libcxx-msan/include/c++/v1/__tree:1887
1887	    if (__nd != nullptr)
(gdb) 
__emplace_unique_key_args<unsigned long, const std::__1::piecewise_construct_t &, std::__1::tuple<const unsigned long &>, std::__1::tuple<> > () at /usr/local/google/home/tejohnson/llvm/llvm_msan/build-libcxx-msan/include/c++/v1/__tree:2023
2023	        __insert_node_at(__parent, __child, static_cast<__node_base_pointer>(__h.get()));
(gdb) 
__insert_node_at () at /usr/local/google/home/tejohnson/llvm/llvm_msan/build-libcxx-msan/include/c++/v1/__tree:1997
1997	    ++size();

// Now enter the operator++() in the range for loop, executes the Writer.ModuleToSummariesForIndex!=null case as expected:

(gdb) 
357	        if (ModuleGVSummariesIter == ModuleSummariesIter->second.end() &&
(gdb) list
352	      // First the inner iterator is incremented, then if it is at the end
353	      // and there are more outer iterations to go, the inner is reset to
354	      // the start of the next inner list.
355	      if (Writer.ModuleToSummariesForIndex) {
356	        ++ModuleGVSummariesIter;
357	        if (ModuleGVSummariesIter == ModuleSummariesIter->second.end() &&
358	            ModuleSummariesIter != ModuleSummariesBack) {
359	          ++ModuleSummariesIter;
360	          ModuleGVSummariesIter = ModuleSummariesIter->second.begin();
361	        }
(gdb) up
#1  IndexBitcodeWriter () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:291
291	    for (const auto &I : *this)
(gdb) down
#0  operator++ () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:357
357	        if (ModuleGVSummariesIter == ModuleSummariesIter->second.end() &&
(gdb) s
end () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:357
357	        if (ModuleGVSummariesIter == ModuleSummariesIter->second.end() &&
(gdb) 
end () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:357
357	        if (ModuleGVSummariesIter == ModuleSummariesIter->second.end() &&
(gdb) 
__end_node () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:357
357	        if (ModuleGVSummariesIter == ModuleSummariesIter->second.end() &&
(gdb) 
first () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:357
357	        if (ModuleGVSummariesIter == ModuleSummariesIter->second.end() &&
(gdb) 
first () at /usr/local/google/home/tejohnson/llvm/llvm_msan/build-libcxx-msan/include/c++/v1/memory:2312
2312	    _LIBCPP_INLINE_VISIBILITY _T1_reference       first() _NOEXCEPT       {return __first_;}
(gdb) 
operator++ () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:358
358	            ModuleSummariesIter != ModuleSummariesBack) {
(gdb) s
operator!= () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:358
358	            ModuleSummariesIter != ModuleSummariesBack) {
(gdb) s
operator!= () at /usr/local/google/home/tejohnson/llvm/llvm_msan/build-libcxx-msan/include/c++/v1/__tree:810
810	        {return !(__x == __y);}
(gdb) 
operator++ () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:357
357	        if (ModuleGVSummariesIter == ModuleSummariesIter->second.end() &&
(gdb) 
IndexBitcodeWriter () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:291
291	    for (const auto &I : *this)

// Next we execute the operator!= to check if for loop at end():

(gdb) s
operator!= () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:291
291	    for (const auto &I : *this)
(gdb) 
operator== () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:291
291	    for (const auto &I : *this)
(gdb) up
#1  operator!= () at llvm/include/llvm/ADT/iterator.h:97
97	    return !static_cast<const DerivedT *>(this)->operator==(RHS);
(gdb) up
#2  IndexBitcodeWriter () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:291
291	    for (const auto &I : *this)
(gdb) down
#1  operator!= () at llvm/include/llvm/ADT/iterator.h:97
97	    return !static_cast<const DerivedT *>(this)->operator==(RHS);
(gdb) down
#0  operator== () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:291
291	    for (const auto &I : *this)
(gdb) s
operator== () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:291
291	    for (const auto &I : *this)

// This is the Writer.ModuleToSummariesForIndex!=null case of operator==() as expected:

(gdb) up
#1  operator== () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:395
395	        return ModuleGVSummariesIter == RHS.ModuleGVSummariesIter;
(gdb) s
operator== () at /usr/local/google/home/tejohnson/llvm/llvm_msan/build-libcxx-msan/include/c++/v1/__tree:807
807	        {return __x.__ptr_ == __y.__ptr_;}
(gdb) s
IndexBitcodeWriter () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:291
291	    for (const auto &I : *this)

// Here's where things go weird. This is the Writer.ModuleToSummariesForIndex==null case of operator++():

(gdb) s
operator++ () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:364
364	        if (IndexGVSummariesIter == IndexSummariesIter->second.end() &&
(gdb) up
#1  IndexBitcodeWriter () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:291
291	    for (const auto &I : *this)
(gdb) down
#0  operator++ () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:364
364	        if (IndexGVSummariesIter == IndexSummariesIter->second.end() &&
(gdb) s
365	            IndexSummariesIter != IndexSummariesBack) {
(gdb) s
operator!= () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:365
365	            IndexSummariesIter != IndexSummariesBack) {
(gdb) s
operator!= () at /usr/local/google/home/tejohnson/llvm/llvm_msan/build-libcxx-msan/include/c++/v1/__tree:886
886	        {return !(__x == __y);}
(gdb) up
#1  operator!= () at /usr/local/google/home/tejohnson/llvm/llvm_msan/build-libcxx-msan/include/c++/v1/map:798
798	        {return __x.__i_ != __y.__i_;}
(gdb) up
#2  operator++ () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:365
365	            IndexSummariesIter != IndexSummariesBack) {
(gdb) down   
#1  operator!= () at /usr/local/google/home/tejohnson/llvm/llvm_msan/build-libcxx-msan/include/c++/v1/map:798
798	        {return __x.__i_ != __y.__i_;}
(gdb) down
#0  operator!= () at /usr/local/google/home/tejohnson/llvm/llvm_msan/build-libcxx-msan/include/c++/v1/__tree:886
886	        {return !(__x == __y);}
(gdb) s
operator++ () at llvm/lib/Bitcode/Writer/BitcodeWriter.cpp:364
364	        if (IndexGVSummariesIter == IndexSummariesIter->second.end() &&
(gdb) s

Breakpoint 2, __msan_warning_noreturn ()

  at /usr/local/google/home/tejohnson/llvm/llvm_msan/llvm/projects/compiler-rt/lib/msan/msan.cc:362

362	void __msan_warning_noreturn() {

Repository:
  rL LLVM

http://reviews.llvm.org/D19556