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

160

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

150

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

136–139

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

155–162

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–107

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

114–134

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

136–139

 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

155–162

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–107

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–134

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

134–136

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–107

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.

167

make this sizeof(guid) for consistency.

169–172

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

169–172

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

386

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

156–161

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

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

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

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

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

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.

lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp

+
+// nodefaultlib build: cl -Zi sigsegv.cpp /link /nodefaultlib
+
+#ifdef USE_CRT
+#include <stdio.h>
+#else
+int main();
+extern "C"
+{
+    int _fltused;
+    void mainCRTStartup() { main(); }
+    void printf(const char*, ...) {}
+}
+#endif
+
+void crash(bool crash_self)
+{
+    printf("Before...\n");
+    if(crash_self)
+    {
+        printf("Crashing in 3, 2, 1 ...\n");
+        *(volatile int*)nullptr = 0;
+    }
+    printf("After...\n");
+}
+
+int foo(int x, float y, const char* msg)
+{
+    bool flag = x > y;
+    if(flag)
+        printf("x = %d, y = %f, msg = %s\n", x, y, msg);
+    crash(flag);
+    return x << 1;
+}
+
+int main()
+{
+    foo(10, 3.14, "testing");
+}
+

lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.lldbinit

+target symbols add -s sigsegv.exe sigsegv.pdb
+bt all
+dis

lit/Minidump/Windows/Sigsegv/sigsegv.test

