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

[Greg Clayton via lldb-commits]

You could add a test to that grabs functions bounds, once you implement functions, and then lookup each address inside of a function and make sure each one has a file and line. This might fall down for complex examples, but we also ran into the Swift compiler saying a function went from 0x1000-0x2000 and yet there were address ranges inside that function that had no line entries.

Another convention that is widely used is the compiler can generate line entries whose line numbers are zero and this indicates it is a compiler auto generated code that doesn't point to any code in particular, but the code belongs to a code for a function still. 

Greg

> On Mar 1, 2016, at 11:42 AM, Zachary Turner <zturner at google.com> wrote:
> 
> Regarding the refactoring of ResolveSymbolContext to a lower level.  It seems like a worthwhile refactor but probably one that should be done as an independent CL.  It seems like it has potential to open up a bit of a rats nest so to speak, and if something ends up breaking as a result, we can revert just the targeted refactor instead of the entirety of PDB support.
> 
> On Tue, Mar 1, 2016 at 10:10 AM Greg Clayton <clayborg at gmail.com> wrote:
> clayborg requested changes to this revision.
> clayborg added a comment.
> This revision now requires changes to proceed.
> 
> One general comment is the use of "auto". Although it makes the code shorter, it does make it quite a bit less readable. I will leave the decision to you since this is your code, but in general I think this is where auto is less than it is cracked up to be.
> 
> 
> ================
> Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:282-285
> @@ +281,6 @@
> +
> +uint32_t
> +SymbolFilePDB::ResolveSymbolContext(const lldb_private::FileSpec &file_spec, uint32_t line, bool check_inlines,
> +                                    uint32_t resolve_scope, lldb_private::SymbolContextList &sc_list)
> +{
> +    if (resolve_scope & lldb::eSymbolContextCompUnit)
> ----------------
> So looking at SymbolFileDWARF::ResolveSymbolContext() I see it is very close to being SymbolFile agnostic... We would make SymbolFileDWARF::ResolveSymbolContext() into SymbolFile::ResolveSymbolContext() and clean it up to just use virtual SymbolFile calls. Then all SymbolFile plug-ins wouldn't need to implement this function. The basic flow of the function in DWARF is to iterate through all compile units. If check_inlines is true or the compile unit matches, grab the support files via lldb_private::CompileUnit::GetSupportFiles() and see if "file_spec" is in that support files list and find the one and only index for that file. If the index is valid, then get the LineTable from the compile unit via lldb_private::CompileUnit::GetLineTable(). Then find all matching entries. So with a quick refactor, all we need new SymbolFile instances to implement is GetSupportFiles() (which calls SymbolFile::ParseCompileUnitSupportFiles()) and CompileUnit::GetLineTable() (which calls into SymbolFile::ParseCompileUnitLineTable()). What do you think? This also helps others implementing new SymbolFile classes to not have to worry about the check_inlines thing.
> 
> ================
> Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:291-292
> @@ +290,4 @@
> +        // <vector>, either directly or indirectly.
> +        auto compilands =
> +            m_session_up->findCompilandsForSourceFile(file_spec.GetPath(), llvm::PDB_NameSearchFlags::NS_CaseInsensitive);
> +
> ----------------
> So if file_spec is "vector", this function will return all compile units that have line table entries that match "vector"? It doesn't seem like this is correct. If "check_inlines" is true, I would expect that you need to traverse all compilands?
> 
> ================
> Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:428-448
> @@ +427,23 @@
> +{
> +    auto found_cu = m_comp_units.find(id);
> +    if (found_cu != m_comp_units.end())
> +        return found_cu->second;
> +
> +    auto cu = m_session_up->getConcreteSymbolById<llvm::PDBSymbolCompiland>(id);
> +
> +    // `getSourceFileName` returns the basename of the original source file used to generate this compiland.  It does
> +    // not return the full path.  Currently the only way to get that is to do a basename lookup to get the
> +    // IPDBSourceFile, but this is ambiguous in the case of two source files with the same name contributing to the
> +    // same compiland. This is a moderately extreme edge case, so we consider this ok for now, although we need to find
> +    // a long term solution.
> +    auto file = m_session_up->findOneSourceFile(cu.get(), cu->getSourceFileName(),
> +                                                llvm::PDB_NameSearchFlags::NS_CaseInsensitive);
> +    std::string path = file->getFileName();
> +
> +    lldb::LanguageType lang;
> +    auto details = cu->findOneChild<llvm::PDBSymbolCompilandDetails>();
> +    if (!details)
> +        lang = lldb::eLanguageTypeC_plus_plus;
> +    else
> +        lang = TranslateLanguage(details->getLanguage());
> +
> ----------------
> "auto" really makes it hard to read this code to figure out what each variable actually is from someone that doesn't know the code. I will leave it up to you to do what you will with this, but this is where auto falls down for me.
> 
> 
> http://reviews.llvm.org/D17363
> 
> 
>