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

2016-03-02 Thread Ying Chen via lldb-commits
chying added a comment. Yes, it is fixed now. Thank you! http://reviews.llvm.org/D17363 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

2016-03-02 Thread Greg Clayton via lldb-commits
clayborg added a subscriber: clayborg. clayborg added a comment. This is already fixed. Update your sources and let us know if things are working. http://reviews.llvm.org/D17363 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists

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

2016-03-02 Thread Greg Clayton via lldb-commits
This is already fixed. Update your sources and let us know if things are working. > On Mar 2, 2016, at 5:10 PM, Ying Chen wrote: > > chying added a subscriber: chying. > chying added a comment. > > Seems this patch breaks OSX build. I guess the new files were not added to > xcode project file

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

2016-03-02 Thread Jim Ingham via lldb-commits
Fixed with r262543 Jim > On Mar 2, 2016, at 5:10 PM, Ying Chen via lldb-commits > wrote: > > chying added a subscriber: chying. > chying added a comment. > > Seems this patch breaks OSX build. I guess the new files were not added to > xcode project file. > Could you please take a look? > htt

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

2016-03-02 Thread Jim Ingham via lldb-commits
jingham added a subscriber: jingham. jingham added a comment. Fixed with r262543 Jim http://reviews.llvm.org/D17363 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

2016-03-02 Thread Ying Chen via lldb-commits
chying added a subscriber: chying. chying added a comment. Seems this patch breaks OSX build. I guess the new files were not added to xcode project file. Could you please take a look? http://lab.llvm.org:8011/builders/lldb-x86_64-darwin-13.4/builds/8892/steps/ninja%20build%20local/logs/stdio ht

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

2016-03-02 Thread Greg Clayton via lldb-commits
clayborg added a comment. The empty line signifies that the previous entry was a termination entry. http://reviews.llvm.org/D17363 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

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

2016-03-02 Thread Greg Clayton via lldb-commits
clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. I guess you already tackled the termination entry! That is exactly what I wanted to see... Sounds like line tables are done! http://reviews.llvm.org/D17363

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

2016-03-02 Thread Zachary Turner via lldb-commits
zturner added a comment. Is this what I should be seeing? (lldb) target create D:\src\llvm\tools\lldb\unittests\SymbolFile\PDB\Inputs\test-pdb.exe Current executable set to 'D:\src\llvm\tools\lldb\unittests\SymbolFile\PDB\Inputs\test-pdb.exe' (i686). (lldb) target modules dump line-table

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

2016-03-02 Thread Zachary Turner via lldb-commits
zturner added a comment. That should be easy enough. If we make the simplifying assumption "all functions are represented contiguously in memory with no gaps", then I can just create a termination entry at `start_of_function + num_bytes_in_function`. Obviously this is a wrong assumption in the

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

2016-03-02 Thread Greg Clayton via lldb-commits
clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. As long as you don't mind your last line entry potentially being really large because it isn't terminated? (lldb) target create D:\src\llvm\tools\lldb\unittests\SymbolFile\PDB

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

2016-03-02 Thread Zachary Turner via lldb-commits
zturner added a comment. fwiw, here's my current output showing that things behave correctly for the test case I've created. (lldb) target create D:\src\llvm\tools\lldb\unittests\SymbolFile\PDB\Inputs\test-pdb.exe Current executable set to 'D:\src\llvm\tools\lldb\unittests\SymbolFile\PDB\I

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

2016-03-02 Thread Zachary Turner via lldb-commits
zturner added a comment. I looked into this a little bit, it's going to be a couple days of work for me to get termination entries set correctly. Can I go in as-is? I know you already Accepted the revision, just want to double check. I need to go make some changes to `llvm-pdbdump` on the LLV

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

2016-03-02 Thread Zachary Turner via lldb-commits
zturner added a comment. In http://reviews.llvm.org/D17363#366522, @clayborg wrote: > You are right. I am slowly seeing that PDB was designed well to be consumed > as a user would want to, not compressed to the point of obfuscation and very > difficult to extract data from like DWARF. > > So th

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

2016-03-02 Thread Greg Clayton via lldb-commits
clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. You are right. I am slowly seeing that PDB was designed well to be consumed as a user would want to, not compressed to the point of obfuscation and very difficult to extract data from like

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

