[PATCH] D78128: Implement some functions in NativeSession.

[Reid Kleckner via Phabricator via llvm-commits]

rnk added inline comments.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeSession.cpp:103
+    PdbPath = TmpPath;
+  }
+
----------------
amccarth wrote:
> I think the LLDB folks might be concerned about searching the file system for the corresponding PDB file.  The debugger folks are probably going to want a way to override that for remote debugging, symbol servers, virtual file systems, etc.
> 
> Perhaps the right thing to do is to have an API that takes paths to both the EXE and the PDB.  If everything loads, great, otherwise return an error.  Then you could have an API like this that tries to find the PDB, but rather than having it then load the session, it just forwards the PDB path to the two-path API.
> 
> That would make it possible for a client to get the automatic behavior you have here while also enabling LLDB to be explicit about where the PDB should be.
Building on Adrian's idea, I think the API might look kind of like this:
```
struct PdbSearchOptions {
  std::string ExePath;
  // Options we can add later:
  // std::vector<std::string> SearchDirs;
  // bool UseEnvNtSymbolPath = false;
  // llvm::VirtualFileSystem *SearchVFS = nullptr; // Way to remap paths
};
Expected<std::string> searchForPdb(const PdbSearchOptions &Opts) {
  ...
}
```

The search would return the first existing file with matching PDB magic.

The process of searching for a PDB file can be quite involved. Here are some things that we may implement one day:
- _NT_SYMBOL_PATH: an environment variable with PDB search paths
- Symbol servers: Maybe we will want to learn to talk to MS symbol servers to download PDBs

Even if we don't implement the symsrv protocol, it would be useful to teach LLVM how to find PDBs in a symbol server cache. For example, I have these kernel32.dll PDBs in C:\symbols, which windbg put them:
```
$ ls /c/symbols/kernel32.pdb/*
/c/symbols/kernel32.pdb/548CAA89F1FCF0193D12D2DC02F8920E1:
kernel32.pdb

/c/symbols/kernel32.pdb/63816243EC704DC091BC31470BAC48A31:
kernel32.pdb
```

If llvm-symbolizer wants to be able to symbolize system DLLs, that will be our best bet.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeSession.cpp:82
 
 Error NativeSession::createFromExe(StringRef Path,
                                    std::unique_ptr<IPDBSession> &Session) {
----------------
Maybe rename `Path` to `ExePath` so the code below that replaces the exe filename with the pdb filename is a bit more readable.


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeSession.cpp:98
                                  uint32_t &Offset) const {
-  return false;
 }
----------------
I see what you mean that it was mostly unimplemented. :)


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeSession.cpp:165
+
+  if (RVA >> 31 & 1)
+    return true;
----------------
Checking the sign bit makes sense, but these seem like simpler ways to express that:
- `(int32_t) RVA < 0`
- `RVA > INT32_MAX` (unsure on macro name)


================
Comment at: llvm/lib/DebugInfo/PDB/Native/NativeSession.cpp:173
+      return true;
+    Offset = RVA - Sec.VirtualAddress;
+  }
----------------
If we can subtract the virtual address out of it, are you sure it is an RVA? It seems like it's supposed to be a virtual address... If it's a virtual address, though, I would expect it to require a uint64_t. Hm.


================
Comment at: llvm/unittests/DebugInfo/PDB/Inputs/SimpleTest.cpp:4
+
+int main() { return 0; }
----------------
I tried to think of how to use yaml2pdb to avoid the binary files, but I don't like any of my solutions. I'm OK with the binary files.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D78128/new/

https://reviews.llvm.org/D78128