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

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

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

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

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.

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

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

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

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

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

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

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. > > >

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

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

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

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

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

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

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

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

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

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

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,

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

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

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

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

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

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

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

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

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/ > >

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