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

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM. Comment at: lit/Modules/MachO/basic-info.yaml:10 +# CHECK: Strata: user +# CHECK: Base VM address: 0x1 + Just so I understand, this 0x1

[Lldb-commits] [PATCH] D56418: Change lldb-test to use ParseAllDebugSymbols instead of ParseDeclsForContext

amccarth added inline comments. Comment at: lldb/source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:318 + + TypeSystem *type_system = GetTypeSystemForLanguage(lldb::eLanguageTypeC_plus_plus); + if (!type_system) Is it appropriate to hard code that language type h

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

amccarth added inline comments. Comment at: lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py:78 +self.assertFalse(self.process, PROCESS_IS_VALID) +self.assertTrue(error.Fail()) + Is it worth che

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

amccarth added a comment. My motivation was not to check for different kinds of errors but to ensure that the error code detail doesn't get lost through the SWIG plumbing. SBErrors seem to be marshaled properly when used as return values, but this is the first one I've seen returned by referen

[Lldb-commits] [PATCH] D48960: Use an unwinder to get register contexts of frames other than zeroth under Windows

amccarth added inline comments. Comment at: source/Plugins/Process/Windows/Common/TargetThreadWindows.cpp:52 if (!m_reg_context_sp) -m_reg_context_sp = CreateRegisterContextForFrameIndex(0); +m_reg_context_sp = CreateRegisterContextForZerothFrame();

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

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

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

amccarth added a comment. Also, I'm not seeing tests for the other consistency checks you're adding (like whether there are any streams or whether the streams overlap). Is it difficult to create such malformed minidumps? Comment at: unittests/Process/minidump/MinidumpParserT

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

amccarth added inline comments. Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:172 + + // Do we support the minidump's architecture? + ArchSpec arch = GetArchitecture(); lemo wrote: > amccarth wrote: > > Should the architecture check be in the

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

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM if you don't want to consider my remaining suggestion for this patch. Thanks for the extra tests. Comment at: unittests/Process/minidump/MinidumpParserTest.cpp:52

[Lldb-commits] [PATCH] D43202: Remove dead code for handling DWARF pubnames

