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 <ztur...@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 <clayb...@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
>
>
>
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