+// RUN: cd %p/Inputs
+// RUN: env LLDB_USE_NATIVE_PDB_READER=1 \
+// RUN:   %lldb -c sigsegv.dmp -s sigsegv.lldbinit | FileCheck %s
+
+CHECK: * thread #1, stop reason = Exception 0xc0000005 encountered at address 0x7ff7a13110d9
+CHECK:   * frame #0: 0x00007ff7a13110d9 sigsegv.exe`crash at sigsegv.cpp:22
+
+CHECK: sigsegv.exe`crash:
+CHECK:     0x7ff7a13110b0 <+0>:  movb   %cl, 0x8(%rsp)
+CHECK:     0x7ff7a13110b4 <+4>:  subq   $0x28, %rsp
+CHECK:     0x7ff7a13110b8 <+8>:  leaq   0x1f41(%rip), %rcx
+CHECK:     0x7ff7a13110bf <+15>: callq  0x7ff7a1311019
+CHECK:     0x7ff7a13110c4 <+20>: movzbl 0x30(%rsp), %eax
+CHECK:     0x7ff7a13110c9 <+25>: testl  %eax, %eax
+CHECK:     0x7ff7a13110cb <+27>: je     0x7ff7a13110e4            ; <+52> at sigsegv.cpp:24
+CHECK:     0x7ff7a13110cd <+29>: leaq   0x1f3c(%rip), %rcx
+CHECK:     0x7ff7a13110d4 <+36>: callq  0x7ff7a1311019
+CHECK: ->  0x7ff7a13110d9 <+41>: movl   $0x0, 0x0
+CHECK:     0x7ff7a13110e4 <+52>: leaq   0x1f45(%rip), %rcx
+CHECK:     0x7ff7a13110eb <+59>: callq  0x7ff7a1311019
+CHECK:     0x7ff7a13110f0 <+64>: addq   $0x28, %rsp
+CHECK:     0x7ff7a13110f4 <+68>: retq

source/Commands/CommandObjectTarget.cpp

if (symbol_fspec) {
         module_sp->SetSymbolFileFileSpec(symbol_fspec);
 
         SymbolVendor *symbol_vendor =
             module_sp->GetSymbolVendor(true, &result.GetErrorStream());
         if (symbol_vendor) {
           SymbolFile *symbol_file = symbol_vendor->GetSymbolFile();
 
           if (symbol_file) {
-            ObjectFile *object_file = symbol_file->GetObjectFile();
-
-            if (object_file && object_file->GetFileSpec() == symbol_fspec) {
-              // Provide feedback that the symfile has been successfully added.
-              const FileSpec &module_fs = module_sp->GetFileSpec();
-              result.AppendMessageWithFormat(
-                  "symbol file '%s' has been added to '%s'\n", symfile_path,
-                  module_fs.GetPath().c_str());
 
-              // Let clients know something changed in the module if it is
-              // currently loaded
-              ModuleList module_list;
-              module_list.Append(module_sp);
-              target->SymbolsDidLoad(module_list);
 
-              // Make sure we load any scripting resources that may be embedded
-              // in the debug info files in case the platform supports that.
-              Status error;
-              StreamString feedback_stream;
-              module_sp->LoadScriptingResourceInTarget(target, error,
-                                                       &feedback_stream);
-              if (error.Fail() && error.AsCString())
-                result.AppendWarningWithFormat(
-                    "unable to load scripting data for module %s - error "
-                    "reported was %s",
-                    module_sp->GetFileSpec()
-                        .GetFileNameStrippingExtension()
-                        .GetCString(),
-                    error.AsCString());
-              else if (feedback_stream.GetSize())
-                result.AppendWarningWithFormat("%s", feedback_stream.GetData());
 
-              flush = true;
-              result.SetStatus(eReturnStatusSuccessFinishResult);
-              return true;
-            }
-          }
-        }
         // Clear the symbol file spec if anything went wrong
         module_sp->SetSymbolFileFileSpec(FileSpec());
       }
 
       namespace fs = llvm::sys::fs;
       if (module_spec.GetUUID().IsValid()) {
         StreamString ss_symfile_uuid;
         module_spec.GetUUID().Dump(&ss_symfile_uuid);

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, const UUID &uuid,
+                        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;
+    m_uuid = uuid;
+  }
+
+private:
+  void Dump(Stream *s) override { *s << "PlaceholderObjectFile\n"; }
+  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; }
+  void CreateSections(SectionList &unified_section_list) override {}
+
+  uint32_t GetAddressByteSize() const override {
+    return GetModule()->GetArchitecture().GetAddressByteSize();
+  }
+
+  lldb::ByteOrder GetByteOrder() const override {
+    return GetModule()->GetArchitecture().GetByteOrder();
+  }
+
+  bool GetUUID(lldb_private::UUID *uuid) override {
+    *uuid = m_uuid;
+    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; }
+
+private:
+  UUID m_uuid;
+};
+
+//------------------------------------------------------------------
 /// 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(), m_uuid, 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 (size_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 HasCompilandInfo(uint16_t modi) const;
 };
 } // namespace npdb
 } // namespace lldb_private
 
 #endif

source/Plugins/SymbolFile/NativePDB/CompileUnitIndex.cpp

llvm::cantFail(
       SymbolDeserializer::deserializeAs<ObjNameSym>(sym, *cci.m_obj_name));
 }
 
 static void ParseBuildInfo(PdbIndex &index, const CVSymbol &sym,
                            CompilandIndexItem &cci) {
   BuildInfoSym bis(SymbolRecordKind::BuildInfoSym);
   llvm::cantFail(SymbolDeserializer::deserializeAs<BuildInfoSym>(sym, bis));
 
+  if (!index.hasIpiStream())
+    return;
+
   // S_BUILDINFO just points to an LF_BUILDINFO in the IPI stream.  Let's do
   // a little extra work to pull out the LF_BUILDINFO.
   LazyRandomTypeCollection &types = index.ipi().typeCollection();
   llvm::Optional<CVType> cvt = types.tryGetType(bis.BuildId);
 
   if (!cvt || cvt->kind() != LF_BUILDINFO)
     return;
 

 }
 
 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::HasCompilandInfo(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>>
       create(std::unique_ptr<llvm::pdb::PDBFile>);
 
   void SetLoadAddress(lldb::addr_t addr) { m_load_address = addr; }
   void ParseSectionContribs();
 
   llvm::pdb::PDBFile &pdb() { return *m_file; }
   const llvm::pdb::PDBFile &pdb() const { return *m_file; }
 
   llvm::pdb::DbiStream &dbi() { return *m_dbi; }
   const llvm::pdb::DbiStream &dbi() const { return *m_dbi; }
 
   llvm::pdb::TpiStream &tpi() { return *m_tpi; }
   const llvm::pdb::TpiStream &tpi() const { return *m_tpi; }
 
+  bool hasIpiStream() const { return m_ipi != nullptr; }
+
   llvm::pdb::TpiStream &ipi() { return *m_ipi; }
   const llvm::pdb::TpiStream &ipi() const { return *m_ipi; }
 
   llvm::pdb::InfoStream &info() { return *m_info; }
   const llvm::pdb::InfoStream &info() const { return *m_info; }
 
   llvm::pdb::PublicsStream &publics() { return *m_publics; }
   const llvm::pdb::PublicsStream &publics() const { return *m_publics; }

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());
 
+  // The IPI stream is not always present
+  if (file->hasPDBIpiStream())
+    ASSIGN_PTR_OR_RETURN(result->m_ipi, file->getPDBIpiStream());
+
   result->m_tpi->buildHashMap();
 
   result->m_file = std::move(file);
 
   return std::move(result);
 }
 
 lldb::addr_t PdbIndex::MakeVirtualAddress(uint16_t segment,
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) {
+      // TODO: https://bugs.llvm.org/show_bug.cgi?id=39882
+      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: https://bugs.llvm.org/show_bug.cgi?id=39897
+    //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)
     return result;

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.
+  // Try to find a matching PDB for an EXE.
+  std::string pdb_file;
+  llvm::codeview::GUID guid;
+
+  const auto module = obj_file.GetModule();
+  const auto symbol_file_filespec = module->GetSymbolFileFileSpec();
+  if (symbol_file_filespec.IsResolved()) {
+    // If we have the symbol file filespec set, use it
+    pdb_file = symbol_file_filespec.GetPath();
+
+    // Extract the debug GUID from the ObjectFile
+    lldb_private::UUID uuid;
+    obj_file.GetUUID(&uuid);
+    auto uuid_bytes = uuid.GetBytes();
+    if (uuid_bytes.size() != sizeof(guid) + 4) // CvRecordPdb70
+      return nullptr;
+    memcpy(&guid, uuid_bytes.data(), sizeof(guid));
+  } else {
+    // Use the PDB path embedded in the binary
+    auto expected_binary = createBinary(obj_file.GetFileSpec().GetPath());
     if (!expected_binary) {
       llvm::consumeError(expected_binary.takeError());
       return nullptr;
     }
+
     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;
 
+    const llvm::codeview::DebugInfo *pdb_info = nullptr;
+    llvm::StringRef pdb_file_name_ref;
+    if (obj->getDebugPDBInfo(pdb_info, pdb_file_name_ref)) {
       // If it doesn't have a debug directory, fail.
-  llvm::StringRef pdb_file;
-  auto ec = obj->getDebugPDBInfo(pdb_info, pdb_file);
-  if (ec)
       return nullptr;
+    }
+    pdb_file = pdb_file_name_ref;
+
+    // Extract the debug GUID from the debug directory
+    memcpy(&guid, pdb_info->PDB70.Signature, 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().HasCompilandInfo(*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)