Minidump debugging using the native PDB reader

[lemo]

Summary

A few changes required to enable the use of the native PDB reader when debugging minidumps:

1. Consume PDBs even if the module binary is not available Implementing a PlaceholderObjectFile to complement the existing PlaceholderModule

2. Use the minidump exception record if present

3. Add support for "target symbols add" with PDBs

Overall this is a modest step towards improving the minidump debugging experience. But more importantly,
it enables the basic consumption of PDBs using the NativePDB(TM) reader as a foundation to add more functionality incrementally.

[amccarth]

This looks good to me, save for a couple comment nits.

source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp

155

Every apparently valid PDB or certain ones? If it's particular ones, then perhaps we should create a bug report with a repro to make it easier to investigate in the future.

source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

133

Did you mean "directory" instead of "directly"?

Also, this is worded as a TODO, but it describes the current situation. Describing the current situation is fine, but I'd like the TODO to more clearly say what's do be done. I gather that's "implement a proper SymbolVendor."

[zturner]

source/Plugins/Process/minidump/ProcessMinidump.cpp

60

Should we return something valid for this function?

source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.h

92

This method name is a little bit confusing. Is it referring to a numeric index like 1, 2, 3, 4, 5. Or a CompilandIndexItem? I would call this something like HasCompilandInfo(uint16_t modi)

source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp

132–135

What kind of symbols have a segment and offset of 0?

150–157

If we're going to comment it out, then just delete the code IMO.

Do you have some llvm-pdbutil output that demonstrates this on a valid PDB? I guess we'd be looking for 2 symbols with the same Virtual Address. Seems odd to have that, but maybe an example of where it's happening would make it clear whether this is actually a valid case.

source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

106

Perhaps obj_file should be a reference just to clarify that it can't be null.

114–115

Instead of trying to load it as a PE/COFF fail and then trying something else if it fails (hoping that it's a minidump), perhaps we could use llvm::identify_magic() to figure out what it is actually is first. That function currently cannot detect a Windows minidump, but we coudl teach it to support that. I think that would make this code more logical.

if (type == exe) {
} else if (type == minidump) {
} else {
  // actual error
}

[labath]

I don't see any tests :(.

Also, the three bullet points in the description sound like rather independent pieces of functionality. Would it be possible to split them up into separate patches? That would make things easier to review, particularly for those who don't look at this code very often :).

[lemo]

In D55142#1316153, @labath wrote:

I don't see any tests :(.
Also, the three bullet points in the description sound like rather independent pieces of functionality. Would it be possible to split them up into separate patches? That would make things easier to review, particularly for those who don't look at this code very often :).

I agree with both comments. The intention is to add some tests but I wanted to get the review out early to surface concerns, if any. I also needed more time to investigate a few complex failures uncovered by this change (ex. https://bugs.llvm.org/show_bug.cgi?id=39882 and https://bugs.llvm.org/show_bug.cgi?id=39897)

Yes, this change can also be split in three parts: the reason it's bundled up in this review is that all three parts are required to enable the basic functionality (and overall it's a relatively small change). Maybe it was better if I sent out the parts separately, but right now I'd like to preserve the continuity in the review comments.
I'm about to send out a new revision and once this review satisfies all the comments I'll split it out and send individual reviews.

source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp

132–135

 68 | S_LDATA32 [size = 44] `trace_event_unique_atomic140`
      type = 0x0013 (__int64), addr = 0000:0000
112 | S_LDATA32 [size = 44] `trace_event_unique_atomic171`
      type = 0x0013 (__int64), addr = 0000:0000
156 | S_LDATA32 [size = 44] `trace_event_unique_atomic196`
      type = 0x0013 (__int64), addr = 0000:0000

...

Tracked by : https://bugs.llvm.org/show_bug.cgi?id=39882

150–157

I spent more time to investigate this and opened a bug to track it: https://bugs.llvm.org/show_bug.cgi?id=39897

With very large PDBs it's not easy to have a definitive verdict so a 2nd opinion is welcome, but as I noted in the bug is seems that ICF is one case where we can end up with multiple symbols at the same address. This means that the data structure needs to be revisited.

For now, I think it's better to suppress the assert so we can exercise the rest of the native PDB reader (I updated the comment to point to the new bug I just opened)

source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

106

That would make sense. Unfortunately, obj_file can't be const (ObjectFile::GetUUID() is non const and it's not easy to surgically fix it)

So we'd have to pass by non-const ref, which would read "out-parameter", which IMO is more confusing than the non-null part is worth.

114–115

We need to load the binary anyway (this part was kept from the original code). If we can't find the binary we just look for the PDB in the current directly, none of which is specific to minidumps.

I'm sure there's a more elegant solution but again, this is a (hopefully short lived) stop-gap that should go away.

[markmentovai]

This seems like a step in the right direction.

[labath]

In D55142#1320447, @lemo wrote:

I agree with both comments. The intention is to add some tests but I wanted to get the review out early to surface concerns, if any. I also needed more time to investigate a few complex failures uncovered by this change (ex. https://bugs.llvm.org/show_bug.cgi?id=39882 and https://bugs.llvm.org/show_bug.cgi?id=39897)

Yes, this change can also be split in three parts: the reason it's bundled up in this review is that all three parts are required to enable the basic functionality (and overall it's a relatively small change). Maybe it was better if I sent out the parts separately, but right now I'd like to preserve the continuity in the review comments.
I'm about to send out a new revision and once this review satisfies all the comments I'll split it out and send individual reviews.

Sounds good. I think it would be nice to see the isolated patches standing next to their tests.

source/Plugins/Process/minidump/ProcessMinidump.cpp

