[Lldb-commits] [PATCH] D55574: Remove else statements after returns

JDevlieghere abandoned this revision. JDevlieghere added a comment. I'm going to abandon this for now until we have better tooling to address Jim's concerns here. I'm also not super happy with the formatting, which seems to be off in quite a few locations. The problem is that clang-tidy knows

[Lldb-commits] [PATCH] D55584: [LLDB] Simplify Boolean expressions

Eugene.Zelenko added a comment. Thank you for cleanup effort! I would suggest to also run modernize checks and at least next readability checks: readability-container-size-empty readability-isolate-declaration readability-redundant-control-flow readability-redundant-member-init

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

[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

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

[Lldb-commits] [PATCH] D55584: Simplify boolean expressions

shafik added a comment. I find the `static_cast` to be a bit too clever, I don't think it helps readability. This is also too large to digest in a way I would feel satisfied that I did not miss anything. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D55574: Remove else statements after returns

aprantl added inline comments. Comment at: source/Plugins/Process/Utility/RegisterContextDarwin_arm.cpp:994 return FPURegSet; else if (reg < k_num_registers) return EXCRegSet; This is inconsistent (do you need to re-run it until it reaches a

[Lldb-commits] [PATCH] D55584: Simplify boolean expressions

aprantl added a comment. This one is probably less controversial than D55574 :-) Comment at: include/lldb/Target/ThreadPlanTracer.h:46 TracingStarted(); -else if (old_value == true && value == false) +else if (old_value &&

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] D55356: Add a method to get the "base" file address of an object file

On Tue, Dec 11, 2018 at 11:57 AM Pavel Labath wrote: > The part I know nothing about is whether something similar could be done > for PE/COFF files (and I'll need something like that there too). Adrian, > Zachary, what is the relation ship between "image base" of an object > file and its

[Lldb-commits] [PATCH] D55574: Remove else statements after returns

vsk added a comment. + 1 to this. If there's a tidy plugin for misleading indention, that might address some of Adrian's concerns. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55574/new/ https://reviews.llvm.org/D55574

[Lldb-commits] [PATCH] D55574: Remove else statements after returns

aprantl added a comment. I'm mostly on board with making these changes, as it's good LLVM style to do this. I highlighted a couple changes that might warrant a closer look. Comment at: source/API/SBProcess.cpp:1233 error.SetErrorString("process is invalid"); } +

[Lldb-commits] [lldb] r348901 - Remove unused file

Author: jdevlieghere Date: Tue Dec 11 14:46:56 2018 New Revision: 348901 URL: http://llvm.org/viewvc/llvm-project?rev=348901=rev Log: Remove unused file I removed the dotest-style reproducer test but forgot to delete the source file. Thanks Jim for the heads up! Removed:

[Lldb-commits] [PATCH] D55582: [Reproducers] Add command reproducer

JDevlieghere created this revision. JDevlieghere added reviewers: labath, jingham, sgraenitz. JDevlieghere added a project: LLDB. Herald added a subscriber: teemperor. Add support for reproducing to the command interpreter. In capture mode we capture all the command to file and in reproducer

[Lldb-commits] [PATCH] D55571: [ast] CreateParameterDeclaration should use an appropriate DeclContext.

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks good to me. Jim should ok as well. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55571/new/ https://reviews.llvm.org/D55571

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] D55356: Add a method to get the "base" file address of an object file

> On Dec 11, 2018, at 11:58 AM, Pavel Labath wrote: > > On 11/12/2018 20:39, Jim Ingham wrote: >> Sections can have parents. In MachO the text and data sections are actually >> contained in the TEXT and DATA segments respectively. LLDB represents this >> by having an lldb_private::Section

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

[Lldb-commits] [PATCH] D55569: [lit] Add a basic implementation of build for GccBuilder

stella.stamenova abandoned this revision. stella.stamenova added a comment. Let's use @labath's change instead. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55569/new/ https://reviews.llvm.org/D55569 ___

[Lldb-commits] [PATCH] D55430: build.py: Implement "gcc" builder

stella.stamenova accepted this revision. stella.stamenova added a comment. This revision is now accepted and ready to land. A couple of small comments, but it looks good otherwise. Thanks! Comment at: lit/BuildScript/toolchain-clang.test:1 +RUN: %build -n --verbose --arch=32

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

[Lldb-commits] [PATCH] D55575: [NativePDB] Support local variables

