[Lldb-commits] [PATCH] D57751: minidump: Add ability to attach (breakpad) symbol files to placeholder modules

lemo accepted this revision. lemo added a comment. Looks good - a few comments inline. Comment at: include/lldb/Core/Module.h:178 +module_sp->m_file = module_sp->m_objfile_sp->GetFileSpec(); +return std::move(module_sp); } nit: return std::move(x) is

[Lldb-commits] [PATCH] D56595: SymbolFileBreakpad: Add line table support

lemo accepted this revision. lemo added a comment. This revision is now accepted and ready to land. The latest version looks good to me. Please update the description (it still says it uses the one CU per symbols file) Comment at: include/lldb/Core/FileSpecList.h:72 + /// Mov

[Lldb-commits] [PATCH] D57037: BreakpadRecords: Address post-commit feedback

lemo accepted this revision. lemo added a comment. This revision is now accepted and ready to land. Looks good - the all-zero UUID case is a bit unfortunate but it seems we have to handle it (like Greg suggested) Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.c

[Lldb-commits] [PATCH] D56844: Breakpad: Extract parsing code into a separate file

lemo added a comment. Looks good. A few questions/suggestions inline. Comment at: lldb/trunk/source/Plugins/ObjectFile/Breakpad/BreakpadRecords.cpp:74 + static_assert(sizeof(data) == 20, ""); + // The textual module id encoding should be between 33 and 40 bytes long, + // de

[Lldb-commits] [PATCH] D56461: [NativePDB] Add support for parsing typedefs and make lldb-test work with the native reader.

lemo added inline comments. Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:626 +// a series of namespaces. +// FIXME: do this. +CVSymbol global = m_index.ReadSymbolRecord(uid.asGlobalSym()); leftover comment?

[Lldb-commits] [PATCH] D56293: Use the minidump exception record if present

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB350546: Use the minidump exception record if present (authored by lemo, committed by ). Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56293/new/ https://reviews.llvm.

[Lldb-commits] [PATCH] D56293: Use the minidump exception record if present

lemo updated this revision to Diff 180515. lemo added a comment. Incorporating Pavel's suggestions (range-based loop, explicit type instead of auto) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56293/new/ https://reviews.llvm.org/D56293 Files: lit/Minidump/Windows/Sigsegv/Inputs/si

[Lldb-commits] [PATCH] D56293: Use the minidump exception record if present

lemo updated this revision to Diff 180263. lemo added a comment. Removed sigsegv.exe from the test inputs CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56293/new/ https://reviews.llvm.org/D56293 Files: lit/Minidump/Windows/Sigsegv/Inputs/sigsegv.cpp lit/Minidump/Windows/Sigsegv/Inp

[Lldb-commits] [PATCH] D56293: Use the minidump exception record if present