2016-03-02 Thread Zachary Turner via lldb-commits
zturner added inline comments. Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:291-292 @@ +290,4 @@ +// , either directly or indirectly. +auto compilands = +m_session_up->findCompilandsForSourceFile(file_spec.GetPath(), llvm::PDB_NameSearch

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

2016-03-01 Thread 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-0

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

2016-03-01 Thread Zachary Turner via lldb-commits
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

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

2016-03-01 Thread Greg Clayton via lldb-commits
You don't add plug-in specify unique_ptr and shared_ptr definitions. You would put these in SymbolFilePDB.h only. > On Mar 1, 2016, at 10:51 AM, Jim Ingham wrote: > > There's an lldb-private-forward.h that you can use for this purpose if you > wish. > > Jim > >> On Mar 1, 2016, at 10:32 AM,

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

2016-03-01 Thread Jim Ingham via lldb-commits
There's an lldb-private-forward.h that you can use for this purpose if you wish. Jim > On Mar 1, 2016, at 10:32 AM, Zachary Turner via lldb-commits > wrote: > > > > On Tue, Mar 1, 2016 at 10:10 AM Greg Clayton wrote: > clayborg requested changes to this revision. > clayborg added a comment.

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

2016-03-01 Thread Zachary Turner via lldb-commits
On Tue, Mar 1, 2016 at 10:10 AM Greg Clayton 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 read

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

2016-03-01 Thread Greg Clayton via lldb-commits
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,

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

2016-02-29 Thread Zachary Turner via lldb-commits
zturner updated this revision to Diff 49442. zturner added a comment. Note this should compile now on every platform. Look over the unit tests to see my assumptions about how the API should work. Hopefully this makes reviewing things easier (it definitely made getting things to work easier)

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

2016-02-29 Thread Zachary Turner via lldb-commits
I think I see the problem. Somewhere I was calling SymbolVendor::FindPlugin(module) instead of module->GetSymbolVendor() On Mon, Feb 29, 2016 at 2:48 PM Greg Clayton wrote: > You shouldn't have to do any of this. There should be a 1 to 1 mapping > between a Module and a SymbolVendor which has

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

2016-02-29 Thread Greg Clayton via lldb-commits
You shouldn't have to do any of this. There should be a 1 to 1 mapping between a Module and a SymbolVendor which has a single SymbolFile. My guess is that you should just return: kAllAbilities for SymbolFilePDB::CalculateAbilities (). Try that and let me know how this goes. These abilities are

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

2016-02-29 Thread Zachary Turner via lldb-commits
I guess I could make the CreateInstance() method of SymbolFilePDB do some "smarts" to cache instances of SymbolFilePDB already created, and either return those or create a new one depending on whether we've loaded an instance for the requested executable or not. On Mon, Feb 29, 2016 at 1:26 PM Zac

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

2016-02-29 Thread Zachary Turner via lldb-commits
I'm running into what I think is the final issue. My SymbolFilePDB plugin creates created many many times. Specifically, every single call to SymbolVendor::FindPlugin(module) will cause a brand new instance of SymbolFilePDB to get created. This effectively kills any caching I've got going on, an

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

2016-02-29 Thread Zachary Turner via lldb-commits
Thanks! I think I've got a handle on it. I'll upload another patch this week hopefully. On Mon, Feb 29, 2016 at 10:30 AM Greg Clayton wrote: > > > On Feb 26, 2016, at 3:33 PM, Zachary Turner wrote: > > > > Ok, so back to check_inlines. I realized after I hit send that the > explanation I had

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

2016-02-29 Thread Greg Clayton via lldb-commits
> On Feb 26, 2016, at 3:33 PM, Zachary Turner wrote: > > Ok, so back to check_inlines. I realized after I hit send that the > explanation I had written out is exactly what I thought I had to do for > check_inlines == true. > > I guess a concrete example would make it clearer. If I have th

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

2016-02-26 Thread Zachary Turner via lldb-commits
Ok, so back to check_inlines. I realized after I hit send that the explanation I had written out is exactly what I thought I had to do for check_inlines == true. I guess a concrete example would make it clearer. If I have this code: // foo.cpp #include "foo.h" int main(int argc, char **argv) {

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