zturner created this revision. zturner added reviewers: labath, lemo, aleksandr.urakov, amccarth, clayborg, leonid.mashinskiy. Herald added subscribers: JDevlieghere, aprantl. This patch adds support for parsing and evaluating local variables. using the native pdb plugin. While implementing

[Lldb-commits] [PATCH] D55574: Remove else statements after returns

JDevlieghere created this revision. JDevlieghere added a reviewer: LLDB. Herald added subscribers: jsji, teemperor, abidh, arphaman, atanasyan, kbarton, arichardson, javed.absar, ki.stfu, nemanjai, kubamracek, sdardis, emaste, srhines. Herald added a reviewer: espindola. Herald added a reviewer:

[Lldb-commits] [PATCH] D55571: [ast] CreateParameterDeclaration should use an appropriate DeclContext.

zturner created this revision. zturner added reviewers: clayborg, jingham, labath, davide. Herald added subscribers: JDevlieghere, kristof.beyls, javed.absar. Previously CreateParameterDeclaration was always using the translation unit DeclContext. We would later go and add parameters to the

[Lldb-commits] [PATCH] D55569: [lit] Add a basic implementation of build for GccBuilder

zturner added a comment. @labath: Sorry, I didn't miss it, I just forgot about it (that's not much better, I know). I'll look today. If Pavel's is more complete let's use that as a starting point, although TBH I'm fine going with either one. Repository: rLLDB LLDB CHANGES SINCE LAST

[Lldb-commits] [PATCH] D55569: [lit] Add a basic implementation of build for GccBuilder

labath added a comment. Btw, I also wrote something like this in D55430 . (Zachary asked me to do it, but it looks like he missed the review request. :P) I think my version is more complete, though I am missing lto support, because I couldn't be bothered to

[Lldb-commits] [PATCH] D55569: [lit] Add a basic implementation of build for GccBuilder

stella.stamenova created this revision. stella.stamenova added reviewers: zturner, asmith. Herald added subscribers: lldb-commits, abidh. This adds an implementation for compile as well as compile-and-link, but not link. For compile-and-link we rely on clang to orchestrate both similarly to how

[Lldb-commits] [lldb] r348894 - [Driver] Simplify OptionData. NFC

Author: jdevlieghere Date: Tue Dec 11 12:19:53 2018 New Revision: 348894 URL: http://llvm.org/viewvc/llvm-project?rev=348894=rev Log: [Driver] Simplify OptionData. NFC Hopefully this makes the option data easier to understand and maintain. - Group the member variables. - Do the initialization