lemo created this revision. lemo added reviewers: labath, zturner. lemo added a project: LLDB. Herald added subscribers: jfb, abidh. If the minidump contains a saved exception record use it automatically. (This patch is cherry picked from the larger https://reviews.llvm.org/D55142) Repository:

[Lldb-commits] [PATCH] D55727: Add "dump" command as a custom "process plugin" subcommand when ProcessMinidump is used.

lemo accepted this revision. lemo added a comment. This revision is now accepted and ready to land. I love the idea of custom dump commands, thanks Greg. The changes look good to me (I agree with Zach suggestion to add lit tests) Comment at: source/Plugins/Process/minidump/Min

[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

[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

[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

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

lemo added inline comments. Comment at: include/lldb/Symbol/ObjectFile.h:569 + /// Returns the base file address of an object file (also known as the + /// preferred load address or image base address). This is typically the file "file address" can mean an of

[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] D55214: Introduce ObjectFileBreakpad

lemo accepted this revision. lemo added a comment. Looks like a great start Comment at: source/Plugins/ObjectFile/Breakpad/ObjectFileBreakpad.cpp:53 + static_assert(sizeof(data) == 20, ""); + if (str.size() < 33 || str.size() > 40) +return UUID(); these m

[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

[Lldb-commits] [PATCH] D54692: [Driver] Use libOption with tablegen.

lemo added a comment. I noticed a small problem, this change breaks "lldb -c ". The inline comment explains the root cause. Thanks Comment at: lldb/trunk/tools/driver/Driver.cpp:310 + if (args.hasArg(OPT_core)) { +SBFileSpec file(optarg); +if (file.Exists()) { --

[Lldb-commits] [PATCH] D54452: [NativePDB] Add support for handling S_CONSTANT records

lemo accepted this revision. lemo added inline comments. This revision is now accepted and ready to land. Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:329 +std::pair GetIntegralTypeInfo(TypeIndex ti, TpiStream &tpi) { + if (ti.isSimple()) {

[Lldb-commits] [PATCH] D54241: Fix bug in printing ValueObjects and in PE/COFF Plugin

lemo added a comment. Shouldn't we also handle the general case where raw size < virtual size? (which would naturally subsume this fix) Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:1073 + + if (section_offset >= section->GetByteSize()) +return 0;

[Lldb-commits] [PATCH] D54053: [NativePDB] Add the ability to create clang record decls from mangled names.

lemo accepted this revision. lemo added a comment. This revision is now accepted and ready to land. nice! Comment at: lldb/source/Plugins/SymbolFile/NativePDB/MangledAST.cpp:80 + + lldb::AccessType access = + (ttn->Tag == TagKind::Class) ? lldb::eAccessPrivate : lldb::eA

[Lldb-commits] [PATCH] D53731: [NativePDB] Add the ability to display global variables

lemo accepted this revision. lemo added a comment. This revision is now accepted and ready to land. looks good. a few comments inline. Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbSymUid.h:46 + // due to the debug magic at the beginning of the

[Lldb-commits] [PATCH] D53094: [pecoff] Implement ObjectFilePECOFF::GetDependedModules()

lemo added a comment. drive by CR notes: 1. does this handle forwarding imports? (it doesn't seem to from a quick glance at the code) 2. can you please add, or extend the existing test to cover the transitive case? A simple dag would suffice (ex. make both dllA and dllB implicitly import dllC)

[Lldb-commits] [PATCH] D53511: [NativePDB] Support type lookup by name

lemo accepted this revision. lemo added a comment. This revision is now accepted and ready to land. Looks good to me, minor notes inline. Comment at: lldb/lit/SymbolFile/NativePDB/tag-types.cpp:131 + +int main(int argc, char **argv) { + Struct S; zturner wrote

[Lldb-commits] [PATCH] D53511: [NativePDB] Support type lookup by name

lemo added a comment. Nice :) Comment at: lldb/lit/SymbolFile/NativePDB/tag-types.cpp:87 +// Test single inheritance. +class Derived : public Class { +public: - at least one virtual function + override? - at least one method returning void? C

[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

lemo added a comment. Hi Greg, looking at request_evaluate() I noticed that it will evaluate the string as a lldb command if prefixed by ` . This is a great feature (it allows building REPL consoles on top of DAP), but I'm curious how you picked up this convention? For example I believe that the

[Lldb-commits] [PATCH] D51604: Terminate debugger if an assert was hit

lemo added inline comments. Comment at: source/Utility/LLDBAssert.cpp:31 +"log, and as many details as possible\n"; + exit(1); } abort() may be a better choice here (exit() path calls a lot of shutdown code and it's not safe in a number of cases)

[Lldb-commits] [PATCH] D51442: Don't include the Age in the UUID for CvRecordPdb70 UUID records in minidump files

lemo accepted this revision. lemo added a comment. This revision is now accepted and ready to land. Looks good (with one inline request for a comment) Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:85 + auto arch = GetArchitecture(); + if (arch.GetTrip

[Lldb-commits] [PATCH] D51442: Don't include the Age in the UUID for CvRecordPdb70 UUID records in minidump files

lemo added a subscriber: zturner. lemo added a comment. I'm curious too: where did the PDB70 age create matching problems? On a related note, I just noticed that ObjectFilePECOFF::GetUUID() doesn't have a real implementation (just returns false). How do we extract module UUID for PE/COFF files?

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

This revision was automatically updated to reflect the committed changes. Closed by commit rLLDB340578: Restrict the set of plugins used for ProcessMinidump (authored by lemo, committed by ). Repository: rLLDB LLDB https://reviews.llvm.org/D51176 Files: source/Plugins/Process/minidump/Proce

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

lemo marked an inline comment as done. lemo added a comment. In https://reviews.llvm.org/D51176#1211433, @jingham wrote: > The other option would be to move the code that populates the section load > list into the mini dump dynamic loader. That has the benefit of keeping this > consistent with

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

lemo updated this revision to Diff 162270. lemo added a comment. Added the comment requested by zturner https://reviews.llvm.org/D51176 Files: source/Plugins/Process/minidump/ProcessMinidump.cpp source/Plugins/Process/minidump/ProcessMinidump.h Index: source/Plugins/Process/minidump/Proce

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

lemo added subscribers: bgianfo, clayborg, zturner. lemo added a comment. It's an interesting idea, thanks! I don't object moving code around if there's a strong case for it, but I'd like to keep the fix small and simple for now, but it's worth considering if the current minidump loading path will

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

lemo added a comment. In https://reviews.llvm.org/D51176#1211358, @clayborg wrote: > So this might actually work. Take a look around and see what else the dynamic > loader is used for and make sure that they won't be needed for anything else. > If not, this fix should work, but we should docume

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

lemo added a comment. In https://reviews.llvm.org/D51176#1211307, @clayborg wrote: > Dynamic loaders will fill out the section load list in the target that saids > "__TEXT" from "/tmp/a.out" was loaded at address 0x120200. So yes they > are needed. If the process minidump is manually settin

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

lemo added a comment. > That's not changing the module list, that's changing the loaded section > information. It is the dynamic loader's job to figure out what got loaded > where, plus your stack trace show this happening after we've selected a > plugin, not in the process of negotiating for t

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

lemo added a comment. > Dynamic loaders are needed for loading breakpad minidumps that are for MacOSX > and iOS processes. They should also be needed for loading any minidumps that > have stack traces. Thanks. I just validated the change against a macOS minidump and everything works fine in my

[Lldb-commits] [PATCH] D51176: Restrict the set of plugins used for ProcessMinidump

lemo created this revision. lemo added reviewers: clayborg, zturner. lemo added a project: LLDB. Herald added a subscriber: abidh. 1. The dynamic loaders should not be needed for loading minidumps and they may create problems (ex. the macOS loader resets the list of loaded modules) 2. In general,

[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

lemo added a subscriber: zturner. lemo added a comment. > The LLDB MI plugin didn't work very well as was quite flaky when I tested > it a while back. Just curious, what was the flaky part, the debug adapter or the LLDB MI interface? https://reviews.llvm.org/D50365 _

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

This revision was automatically updated to reflect the committed changes. Closed by commit rL339161: Misc module/dwarf logging improvements (authored by lemo, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D50274?vs=159432&id=159551#to

[Lldb-commits] [PATCH] D50365: Add a new tool named "lldb-vscode" that implements the Visual Studio Code Debug Adaptor Protocol

lemo added subscribers: clayborg, jingham, jasonmolenda, labath, lemo. lemo added a comment. Really cool! Are you planning to add some documentation for it? (set up instructions, etc...) https://reviews.llvm.org/D50365 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

lemo added inline comments. Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413 +section->GetName().GetCString(), +llvm::toString(Decompressor.takeError()).c_str()); +return 0; labath wrote: > lemo wrote: > > labath wrote: > > > T

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

lemo updated this revision to Diff 159432. lemo marked an inline comment as done. lemo added a comment. Updated the LIT file too https://reviews.llvm.org/D50274 Files: lit/Modules/compressed-sections.yaml source/Core/Module.cpp source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp source/Plug

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

lemo added inline comments. Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413 +section->GetName().GetCString(), +llvm::toString(Decompressor.takeError()).c_str()); +return 0; labath wrote: > This needs to be `std::move(Error)`.

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

lemo marked 2 inline comments as done. lemo added inline comments. Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:3413 +section->GetName().GetCString()); +return 0; } labath wrote: > `lit/Modules/compressed-sections.yaml` test will nee

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

lemo updated this revision to Diff 159368. lemo added a comment. Incorporating feedback, thanks. https://reviews.llvm.org/D50274 Files: source/Core/Module.cpp source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp source/Plugins/Process/minidump/ProcessMinidump.cpp source/Target/Process.cpp I

[Lldb-commits] [PATCH] D50290: Fix a bug in VMRange

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL338949: Fix a bug in VMRange (authored by lemo, committed by ). Herald added a subscriber: llvm-commits. Changed prior to

[Lldb-commits] [PATCH] D50290: Fix a bug in VMRange

lemo added subscribers: bgianfo, labath, penryu, teemperor, zturner. lemo added a comment. Thanks Zach. I can't find llvm::contains() though, any pointers to it? Repository: rLLDB LLDB https://reviews.llvm.org/D50290 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D49415: Add unit tests for VMRange

lemo added subscribers: teemperor, lemo. lemo added a comment. The new test cases did catch a real bug: [ RUN ] VMRange.CollectionContains llvm/src/tools/lldb/unittests/Utility/VMRangeTest.cpp:146: Failure Value of: VMRange::ContainsRange(collection, VMRange(0x100, 0x104)) Actual: false

[Lldb-commits] [PATCH] D50290: Fix a bug in VMRange

lemo created this revision. lemo added reviewers: zturner, labath, teemperor. lemo added a project: LLDB. I noticed a suspicious failure: [ RUN ] VMRange.CollectionContains llvm/src/tools/lldb/unittests/Utility/VMRangeTest.cpp:146: Failure Value of: VMRange::ContainsRange(collection, VMRange

[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements

lemo created this revision. lemo added reviewers: labath, clayborg. lemo added a project: LLDB. Herald added subscribers: JDevlieghere, arichardson, aprantl, emaste. Herald added a reviewer: espindola. This change improves the logging for the lldb.module category to note a few interesting cases:

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

lemo added a comment. Thanks Greg, looks good to me (a couple of inline comments left at your discretion) Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:15 // Other libraries and framework includes +//#include "lldb/Core/Architecture.h" #include "lldb/Core/M

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

lemo added subscribers: clayborg, labath, markmentovai, t.p.northover, zturner. lemo added a comment. FYI, Breakpad & Crashpad will start generating the Microsoft flavor of ARM minidumps soon. https://reviews.llvm.org/D49750 ___ lldb-commits mailing

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

lemo added a comment. > I never *ran LLDB tests*, not sure where they are and what they are. I hope you're planning to look into this before submitting the change :) https://reviews.llvm.org/D49685 ___ lldb-commits mailing list lldb-commits@lists.l

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

lemo added inline comments. Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:51 + reg_fpscr, + reg_d0, reg_d1, reg_d2, reg_d3, reg_d4, reg_d5, reg_d6, reg_d7, + reg_d8, reg_d9, reg_d10, reg_d11, reg_d12, reg_d13, reg_d14, reg_d15, -

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

lemo added inline comments. Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:216 + if (csd_version.find("Linux") != std::string::npos) +triple.setOS(llvm::Triple::OSType::Linux); + break; clayborg wrote: > I have run into some mini

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

lemo added a comment. Looks really good, a few comments inline. This may not big a big deal, but the part about FP (and Apple vs. non-Apple) is confusing: the FP is a pretty weak convention, and in some ABIs is not actually "fixed" (ex. FP can be either https://reviews.llvm.org/source/openmp/ o

[Lldb-commits] [PATCH] D49685: LLDB does not respect platform sysroot when loading core on Linux

lemo added subscribers: clayborg, EugeneBi, labath, lemo. lemo added a comment. > The problem is that shared libraries differ on these machines and > LLDB either fails to load some libraries *or loads wrong ones*. Not finding the modules is not surprising but the latter (loading the wrong module

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

lemo added a comment. Great timing! ARM support would be most welcome. Are you planning to support the Breakpad flavor or ARM minidumps or the Microsoft one? (Mark Mentovai just reminded me that the ARM support was added independently and some of the structures are different) Regarding the invali

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

lemo added subscribers: amccarth, bgianfo, labath, penryu. lemo added a comment. The problem is not returning an error from Minidump::Create() - if that was the case this could easily be improved indeed. The two-phase initialization is a consequence of the LLDB plugin lookup: 1. Target::CreatePro

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

This revision was automatically updated to reflect the committed changes. Closed by commit rL336918: Restructure the minidump loading path and add early & explicit consistency… (authored by lemo, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.l

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

lemo added a comment. Thanks Adrian for the thorough review. Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52 ASSERT_GT(parser->GetData().size(), 0UL); +auto result = parser->Initialize(); +if (expect_failure) amccarth wrote: > lemo

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

lemo updated this revision to Diff 155080. Herald added a subscriber: mgorny. https://reviews.llvm.org/D49202 Files: source/Plugins/Process/minidump/MinidumpParser.cpp source/Plugins/Process/minidump/MinidumpParser.h source/Plugins/Process/minidump/MinidumpTypes.cpp source/Plugins/Process

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

lemo updated this revision to Diff 155076. lemo added a comment. Adding a few ill-formed minidumps for testing the new checks https://reviews.llvm.org/D49202 Files: source/Plugins/Process/minidump/MinidumpParser.cpp source/Plugins/Process/minidump/MinidumpParser.h source/Plugins/Process/m

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

lemo added a comment. Regarding test for the other checks, I'll try to fabricate a few invalid minidumps (although it would obviously provide limited coverage) Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52 ASSERT_GT(parser->GetData().size(), 0UL); +

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

lemo updated this revision to Diff 155069. lemo marked 9 inline comments as done. lemo added a comment. Incorporating CR feedback https://reviews.llvm.org/D49202 Files: source/Plugins/Process/minidump/MinidumpParser.cpp source/Plugins/Process/minidump/MinidumpParser.h source/Plugins/Proce

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

lemo added inline comments. Comment at: source/Plugins/Process/minidump/MinidumpParser.cpp:23 #include +#include +#include amccarth wrote: > Why add ``? It looks like your new map is just a vector. Good catch Comment at: source/Plugins/Pro

[Lldb-commits] [PATCH] D49202: Restructure the minidump loading path and add early & explicit consistency checks

lemo created this revision. lemo added reviewers: amccarth, labath. lemo added a project: LLDB. Herald added a subscriber: mgrang. Corrupted minidumps was leading to unpredictable behavior. This change adds explicit consistency checks for the minidump early on. The checks are not comprehensive b

[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero

lemo added a comment. > I'm not sure if there is a suitable place for that function. This is > needed in "ObjectFileMachO" and two dynamic loader plugins. Then your factory idea may be the next best thing. While we're at it, maybe we can remove the UUID::SetBytes() from the public interface, an

[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero

lemo added subscribers: clayborg, labath, sas. lemo added a comment. > However, during parsing you need to know the meaning of a "...0" UUID. > In a MachO file (at least based on the comments in the code) this value is > used to denote the fact that the object file has no UUID. For elf, a >

[Lldb-commits] [PATCH] D48479: Represent invalid UUIDs as UUIDs with length zero

lemo added a comment. > The slight complication here is that > some clients (MachO) actually use the all-zero notation to mean "no UUID > has been set". To keep this use case working, I have introduced an > additional argument to the UUID constructor, which specifies whether an > all-zero vect

[Lldb-commits] [PATCH] D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

lemo added a comment. The intention in the scope of this change is just to check that the new overload is exposed correctly through the Python API. In general, guaranteeing specific error codes (messages?) is likely impractical: 1. It's not always easy to do the proper checks (ex. for 'file-not-

[Lldb-commits] [PATCH] D47991: Improve SBThread's stepping API using SBError parameter.

lemo added subscribers: clayborg, jingham, labath, apolyakov. lemo added a comment. > Ah I see. That's because the last argument is a C++ default argument. It > looks like the convention in this file is that the error argument should be > the last non-defaulted argument. I think it should be:

[Lldb-commits] [PATCH] D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

This revision was automatically updated to reflect the committed changes. Closed by commit rL334439: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails (authored by lemo, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://revie

[Lldb-commits] [PATCH] D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

lemo marked 3 inline comments as done. lemo added a comment. spaces removed :) https://reviews.llvm.org/D48049 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

lemo updated this revision to Diff 150844. https://reviews.llvm.org/D48049 Files: include/lldb/API/SBTarget.h packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py scripts/interface/SBTarget.i source/API/SBTarget.cpp Index: source/API/SBTarget.cpp ===

[Lldb-commits] [PATCH] D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

lemo updated this revision to Diff 150843. https://reviews.llvm.org/D48049 Files: include/lldb/API/SBTarget.h packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py scripts/interface/SBTarget.i source/API/SBTarget.cpp Index: source/API/SBTarget.cpp ===

[Lldb-commits] [PATCH] D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

lemo added subscribers: amccarth, clayborg, zturner. lemo added a comment. > remove space before ( I'd be happy to make the change, but looking at the rest of the file it seems that almost everything uses the opposite convention: Foo (...). So, to make sure I'm making the right change, is this h

[Lldb-commits] [PATCH] D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

lemo updated this revision to Diff 150835. lemo added a comment. Adding test cases for the new overload. https://reviews.llvm.org/D48049 Files: include/lldb/API/SBTarget.h packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py scripts/interface/SBTarget

[Lldb-commits] [PATCH] D48049: Add a new SBTarget::LoadCore() overload which surfaces errors if the load fails

lemo created this revision. lemo added reviewers: zturner, clayborg, amccarth. There was no way to find out what's wrong if SBProcess SBTarget::LoadCore(const char *core_file) failed. Additionally, the implementation was unconditionally setting sb_process, so it wasn't even possible to check if

[Lldb-commits] [PATCH] D47991: Add method SBThread::StepOver with SBError parameter.

lemo added inline comments. Comment at: include/lldb/API/SBThread.h:96 + void StepOver(SBError &error, +lldb::RunMode stop_other_threads = lldb::eOnlyDuringStepping); apolyakov wrote: > aprantl wrote: > > Where is the SBAPI documentation that i

[Lldb-commits] [PATCH] D47708: PDB support of function-level linking and splitted functions

lemo added a comment. I agree, checked in binaries are not always pretty. But some coverage depends checked in binaries (or at very least is dramatically harder to get the same thing from source) Are we saying that sacrificing coverage to keep tests smaller should be the default trade off? http

[Lldb-commits] [PATCH] D47708: PDB support of function-level linking and splitted functions

lemo added subscribers: zturner, lemo. lemo added a comment. Doesn't the LIT based test drop the split-function case (originally produced with PGO)? Sorry for being late to the party, but it seems beneficial to have both LIT *and* checked in binaries since in general they are complementary: check

[Lldb-commits] [PATCH] D46292: Use the UUID from the minidump's CodeView Record for placeholder modules

This revision was automatically updated to reflect the committed changes. lemo marked an inline comment as done. Closed by commit rL331394: Use the UUID from the minidump's CodeView Record for placeholder modules (authored by lemo, committed by ). Herald added a subscriber: llvm-commits. Changed

[Lldb-commits] [PATCH] D46292: Use the UUID from the minidump's CodeView Record for placeholder modules

lemo marked an inline comment as done. lemo added inline comments. Comment at: source/Commands/CommandObjectTarget.cpp:1390 +else { + strm.Printf("No object file for module: %s\n", + module->GetFileSpec().GetFilename().GetCString());

[Lldb-commits] [PATCH] D46292: Use the UUID from the minidump's CodeView Record for placeholder modules

lemo added inline comments. Comment at: include/lldb/Core/Module.h:779 + //-- + void SetUUID(const lldb_private::UUID &uuid); + labath wrote: > lemo wrote: > > labath wrote: > > > amccarth wrote: >

[Lldb-commits] [PATCH] D46292: Use the UUID from the minidump's CodeView Record for placeholder modules

lemo updated this revision to Diff 144790. https://reviews.llvm.org/D46292 Files: include/lldb/Core/Module.h include/lldb/Core/ModuleSpec.h packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py packages/Python/lldbsuite/test/functionalities/postmortem

[Lldb-commits] [PATCH] D46292: Use the UUID from the minidump's CodeView Record for placeholder modules

lemo added a comment. Thanks for the feedback. I uploaded a new revision (incorporating some of the feedback, including an ELF test case) Comment at: include/lldb/Core/Module.h:779 + //-- + void SetUUID(const ll

[Lldb-commits] [PATCH] D46292: Use the UUID from the minidump's CodeView Record for placeholder modules

lemo updated this revision to Diff 144751. lemo marked 5 inline comments as done. https://reviews.llvm.org/D46292 Files: include/lldb/Core/Module.h include/lldb/Core/ModuleSpec.h packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py packages/Python/ll

[Lldb-commits] [PATCH] D46292: Use the UUID from the minidump's CodeView Record for placeholder modules

lemo created this revision. lemo added reviewers: amccarth, clayborg, labath. lemo added a project: LLDB. This change adds support for two types of Minidump CodeView records: 1. PDB70 (reference: https://crashpad.chromium.org/doxygen/structcrashpad_1_1CodeViewRecordPDB70.html) This is by far th

[Lldb-commits] [PATCH] D45628: [LLDB] Support compressed debug info sections (.zdebug*)

lemo added subscribers: bgianfo, clayborg, alur, labath, penryu, lemo. lemo added a comment. Hi Erik, the review is still marked as requiring changes. Once that is sorted out I'd be happy to submit this on your behalf (what is the base SVN revision for the latest patch?) Davide Italiano, is all t

[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps

lemo added subscribers: bgianfo, penryu. lemo added a comment. > It looks like nobody except me is worried about the > module-without-an-object-file situation, so I guess we can try this out and > see how it goes. Sorry Pavel, I meant to respond to this: most of the code seems to explicitly han

[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps

This revision was automatically updated to reflect the committed changes. Closed by commit rL330302: Improve LLDB's handling of non-local minidumps (authored by lemo, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D45700?vs=142960&id=1

[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps

lemo added subscribers: amccarth, clayborg, labath, zturner. lemo marked an inline comment as done. lemo added a comment. Greg/Pavel, does the latest revision look good to you? Thanks! https://reviews.llvm.org/D45700 ___ lldb-commits mailing list ll

[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps

lemo marked an inline comment as done. lemo added a comment. In https://reviews.llvm.org/D45700#1070491, @amccarth wrote: > LGTM, but consider highlighting the side effect to `target` when the factory > makes a Placeholder module. Good observation: taking a step back, the factory introduces to

[Lldb-commits] [PATCH] D45700: Improve LLDB's handling of non-local minidumps

lemo updated this revision to Diff 142960. https://reviews.llvm.org/D45700 Files: packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py packages/Python/lldbsuite/test/functionalities/postmortem/minidump/TestMiniDump.py source/Commands/CommandObjectTarge

  1   2   >