123–125

I don't know whether you'll run into any problems because of this, but the way this works for "normal" object files is that the each object file has a list of "own" sections, and then the Module has a "unified" list, which contains the sections of the main object file possibly combined with sections from the symbol object files. Here you are adding a section to the unified list without adding it to the object file, which is a bit nonstandard. I think it would be better to just add it to both places.

source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

106

I don't think the ObjectFile& would be an out-parameter any more than the existing BumpPtrAllocator& is an out-parameter. So making this a reference would also make things locally consistent.

150

make this sizeof(guid) for consistency.

165–168

Mainly out of curiosity, what's the complication here? The llvm interface does not provide the means to retrieve the age?

[lemo]

Incorporating feedback + adding a test case

[lemo]

source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

165–168

Yes, the interface doesn't expose the age, although that's not not really the problem (the age is a detail - part of the generic "Debug ID")

We just need to make the handling of PE/COFF & VC UUIDs more consistent: right now most places expect a stripped version (just the GUID). This is wrong since a correct PDB match should take the age into account, but it's outside the scope of this change.

[clayborg]

BTW: check my changes in: https://reviews.llvm.org/D55522
It will be interesting to you since it parses the linux maps info if it is available in breakpad generated minidump files. This will give us enough info to create correct sections for object files when we have no ELF file or no symbol file (ELF/DWARF or breakpad).

[labath]

This looks fine to me. The test is a bit more coarse-grained than I'd like, though I can't think of a substantially better approach right now. One of the pdb folks should ok this too.

source/Plugins/Process/minidump/ProcessMinidump.cpp

375

Since you're changing this, you might as well change the type to something more appropriate (size_t?).

[zturner]

How large is the PDB file here?

[clayborg]

FYI: my approach to getting symbols to load was a bit different. I always let a simple PlaceholderModule be created, but I played some tricks in the GetObjectFile() method if someone had setting symbols for the module with "target symbols add ..". I will attach my PlaceholderModule so you can see what I had done. Since these modules always must be associated with a target, I keep a weak pointer to the target in the constructor. Then later, if someone does "target symbols add ..." the module will have Module:: m_symfile_spec filled in, so I set the m_platform_file to be m_file, and then move m_symfile_spec into m_file and then call Module::GetObjectFile(). This is nice and clean because you don't have to make up fake symbols to fake sections. When you create the placeholder module it needs the target:

auto placeholder_module =
    std::make_shared<PlaceholderModule>(module_spec, GetTarget());

Here is the copy of the PlaceholderModule that does what was discussed above:

//------------------------------------------------------------------
/// A placeholder module used for minidumps, where the original
/// object files may not be available (so we can't parse the object
/// files to extract the set of sections/segments)
///
/// This placeholder module has a single synthetic section (.module_image)
/// which represents the module memory range covering the whole module.
//------------------------------------------------------------------
class PlaceholderModule : public Module {
public:
  PlaceholderModule(const ModuleSpec &module_spec, Target& target) :
    Module(module_spec.GetFileSpec(), module_spec.GetArchitecture()),
    m_target_wp(target.shared_from_this()),
    m_base_of_image(LLDB_INVALID_ADDRESS), m_size_of_image(0) {
    if (module_spec.GetUUID().IsValid())
      SetUUID(module_spec.GetUUID());
  }

  // Creates a synthetic module section covering the whole module image (and
  // sets the section load address as well)
  void CreateImageSection(const MinidumpModule *module) {
    m_base_of_image = module->base_of_image;
    m_size_of_image = module->size_of_image;
    TargetSP target_sp = m_target_wp.lock();
    if (!target_sp)
      return;
    const ConstString section_name(".module_image");
    lldb::SectionSP section_sp(new Section(
        shared_from_this(),     // Module to which this section belongs.
        nullptr,                // ObjectFile
        0,                      // Section ID.
        section_name,           // Section name.
        eSectionTypeContainer,  // Section type.
        module->base_of_image,  // VM address.
        module->size_of_image,  // VM size in bytes of this section.
        0,                      // Offset of this section in the file.
        module->size_of_image,  // Size of the section as found in the file.
        12,                     // Alignment of the section (log2)
        0,                      // Flags for this section.
        1));                    // Number of host bytes per target byte
    section_sp->SetPermissions(ePermissionsExecutable | ePermissionsReadable);
    GetSectionList()->AddSection(section_sp);
    target_sp->GetSectionLoadList().SetSectionLoadAddress(
        section_sp, module->base_of_image);
  }

  ObjectFile *GetObjectFile() override {
    // Since there is no object file for these place holder modules, check
    // if the symbol file spec has been set, and if so, then transfer it over
    // to the file spec so the module can make a real object file out of it.
    if (m_symfile_spec) {
      // We need to load the sections once. We check of m_objfile_sp is valid.
      // If it is, we already have loaded the sections. If it isn't, we will
      // load the sections.
      const bool load_sections = !m_objfile_sp;
      if (load_sections) {
        m_platform_file = m_file;
        m_file = m_symfile_spec;
      }
      ObjectFile *obj_file = Module::GetObjectFile();
      if (load_sections && obj_file) {
        TargetSP target_sp = m_target_wp.lock();
        // The first time we create the object file from the external symbol
        // file, we must load its sections and unload the ".module_image"
        // section
        bool changed;
        SetLoadAddress(*target_sp, m_base_of_image, false, changed);
      }
      return obj_file;
    }
    return nullptr;
  }

  SectionList *GetSectionList() override {
    return Module::GetUnifiedSectionList();
  }
protected:
  // In case we need to load the sections in PlaceholderModule::GetObjectFile()
  // after a symbol file has been specified, we might need the target.
  lldb::TargetWP m_target_wp;
  lldb::addr_t m_base_of_image;
  lldb::addr_t m_size_of_image;
};

