[Lldb-commits] [PATCH] D17363: Add SymbolFilePDB with basic support for line tables.

[Zachary Turner via lldb-commits]

zturner marked 6 inline comments as done.

================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:82-83
@@ +81,4 @@
+        auto error = llvm::loadDataForEXE(llvm::PDB_ReaderType::DIA, llvm::StringRef(exePath), m_session);
+        if (error != llvm::PDB_ErrorCode::Success)
+            return 0;
+    }
----------------
clayborg wrote:
> clayborg wrote:
> > how about reversing this just to be safe:
> > 
> > ```
> > if (error == llvm::PDB_ErrorCode::Success)
> >     return CompileUnits | LineTables;
> > ```
> how about reversing this just to be safe:
> 
> ```
> if (error == llvm::PDB_ErrorCode::Success)
>     return CompileUnits | LineTables;
> ```
> 
> Do you also need to clear m_session in the else clause?
I wrote it this way because then I only have to construct a non-zero value by ORing enums together once.  So I don't have to worry about keeping the different return values in sync with each other.  Not a big deal either awy, but that was my thinking.

================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:82-84
@@ +81,5 @@
+        auto error = llvm::loadDataForEXE(llvm::PDB_ReaderType::DIA, llvm::StringRef(exePath), m_session);
+        if (error != llvm::PDB_ErrorCode::Success)
+            return 0;
+    }
+    return CompileUnits | LineTables;
----------------
zturner wrote:
> clayborg wrote:
> > clayborg wrote:
> > > how about reversing this just to be safe:
> > > 
> > > ```
> > > if (error == llvm::PDB_ErrorCode::Success)
> > >     return CompileUnits | LineTables;
> > > ```
> > how about reversing this just to be safe:
> > 
> > ```
> > if (error == llvm::PDB_ErrorCode::Success)
> >     return CompileUnits | LineTables;
> > ```
> > 
> > Do you also need to clear m_session in the else clause?
> I wrote it this way because then I only have to construct a non-zero value by ORing enums together once.  So I don't have to worry about keeping the different return values in sync with each other.  Not a big deal either awy, but that was my thinking.
Shouldn't need to clear `m_session`.  If it returns an error, `m_session` will not have been initialized by the API.

================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:92
@@ +91,3 @@
+    lldb::addr_t obj_load_address = m_obj_file->GetFileOffset();
+    m_session->setLoadAddress(obj_load_address);
+}
----------------
clayborg wrote:
> Do you need to check m_session?
Shouldn't need to.  `m_session` will be valid if and only if the error code returned in `CalculateAbilities` was non-zero, and CalculateAbilities will return non-zero if and only if that call succeeded as well.  So we should never get here unless `CalculateAbilities` returned non-zero, which implies the session is valid.

================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:146
@@ +145,3 @@
+
+        uint32_t va = line->getVirtualAddress();
+        uint32_t lno = line->getLineNumber();
----------------
zturner wrote:
> clayborg wrote:
> > clayborg wrote:
> > > Is this really a 32 bit value for 32 and 64 bit binaries?
> > Again, is this really only a 32 bit value??? This seems wrong.
> Will have to check.  I think you're right though that it's a 64-bit value.  relative virtual address is 32-bit though, that's probably what confused me.
I checked, it is indeed a 64-bit value.  Thanks for catching this.

================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:149
@@ +148,3 @@
+        uint32_t cno = line->getColumnNumber();
+        uint32_t source_id = line->getSourceFileId();
+        bool is_statement = line->isStatement();
----------------
zturner wrote:
> clayborg wrote:
> > So line table entries get stored as lldb_private::LineTable::Entry values. The "source_id" you use is currently assumed to be an index into your compile unit's support files (CompileUnit::GetSupportFiles()). So you will either need to:
> > - make each CompileUnit create the support files array and translate your "source_id" over into an index into the compile units FileSpecList that will get returned from CompileUnit::GetSupportFiles().
> > - Change CompileUnit so that it no longer has a "FileSpecList &CompileUnit::GetSupportFiles()" function and make a "const FileSpec *CompileUnit::GetSupportFilebyID (lldb::user_id_t file_id);" and convert all users of "FileSpecList &CompileUnit::GetSupportFiles()" over to the new "const FileSpec *CompileUnit::GetSupportFilebyID (lldb::user_id_t file_id);"
> > 
> > So does a PDB file have a source file array itself that is accessed by a zero or one based index ID? If so it might be possible to keep GetSupportFiles(), else you might have better luck switching us over to "const FileSpec *CompileUnit::GetSupportFilebyID (lldb::user_id_t file_id);" so we allow each SymbolFile to determine how it speaks about files. In DWARF, we would continue to use the file index as the file_id, and PDB would end up using the "getSourceFileId()". Hopefully all source file accesses use this same source file ID??
> The file itself is a black box.  But the SDK the operating system provides to query the PDB does have its own notion of ids that it hands out.  It's unfortunate that I can't use this.  Your second solution of adding a `CompileUnit::GetSupportFileByID` might work, although it's kind of invasive change.  I'll see how much work is involved tomorrow.
> 
In `CompileUnit.h`, it says this:

```
/// A representation of a compilation unit, or compiled source file.
/// The UserID of the compile unit is specified by the SymbolFile
/// plug-in and can have any value as long as the value is unique
/// within the Module that owns this compile units.
```

Are you sure it's an index into the support files?  It sounds like maybe the DWARF plugin treats it as such, but it's not assumed to be such by any generic code.


http://reviews.llvm.org/D17363