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

[Greg Clayton via lldb-commits]

clayborg added a comment.

There are problems with the source file IDs you are using as they currently must be indexes into the compile unit's support files. See inline comments for more details.


================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:146
@@ +145,3 @@
+
+        uint32_t va = line->getVirtualAddress();
+        uint32_t lno = line->getLineNumber();
----------------
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.

================
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();
----------------
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??

================
Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:256-257
@@ +255,4 @@
+        // Locate all compilation units for the given source file.
+        auto compilands =
+            m_session->findCompilandsForSourceFile(file_spec.GetPath(), llvm::PDB_NameSearchFlags::NS_CaseInsensitive);
+
----------------
zturner wrote:
> clayborg wrote:
> > You need to still look through all line tables if "check_inlines" is true. Someone might ask for "vector" as the FileSpec (no directory) and 12 for the line table. What we do in DWARF is grab all compile units, get the "LineTable*" from each compile unit, and look to see if "file_spec" is in the support files by asking the support files to find the index for "file_spec" and if the file index is not UINT32_MAX, then we iterate through all line table entries and extract the entries that match. See the SymbolFileDWARF::ResolveSymbolContext() for more details.
> So does `check_inlines` really mean `check_header_files`?  I kind of ignored the argument because it wasn't obvious to me what it did based on the name.
> 
> What is supposed to happen if someone specifies `vector` as the FileSpec, but vector is #included from 10 different compile units?  Will this return line information from all 10 of those compile units?
> 
> I'm not using support files for anything right now because I can't figure out if it makes sense or if we need it.  The PDB tells us, for each compile unit, the list of all source files that contribute to that compile unit.  It sounds like that's what you're using support files for, but in my case I'm comfortable (at least for now) simply querying the PDB every time.
> 
> Just to make sure I understand, is it safe to say that:
> 
> 1. If `check_inlines` is false, `sc_list` should return with exactly 1 `SymbolContext` with `m_comp_unit` set to the main source file?
> 2. The same for the `check_inlines` is false calse -- we still just have ` SymbolContext` but we add line table entries from all additional files that contribute to the compilation unit?
> 
> When will `sc_list`  end up with multiple entries?
> So does check_inlines really mean check_header_files? I kind of ignored the argument because it wasn't obvious to me what it did based on the name.

So yes "check_inlines" means to check header files.
 
> What is supposed to happen if someone specifies vector as the FileSpec, but vector is #included from 10 different compile units? Will this return line information from all 10 of those compile units?

Yes, and there might be multiple vector line 10 entries in each compile unit, so you may end up with many.

> I'm not using support files for anything right now because I can't figure out if it makes sense or if we need it. The PDB tells us, for each compile unit, the list of all source files that contribute to that compile unit. It sounds like that's what you're using support files for, but in my case I'm comfortable (at least for now) simply querying the PDB every time.

That is fine as long as it is quick and also supports looking up things by basename. Does PDB support that? Most of the time you won't be getting a full path since users rarely type a full path.

You are specifying a source_id for your line entries when you create your line tables, and you will need to have them be valid indexes into your support files otherwise, your LineTable entries won't ever be able to lookup the file... See above comment in your line table code...
> 
> Just to make sure I understand, is it safe to say that:
> 
> If check_inlines is false, sc_list should return with exactly 1 SymbolContext with m_comp_unit set to the main source file?

You would get one SymbolContext per compile unit whose path matches the file_spec _and_ contains a line number. There might be multiple "Foo.cpp" files in different directories withing one PDB file, so you might still end up finding N matches. So your existing code can be used when check_inlines if false as long as the PDB can search by fullname and by basename. I am guessing it doesn't though, am I wrong?

> The same for the check_inlines is false calse -- we still just have  SymbolContext but we add line table entries from all additional files that contribute to the compilation unit?
> When will sc_list end up with multiple entries?

You always look through all compile units. If check_inlines is false, then you make sure "file_spec" matches (by basename  if only basename is specified, or by full path if a directory and filename are valid inside file_spec. If the file matches, then look for any lines that match and return ALL instances of them. Auto inlining of a function inside a compile unit might cause there to be many entries for line 10.

If check_inlines is true, then you must find all inlined entries that match. This means the compile unit itself, or any other line entries whose file matches must be considered. This is the default scenario. With each matching file, you must find all of the matches. Such would be the case for "vector" line 10 since you might have 20 std::vector::append() instantiations in the current compile unit and you would want to stop at each of these instances if you said "b vector:10".

If someone specifies "vector" and line 10, then yes, it should return line info for all matches for "vector" line 10 from all compile units. This is the default thing the user wants so we need to make sure looking up inline entries works well and is performant. If you have a way to query the PDB for all matches by file and line that can also do inline files, feel free to use that and then ask each line entry for its file address, and then look up the file address in your LineTable entries using the:

```
SymbolFilePDB::ResolveSymbolContext(const lldb_private::Address &so_addr, uint32_t resolve_scope,
                                    lldb_private::SymbolContext &sc)
```

So it depends on how well PDB supports lookups by file and line. And also be sure to handle the case where file_spec has only a basename.


http://reviews.llvm.org/D17363