[lemo]

How large is the PDB file here?

~100kb

[zturner]

source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

139–144

We talked about this offline, but bringing the discussion back here. Can you describe the use case that this is addressing? As you mention, this is a temporary hack until we have proper symbol searching logic, but proper symbol searching logic will do more than just look up symbols in a symbol server. It will also, for example, look in the same directory as the executable file. If we changed this logic to do that, would your use case still be addressed? At least that way, the logic we're adding is not temporary, even if it will eventually live in a different place (e.g. the SymbolVendor).

[zturner]

In D55142#1326247, @lemo wrote:

How large is the PDB file here?

~100kb

We have a couple of tests in LLVM where PDB files are checked in, but they are very few. We cannot explode the repo with large numbers of binary files. So this is probably fine, but if this becomes a pattern, we will need to come up with a different solution.

[lemo]

source/Commands/CommandObjectTarget.cpp

4246 ↗

(On Diff #177898)

note I had to bypass this check: we don't (yet) have a ObjectFilePDB so the SymbolFileNativePDB always points to the associated PE binary.

the check itself seems valuable as a diagnostic but not strictly required. Should I add a TODO comment and/or open a bug to revisit this?

[labath]

source/Commands/CommandObjectTarget.cpp

4246 ↗

(On Diff #177898)

I not sure this is a good idea. Isn't this the only way of providing feedback about whether the symbols were actually added? If we are unable to load the symbol file specified (perhaps because the user made a typo, or the file is corrupted), then the symbol vendor will just create a default SymbolFile backed by the original object file. Doesn't that mean this will basically always return true now?

I think this is strictly worse that the previous solution as it lets the objectless-symbol-file hack leak out of SymbolFilePDB.

[clayborg]

Just need a way to verify symbols are good. See my inline comment.

source/Commands/CommandObjectTarget.cpp

4246 ↗

(On Diff #177898)

We need to add some sanity check where we ask the symbol file if it is valid. It should be virtual function in SymbolFile that defaults to:

virtual bool SymbolFile::IsValid() const {
  return GetObjectFile() != nullptr;
}

And we can override for SymbolFile subclasses that arenb't objfile based. How does this sound?

[zturner]

I thought about what my "ideal" design would look like. I'm not suggesting anyone should actually go off and do this, but since we're brainstorming design anyway, it doesn't hurt to consider what an optimal one should look like (and for the record, there may be things I haven't considered that cuase this design to not work or be suboptimal or not work at all). Just throwing it out there.

1. Rename ObjectFile to ModuleFile to indicate that a) it makes no sense without a Module, and b) it doesn't make sense for symbol-only files.

2. Rename SymbolVendor to SymbolFileLocator. Mostly cosmetic, but more clearly differentiates the responsibilities of the two classes. (e.g. wouldn't you expect SymbolFiles to be able to "vend" symbols)?

3. SymbolFileLocator returns a unique_ptr<SymbolFile>. Nothing more. It has no other methods.

4. Target contains a vector<unique_ptr<SymbolFileLocator>> as well as methods AddSymbolFileLocator(unique_ptr<SymbolFileLocator>) and unique_ptr<SymbolFile> LocateSymboleFile(Module&). The former gives things like the Platform or Process the ability to customize the behavior. The latter, when called, will iterate through the list of SymbolFileLocators, trying to find one that works.

5. Module contains a unique_ptr<SymbolFile>. When GetSymbolFile() is called for the first time, it calls Target::LocateSymbolFile(*this) and updates its member variable.

6. ModuleFile is updated to support "capabilities" much like SymbolFile is today. One of these capabilities can be "has unwind info". Likewise, "has unwind info is added to SymbolFile capabilities as well. Module can then select the best provider of unwind info using this approach.

And finally

7. SymbolFile now stores a variable m_module instead of m_obj_file since this is usually what the SymbolFile requires anyway, and it also eliminates the dependency on SymbolFile being backed by an ObjectFile. Note that in this case, a SymbolFileBreakpad for example would have its m_module variable set to the actual module that its providing symbols for.

[labath]

Some interesting ideas here. I like some of them, but I have reservations about others. I'm going to go through them one by one.

In D55142#1333077, @zturner wrote:

I thought about what my "ideal" design would look like. I'm not suggesting anyone should actually go off and do this, but since we're brainstorming design anyway, it doesn't hurt to consider what an optimal one should look like (and for the record, there may be things I haven't considered that cuase this design to not work or be suboptimal or not work at all). Just throwing it out there.

1. Rename ObjectFile to ModuleFile to indicate that a) it makes no sense without a Module, and b) it doesn't make sense for symbol-only files.

