[Lldb-commits] [PATCH] D41092: Enable more abilities in SymbolFilePDB

[Zachary Turner via Phabricator via lldb-commits]

zturner added inline comments.


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:108-125
+      if (auto module_sp = m_obj_file->GetModule()) {
+        // See if a symbol file was specified through the `--symfile` option.
+        FileSpec symfile = module_sp->GetSymbolFileFileSpec();
+        if (symfile) {
+          error = loadDataForPDB(PDB_ReaderType::DIA,
+                                 llvm::StringRef(symfile.GetPath()),
+                                 m_session_up);
----------------
LLVM coding style dictates the use of early returns whenever possible.  So this block should be changed to something like.

```
auto module_sp = m_obj_file->GetModule();
if (!module_sp)
  return 0;

FileSpec symfile = module_sp->GetSymbolFileSpec();
if (!symfile)
  return 0;

error = loadDataForPDB(...);
if (error) {
  llvm::consumeError(error);
  return 0;
}

// etc
```


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:141
+          // in the `SECTIONHEADERS` data stream.
+          if (auto enum_dbg_streams_up = m_session_up->getDebugStreams()) {
+            while (auto dbg_stream_up = enum_dbg_streams_up->getNext()) {
----------------
Similar thing here.  Assign first, and then add:

```
if (!enum_dbg_streams_up)
  break;
```


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:144
+              auto stream_name = dbg_stream_up->getName();
+              if (stream_name == "SECTIONHEADERS") {
+                for (int rec_idx = 0;
----------------
```
if (stream_name != "SECTIONHEADERS")
  continue;
```


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:151-157
+                  if (strncmp(record_pointer, ".data", 5) == 0 ||
+                      strncmp(record_pointer, ".bss", 4) == 0 ||
+                      strncmp(record_pointer, ".rdata", 6) == 0) {
+                    abilities |= (GlobalVariables | LocalVariables |
+                                  VariableTypes);
+                    break;
+                  }
----------------
Is this correct?  If *any* of these 3 sections of present, then *all* of these capabilities are present?

Shouldn't we be actually trying to query DIA for a global, local, or variable and then seeing what it returns?

(Incidentally, it's easy to figure this out with the native PDB reader, since you can just look for the presence of a module symbol stream, globals stream, and/or TPI stream).


Repository:
  rL LLVM

https://reviews.llvm.org/D41092