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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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,
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.
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
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,
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)
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
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
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
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
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
> 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
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) {
> 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
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
> 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
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
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
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
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:
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)
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
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
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
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 @@
+
+
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
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,
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
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
46 matches
Mail list logo