This is the one I have most problems with, actually, as it goes against my "ideal" design. I would actually like to have ObjectFile (or whatever it's called) be completely independent of the Module class. The motivation for this is, unsurprisingly, lldb-server, which needs to have access to very lightweight info about an object file (on the level of: UUID, architecture, maybe entry point, etc.), but nothing like the fancy interface the Module class provides. So ideally, I'd like for the ObjectFile to live on a completely different level, so that it can be used in lldb-server without pulling in Module and everything that goes with it. So, I don't agree with your (a) goal here. The (b) goal seems completely reasonable though.

2. Rename SymbolVendor to SymbolFileLocator. Mostly cosmetic, but more clearly differentiates the responsibilities of the two classes. (e.g. wouldn't you expect SymbolFiles to be able to "vend" symbols)?

+1000. It has always bugged me that SymbolVendor has all these methods that don't do anything and just forward their arguments to the SymbolFile (after performing a dozen null checks). I think it would be better if the symbol file interface was called directly. If some SymbolVendor/SymbolLocator ever wants to take an existing SymbolFile and alter its behavior slightly, it can always use the composition pattern and return it's own delegating SymbolFile object instead.

3. SymbolFileLocator returns a unique_ptr<SymbolFile>. Nothing more. It has no other methods.

That seems to be the logical consequence of the previous item.

4. Target contains a vector<unique_ptr<SymbolFileLocator>> as well as methods AddSymbolFileLocator(unique_ptr<SymbolFileLocator>) and unique_ptr<SymbolFile> LocateSymboleFile(Module&). The former gives things like the Platform or Process the ability to customize the behavior. The latter, when called, will iterate through the list of SymbolFileLocators, trying to find one that works.

I am not sure if Target is a good receptacle for this functionality, because one of lldb's goals (which seems completely reasonable to me) is to be able to reuse the same Module instance if the module happens to be loaded in multiple targets. If the target is responsible for locating the symbol files, then this opens the door for one target to unwittingly alter the symbols of another target. One way to get out of this would be to say that targets share a Module only if they (independently) choose the same symbol file. This might enable us to also solve the current problem, where "target symbols add" executed in one debugger, alters the behavior of another debugger instance. Another would be to store the list of SymbolLocators in a more global place. This would actually be pretty similar to what we have now.

5. Module contains a unique_ptr<SymbolFile>. When GetSymbolFile() is called for the first time, it calls Target::LocateSymbolFile(*this) and updates its member variable.

Nothing to argue about here.

6. ModuleFile is updated to support "capabilities" much like SymbolFile is today. One of these capabilities can be "has unwind info". Likewise, "has unwind info is added to SymbolFile capabilities as well. Module can then select the best provider of unwind info using this approach.

I don't believe a single bit is enough to describe the suitability of unwind info, as the choice of unwind strategy may vary from function to function and depend on other circumstances too (e.g., whether we are stopped at a call site). Instead, I'd try to have much more granular capabilities. E.g., one of those capabilities might be "has function boundaries". Then if either entity provides the information about function boundaries, the Module can create an InstructionEmulationUnwinder, which uses these boundaries to create emulated unwind plans. And even this capability might not be just a "bit", but rather a pooling of function boundary information gathered from various sources.

And finally

7. SymbolFile now stores a variable m_module instead of m_obj_file since this is usually what the SymbolFile requires anyway, and it also eliminates the dependency on SymbolFile being backed by an ObjectFile. Note that in this case, a SymbolFileBreakpad for example would have its m_module variable set to the actual module that its providing symbols for.

Sounds good.

[teemperor]

lit/Minidump/Windows/Sigsegv/sigsegv.test

6 ↗

(On Diff #177898)

This test fails for me because lldb is picking up my custom lldbinit (which changes the format of the frame output). Not sure what's the right way to fix it, but setting a custom fixed frameformat in the sigsegv.lldbinit seems like an obvious solution.

[labath]

lit/Minidump/Windows/Sigsegv/sigsegv.test

6 ↗

(On Diff #177898)

Are you sure you have the right patch here?

Regardless, I actually think the right solution would be to pass --no-lldb-init as a part of the %lldb substitution (I'm surprised we don't do that already) to make sure no test sources random commands from users' .lldbinit files.

source/Plugins/Process/minidump/MinidumpParser.h

public:
   llvm::ArrayRef<uint8_t> GetStream(MinidumpStreamType stream_type);
 
   llvm::Optional<std::string> GetMinidumpString(uint32_t rva);
 
   UUID GetModuleUUID(const MinidumpModule* module);
 
   llvm::ArrayRef<MinidumpThread> GetThreads();
 
+  llvm::ArrayRef<uint8_t>
+  GetThreadContext(const MinidumpLocationDescriptor &location);
+
   llvm::ArrayRef<uint8_t> GetThreadContext(const MinidumpThread &td);
 
   llvm::ArrayRef<uint8_t> GetThreadContextWow64(const MinidumpThread &td);
 
   const MinidumpSystemInfo *GetSystemInfo();
 
   ArchSpec GetArchitecture();
 

source/Plugins/Process/minidump/MinidumpParser.cpp

llvm::ArrayRef<MinidumpThread> MinidumpParser::GetThreads() {
 
   if (data.size() == 0)
     return llvm::None;
 
   return MinidumpThread::ParseThreadList(data);
 }
 
 llvm::ArrayRef<uint8_t>
-MinidumpParser::GetThreadContext(const MinidumpThread &td) {
-  if (td.thread_context.rva + td.thread_context.data_size > GetData().size())
+MinidumpParser::GetThreadContext(const MinidumpLocationDescriptor &location) {
+  if (location.rva + location.data_size > GetData().size())
     return {};
+  return GetData().slice(location.rva, location.data_size);
+}
 
-  return GetData().slice(td.thread_context.rva, td.thread_context.data_size);
+llvm::ArrayRef<uint8_t>
+MinidumpParser::GetThreadContext(const MinidumpThread &td) {
+  return GetThreadContext(td.thread_context);
 }
 
 llvm::ArrayRef<uint8_t>
 MinidumpParser::GetThreadContextWow64(const MinidumpThread &td) {
   // On Windows, a 32-bit process can run on a 64-bit machine under WOW64. If
   // the minidump was captured with a 64-bit debugger, then the CONTEXT we just
   // grabbed from the mini_dump_thread is the one for the 64-bit "native"
   // process rather than the 32-bit "guest" process we care about.  In this

source/Plugins/Process/minidump/ProcessMinidump.cpp

 #include "lldb/Target/JITLoaderList.h"
 #include "lldb/Target/MemoryRegionInfo.h"
 #include "lldb/Target/SectionLoadList.h"
 #include "lldb/Target/Target.h"
 #include "lldb/Target/UnixSignals.h"
 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/State.h"
+#include "lldb/Symbol/ObjectFile.h"
+#include "lldb/Symbol/SymbolVendor.h"
+#include "lldb/Utility/Timer.h"
 
 #include "llvm/Support/MemoryBuffer.h"
 #include "llvm/Support/Threading.h"
 
 #include "Plugins/Process/Utility/StopInfoMachException.h"
+
 // C includes
 // C++ includes
+#include <memory>
 
 using namespace lldb;
 using namespace lldb_private;
 using namespace minidump;
 
 //------------------------------------------------------------------
+/// A minimal ObjectFile implementation intended to enable the ability to
+/// consume debug information even when the module binary is not available
+///
+/// See also: PlaceholderModule
+//------------------------------------------------------------------
+class PlaceholderObjectFile : public ObjectFile {
+public:
+  PlaceholderObjectFile(const lldb::ModuleSP &module_sp, lldb::offset_t base,
+                        lldb::offset_t size)
+      : ObjectFile(module_sp, &module_sp->GetFileSpec(), 0, 0, nullptr, 0) {
+    m_type = ObjectFile::eTypeObjectFile;
+    m_file_offset = base;
+    m_length = size;
+  }
+
+private:
+  void Dump(Stream *s) override { *s << "PlaceholderObjectFile\n"; }
+  uint32_t GetAddressByteSize() const override { return 0; }
+  uint32_t GetDependentModules(FileSpecList &file_list) override { return 0; }
+  bool IsExecutable() const override { return false; }
+  bool IsStripped() override { return true; }
+  bool ParseHeader() override { return true; }
+  Type CalculateType() override { return m_type; }
+  Strata CalculateStrata() override { return eStrataUnknown; }
+  lldb::ByteOrder GetByteOrder() const override { return eByteOrderLittle; }
+  void CreateSections(SectionList &unified_section_list) override {}
+
+  bool GetUUID(lldb_private::UUID *uuid) override {
+    *uuid = GetModule()->GetUUID();
+    return true;
+  }
+
+  bool GetArchitecture(ArchSpec &arch) override {
+    arch = GetModule()->GetArchitecture();
+    return true;
+  }
+
+  Symtab *GetSymtab() override {
+    if (!m_symtab_ap) {
+      m_symtab_ap = llvm::make_unique<Symtab>(this);
+    }
+    return m_symtab_ap.get();
+  }
+
+  ConstString GetPluginName() override {
+    return ConstString("placeholder-obj");
+  }
+
+  uint32_t GetPluginVersion() override { return 1; }
+};
+
+//------------------------------------------------------------------
 /// A placeholder module used for minidumps, where the original
 /// object files may not be available (so we can't parse the object
 /// files to extract the set of sections/segments)
 ///
 /// This placeholder module has a single synthetic section (.module_image)
 /// which represents the module memory range covering the whole module.
 //------------------------------------------------------------------
 class PlaceholderModule : public Module {
 public:
   PlaceholderModule(const ModuleSpec &module_spec) :
     Module(module_spec.GetFileSpec(), module_spec.GetArchitecture()) {
     if (module_spec.GetUUID().IsValid())
       SetUUID(module_spec.GetUUID());
   }
 
   // Creates a synthetic module section covering the whole module image (and
   // sets the section load address as well)
   void CreateImageSection(const MinidumpModule *module, Target& target) {
+    // Create the placeholder object file
+    {
+      std::lock_guard<std::recursive_mutex> guard(m_mutex);
+      lldbassert(!m_did_load_objfile.load());
+      m_objfile_sp = std::make_shared<PlaceholderObjectFile>(
+          shared_from_this(), module->base_of_image, module->size_of_image);
+      m_did_load_objfile = true;
+    }
+
     const ConstString section_name(".module_image");
     lldb::SectionSP section_sp(new Section(
         shared_from_this(),     // Module to which this section belongs.
-        nullptr,                // ObjectFile
+        GetObjectFile(),        // ObjectFile
         0,                      // Section ID.
         section_name,           // Section name.
         eSectionTypeContainer,  // Section type.
         module->base_of_image,  // VM address.
         module->size_of_image,  // VM size in bytes of this section.
         0,                      // Offset of this section in the file.
         module->size_of_image,  // Size of the section as found in the file.
         12,                     // Alignment of the section (log2)
         0,                      // Flags for this section.
         1));                    // Number of host bytes per target byte
     section_sp->SetPermissions(ePermissionsExecutable | ePermissionsReadable);
     GetSectionList()->AddSection(section_sp);
     target.GetSectionLoadList().SetSectionLoadAddress(
         section_sp, module->base_of_image);
   }
 
-  ObjectFile *GetObjectFile() override { return nullptr; }
+  ObjectFile *GetObjectFile() override {
+    std::lock_guard<std::recursive_mutex> guard(m_mutex);
+    lldbassert(m_did_load_objfile);
+    return m_objfile_sp.get();
+  }
 
   SectionList *GetSectionList() override {
     return Module::GetUnifiedSectionList();
   }
 };
 
 ConstString ProcessMinidump::GetPluginNameStatic() {
   static ConstString g_name("minidump");

 void ProcessMinidump::Clear() { Process::m_thread_list.Clear(); }
 
 bool ProcessMinidump::UpdateThreadList(ThreadList &old_thread_list,
                                        ThreadList &new_thread_list) {
   uint32_t num_threads = 0;
   if (m_thread_list.size() > 0)
     num_threads = m_thread_list.size();
 
-  for (lldb::tid_t tid = 0; tid < num_threads; ++tid) {
+  for (lldb::tid_t t_index = 0; t_index < num_threads; ++t_index) {
     llvm::ArrayRef<uint8_t> context;
+    const auto& thread = m_thread_list[t_index];
+    auto context_location = thread.thread_context;
+    if (m_active_exception != nullptr &&
+        m_active_exception->thread_id == thread.thread_id) {
+      // If the minidump contains an exception context, use it
+      context_location = m_active_exception->thread_context;
+    }
     if (!m_is_wow64)
-      context = m_minidump_parser.GetThreadContext(m_thread_list[tid]);
+      context = m_minidump_parser.GetThreadContext(context_location);
     else
-      context = m_minidump_parser.GetThreadContextWow64(m_thread_list[tid]);
+      context = m_minidump_parser.GetThreadContextWow64(thread);
 
     lldb::ThreadSP thread_sp(
-        new ThreadMinidump(*this, m_thread_list[tid], context));
+        new ThreadMinidump(*this, m_thread_list[t_index], context));
     new_thread_list.AddThread(thread_sp);
   }
   return new_thread_list.GetSize(false) > 0;
 }
 
 void ProcessMinidump::ReadModuleList() {
   std::vector<const MinidumpModule *> filtered_modules =
       m_minidump_parser.GetFilteredModuleList();
for (auto module : filtered_modules) {
     if (llvm::StringRef(name.getValue()).endswith_lower("wow64.dll")) {
       m_is_wow64 = true;
     }
 
     const auto uuid = m_minidump_parser.GetModuleUUID(module);
     auto file_spec = FileSpec(name.getValue(), GetArchitecture().GetTriple());
     FileSystem::Instance().Resolve(file_spec);
     ModuleSpec module_spec(file_spec, uuid);
+    module_spec.GetArchitecture() = GetArchitecture();
     Status error;
     lldb::ModuleSP module_sp = GetTarget().GetSharedModule(module_spec, &error);
     if (!module_sp || error.Fail()) {
       // We failed to locate a matching local object file. Fortunately, the
       // minidump format encodes enough information about each module's memory
       // range to allow us to create placeholder modules.
       //
       // This enables most LLDB functionality involving address-to-module
       // translations (ex. identifing the module for a stack frame PC) and
       // modules/sections commands (ex. target modules list, ...)
       if (log) {
         log->Printf("Unable to locate the matching object file, creating a "
                     "placeholder module for: %s",
                     name.getValue().c_str());
       }
 
       auto placeholder_module =
           std::make_shared<PlaceholderModule>(module_spec);
       placeholder_module->CreateImageSection(module, GetTarget());
       module_sp = placeholder_module;
       GetTarget().GetImages().Append(module_sp);
+
+      if (GetTarget().GetPreloadSymbols())
+        module_sp->PreloadSymbols();
     }
 
     if (log) {
       log->Printf("ProcessMinidump::%s load module: name: %s", __FUNCTION__,
                   name.getValue().c_str());
     }
 
     bool load_addr_changed = false;

source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.h

public:
 
   CompilandIndexItem &GetOrCreateCompiland(uint16_t modi);
 
   const CompilandIndexItem *GetCompiland(uint16_t modi) const;
 
   CompilandIndexItem *GetCompiland(uint16_t modi);
 
   llvm::SmallString<64> GetMainSourceFile(const CompilandIndexItem &item) const;
+
+  bool HasIndexForModule(uint16_t modi) const;
 };
 } // namespace npdb
 } // namespace lldb_private
 
 #endif

source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.cpp

 }
 
 CompilandIndexItem::CompilandIndexItem(
     PdbCompilandId id, llvm::pdb::ModuleDebugStreamRef debug_stream,
     llvm::pdb::DbiModuleDescriptor descriptor)
     : m_id(id), m_debug_stream(std::move(debug_stream)),
       m_module_descriptor(std::move(descriptor)) {}
 
+bool CompileUnitIndex::HasIndexForModule(uint16_t modi) const {
+  const DbiModuleList &modules = m_index.dbi().modules();
+  llvm::pdb::DbiModuleDescriptor descriptor = modules.getModuleDescriptor(modi);
+  uint16_t stream = descriptor.getModuleStreamIndex();
+  return stream != kInvalidStreamIndex;
+}
+
 CompilandIndexItem &CompileUnitIndex::GetOrCreateCompiland(uint16_t modi) {
   auto result = m_comp_units.try_emplace(modi, nullptr);
   if (!result.second)
     return *result.first->second;
 
   // Find the module list and load its debug information stream and cache it
   // since we need to use it for almost all interesting operations.
   const DbiModuleList &modules = m_index.dbi().modules();
   llvm::pdb::DbiModuleDescriptor descriptor = modules.getModuleDescriptor(modi);
   uint16_t stream = descriptor.getModuleStreamIndex();
   std::unique_ptr<llvm::msf::MappedBlockStream> stream_data =
       m_index.pdb().createIndexedStream(stream);
+  lldbassert(stream_data);
   llvm::pdb::ModuleDebugStreamRef debug_stream(descriptor,
                                                std::move(stream_data));
   cantFail(debug_stream.reload());
 
   std::unique_ptr<CompilandIndexItem> &cci = result.first->second;
 
   cci = llvm::make_unique<CompilandIndexItem>(
       PdbCompilandId{modi}, std::move(debug_stream), std::move(descriptor));

source/Plugins/SymbolFile/NativePDB/PdbIndex.h

class PdbIndex {
   CompileUnitIndex m_cus;
 
   /// An allocator for the interval maps
   llvm::IntervalMap<lldb::addr_t, uint32_t>::Allocator m_allocator;
 
   /// Maps virtual address to module index
   llvm::IntervalMap<lldb::addr_t, uint16_t> m_va_to_modi;
 
-  /// The address at which the program has been loaded into memory.
+  /// The address at which the module has been loaded into memory.
   lldb::addr_t m_load_address = 0;
 
   PdbIndex();
 
   void BuildAddrToSymbolMap(CompilandIndexItem &cci);
 
 public:
   static llvm::Expected<std::unique_ptr<PdbIndex>>

source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp

 
 llvm::Expected<std::unique_ptr<PdbIndex>>
 PdbIndex::create(std::unique_ptr<llvm::pdb::PDBFile> file) {
   lldbassert(file);
 
   std::unique_ptr<PdbIndex> result(new PdbIndex());
   ASSIGN_PTR_OR_RETURN(result->m_dbi, file->getPDBDbiStream());
   ASSIGN_PTR_OR_RETURN(result->m_tpi, file->getPDBTpiStream());
-  ASSIGN_PTR_OR_RETURN(result->m_ipi, file->getPDBIpiStream());
   ASSIGN_PTR_OR_RETURN(result->m_info, file->getPDBInfoStream());
   ASSIGN_PTR_OR_RETURN(result->m_publics, file->getPDBPublicsStream());
   ASSIGN_PTR_OR_RETURN(result->m_globals, file->getPDBGlobalsStream());
   ASSIGN_PTR_OR_RETURN(result->m_symrecords, file->getPDBSymbolStream());
 
   result->m_tpi->buildHashMap();
 
   result->m_file = std::move(file);
void visit(const SectionContrib &C) override {
         return;
 
       uint64_t va = m_ctx.MakeVirtualAddress(C.ISect, C.Off);
       uint64_t end = va + C.Size;
       // IntervalMap's start and end represent a closed range, not a half-open
       // range, so we have to subtract 1.
       m_imap.insert(va, end - 1, C.Imod);
     }
+
     void visit(const SectionContrib2 &C) override { visit(C.Base); }
   };
   Visitor v(*this, m_va_to_modi);
   dbi().visitSectionContributions(v);
 }
 
 void PdbIndex::BuildAddrToSymbolMap(CompilandIndexItem &cci) {
   lldbassert(cci.m_symbols_by_va.empty() &&
              "Addr to symbol map is already built!");
   uint16_t modi = cci.m_id.modi;
   const CVSymbolArray &syms = cci.m_debug_stream.getSymbolArray();
   for (auto iter = syms.begin(); iter != syms.end(); ++iter) {
     if (!SymbolHasAddress(*iter))
       continue;
 
     SegmentOffset so = GetSegmentAndOffset(*iter);
+    if (so.segment == 0) {
+      lldbassert(so.offset == 0);
+      continue;
+    }
     lldb::addr_t va = MakeVirtualAddress(so);
 
     // We need to add 4 here to adjust for the codeview debug magic
     // at the beginning of the debug info stream.
     uint32_t sym_offset = iter.offset() + 4;
     PdbCompilandSymId cu_sym_id(modi, sym_offset);
 
     // If the debug info is incorrect, we could have multiple symbols with the
     // same address.  So use try_emplace instead of insert, and the first one
     // will win.
     auto insert_result =
         cci.m_symbols_by_va.insert(std::make_pair(va, PdbSymUid(cu_sym_id)));
     (void)insert_result;
 
     // The odds of an error in some function such as GetSegmentAndOffset or
     // MakeVirtualAddress are much higher than the odds of encountering bad
     // debug info, so assert that this item was inserted in the map as opposed
     // to having already been there.
-    lldbassert(insert_result.second);
+    //
+    // TODO: revisit this check since it fires for apparently valid PDBs
+    //
+    //lldbassert(insert_result.second);
   }
 }
 
 std::vector<SymbolAndUid> PdbIndex::FindSymbolsByVa(lldb::addr_t va) {
   std::vector<SymbolAndUid> result;
 
   llvm::Optional<uint16_t> modi = GetModuleIndexForVa(va);
   if (!modi)

source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp

 #include "lldb/Symbol/ClangUtil.h"
 #include "lldb/Symbol/CompileUnit.h"
 #include "lldb/Symbol/LineTable.h"
 #include "lldb/Symbol/ObjectFile.h"
 #include "lldb/Symbol/SymbolContext.h"
 #include "lldb/Symbol/SymbolVendor.h"
 #include "lldb/Symbol/Variable.h"
 #include "lldb/Symbol/VariableList.h"
+#include "lldb/Utility/UUID.h"
 
 #include "llvm/DebugInfo/CodeView/CVRecord.h"
 #include "llvm/DebugInfo/CodeView/CVTypeVisitor.h"
 #include "llvm/DebugInfo/CodeView/DebugLinesSubsection.h"
 #include "llvm/DebugInfo/CodeView/LazyRandomTypeCollection.h"
 #include "llvm/DebugInfo/CodeView/RecordName.h"
 #include "llvm/DebugInfo/CodeView/SymbolDeserializer.h"
 #include "llvm/DebugInfo/CodeView/TypeDeserializer.h"
if (auto EC = File->parseStreamData()) {
     llvm::consumeError(std::move(EC));
     return nullptr;
   }
 
   return File;
 }
 
 static std::unique_ptr<PDBFile>
-loadMatchingPDBFile(std::string exe_path, llvm::BumpPtrAllocator &allocator) {
-  // Try to find a matching PDB for an EXE.
+loadMatchingPDBFile(lldb_private::ObjectFile *obj_file,
+                    llvm::BumpPtrAllocator &allocator) {
   using namespace llvm::object;
-  auto expected_binary = createBinary(exe_path);
 
-  // If the file isn't a PE/COFF executable, fail.
-  if (!expected_binary) {
-    llvm::consumeError(expected_binary.takeError());
-    return nullptr;
-  }
+  // Try to find a matching PDB for an EXE.
+  std::string pdb_file;
+  llvm::codeview::GUID guid;
+
+  auto expected_binary = createBinary(obj_file->GetFileSpec().GetPath());
+  if (expected_binary) {
     OwningBinary<Binary> binary = std::move(*expected_binary);
 
-  auto *obj = llvm::dyn_cast<llvm::object::COFFObjectFile>(binary.getBinary());
+    auto *obj = llvm::dyn_cast<COFFObjectFile>(binary.getBinary());
     if (!obj)
       return nullptr;
     const llvm::codeview::DebugInfo *pdb_info = nullptr;
 
     // If it doesn't have a debug directory, fail.
-  llvm::StringRef pdb_file;
-  auto ec = obj->getDebugPDBInfo(pdb_info, pdb_file);
-  if (ec)
+    llvm::StringRef pdb_file_name_ref;
+    if (obj->getDebugPDBInfo(pdb_info, pdb_file_name_ref))
       return nullptr;
+    pdb_file = pdb_file_name_ref;
+
+    // Extract the debug GUID from the debug directory
+    memcpy(&guid, pdb_info->PDB70.Signature, sizeof(guid));
+  } else {
+    // TODO: If the file isn't a PE/COFF executable, look for the PDB in the
+    //  current directly. This provides a basic solution for debugging minidumps
+    //  although it's only a stop-gap until we implement a proper SymbolVendor
+    //  for PDBs.
+    llvm::consumeError(expected_binary.takeError());
+    pdb_file =
+        obj_file->GetFileSpec().GetFileNameStrippingExtension().GetCString();
+    pdb_file += ".pdb";
+
+    // Extract the debug GUID from the ObjectFile
+    lldb_private::UUID uuid;
+    obj_file->GetUUID(&uuid);
+    auto uuid_bytes = uuid.GetBytes();
+    lldbassert(uuid_bytes.size() == sizeof(llvm::codeview::GUID) + 4);
+    memcpy(&guid, uuid_bytes.data(), sizeof(guid));
+  }
 
   // if the file doesn't exist, is not a pdb, or doesn't have a matching guid,
   // fail.
   llvm::file_magic magic;
-  ec = llvm::identify_magic(pdb_file, magic);
+  auto ec = llvm::identify_magic(pdb_file, magic);
   if (ec || magic != llvm::file_magic::pdb)
     return nullptr;
   std::unique_ptr<PDBFile> pdb = loadPDBFile(pdb_file, allocator);
   if (!pdb)
     return nullptr;
 
   auto expected_info = pdb->getPDBInfoStream();
   if (!expected_info) {
     llvm::consumeError(expected_info.takeError());
     return nullptr;
   }
-  llvm::codeview::GUID guid;
-  memcpy(&guid, pdb_info->PDB70.Signature, 16);
 
+  // TODO: we need to compare the age, in addition to the GUID
   if (expected_info->getGuid() != guid)
     return nullptr;
+
   return pdb;
 }
 
 static bool IsFunctionPrologue(const CompilandIndexItem &cci,
                                lldb::addr_t addr) {
   // FIXME: Implement this.
   return false;
 }

 uint32_t SymbolFileNativePDB::CalculateAbilities() {
   uint32_t abilities = 0;
   if (!m_obj_file)
     return 0;
 
   if (!m_index) {
     // Lazily load and match the PDB file, but only do this once.
     std::unique_ptr<PDBFile> file_up =
-        loadMatchingPDBFile(m_obj_file->GetFileSpec().GetPath(), m_allocator);
+        loadMatchingPDBFile(m_obj_file, m_allocator);
 
     if (!file_up) {
       auto module_sp = m_obj_file->GetModule();
       if (!module_sp)
         return 0;
       // See if any symbol file is specified through `--symfile` option.
       FileSpec symfile = module_sp->GetSymbolFileFileSpec();
       if (!symfile)
uint32_t SymbolFileNativePDB::ResolveSymbolContext(
     const Address &addr, SymbolContextItem resolve_scope, SymbolContext &sc) {
   uint32_t resolved_flags = 0;
   lldb::addr_t file_addr = addr.GetFileAddress();
 
   if (NeedsResolvedCompileUnit(resolve_scope)) {
     llvm::Optional<uint16_t> modi = m_index->GetModuleIndexForVa(file_addr);
     if (!modi)
       return 0;
-    CompilandIndexItem *cci = m_index->compilands().GetCompiland(*modi);
-    if (!cci)
+    if (!m_index->compilands().HasIndexForModule(*modi))
       return 0;
-
-    sc.comp_unit = GetOrCreateCompileUnit(*cci).get();
+    CompilandIndexItem cci = m_index->compilands().GetOrCreateCompiland(*modi);
+    sc.comp_unit = GetOrCreateCompileUnit(cci).get();
     resolved_flags |= eSymbolContextCompUnit;
   }
 
   if (resolve_scope & eSymbolContextFunction) {
     lldbassert(sc.comp_unit);
     std::vector<SymbolAndUid> matches = m_index->FindSymbolsByVa(file_addr);
     for (const auto &match : matches) {
       if (match.uid.kind() != PdbSymUidKind::CompilandSym)