Re: [Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

In MachO a segment fully contains its sections and a section can only be in one segment. I don't remember how much the lldb Section nesting code enforces this. It would be weird to have two Sections at the same level overlap, I don't see any "GetSectionsForAddress(lldb::addr_t address)" API's

Re: [Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

On 11/12/2018 20:39, Jim Ingham wrote: Sections can have parents. In MachO the text and data sections are actually contained in the TEXT and DATA segments respectively. LLDB represents this by having an lldb_private::Section for the segment, and then all the sections in that segment are

Re: [Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

Sections can have parents. In MachO the text and data sections are actually contained in the TEXT and DATA segments respectively. LLDB represents this by having an lldb_private::Section for the segment, and then all the sections in that segment are children of the parent Section (the MachO

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] D55356: Add a method to get the "base" file address of an object file

On 11/12/2018 20:10, Jim Ingham via Phabricator wrote: jingham added a comment. In D55356#1327280 , @clayborg wrote: In D55356#1327242 , @labath wrote: In D55356#1327224

[Lldb-commits] [lldb] r348890 - Add ObjectFileBreakpad.{cpp, h} to the Xcode project.

Author: jingham Date: Tue Dec 11 11:25:03 2018 New Revision: 348890 URL: http://llvm.org/viewvc/llvm-project?rev=348890=rev Log: Add ObjectFileBreakpad.{cpp,h} to the Xcode project. Modified: lldb/trunk/lldb.xcodeproj/project.pbxproj Modified: lldb/trunk/lldb.xcodeproj/project.pbxproj URL:

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

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

jingham added a comment. In D55356#1327280 , @clayborg wrote: > In D55356#1327242 , @labath wrote: > > > In D55356#1327224 , @clayborg > > wrote: > > > > > In

Re: [Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

The current behavior is definitely correct. An Address without a section is generally going to be something like a stack or heap address. Those definitely have load addresses of whatever their value is. I'm not sure what it means to have a file address represented as an offset not a section

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

clayborg added a comment. In D55356#1327242 , @labath wrote: > In D55356#1327224 , @clayborg wrote: > > > In D55356#1327099 , @labath wrote: > > > > > Actually, this now

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] D55356: Add a method to get the "base" file address of an object file

On 11/12/2018 19:17, Jim Ingham wrote: It the section isn't in the target's SectionLoadList, then GetLoadAddress should return LLDB_INVALID_ADDRESS. I.e. this bit from Section::GetLoadBaseAddress: load_base_addr = target->GetSectionLoadList().GetSectionLoadAddress(

[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() +

Re: [Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

It the section isn't in the target's SectionLoadList, then GetLoadAddress should return LLDB_INVALID_ADDRESS. I.e. this bit from Section::GetLoadBaseAddress: load_base_addr = target->GetSectionLoadList().GetSectionLoadAddress( const_cast(this)->shared_from_this()); should return

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

labath added a comment. In D55356#1327224 , @clayborg wrote: > In D55356#1327099 , @labath wrote: > > > Actually, this now causes an lldb-mi test to fail, but it's not clear to me > > if the problem is in the

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

clayborg added a comment. In D55356#1327224 , @clayborg wrote: > In D55356#1327099 , @labath wrote: > > > Actually, this now causes an lldb-mi test to fail, but it's not clear to me > > if the problem is in the

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

[Lldb-commits] [PATCH] D55522: Cache memory regions in ProcessMinidump and use the linux maps as the source of the information if available.

clayborg added a comment. In D55522#1326955 , @labath wrote: > It just occurred to me that we have another copy of /proc//maps -> > MemoryRegionInfo conversion code. It lives in NativeProcessLinux.cpp > (ParseMemoryRegionInfoFromProcMapsLine). It would

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

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

clayborg added a comment. In D55356#1327099 , @labath wrote: > Actually, this now causes an lldb-mi test to fail, but it's not clear to me > if the problem is in the test, or this patch. This issue happens when lldb-mi > is printing the "library loaded"

[Lldb-commits] [PATCH] D53677: Fix a bug PlatformDarwin::SDKSupportsModule

aprantl closed this revision. aprantl added a comment. Looks like this landed in r345274 and wasn't properly closed. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D53677/new/ https://reviews.llvm.org/D53677 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D55559: Remove the Disassembly benchmarks.

aprantl created this revision. aprantl added reviewers: jingham, jasonmolenda. While I was out hunting for remaining pexpect-based tests, I came across these tests that can't possibly work an any modern system, as they rely on having gdb available in /Developer. This patch simply removes the

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

labath added a comment. Actually, this now causes an lldb-mi test to fail, but it's not clear to me if the problem is in the test, or this patch. This issue happens when lldb-mi is printing the "library loaded" message after a module gets added to a not-yet-running target. It tries to print

[Lldb-commits] [PATCH] D55356: Add a method to get the "base" file address of an object file

labath updated this revision to Diff 177717. labath added a comment. Rebase after comitting D55422 . Now this patch just implements GetBaseAddress for ELF and PECOFF object files, and adds tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55356/new/

[Lldb-commits] [PATCH] D55422: Rename ObjectFile::GetHeaderAddress to GetBaseAddress

This revision was automatically updated to reflect the committed changes. Closed by commit rL348849: Rename ObjectFile::GetHeaderAddress to GetBaseAddress (authored by labath, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit:

[Lldb-commits] [lldb] r348849 - Rename ObjectFile::GetHeaderAddress to GetBaseAddress

Author: labath Date: Tue Dec 11 07:21:15 2018 New Revision: 348849 URL: http://llvm.org/viewvc/llvm-project?rev=348849=rev Log: Rename ObjectFile::GetHeaderAddress to GetBaseAddress Summary: This function was named such because in the case of MachO files, the mach header is located at this

[Lldb-commits] [PATCH] D55422: Rename ObjectFile::GetHeaderAddress to GetBaseAddress

labath marked 2 inline comments as done. labath added inline comments. Comment at: source/Plugins/ObjectFile/Mach-O/ObjectFileMachO.cpp:1677 // Check to see if the module was read from memory? - if (module_sp->GetObjectFile()->GetHeaderAddress().IsValid()) { +

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

[Lldb-commits] [PATCH] D55522: Cache memory regions in ProcessMinidump and use the linux maps as the source of the information if available.

labath added a comment. It just occurred to me that we have another copy of /proc//maps -> MemoryRegionInfo conversion code. It lives in NativeProcessLinux.cpp (ParseMemoryRegionInfoFromProcMapsLine). It would be nice to extract this parser into a some place (`Plugins/Process/Utility` ?),