2016-02-26 Thread Greg Clayton via lldb-commits
> On Feb 26, 2016, at 3:22 PM, Zachary Turner wrote: > > > > On Fri, Feb 26, 2016 at 3:16 PM Greg Clayton wrote: > > > On Feb 26, 2016, at 2:49 PM, Zachary Turner wrote: > > > I'm coming back around to this now. What happens if check_inlines is > > False, but the FileSpec is a header fil

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

2016-02-26 Thread Zachary Turner via lldb-commits
On Fri, Feb 26, 2016 at 3:16 PM Greg Clayton wrote: > > > On Feb 26, 2016, at 2:49 PM, Zachary Turner wrote: > > > I'm coming back around to this now. What happens if check_inlines is > False, but the FileSpec is a header file like . You said "If > check_inlines is false, make sure file_spec m

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

2016-02-26 Thread Greg Clayton via lldb-commits
> On Feb 26, 2016, at 2:49 PM, Zachary Turner wrote: > > > > On Thu, Feb 18, 2016 at 6:16 PM Greg Clayton wrote: > > > 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 th

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

2016-02-26 Thread Zachary Turner via lldb-commits
On Thu, Feb 18, 2016 at 6:16 PM Greg Clayton wrote: > > > 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 u

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

2016-02-19 Thread Greg Clayton via lldb-commits
clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. So your LineTable::Entry objects that you create are still invalid since you didn't implement the support files. All comments became unaligned but you did comment that: > In Com

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

2016-02-19 Thread Zachary Turner via lldb-commits
zturner added inline comments. Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:150 @@ +149,3 @@ +return TranslateLanguage(details->getLanguage()); +} + Ahh i see the problem. The problem is not the value Im' specifying for the id of the compile u

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

2016-02-19 Thread Zachary Turner via lldb-commits
zturner updated this revision to Diff 48544. zturner added a comment. This should address all (I think) issues except for the one about `check_inlines`, which I'll do in a followup. Can you respond to my comment in a previous update about the uniqueness / index-ness of the comp_unit id? http:

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

2016-02-19 Thread 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)

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

2016-02-19 Thread Zachary Turner via lldb-commits
On Fri, Feb 19, 2016 at 10:43 AM Greg Clayton wrote: > > We should be covering this using API tests that make an example like the > one above and setting breakpoints in the inline header file. We have many > tests for this. It is hard to get compilers to generate things that you can > put in unit

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

2016-02-19 Thread Greg Clayton via lldb-commits
clayborg added a comment. General things to know about SymbolFiles: - We use SymbolContext objects to refer to a specific symbol context: module, compile unit, function, deepest block, line entry and symbol. - A load address should produce 1 symbol context. Never more. - A file address can produ

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

2016-02-18 Thread Zachary Turner via lldb-commits
zturner added a comment. I didn't respond to all your comments because I don't have the code in front of me. I'll take a look at the rest of the comments tomorrow. Comment at: source/Initialization/SystemInitializerCommon.cpp:202-204 @@ -198,1 +201,5 @@ + +#if defined(_MSC_VER

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

2016-02-18 Thread 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 @@ + +

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

2016-02-18 Thread Zachary Turner via lldb-commits
zturner added inline comments. Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:98-100 @@ +97,5 @@ +{ +auto global = m_session->getGlobalScope(); +auto compilands = global->findAllChildren(); +return compilands->getChildCount(); +} clayborg

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

2016-02-18 Thread Greg Clayton via lldb-commits
clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. So the main issue is to fix the: uint32_t SymbolFilePDB::ResolveSymbolContext(const lldb_private::FileSpec &file_spec, uint32_t line, bool check_inlines,

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

2016-02-17 Thread Zachary Turner via lldb-commits
zturner added a comment. Added a few notes to clarify things that might not be obvious upon a first look at the patch. Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:81 @@ +80,3 @@ +std::string exePath = m_obj_file->GetFileSpec().GetPath(); +auto er

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

2016-02-17 Thread Zachary Turner via lldb-commits
zturner created this revision. zturner added a reviewer: clayborg. zturner added a subscriber: lldb-commits. This is a first attempt at getting `SymbolFilePDB` working with line table support. A few notes: * This won't compile until a corresponding patch goes in on the LLVM side. The patch is