This revision was automatically updated to reflect the committed changes. Closed by commit rL324925: Remove dead code for handling DWARF pubnames (authored by amccarth, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D43202?vs=133903&id

[Lldb-commits] [PATCH] D43215: Supply missing break in case statement.

amccarth updated this revision to Diff 134252. amccarth added a comment. Per Aaron's suggestion, I ran the tests with an unconditional return (and an assertion). There was no change in the test results, so I'm making this return unconditionally. https://reviews.llvm.org/D43215 Files: lldb/

[Lldb-commits] [PATCH] D43215: Supply missing break in case statement.

amccarth added a comment. Any remaining concerns? https://reviews.llvm.org/D43215 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D43215: Supply missing break in case statement.

This revision was automatically updated to reflect the committed changes. Closed by commit rL325188: Supply missing break in case statement. (authored by amccarth, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D43215?vs=134252&id=1343

[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows

amccarth created this revision. amccarth added a reviewer: jasonmolenda. Herald added a subscriber: sanjoy. This test was failing on Windows because it expected the breakpoint in the dynamic library to be resolved before the process is launched. Since the DLL isn't loaded until the process is lau

[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows

amccarth added a comment. OK, so my fix is too simplistic. I'll have to (1) make LLDB on Windows also attempt to pre-load the DLLs, (2) make the test use platform-specific expectations, or (3) use a looser expectation. I'll give it some thought over the weekend. https://reviews.llvm.org/D43

[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows

amccarth added a comment. I have reservations about making the debugger try to pre-locate the modules and their PDBs before those modules are actually loaded. For a few reasons. (1) On Windows, module resolution is complex. Search paths, the safe search path, manifests, the side-by-side cache

[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows

amccarth updated this revision to Diff 135120. amccarth added a comment. Added new special value to accommodate the fact that Windows may postpone resolving the breakpoints for DLLs that aren't yet loaded. https://reviews.llvm.org/D43419 Files: lldb/packages/Python/lldbsuite/test/functional

[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows

amccarth updated this revision to Diff 135124. amccarth added a comment. Wrapped the modified docstring. https://reviews.llvm.org/D43419 Files: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/TestBreakpointInGlobalConstructor.py lldb/packages/Python/lldbsu

[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows

amccarth added a comment. In https://reviews.llvm.org/D43419#1013559, @jingham wrote: > At some point we should introduce "platformCapacities" so that you could do: > > if not test.getPlatformCapacities("preload-dylibs"): > > > so we are testing specific features not the whole platform. Ye

[Lldb-commits] [PATCH] D43532: Fix TestSBData.py on Windows (which uses Python 3)

amccarth created this revision. amccarth added a reviewer: zturner. Herald added a subscriber: sanjoy. This test uses the SB API to set and read back bytes of data, and it works fine when Python 2 is the scripting language. On Windows, however, Python 3 is the default. Note this line from the

[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows

amccarth added a comment. By unconditional, do you mean allowing any value for `out_num_locations` in these cases? I'm happy to do that, but I'm not sure if I've understood you correctly. https://reviews.llvm.org/D43419 ___ lldb-commits mailing l

[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows

amccarth updated this revision to Diff 135272. amccarth added a comment. Per Pavel's suggestion, change special value to mean don't check the number of locations found. https://reviews.llvm.org/D43419 Files: lldb/packages/Python/lldbsuite/test/functionalities/breakpoint/global_constructor/T

[Lldb-commits] [PATCH] D43419: Fix TestBreakpointInGlobalConstructor for Windows

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 rL325704: Fix TestBreakpointInGlobalConstructor for Windows (authored by amccarth, committed by ). Herald added a subscriber

[Lldb-commits] [PATCH] D43599: FFix TestSBData.py on Windows (which uses Python 3)

amccarth created this revision. amccarth added a reviewer: zturner. Herald added a subscriber: sanjoy. This test uses the SB API to set and read back bytes of data, and it works fine when Python 2 is the scripting language. On Windows, however, Python 3 is the default. Note this line from the tes

[Lldb-commits] [PATCH] D43600: Fix TestMoveNearest on Windows

amccarth created this revision. amccarth added a reviewer: zturner. Herald added a subscriber: sanjoy. The header file for the DLL tried to declare inline functions and a local function as dllexport which broke the compile and link. Removing the bad declarations solves the problem, and the test p

[Lldb-commits] [PATCH] D43599: FFix TestSBData.py on Windows (which uses Python 3)

amccarth abandoned this revision. amccarth added a comment. Please ignore. I'm still trying to figure out arc. https://reviews.llvm.org/D43599 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/

[Lldb-commits] [PATCH] D43600: Fix TestMoveNearest on Windows

amccarth added a comment. In https://reviews.llvm.org/D43600#1016483, @zturner wrote: > Do we not need `call_foo2` anymore? call_foo2 is defined in the executable, so attempting to report it as dllexport from the DLL is nonsensical and causes link-time errors. https://reviews.llvm.org/D43600

[Lldb-commits] [PATCH] D43600: Fix TestMoveNearest on Windows

amccarth added a comment. call_foo1() is defined in foo.cpp, which is built into a DLL, and thus needs the __declspec(dllimport/dllexport) from the LLDB_TEST_API. foo.h defines the API for the DLL. call_foo2() is defined in main.cpp, which is built into an executable that depends on foo.dll.

[Lldb-commits] [PATCH] D43600: Fix TestMoveNearest on Windows

This revision was automatically updated to reflect the committed changes. Closed by commit rL325836: Fix TestMoveNearest on Windows (authored by amccarth, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D43600?vs=135345&id=135524#toc R

[Lldb-commits] [PATCH] D43532: Fix TestSBData.py on Windows (which uses Python 3)

This revision was automatically updated to reflect the committed changes. Closed by commit rL325835: Fix TestSBData.py on Windows (authored by amccarth, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D43532?vs=135148&id=135523#toc Rep

[Lldb-commits] [PATCH] D43688: Partial fix for TestConflictingSymbol.py on Windows

amccarth created this revision. amccarth added a reviewer: labath. Herald added subscribers: aprantl, sanjoy. Without this fix, the test ERRORs because the link of the inferior fails. This patch adds the LLDB_TEST_API macro where needed and uses the new -2 magic value for num_expected_locations

[Lldb-commits] [PATCH] D43688: Partial fix for TestConflictingSymbol.py on Windows

amccarth added a comment. In https://reviews.llvm.org/D43688#1017638, @labath wrote: > This test deliberately compiles without `-g`, so the variable should not be > in the debug info. Ah, that makes more sense than what I was thinking: that it was trying to make sure the unused symbol wasn't

[Lldb-commits] [PATCH] D43688: Partial fix for TestConflictingSymbol.py on Windows

amccarth added a comment. In https://reviews.llvm.org/D43688#1017691, @labath wrote: > In https://reviews.llvm.org/D43688#1017652, @amccarth wrote: > > > In https://reviews.llvm.org/D43688#1017638, @labath wrote: > > > > > On non-windows systems we're expected to pick up the name from the symbol

[Lldb-commits] [PATCH] D43662: [Utility] Simplify and generalize the CleanUp helper, NFC

amccarth added a comment. Nit: As a single word, "cleanup" is a noun (or an adjective). As two words, "clean up" is a verb. Given that, I'd expect to see classes and objects with names like `Cleanup` or `cleanup` and functions to contain `CleanUp`. When I see an identifier like `CleanUpTes

[Lldb-commits] [PATCH] D43596: Replace HashStringUsingDJB with llvm::djbHash

amccarth added inline comments. Comment at: lldb/trunk/packages/Python/lldbsuite/test/lang/c/unicode/TestUnicodeSymbols.py:13 +self.build() + spec = lldb.SBModuleSpec() + spec.SetFileSpec(lldb.SBFileSpec(self.getBuildArtifact("a.out"))) Oops!

[Lldb-commits] [PATCH] D43705: Fix tabs/spaces in TestUnicodeSymbols.py

amccarth created this revision. amccarth added a reviewer: labath. Herald added a subscriber: sanjoy. https://reviews.llvm.org/D43705 Files: lldb/packages/Python/lldbsuite/test/lang/c/unicode/TestUnicodeSymbols.py Index: lldb/packages/Python/lldbsuite/test/lang/c/unicode/TestUnicodeSymbols.py

[Lldb-commits] [PATCH] D43705: Fix tabs/spaces in TestUnicodeSymbols.py

This revision was automatically updated to reflect the committed changes. Closed by commit rL326095: Fix tabs/spaces indentation problem in TestUnicodeSymbols.py (authored by amccarth, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D43

[Lldb-commits] [PATCH] D43688: Partial fix for TestConflictingSymbol.py on Windows

This revision was automatically updated to reflect the committed changes. Closed by commit rL326130: Partial fix for TestConflictingSymbol.py on Windows (authored by amccarth, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D43688?vs=13

[Lldb-commits] [PATCH] D43784: Un-XFAIL TestCallStdStringFunction.test for Windows

amccarth created this revision. amccarth added a reviewer: zturner. Herald added a subscriber: sanjoy. This test was passing unexpectedly, which causes ninja to short-circuit out of running the dotest.py tests. The bug report (https://bugs.llvm.org/show_bug.cgi?id=21765) appears to be obsolete

[Lldb-commits] [PATCH] D43784: Un-XFAIL TestCallStdStringFunction.test for Windows

amccarth added a comment. Nice catch. I'll take a closer look to see what exactly is happening on Windows. https://reviews.llvm.org/D43784 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lld

[Lldb-commits] [PATCH] D43784: Un-XFAIL TestCallStdStringFunction.test for Windows

amccarth abandoned this revision. amccarth added a comment. Abandoning. This isn't really working on Windows. The breakpoint fails to set for reasons I don't understand yet. https://reviews.llvm.org/D43784 ___ lldb-commits mailing list lldb-commi

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

amccarth added a comment. Nice! Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:53 + // Creates a synthetic module section covering the whole module image + void CreateImageSection(const MinidumpModule *module, Target& target) { +const ConstString section_

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

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM, but consider highlighting the side effect to `target` when the factory makes a Placeholder module. In the future, we need to refactor the minidump tests to get rid of the redundancy

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

amccarth accepted this revision. amccarth added a comment. I actually preferred the factory solution. What I intended to express is that the modification of the target doesn't seems like it should be inside the PlaceholderModule class, so whether you use a factory or not doesn't really address

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

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. The big picture here is fine. I see a couple opportunities in the details, but I won't block on them. Comment at: include/lldb/Core/Module.h:779 + //---

[Lldb-commits] [PATCH] D34553: Shorten sanitizer plugin names

amccarth added a comment. Thanks for doing this. This seems a reasonable alternative to fixing all the build tools upstream. And overly long paths are difficult to read. https://reviews.llvm.org/D34553 ___ lldb-commits mailing list lldb-commits@l

[Lldb-commits] [PATCH] D35506: Fix after r308190.

amccarth created this revision. Update some LLDB code after clang::MacroInfo::arg_iterator was reandmed to param_iterator. https://reviews.llvm.org/D35506 Files: lldb/source/Plugins/ExpressionParser/Clang/ClangModulesDeclVendor.cpp Index: lldb/source/Plugins/ExpressionParser/Clang/ClangModu

[Lldb-commits] [PATCH] D35506: Fix after r308190.

amccarth abandoned this revision. amccarth added a comment. Beaten to it by r308219. https://reviews.llvm.org/D35506 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37527: Fix for bug 34510 - Minidump target does not resolve new symbols correctly

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. This looks fine. Make sure you've run the LLDB tests. I don't expect this change to have much risk for breaking any of them, but it's a good habit. `ninja check-lldb` For future patch r

[Lldb-commits] [PATCH] D37527: Fix for bug 34510 - Minidump target does not resolve new symbols correctly

amccarth added a comment. I'll commit it momentarily. Repository: rL LLVM https://reviews.llvm.org/D37527 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37527: Fix for bug 34510 - Minidump target does not resolve new symbols correctly

This revision was automatically updated to reflect the committed changes. Closed by commit rL312735: Fix for bug 34510 - Minidump target does not resolve new symbols correctly (authored by amccarth). Changed prior to commit: https://reviews.llvm.org/D37527?vs=114056&id=114218#toc Repository:

[Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

amccarth added a comment. I think I agree with Jim that it would be better to propagate an error from DoResume than to introduce CanResume. I could imagine situations where DoResume could fail for a reason that CanResume was unable to predict. Having one path for handling failure to resume se

[Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

amccarth accepted this revision. amccarth added a comment. LGTM. Thanks for adding the tests. https://reviews.llvm.org/D37651 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

amccarth added a comment. @sas: Do the latest changes address your concerns with this patch? https://reviews.llvm.org/D37651 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

This revision was automatically updated to reflect the committed changes. Closed by commit rL313210: Fix for bug 34532 - A few rough corners related to post-mortem debugging… (authored by amccarth). Changed prior to commit: https://reviews.llvm.org/D37651?vs=115068&id=115132#toc Repository:

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

amccarth added a comment. I haven't looked at the whole patch yet, but it seems the SIGINT fix is well isolated. That should probably be in a separate patch. https://reviews.llvm.org/D37923 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

amccarth added inline comments. Comment at: source/Commands/CommandObjectTarget.cpp:2058 + break; +} num_dumped++; Many LLVM developers prefer to omit the braces when the body of the control-flow statement is a single statem

[Lldb-commits] [PATCH] D37926: Fix the SIGINT handlers

This revision was automatically updated to reflect the committed changes. Closed by commit rL313785: Fix the SIGINT handlers (authored by amccarth). Changed prior to commit: https://reviews.llvm.org/D37926?vs=115473&id=116035#toc Repository: rL LLVM https://reviews.llvm.org/D37926 Files:

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

amccarth accepted this revision. amccarth added a comment. LGTM. But just a thought: Is it worth doing all the work to scan for line endings for the interruption points? What if, instead, it just printed the next _n_ characters on each iteration until the entire buffer has been printed. Sure

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

This revision was automatically updated to reflect the committed changes. Closed by commit rL313904: [LLDB] Implement interactive command interruption (authored by amccarth). Changed prior to commit: https://reviews.llvm.org/D37923?vs=116074&id=116247#toc Repository: rL LLVM https://reviews

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

amccarth accepted this revision. amccarth added a comment. LGTM https://reviews.llvm.org/D37923 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D56462: Change SymbolFile::ParseTypes to ParseTypesForCompileUnit

amccarth added a comment. So this Patch is effectively NFC, since no caller (not even a test) was using the functionality you've removed. Seems like a nice refactor. Comment at: lldb/source/Core/Module.cpp:382 // Parse all types for this compile unit + symbols->

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

amccarth added inline comments. Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp:1289 +if (type) + (void)type->GetFullCompilerType(); + } The body of this loop is a little odd because it's disposing of all the results and i

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

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. Nice. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57751/new/ https://reviews.llvm.org/D57751 ___ lldb-commits mailing list lldb-co

[Lldb-commits] [PATCH] D57895: Breakpad: auto-detect path style of file entries

amccarth added inline comments. Comment at: include/lldb/Utility/FileSpec.h:250 + /// unreliable (e.g. "c:\foo.txt" is a valid relative posix path). + static llvm::Optional

[Lldb-commits] [PATCH] D57895: Breakpad: auto-detect path style of file entries

amccarth accepted this revision. amccarth added a comment. I think `absolute_path` is great. Thanks for checking on the `native`/`None`. I didn't want there to be any latent surprises. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D57895/new/ https://reviews.llvm.org/D57895 ___

[Lldb-commits] [PATCH] D56537: ObjectFilePECOFF: Create a "container" section spanning the entire module image

amccarth added a comment. My knowledge of PECOFF is even more limited, but it's my understanding that the ImageBase is the _preferred_ address for the module. That doesn't guarantee that's the actual address it would be loaded at. Not just because of ASLR but also because there may be conflic

[Lldb-commits] [PATCH] D58648: Improve process launch comments for Windows

amccarth created this revision. amccarth added a reviewer: zturner. The existing comment about over-allocating the command line was incorrect. The contents of the command line may be changed, but it's not necessary to over allocate. The changes will be limited to the existing contents of the s

[Lldb-commits] [PATCH] D58648: Improve process launch comments for Windows

amccarth updated this revision to Diff 188263. amccarth added a comment. Forgot to include context in original diff. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58648/new/ https://reviews.llvm.org/D58648 Files: lldb/source/Host/windows/ProcessLauncherWindows.cpp Index: lldb/sourc

[Lldb-commits] [PATCH] D58648: Improve process launch comments for Windows

amccarth added a comment. Thanks. I really don't know what other types of conditions may trigger that "request is not supported" message, so I'm going to shy away from trying to make it more specific. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58648/new/ https://reviews.llvm.org/D

[Lldb-commits] [PATCH] D58648: Improve process launch comments for Windows

amccarth closed this revision. amccarth added a comment. Closed with r355121. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58648/new/ https://reviews.llvm.org/D58648 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.l

[Lldb-commits] [PATCH] D58975: Introduce MinidumpEnums.def textual header

amccarth accepted this revision. amccarth added inline comments. Comment at: include/lldb/Formats/MinidumpEnums.def:91 + +LLDB_HANDLE_PLATFORM(0x, Win32S) // Windows 3.1 +LLDB_HANDLE_PLATFORM(0x0001, Win32Windows) // Windows 95-98-Me I know you're just moving

[Lldb-commits] [PATCH] D59854: [Host] Add LibraryLoader abstraction around dlopen/LoadLibrary

amccarth added a comment. The Windows code looks correct to me, but I've added a couple inline questions/comments. I can patch it in and test it for you tomorrow morning if nobody else has done it by then. I think zturner's still on vacation for another day or two. Comment

[Lldb-commits] [PATCH] D60068: PDBFPO: Refactor register reference resolution

amccarth accepted this revision. amccarth added inline comments. Comment at: source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:287 + + // lookup register reference as lvalue in preceding assignments + auto it = m_dependent_programs.find(symbol->GetName());

[Lldb-commits] [PATCH] D60152: Fix and simplify PrepareCommandsForSourcing

amccarth created this revision. amccarth added reviewers: zturner, labath, rnk. Spotted some problems in the Driver's PrepareCommandsForSourcing while helping a colleague track another problem. 1. One error case was not handled because there was no else clause. Fixed by switching to llvm's early

[Lldb-commits] [PATCH] D60152: Fix and simplify PrepareCommandsForSourcing

amccarth marked an inline comment as done. amccarth added a comment. Thanks for the review. In D60152#1452704 , @labath wrote: > FTR, I believe using pipes here is wrong altogether because it can lead to > deadlock. The size of the pipe buffer is impleme

[Lldb-commits] [PATCH] D60152: Fix and simplify PrepareCommandsForSourcing

amccarth updated this revision to Diff 193515. amccarth edited the summary of this revision. amccarth added a comment. Further simplification per labath's feedback. Specifically: 1. Use a utility function from llvm to avoid making own wrapper for closing a file descriptor. 2. Don't bother reset

[Lldb-commits] [PATCH] D60195: modify-python-lldb.py: (Re)move __len__ and __iter__ support

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. Nice! Thanks for cleaning up the affected comments as well. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60195/new/ https://reviews.llvm.org/D60195 __

[Lldb-commits] [PATCH] D60152: Fix and simplify PrepareCommandsForSourcing

This revision was automatically updated to reflect the committed changes. Closed by commit rL357626: Fix and simplify PrepareCommandsForSourcing (authored by amccarth, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews

[Lldb-commits] [PATCH] D60268: Breakpad: Parse Stack CFI records

amccarth added a comment. This looks good. I have a few questions inline, but none of those are major concerns. Comment at: source/Plugins/ObjectFile/Breakpad/BreakpadRecords.h:173 + llvm::DenseMap UnwindRules; +}; + I'm not a fan of deep class hierarchies,

[Lldb-commits] [PATCH] D60271: PDBFPO: Use references instead of pointers, where possible

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM. I noticed this also deleted two overloads of Visit from FPOProgramASTVisitorDWARFCodegen, but that appears to be harmless (the base class overloads were also no-ops). CHANGES SINC

[Lldb-commits] [PATCH] D60268: Breakpad: Parse Stack CFI records

amccarth added a comment. My concerns were address, so LGTM. I'll leave the rest to you and clayborg. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60268/new/ https://reviews.llvm.org/D60268 ___ lldb-commits mailing list lldb-commits@lists

[Lldb-commits] [PATCH] D60410: PDBFPO: Improvements to the AST visitor

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. Overall, I'm ambivalent. The patch description makes a good case for the change. I find the visitor pattern hard to follow (partly because the `visit` and `accept` naming is not intuitiv

[Lldb-commits] [PATCH] D60498: Clean up docstrings in swig interface files

amccarth added a comment. LGTM, though I'm not clear why this extra visual noise was there in the first place. Does doxygen depend on this style when reading the swigged API? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60498/new/ https://reviews.llvm.org/D60498 __

[Lldb-commits] [PATCH] D60519: [Windows] Dump more information about access violation exception

amccarth added a comment. Does this affect any existing tests? Is there a good way to test it? Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60519/new/ https://reviews.llvm.org/D60519 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D60410: PDBFPO: Improvements to the AST visitor

amccarth accepted this revision. amccarth added a comment. I'm even happier. Thanks for giving the params more specific names. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60410/new/ https://reviews.llvm.org/D60410 ___ lldb-commits mailin

[Lldb-commits] [PATCH] D60667: Allow direct comparison of ConstString against StringRef

amccarth added a comment. I, too, have some concern that this could have unintended side effects. To make the temporary `StringRef`s from the zero-terminated strings requires a `strlen` call each time. So we're making two passes over a string each time (once to measure and once to compare).

[Lldb-commits] [PATCH] D60667: Allow direct comparison of ConstString against StringRef

amccarth accepted this revision. amccarth added a comment. In D60667#1467970 , @teemperor wrote: > In D60667#1467387 , @amccarth wrote: > > > I, too, have some concern that this could have unintended side effects.

[Lldb-commits] [PATCH] D60405: MinidumpYAML: Add support for ModuleList stream

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. Nice! Comment at: include/llvm/ObjectYAML/MinidumpYAML.h:57 +/// record. In memory, we represent these as the ParsedModule struct, which +/// groups minidump::Module with

[Lldb-commits] [PATCH] D60519: [Windows] Dump more information about access violation exception

amccarth added a comment. Thanks for the new test and the test fix. It's unfortunate that the tests are so sensitive to an arbitrary buffer size. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60519/new/ https://reviews.llvm.org/D60519 _

[Lldb-commits] [PATCH] D60817: [NativePDB] Add anonymous namespaces support

amccarth added a comment. Sorry for the slow response; I'm still learning about this code. I like where this is going. Comment at: lit/SymbolFile/NativePDB/ast-types.cpp:77 +// FIXME: Should be located in the namespace `A`, in the struct `C<1>`. +A::C<1>::D AC1D; + ---

[Lldb-commits] [PATCH] D60817: [NativePDB] Add anonymous namespaces support

amccarth marked an inline comment as done. amccarth added inline comments. Comment at: source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:575 std::string ns = name_components.front()->toString(); -context = m_clang.GetUniqueNamespaceDeclaration(ns.c_str(), context); +

[Lldb-commits] [PATCH] D60817: [NativePDB] Add anonymous namespaces support

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. Thanks for the changes. If the tests pass, then this LGTM. Just check the one last question I added about the `AC1D` test. (The diff in Phabricator looks very confused, as though you del

[Lldb-commits] [PATCH] D30172: Replace WINLOG_*** macros with LLDB_LOG

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM. Maybe we have too many categories. Comment at: source/Plugins/Process/Windows/Common/DebuggerThread.cpp:207 if (m_active_exception.get()) { -WINLOG_IFANY(WI

[Lldb-commits] [PATCH] D30789: Make file and directory name completion work properly on Windows.

amccarth added inline comments. Comment at: lldb/source/Commands/CommandCompletions.cpp:108 + StringList &matches, + TildeExpressionResolver *Resolver) { + // Use the default resolver if one isn't explicitly speci

[Lldb-commits] [PATCH] D27010: Refactor LLDB's Windows process plugin (NFC)

amccarth closed this revision. amccarth added a comment. Committed in r287770. https://reviews.llvm.org/D27010 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D109832: [lldb/win] Fix TestIRMemoryMapWindows.test when running tests in git bash

amccarth added a comment. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109832/new/ https://reviews.llvm.org/D109832 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org

[Lldb-commits] [PATCH] D109834: [lldb/win] Improve check-lldb-shell with LLVM_ENABLE_DIA_SDK=NO

amccarth added a comment. I'm trying to understand why I've never seen the problem that this patch is trying to address. I wasn't aware of `LLVM_ENABLE_DIA_SDK` so I've never turned it off. But I've run `check lldb` exclusively with `LLDB_USE_NATIVE_PDB_READER=TRUE` for a long time, so I'm su

[Lldb-commits] [PATCH] D110172: [lldb/win] Default to native PDB reader when building with LLVM_ENABLE_DIA_SDK=NO

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D110172/new/ https://reviews.llvm.org/D110172 ___ lldb-commits mailing list lldb-com

  1   2   3   >