Avoid dangling reference when indexing sections

[serge-sans-paille]

Summary

Bug spotted by https://cookieplmonster.github.io/2020/02/01/emulator-bug-llvm-bug/

The blog post proposes a simple solution (using a std::deque instead of a smallvector to store section entries). They also propose an alternative, namely to avoid taking any section reference at all. This patch enforces the latter using a proxy class. I'm totally fine with the dequeue approach too, and will gladly update the patch if needed.

[nickdesaulniers]

Drive by code review, no comment on the bug or proposed solution.

llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyld.cpp

83 ↗

(On Diff #246411)

How come SectionEntryProxy is sometimes passed by reference, but other times passed by value to different functions in this CL?

llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldELF.h

105 ↗

(On Diff #246411)

Every time I see Sections[] in RuntimeDyldELF.cpp, I think "This should be using getSection()."

llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldMachO.h

134 ↗

(On Diff #246411)

If this is the same between MachO and ELF (and I suspect all other derived classes), can it be a method on a shared base class?

llvm/lib/ExecutionEngine/RuntimeDyld/Targets/RuntimeDyldELFMips.cpp

33 ↗

(On Diff #246411)

Why not a &? Won't this result in a copy-assignment?

[lhames]

Thanks very much for this Serge!

Is the std::deque fix a one-liner? This shouldn't be very performance sensitive, so I'd be inclined to go with that.

[serge-sans-paille]

In D75110#1891890, @lhames wrote:

Thanks very much for this Serge!

Is the std::deque fix a one-liner? This shouldn't be very performance sensitive, so I'd be inclined to go with that.

It is! Compared to this patch, which was a pleasure to write nonetheless, I'd go for the <dequeue> approach ;-)

[serge-sans-paille]

Use the <deque> approach

[serge-sans-paille]

Fix include order

[serge-sans-paille]

@lhames is that okay like this?

[lhames]

Looks good to me. Thanks Serge!

[grimar]

Nasty bug. LGTM too.

llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h

257

LLVM code usually uses "std::" prefix before STL containers names and algorithms.
(See std::map below and other variables).

[serge-sans-paille]

@grimar ok, change added to the commit.

llvm/lib/ExecutionEngine/RuntimeDyld/RuntimeDyldImpl.h

 #include "llvm/ExecutionEngine/RuntimeDyldChecker.h"
 #include "llvm/Object/ObjectFile.h"
 #include "llvm/Support/Debug.h"
 #include "llvm/Support/ErrorHandling.h"
 #include "llvm/Support/Format.h"
 #include "llvm/Support/Host.h"
 #include "llvm/Support/Mutex.h"
 #include "llvm/Support/SwapByteOrder.h"
+#include <deque>
 #include <map>
 #include <system_error>
 #include <unordered_map>
 
 using namespace llvm;
 using namespace llvm::object;
 
 namespace llvm {
protected:
   // The MemoryManager to load objects into.
   RuntimeDyld::MemoryManager &MemMgr;
 
   // The symbol resolver to use for external symbols.
   JITSymbolResolver &Resolver;
 
   // A list of all sections emitted by the dynamic linker.  These sections are
   // referenced in the code by means of their index in this list - SectionID.
-  typedef SmallVector<SectionEntry, 64> SectionList;
+  // Because references may be kept while the list grows, use a container that
+  // guarantees reference stability.
+  typedef std::deque<SectionEntry> SectionList;
   SectionList Sections;
 
   typedef unsigned SID; // Type for SectionIDs
 #define RTDYLD_INVALID_SECTION_ID ((RuntimeDyldImpl::SID)(-1))
 
   // Keep a map of sections from object file to the SectionID which
   // references it.
   typedef std::map<SectionRef, unsigned> ObjSectionToIDMap;