[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

labath added inline comments. Comment at: lit/Minidump/Windows/Sigsegv/sigsegv.test:6 +CHECK: * thread #1, stop reason = Exception 0xc005 encountered at address 0x7ff7a13110d9 +CHECK: * frame #0: 0x7ff7a13110d9 sigsegv.exe`crash at sigsegv.cpp:22 + tee

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

teemperor added inline comments. Comment at: lit/Minidump/Windows/Sigsegv/sigsegv.test:6 +CHECK: * thread #1, stop reason = Exception 0xc005 encountered at address 0x7ff7a13110d9 +CHECK: * frame #0: 0x7ff7a13110d9 sigsegv.exe`crash at sigsegv.cpp:22 +

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

labath added a comment. Some interesting ideas here. I like some of them, but I have reservations about others. I'm going to go through them one by one. In D55142#1333077 , @zturner wrote: > I thought about what my "ideal" design would look like. I'm no

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

zturner added a comment. I thought about what my "ideal" design would look like. I'm not suggesting anyone should actually go off and do this, but since we're brainstorming design anyway, it doesn't hurt to consider what an optimal one should look like (and for the record, there may be things

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

On 13/12/2018 23:19, Zachary Turner wrote: The permanent solution would be figuring out what to do about the ObjectFile situation (e.g. do we want to make an ObjectFilePDB or do we want to live in a world where SymbolFiles need not be backed by ObjectFiles?), and then regardless of what we deci

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

That said, as you mentioned this enables other developers to start working on things, and if that means we can get the SymbolVendor in more quickly, then we can get rid of this more quickly. I just really want to converge towards the permanent solution, rather than away from it, so if committing t

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

The permanent solution would be figuring out what to do about the ObjectFile situation (e.g. do we want to make an ObjectFilePDB or do we want to live in a world where SymbolFiles need not be backed by ObjectFiles?), and then regardless of what we decide there, implementing the SymbolVendor that ca

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

Thanks Zach. Don't get me wrong, I have no problem tweaking it as long as necessary assuming we all agree on the plan: we could implement a ObjectFilePDB and a PDB SymbolVendor first, then the contentious PDB lookup detail goes away. My intention was to enable other developers to start consuming P

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

At this point it seems like perpetuating the hack, or at least even if that's the direction we decide to go longterm, not implementing that solution fully and missing some of the corner cases. So I think I'd rather just go with the original hack of checking the current directory at this point. St

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Just need a way to verify symbols are good. See my inline comment. Comment at: source/Commands/CommandObjectTarget.cpp:4246 if (symbol_file) { -

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

The problems I have with current directory lookup are: * It makes the behavior dependent on the environment, much like using an environment variable. This is a potential source of flakiness in tests, or different behavior on different peoples' machines. * It doesn't match WinDbg or MSVC * It's te

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

> > I think we can fix that by changing the line to: > > if (!object_file || object_file->GetFileSpec() == symbol_fspec) { > } > The problem is that SymbolFileNativePDB returns the module object file as it's own object file. Are you suggesting we should track the module object file separate from S

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

I think we can fix that by changing the line to: ``` if (!object_file || object_file->GetFileSpec() == symbol_fspec) { } ``` On Thu, Dec 13, 2018 at 12:04 PM Pavel Labath wrote: > On 13/12/2018 19:32, Leonard Mosescu wrote: > > What's the consensus? > > > > Personally I think that, even conside

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

On 13/12/2018 19:32, Leonard Mosescu wrote: What's the consensus? Personally I think that, even considering the potential issue that Paval pointed out, the "target symbols add ..." is the most conservative approach in terms of introducing new behavior. I'm fine with the current directory look

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

Well, Visual Studio also supports remote debugging, and searching next to the .dmp is just one of several places it looks. And LLDB also supports local debugging, and so looking next to the .dmp file, being consistent with Visual Studio, will be the expected behavior for people using LLDB in local

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

What's the consensus? Personally I think that, even considering the potential issue that Paval pointed out, the "target symbols add ..." is the most conservative approach in terms of introducing new behavior. I'm fine with the current directory lookup as well (the original change) since it's consi

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

On irc earlier i was talking about this with Greg and he said it should be fine in his opinion. I’ll point him to this review in the morning so he can comment On Thu, Dec 13, 2018 at 3:30 AM Pavel Labath via Phabricator < revi...@reviews.llvm.org> wrote: > labath added inline comments. > > > =

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

labath added inline comments. Comment at: source/Commands/CommandObjectTarget.cpp:4246 if (symbol_file) { -ObjectFile *object_file = symbol_file->GetObjectFile(); lemo wrote: > note I had to bypass this check: we don't (yet) have a Object

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

I ended up implementing the support for "target symbols add" since it's something we needed anyway. This allowed the removal of the contentious implicit search in the current directory. I tried to verify this behavior, but it seems like it should already work > out of the box? So we're on the sa

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

lemo marked an inline comment as done. lemo added inline comments. Comment at: source/Commands/CommandObjectTarget.cpp:4246 if (symbol_file) { -ObjectFile *object_file = symbol_file->GetObjectFile(); note I had to bypass this check: we do

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

lemo updated this revision to Diff 177898. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55142/new/ https://reviews.llvm.org/D55142 Files: lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.dmp lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.lld

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

There is another option which I was just made aware of. LLDB already has a setting called `target.debug-file-search-paths`. This is basically a symbol path. If you call Symbols::LocateExecutableSymbolFile, it will already add use this setting, and moreover it will implicitly add current working

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

On Tue, Dec 11, 2018 at 4:22 PM Leonard Mosescu wrote: > I guess I don't see why we need a temporary solution at all. If we can >> have logic that can be rolled into the SymbolVendor when we get it, and >> makes sense there, and is also simple, why not go with it? Failing that, >> doesn't the `

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

> > I guess I don't see why we need a temporary solution at all. If we can > have logic that can be rolled into the SymbolVendor when we get it, and > makes sense there, and is also simple, why not go with it? Failing that, > doesn't the `target symbols add` solution also work fine? > I just che

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

> But here, we're talking about a situation where there is no EXE, only a minidump. If there is a minidump and no EXE then neither WinDbg nor VS will search the minidump folder for the PDB. For the record, the experiments do not bear this out. VS will indeed search in the minidump folder for th

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

zturner added a comment. In D55142#1326247 , @lemo wrote: > > How large is the PDB file here? > > ~100kb We have a couple of tests in LLVM where PDB files are checked in, but they are very few. We cannot explode the repo with large numbers of binary fi

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

I guess I don't see why we need a temporary solution at all. If we can have logic that can be rolled into the SymbolVendor when we get it, and makes sense there, and is also simple, why not go with it? Failing that, doesn't the `target symbols add` solution also work fine? On Tue, Dec 11, 2018 a

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

> > But here, we're talking about a situation where there is no EXE, only a > minidump. If there is a minidump and no EXE then neither WinDbg nor VS > will search the minidump folder for the PDB. Indeed, this is key part. > In my mind, the algorithm could be something like: ... > I'm not a b

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

Actually, Adrian just tested this on his machine and it did look in his minidump folder. I don't know why we're seeing different behavior. Either way, regardless of whether MSVC / WinDbg look in the minidump folder, I still think it's a pretty intuitive location to add in the default search path.

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

Only one way to know for sure, and that's to test it :) So I did. Yes, it will search the directory of the EXE for the PDB. But here, we're talking about a situation where there is no EXE, only a minidump. If there is a minidump and no EXE then neither WinDbg nor VS will search the minidump fol

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

I believe the PDB is searched for in the EXE directory before the symbol search path is used. At least, that's what it used to do, back when I used VS debugger for post-mortem debugging. It was the only sane way to ensure it would find the right version of the PDB if you didn't have a local symbo

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

> > The Windowsy thing to do is what Zach said: Check the directory that > contains the .dmp for the .pdb. It's the first place the VS debugger looks > when opening a minidump. It's less sensitive to the user's environment. > (Making the user change the current working directory for this could b

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

It's really frustrating how the email discussion doesn't always make it to Phabricator. The Windowsy thing to do is what Zach said: Check the directory that contains the .dmp for the .pdb. It's the first place the VS debugger looks when opening a minidump. It's less sensitive to the user's envi

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

I think as combination of explicit symbol search path + something similar to Microsoft's symsrv would be a good "real" solution (and yes, that would be packaged as a SymbolVendor, outside SymbolFilePDB) For short term, I don't see a clearly superior alternative to searching the current directory.

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

On 11/12/2018 20:34, Zachary Turner wrote: I meant the location of the minidump.  So if you have C:\A\B\C\foo.dmp which is the dump file for bar.exe which crashed on another machine, then it would look for C:\A\B\C\bar.pdb.  That actually seems like fairly intuitive behavior to me, but maybe I'

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

I meant the location of the minidump. So if you have C:\A\B\C\foo.dmp which is the dump file for bar.exe which crashed on another machine, then it would look for C:\A\B\C\bar.pdb. That actually seems like fairly intuitive behavior to me, but maybe I'm in the minority :) We can see what Pavel, Ad

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

> > But if the minidump and PDBs are in the same directory, then wouldn't my > proposed solution also work (while also being a permanent solution)? > If we're looking in the same directory as the binary file (which is how I read your suggestion) then it would not be found in this case, since the b

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

But if the minidump and PDBs are in the same directory, then wouldn't my proposed solution also work (while also being a permanent solution)? On Tue, Dec 11, 2018 at 10:47 AM Leonard Mosescu wrote: > We talked about this offline, but bringing the discussion back here. Can >> you describe the us

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

> > We talked about this offline, but bringing the discussion back here. Can > you describe the use case that this is addressing? As you mention, this is > a temporary hack until we have proper symbol searching logic, but proper > symbol searching logic will do more than just look up symbols in a

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

zturner added inline comments. Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:139-144 +llvm::consumeError(expected_binary.takeError()); +pdb_file = obj_file.GetFileSpec() + .GetFileNameStrippingExtension() + .Ge

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

Thanks Pavel and Greg. It sounds to me like it would be better to have a separate command > (let's call it "target modules replace" for now) for adding an "object > file" to a "placeholder" module, instead of repurposing "target symbols > add" to do that. Yes, that would be my preference as well

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

> On Dec 11, 2018, at 7:14 AM, Pavel Labath wrote: > > On 11/12/2018 01:08, Greg Clayton wrote: >>> On Dec 10, 2018, at 3:11 PM, Leonard Mosescu >> > wrote: >>> >>> I can see how this works for the PDB, no-module-binary case. What about the >>> PDB & module-binary c

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

On 11/12/2018 01:08, Greg Clayton wrote: On Dec 10, 2018, at 3:11 PM, Leonard Mosescu > wrote: I can see how this works for the PDB, no-module-binary case. What about the PDB & module-binary case? That would work fine because the symbol vendor will make an object

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

> On Dec 10, 2018, at 3:11 PM, Leonard Mosescu wrote: > > I can see how this works for the PDB, no-module-binary case. What about the > PDB & module-binary case? That would work fine because the symbol vendor will make an object file from the m_symfile_spec and use both the original binary +

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

lemo added a comment. > How large is the PDB file here? ~100kb CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55142/new/ https://reviews.llvm.org/D55142 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

I can see how this works for the PDB, no-module-binary case. What about the PDB & module-binary case? Maybe I missed the rationale, but I think that modeling the general case: module and symbols are separate files, is a better foundation for the particular case where the symbols are embedded in th

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

clayborg added a comment. FYI: my approach to getting symbols to load was a bit different. I always let a simple PlaceholderModule be created, but I played some tricks in the GetObjectFile() method if someone had setting symbols for the module with "target symbols add ..". I will attach my Plac

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

> On Dec 10, 2018, at 11:23 AM, Leonard Mosescu wrote: > > > BTW: check my changes in: https://reviews.llvm.org/D55522 > > > > It will be interesting to you since it parses the linux maps info if it is > > available in breakpad generated minidump files. This

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

> How large is the PDB file here? 100Kb On Mon, Dec 10, 2018 at 11:48 AM Zachary Turner via Phabricator < revi...@reviews.llvm.org> wrote: > zturner added a comment. > > How large is the PDB file here? > > > CHANGES SINCE LAST ACTION > https://reviews.llvm.org/D55142/new/ > > https://reviews.ll

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

zturner added a comment. How large is the PDB file here? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55142/new/ https://reviews.llvm.org/D55142 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailma

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

lemo updated this revision to Diff 177576. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55142/new/ https://reviews.llvm.org/D55142 Files: lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.dmp lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.lld

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

labath added a comment. This looks fine to me. The test is a bit more coarse-grained than I'd like, though I can't think of a substantially better approach right now. One of the pdb folks should ok this too. Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:386

Re: [Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

> BTW: check my changes in: https://reviews.llvm.org/D55522 > It will be interesting to you since it parses the linux maps info if it is available in breakpad generated minidump files. This will give us enough info to create correct sections for object files when we have no ELF file or no symbol fi

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

clayborg added a comment. BTW: check my changes in: https://reviews.llvm.org/D55522 It will be interesting to you since it parses the linux maps info if it is available in breakpad generated minidump files. This will give us enough info to create correct sections for object files when we have no

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

lemo added inline comments. Comment at: source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:171-174 + // TODO: we need to compare the age, in addition to the GUID if (expected_info->getGuid() != guid) return nullptr; + labath wrote: > Mainly out

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

lemo updated this revision to Diff 177329. lemo marked 3 inline comments as done. lemo added a comment. Incorporating feedback + adding a test case CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55142/new/ https://reviews.llvm.org/D55142 Files: lit/Minidump/Windows/Sigsegv/Inputs/sigs

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

labath added a comment. In D55142#1320447 , @lemo wrote: > I agree with both comments. The intention is to add some tests but I wanted > to get the review out early to surface concerns, if any. I also needed more > time to investigate a few complex failu

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

markmentovai added a comment. This seems like a step in the right direction. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55142/new/ https://reviews.llvm.org/D55142 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llv

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

lemo updated this revision to Diff 176853. lemo marked an inline comment as done. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55142/new/ https://reviews.llvm.org/D55142 Files: source/Plugins/Process/minidump/MinidumpParser.cpp source/Plugins/Process/minidump/MinidumpParser.h sour

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

lemo marked 9 inline comments as done. lemo added a comment. In D55142#1316153 , @labath wrote: > I don't see any tests :(. > Also, the three bullet points in the description sound like rather > independent pieces of functionality. Would it be possible t

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

labath added a comment. I don't see any tests :(. Also, the three bullet points in the description sound like rather independent pieces of functionality. Would it be possible to split them up into separate patches? That would make things easier to review, particularly for those who don't look

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

zturner added inline comments. Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:60 + void Dump(Stream *s) override { *s << "PlaceholderObjectFile\n"; } + uint32_t GetAddressByteSize() const override { return 0; } + uint32_t GetDependentModules(FileSpecList &file

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

amccarth added a comment. This looks good to me, save for a couple comment nits. Comment at: source/Plugins/SymbolFile/NativePDB/PdbIndex.cpp:155 +// +// TODO: revisit this check since it fires for apparently valid PDBs +// Every apparently valid PD

[Lldb-commits] [PATCH] D55142: Minidump debugging using the native PDB reader

lemo created this revision. lemo added reviewers: labath, zturner, amccarth. lemo added a project: LLDB. Herald added subscribers: jfb, abidh, arphaman. A few changes required to enable the use of the native PDB reader when debugging minidumps: 1. Consume PDBs even if the module binary is not av