[Lldb-commits] [PATCH] D104954: Implement AddSymbols method for SymbolFileNativePDB

amccarth created this revision. amccarth added a reviewer: rnk. amccarth requested review of this revision. In theory, this adds symbols from the module's PDB file to the symtab. This implementation was modeled after the one in SymbolFilePDB, so it relies on the same IPDBSession abstraction as

[Lldb-commits] [PATCH] D75275: [lldb/CMake] Use PYTHON_HOME as a hint to find Python 3.

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. Thanks for helping figure this out! Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75275/new/ https://reviews.llvm.org/D75275 _

[Lldb-commits] [PATCH] D76835: [lldb] Fix TestSettings.test_pass_host_env_vars on windows

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. The code is correct, so I'm approving, but I suggest a couple fixes to the comments before committing.. Comment at: lldb/source/Host/windows/ProcessLauncherWindows.cpp:2

[Lldb-commits] [PATCH] D77662: [WIP][lldb/test] Make TestLoadUnload compatible with windows

amccarth added a comment. I like where this is going. Comment at: lldb/packages/Python/lldbsuite/test/make/dylib.h:50 + FormatMessageA(FORMAT_MESSAGE_ALLOCATE_BUFFER | FORMAT_MESSAGE_FROM_SYSTEM, + NULL, err, MAKELANGID(LANG_NEUTRAL, SUBLANG_DEFAULT), (char *)&msg, 0, NU

[Lldb-commits] [PATCH] D77662: [lldb/test] Make TestLoadUnload compatible with windows

amccarth added inline comments. Comment at: lldb/packages/Python/lldbsuite/test/make/Makefile.rules:477 ifeq (1,$(USE_LIBDL)) - ifneq ($(OS),NetBSD) + ifeq (,$(filter $(OS), NetBSD Windows_NT)) LDFLAGS += -ldl I'm no expert in makefil

[Lldb-commits] [PATCH] D77662: [lldb/test] Make TestLoadUnload compatible with windows

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. Well, it looks to go _me_. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77662/new/ https://reviews.llvm.org/D77662 _

[Lldb-commits] [PATCH] D96840: [LLDB] [docs] Update the list of supported architectures on Windows

amccarth added a comment. I'm in no position to evaluate the functionality of lldb on Windows. I've spent the last few months trying to understand why 900+ tests spontaneously started failing on Windows. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D96840/new/ https://reviews.llvm.or

[Lldb-commits] [PATCH] D98529: [lldb] Strip pointer authentication codes from aarch64 pc.

amccarth added a comment. This workaround seems consistent with other overrides of FixCodeAddress, my only concern is about the assumption of the number of bits that need to be preserved/stripped. I hope @jasonmolenda gets a chance to chime in. Comment at: lldb/source/Plugin

[Lldb-commits] [PATCH] D98529: [lldb] Strip pointer authentication codes from aarch64 pc.

amccarth added a comment. Minidumps should have the registers in the processor context. It seems LLDB knows about TCR_ELn for n > 0. Maybe TCR_EL0 is special? If it's not available in the minidump, we'll need a plan for how to deal with these regardless of when Jason's implementation lands.

[Lldb-commits] [PATCH] D99426: [Windows] Add new OF_TextWithCRLF flag and use this flag instead of OF_Text

amccarth added a comment. I like the composability enabled by making `OF_Text` and `OF_CRLF` independent. I wonder, though, if there's a chokepoint where we could/should assert if the caller uses `OF_CRLF` without `OF_Text`, which would be suspicious. I'm not concerned by the number of files t

[Lldb-commits] [PATCH] D102208: Remove Windows editline from LLDB

amccarth added a subscriber: zturner. amccarth added a comment. It looks like this EditLine port was added before I joined this project. @zturner may have worked on it, but I don't know/remember. If it's actually unused, I have no objection to removing it. I harbor some hope that Windows's st

[Lldb-commits] [PATCH] D102208: Remove Windows editline from LLDB

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM. Dead code should go. If somebody wants to own a Windows port of EditLine, they can re-instate it and put some tests on it. Thanks. Repository: rG LLVM Github Monorepo CHANGES

[Lldb-commits] [PATCH] D84815: [LLDB] Improve PDB discovery

amccarth created this revision. amccarth added reviewers: labath, stella.stamenova. amccarth requested review of this revision. When loading a PE/COFF target, the associated PDB file often wasn't found. The executable module contains a path for the associated PDB file, but people often debug fr

[Lldb-commits] [PATCH] D84815: [LLDB] Improve PDB discovery

amccarth updated this revision to Diff 281445. amccarth added a comment. Fixed typos in comments. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84815/new/ https://reviews.llvm.org/D84815 Files: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.cpp Index: lldb/source/Plug

[Lldb-commits] [PATCH] D84815: [LLDB] Improve PDB discovery

amccarth added a comment. Thanks. Working on a test. I found a description of how the Windows debuggers look for PDBs, and it's different than what I expected: 1. The directory that contains the binary 2. The build path embedded in the binary 3. (if enabled) The symbol server cache 4. (if enab

[Lldb-commits] [PATCH] D84815: [LLDB] Improve PDB discovery

amccarth updated this revision to Diff 284025. amccarth added a comment. Added test to check locating the PDB either in the original build directory or adjacent to the executable. I tried but failed to make a negative test. LLDB sends the errors message to stderr when the `target modules dump

[Lldb-commits] [PATCH] D84815: [LLDB] Improve PDB discovery

amccarth closed this revision. amccarth added a comment. Landed on Tuesday. I must have messed up the `Differential Revision:` line. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84815/new/ https://reviews.llvm.org/D84815 ___ lldb-commits ma

[Lldb-commits] [PATCH] D85993: [lldb] Set the access property on member function decls

amccarth created this revision. amccarth added reviewers: clayborg, aganea. amccarth requested review of this revision. This fixes two failures in the PDB tests. LLVM has a "sanity" check on function decls. One of the requirements of member functions is that they have the access property (publ

[Lldb-commits] [PATCH] D85993: [lldb] Set the access property on member function decls

amccarth added a comment. In D85993#2220724 , @labath wrote: > Dwarf parser uses `TypeSystemClang::AddMethodToCXXRecordType` instead of this > function to create methods (which is why there are no assertions like this > when using dwarf). Maybe it would

[Lldb-commits] [PATCH] D88906: [lldb/docs] Clarify python/swig version incompatibility

amccarth accepted this revision. amccarth added a comment. Yes, I also (independently) discovered this problem. I probably have notes somewhere with sources for details about the incompatibility. I believe I also brought it up on the lldb-dev list. Repository: rG LLVM Github Monorepo CHAN

[Lldb-commits] [PATCH] D88975: [LLDB] On Windows, fix tests

amccarth added a comment. It's interesting that I haven't encountered some of these errors. There are five _other_ lldb tests that do fail for me. I have a fix in the works for some of those. I agree with @labath that including error message patterns in various languages isn't scalable. Sinc

[Lldb-commits] [PATCH] D88967: [lldb] Add a cmake warning about the python/swig incompatibility

amccarth added a comment. If I recall correctly, the non-debug builds still had the problem, they just didn't have the assertion that made it obvious. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D88967/new/ https://reviews.llvm.org/D88967 __

[Lldb-commits] [PATCH] D88967: [lldb] Add a cmake warning about the python/swig incompatibility

amccarth added a comment. In D88967#2317545 , @labath wrote: > In D88967#2317522 , @amccarth wrote: > >> If I recall correctly, the non-debug builds still had the problem, they just >> didn't have the assertion tha

[Lldb-commits] [PATCH] D89256: [LLDB] Fix 37 tests on Windows

amccarth created this revision. amccarth added a reviewer: JDevlieghere. Herald added a subscriber: mgorny. amccarth requested review of this revision. A Windows-style LLDB_PYTHON_HOME path in a Cmake template didn't have the backslashes escaped, which led to a garbled paths derived from it. Fix

[Lldb-commits] [PATCH] D89256: [LLDB] Fix 37 tests on Windows

This revision was automatically updated to reflect the committed changes. Closed by commit rGf21fcccef719: [LLDB] Fix 37 tests on Windows (authored by amccarth). Herald added a project: LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D89256/new/

[Lldb-commits] [PATCH] D67347: [Windows] Use information from the PE32 exceptions directory to construct unwind plans

amccarth added a comment. I'm just back from vacation. I agree with Pavel that we need to hear more from Jason at this point. I'm still very interested in helping this land in some form. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67347/new/ https://revie

[Lldb-commits] [PATCH] D68258: [Windows] Introduce a switch for the `lldb-server` mode on Windows

amccarth added a comment. Why an environment variable rather than a command line option? Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68258/new/ https://reviews.llvm.org/D68258 ___ lldb-commits mailing list lldb-c

[Lldb-commits] [PATCH] D68258: [Windows] Introduce a switch for the `lldb-server` mode on Windows

amccarth added a comment. In D68258#1690757 , @aleksandr.urakov wrote: > In D68258#1690756 , > @aleksandr.urakov wrote: > > > I've made it in the way similar to Zachary have made for the > > `SymbolFileNativePDB`

[Lldb-commits] [PATCH] D61886: Support member functions construction and lookup in PdbAstBuilder

amccarth added a comment. Herald added a subscriber: JDevlieghere. Is this still an active review or has this been abandoned? Comment at: lldb/source/Plugins/SymbolFile/NativePDB/PdbAstBuilder.cpp:1047 +function_decl = m_clang.CreateFunctionDeclaration( parent, proc_

[Lldb-commits] [PATCH] D68258: [Windows] Introduce a switch for the `lldb-server` mode on Windows

amccarth accepted this revision. amccarth added a comment. Given Pavel's comment, this LGTM. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68258/new/ https://reviews.llvm.org/D68258 ___ lldb-commits mailing list ll

[Lldb-commits] [PATCH] D68134: [LLDB] Use the llvm microsoft demangler instead of the windows dbghelp api

amccarth accepted this revision. amccarth added a comment. LGTM after one question. Comment at: lldb/lit/SymbolFile/PDB/udt-layout.test:1 REQUIRES: system-windows, lld RUN: %build --compiler=clang-cl --output=%t.exe %S/Inputs/UdtLayoutTest.cpp Is `system-wind

[Lldb-commits] [PATCH] D68770: [LLDB] [Driver] Use llvm::InitLLVM to do unicode argument conversion on Windows

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. Cool. I didn't know about InitLLVM. That makes things much cleaner. Though I do recall recently seeing another complaint about `argv[0]` not being preserved as typed but being replaced b

[Lldb-commits] [PATCH] D68770: [LLDB] [Driver] Use llvm::InitLLVM to do unicode argument conversion on Windows

amccarth added a comment. On Windows, it _does_ rewrite `argv[0]`, but it looks like it tries to not change whether it was relative/absolute, so I think this is fine. Thanks for the clean-up! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68770/new

[Lldb-commits] [PATCH] D68980: [LLDB] [test] Pass --target=-windows-msvc to clang-cl

amccarth added a comment. > I know there are some complexities with configuring msvc/clang-cl, where > one needs to fetch registry keys and whatnot in order to get the right build > environment. My understanding is that the clang-cl driver does some poking around in the file system and regis

[Lldb-commits] [PATCH] D69100: COFF: Create a separate "section" for the file header

amccarth accepted this revision. amccarth added a comment. I'm OK with this. I'm just wondering whether it would be a good idea to make it clear that these header sections are "not considered to be a section in the strictest sense." Is the distinction going to be important to future code read

[Lldb-commits] [PATCH] D69366: [LLDB] [PECOFF] Fix symbols to refer to the right section

amccarth added inline comments. Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:691 + // so we should use section index "symbol.sect - 1 + 1". + Address symbol_addr(sect_list->GetSectionAtIndex(symbol.sect),

[Lldb-commits] [PATCH] D68671: Add the ability to pass extra args to a Python breakpoint command function

amccarth added a comment. FYI: This broke the build for me. Comment at: lldb/include/lldb/Interpreter/ScriptInterpreter.h:323 + const char *function_name, + StructuredData::ObjectSP extra_args_sp) {} This breaks some builds because it doesn't retur

[Lldb-commits] [PATCH] D69535: build: improve python check for Windows

amccarth added a comment. In D69535#1724797 , @labath wrote: > .. Given that this code is python3 specific and other platforms still support > python2, maybe we can make this bump windows-specific at first, and then > extend it to other platforms once we

[Lldb-commits] [PATCH] D69612: [lldb-vscod] fix build with NDEBUG on windows

amccarth added a comment. In D69612#1726849 , @SquallATF wrote: > In D69612#1726829 , @labath wrote: > > > LGTM, modulo the `(void) result` stuff. Do you need someone to commit this > > for you? > > > Yes I need.

[Lldb-commits] [PATCH] D69503: [LLDB] [Windows] Don't crash if the debugged process is unable to start up

amccarth added a comment. In D69503#1726841 , @labath wrote: > (Ideally, I'd just delete ProcessWindows entirely, and move everything to > lldb-server based model -- it's much easier to reason about threads there) I believe there are people working on m

[Lldb-commits] [PATCH] D69503: [LLDB] [Windows] Don't crash if the debugged process is unable to start up

amccarth requested changes to this revision. amccarth added a comment. This revision now requires changes to proceed. Thanks for identifying this race condition. After some poking around, I think there's a cleaner way to address it. First, `m_session_state`'s lifetime is currently managed mostly

[Lldb-commits] [PATCH] D69503: [LLDB] [Windows] Don't crash if the debugged process is unable to start up

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

[Lldb-commits] [PATCH] D70281: Fix -Wunused-result warnings in LLDB

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM. It's too bad that pattern is repeated three times, including the explanatory comment. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D7

[Lldb-commits] [PATCH] D70387: [LLDB] [Windows] Allow making subprocesses use the debugger's console with LLDB_INHERIT_CONSOLE=true

amccarth added a comment. labath has added great context here, and I generally agree with clayborg's ideal of optimal behavior. That said, if memory serves, getting that behavior on Windows can be quite challenging, so I'm not sure if it's worth the effort. Repository: rLLDB LLDB CHANGES S

[Lldb-commits] [PATCH] D65230: [CMake] Loosen Python version check and ignore patch version

amccarth added a comment. In D65230#1602370 , @labath wrote: > Thanks. > > BTW, I tried this on windows, and it didn't blow up in my face, I think it > should be relatively safe. I couldn't reproduce the problems that @amccarth > was experiencing, so I d

[Lldb-commits] [PATCH] D65330: [lldb][docs] Update documentation for monorepo and CMake caches

amccarth added a comment. Looks good. Thanks for doing these updates! Comment at: lldb/docs/resources/build.rst:85 +The LLVM project is migrating to a single monolithic respository for LLVM and +its subprojects. This is the recommended way to build LLDB. Checkout the +source-t

[Lldb-commits] [PATCH] D65330: [lldb][docs] Update documentation for monorepo and CMake caches

amccarth added inline comments. Comment at: lldb/docs/resources/build.rst:94 +project, it generates the files needed by your build tool. The recommended +build tool for LLVM is Ninja. Please also read `Building LLVM with CMake +`_. ---

[Lldb-commits] [PATCH] D65546: Fix TestThreadSpecificBreakpoint on Windows

amccarth created this revision. amccarth added a reviewer: stella.stamenova. This test was frequently hanging on Windows, causing a timeout after 10 minutes. The short delay (100 microsecond) in the sample program could cause a deadlock in the Windows thread pool, as I've explained in the test

[Lldb-commits] [PATCH] D65546: Fix TestThreadSpecificBreakpoint on Windows

This revision was automatically updated to reflect the committed changes. Closed by commit rL367573: Fix TestThreadSpecificBreakpoint on Windows (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] D65691: Various build fixes for lldb on MinGW

amccarth added a comment. Yes, zturner isn't as active here as he was before. I'm happy to review patches regarding LLDB on Windows, even though I don't know much about Mingw. Comment at: lldb/source/Plugins/Process/gdb-remote/ProcessGDBRemote.cpp:19 #endif +#ifdef __MINGW32

[Lldb-commits] [PATCH] D65691: Various build fixes for lldb on MinGW

amccarth marked an inline comment as done. amccarth added inline comments. Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:55 #define PATH_MAX MAX_PATH +#endif typedef int socklen_t; hhb wrote: > amccarth wrote: > > Nothing in the rest of this .cpp file uses

[Lldb-commits] [PATCH] D65955: Minidump/Windows: Fix module lookup

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. This looks fine, and thanks for the tests for a one-line fix. I'm curious, though, where is the matching code? Should "unknown" be treated as a wildcard when trying to find the matching m

[Lldb-commits] [PATCH] D56010: [NativePDB] Fix setting breakpoint by file and line

amccarth added a comment. It looks like the code changes landed (probably as part of another patch) but not the tests. I'll look and see if the tests should to be revived. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56010/new/ https://reviews.llvm.org/D56010 _

[Lldb-commits] [PATCH] D65230: [CMake] Loosen Python version check and ignore patch version

amccarth added a comment. Just closing the loop. Regarding the "Python memory allocator called without holding the GIL" bugs: this is not a "just me" problem. It's for anyone who enables debug mode. This appears to be a test infrastructure problem, but it might be harmless. If I run in "re

[Lldb-commits] [PATCH] D56010: [NativePDB] Fix setting breakpoint by file and line

amccarth added a comment. It turns out these tests all seem to exist, so I guess Zachary submitted them as part of other CLs. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D56010/new/ https://reviews.llvm.org/D56010 ___ lldb-commits mailing

[Lldb-commits] [PATCH] D66345: [lldb][NFC] Allow for-range iterating over StringList

amccarth added inline comments. Comment at: lldb/source/Commands/CommandObjectApropos.cpp:70 +max_len = std::max(max_len, command.size()); } Or ``` const size_t max_len = std::max_element(commands_found.begin(), commands_found.end())

[Lldb-commits] [PATCH] D66445: Explicitly Cast Constants to DWORD

amccarth added a comment. Is this necessary? I see C++ #define STATUS_BREAKPOINT((DWORD )0x8003L) #define STATUS_SINGLE_STEP ((DWORD )0x8004L) in C:\Program Files (x86)\Windows Kits\8.1\Include\um\winnt.h Repository: rLLDB LLDB CHANGES SINCE

[Lldb-commits] [PATCH] D66447: Add char8_t support (C++20)

amccarth added a comment. A couple inline comments. I think this is looking pretty good. Comment at: lldb/trunk/packages/Python/lldbsuite/test/lang/cpp/char8_t/main.cpp:1 +#include + Is this include necessary? Comment at: lldb/trunk/source

[Lldb-commits] [PATCH] D66633: Breakpad: Add support for parsing STACK WIN records

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/D66633/new/ https://reviews.llvm.org/D66633 ___ lldb-commits mailing list lldb-co

[Lldb-commits] [PATCH] D66634: Postfix: move more code out of the PDB plugin

amccarth added a comment. I have a couple inline questions. After that, it looks fine. Comment at: lldb/trunk/source/Plugins/SymbolFile/NativePDB/PdbFPOProgramToDWARFExpression.cpp:61 + for (auto it = parsed.begin(), end = parsed.end(); it != end; ++it) { // Emplace v

[Lldb-commits] [PATCH] D66841: Skip test_target_create_invalid_core_file on Windows

amccarth created this revision. amccarth added a reviewer: teemperor. The operation is supposed to fail, but apparently it fails on Windows for a different reason than the test anticipated. https://reviews.llvm.org/D66841 Files: lldb/packages/Python/lldbsuite/test/functionalities/target_com

[Lldb-commits] [PATCH] D66634: Postfix: move more code out of the PDB plugin

amccarth added a comment. When I added my comments, Phabricator showed this patch as still open. Now it looks like it landed four hours before that. :-( Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66634/new/ https://reviews.llvm.org/D66634 _

[Lldb-commits] [PATCH] D66863: [lldb] Unify target checking in CommandObject

amccarth added a comment. I support anything that reduces the code path differences between user-entered commands and their SBAPI counterparts. Thanks for doing this! Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66863/new/ https://reviews.llvm.org/D66863

[Lldb-commits] [PATCH] D66841: Skip test_target_create_invalid_core_file on Windows

amccarth abandoned this revision. amccarth added a comment. Somebody else already did this while I was waiting for review. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66841/new/ https://reviews.llvm.org/D66841 ___ lldb-commits mailing lis

[Lldb-commits] [PATCH] D66994: [lldb][CMake] Fix windows-x86-debug compilation with python enabled using multi-target generator

amccarth added a comment. I'll look at this in detail soon, but a few caveats: - lldb tests don't work with the debug Python interpreter, which is why I've been complaining about test failures approximately forever when the bots seem fine. - Some of these changes you're undoing caused a lot of

[Lldb-commits] [PATCH] D66994: [lldb][CMake] Fix windows-x86-debug compilation with python enabled using multi-target generator

amccarth requested changes to this revision. amccarth added inline comments. This revision now requires changes to proceed. Comment at: cmake/modules/LLDBConfig.cmake:164 # if(CMAKE_MSVC_RUNTIME_LIBRARY MATCHES MultiThreadedDebug) - if(CMAKE_BUILD_TYPE STREQUAL Debug) -fi

[Lldb-commits] [PATCH] D66994: [lldb][CMake] Fix windows-x86-debug compilation with python enabled using multi-target generator

amccarth added a comment. I'm open to this if we can reduce the code duplication. One of my concerns is that changes in July and August completely broke the build for me for many days. I had to remove all but one version of Python to ensure that it always found the right one. I, too, would ap

[Lldb-commits] [PATCH] D67123: [lldb] Use binary search in RangeDataVector:FindEntryIndexesThatContain

amccarth added a comment. This feels very familiar. I think I've reviewed a similar change back when we first implemented minidumps. Comment at: lldb/include/lldb/Utility/RangeMap.h:739 +auto end = std::lower_bound(m_entries.begin(), m_entries.end(), +

[Lldb-commits] [PATCH] D67083: [dotest] Avoid the need for LEVEL= makefile boilerplate

amccarth accepted this revision. amccarth added a comment. Nice! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67083/new/ https://reviews.llvm.org/D67083 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-b

[Lldb-commits] [PATCH] D67067: Breakpad: Basic support for STACK WIN unwinding

amccarth added a comment. This is looking pretty good. I'm wondering whether it needs some "negative" tests. Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:549 + llvm::Optional record = StackWinRecord::parse(*It); + assert(record.hasValue()); + ---

[Lldb-commits] [PATCH] D67067: Breakpad: Basic support for STACK WIN unwinding

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM. Please consider adding a comment to the assert that summarizes what you explained in the thread. Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad

[Lldb-commits] [PATCH] D67123: [lldb] Early exit in RangeDataVector:FindEntryIndexesThatContain

amccarth added inline comments. Comment at: lldb/include/lldb/Utility/RangeMap.h:739 +auto end = std::lower_bound(m_entries.begin(), m_entries.end(), +Entry(addr, 1), BaseLessEqual); +for (auto I = m_entries.begin(); I != end; ++I) { --

[Lldb-commits] [PATCH] D67123: [lldb] Early exit in RangeDataVector:FindEntryIndexesThatContain

amccarth added a comment. Disregard my last comment. Phabricator wasn't showing me that latest, nor that the patch had already been submitted, which seems to be happening with increasing frequency. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67123/new/ https:

[Lldb-commits] [PATCH] D67168: [Windows] Add support of watchpoints to `ProcessWindows`

amccarth added inline comments. Comment at: lldb/source/Plugins/Process/Windows/Common/ProcessWindows.cpp:397 +if (slot != LLDB_INVALID_INDEX32) { + int id = m_watchpoint_slots[slot]; + LLDB_LOG(log, The names here are a bit confusing: `GetTriggere

[Lldb-commits] [PATCH] D66994: [lldb][CMake] Fix windows-x86-debug compilation with python enabled using multi-target generator

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. Thanks for factoring out the duplication. Fingers crossed. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66994/new/ https://reviews.llvm.org/D66994 _

[Lldb-commits] [PATCH] D66994: [lldb][CMake] Fix windows-x86-debug compilation with python enabled using multi-target generator

amccarth added a comment. Yes, I can commit it for you soon. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66994/new/ https://reviews.llvm.org/D66994 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https:

[Lldb-commits] [PATCH] D66994: [lldb][CMake] Fix windows-x86-debug compilation with python enabled using multi-target generator

This revision was automatically updated to reflect the committed changes. Closed by commit rL371090: Fix windows-x86-debug compilation with python enabled using multi-target… (authored by amccarth, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prio

[Lldb-commits] [PATCH] D66638: Unwind: Add a stack scanning mechanism to support win32 unwinding

amccarth added a comment. I don't have any specific code comments, but I do have a couple general questions and points to consider. 1. `isFoundHeuristically` is very generic. It's true that it's a heuristic approach, but it's a very specific heuristic. Might there be other heuristic approach

[Lldb-commits] [PATCH] D67168: [Windows] Add support of watchpoints to `ProcessWindows`

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. Thanks for the changes! I think this looks good now. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67168/new/ https://reviews.llvm.org/D67168 ___

[Lldb-commits] [PATCH] D67347: [Windows] Use information from the PE32 exceptions directory to construct unwind plans

amccarth added a comment. I've been trying to figure out how to implement the same functionality you've got here, so I'm very interested in helping you land this in some form. I think Clayborg and Pavel makes some good points about where the right layer of abstraction is for the unwind plans.

[Lldb-commits] [PATCH] D66638: Unwind: Add a stack scanning mechanism to support win32 unwinding

amccarth accepted this revision. amccarth added a comment. My concerns are satisfied. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D66638/new/ https://reviews.llvm.org/D66638 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https:/

[Lldb-commits] [PATCH] D67347: [Windows] Use information from the PE32 exceptions directory to construct unwind plans

amccarth added a comment. I have a couple more questions and some renaming requests. Comment at: lldb/include/lldb/Symbol/DWARFCallFrameInfo.h:74 - void ForEachFDEEntries( - const std::function &callback); + void ForEachEntries(const std::function &callback) override

[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

amccarth added a comment. cc: Pavel Labath because I know he's been involved in conversations about how to find symbol files in general (PDB, split DWARF, Breakpad, etc.), especially when doing post-mortem debugging. I believe there are complications when the debugger host is different than the

[Lldb-commits] [PATCH] D61003: PostfixExpression: move parser out of NativePDB internals

amccarth accepted this revision. amccarth added a comment. Nice. Thanks for adding the test, too! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D61003/new/ https://reviews.llvm.org/D61003 ___ lldb-commits mailing list lldb-commits@lists.llv

[Lldb-commits] [PATCH] D60962: [NativePDB] Extend .pdb files search folders

amccarth added a comment. Thanks Pavel! Please address Pavel's inline comments, and I'll accept this. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D60962/new/ https://reviews.llvm.org/D60962 ___ lldb-commits maili

[Lldb-commits] [PATCH] D61056: PostfixExpression: move DWARF generator out of NativePDB internals

amccarth added a comment. Can the test deletions be a separate patch, or will they fail because of the other changes in this patch? They feel like a good but separable step. Comment at: include/lldb/Symbol/PostfixExpression.h:210 + + bool Visit(UnaryOpNode &unary, Node *&ref

[Lldb-commits] [PATCH] D61056: PostfixExpression: move DWARF generator out of NativePDB internals

amccarth accepted this revision. amccarth marked 2 inline comments as done. amccarth added a comment. This revision is now accepted and ready to land. Clever (hopefully not too clever)! Not having to derive a special class from Visitor is really nice. LGTM. Comment at: inclu

[Lldb-commits] [PATCH] D61128: Fix stack unwinding for struct methods

amccarth added a comment. Please add some detail to the commit message and consider re-titling it. It looks like this patch is actually adding missing functionality rather than "fixing" a bug in how structs are handled. If that's correct, then please make the commit message reflect that. Sor

[Lldb-commits] [PATCH] D61128: Support member function types in PdbAstBuilder

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. Thanks for the improved commit message. Again, sorry about the delay. Comment at: lldb/source/Plugins/SymbolFile/NativePDB/SymbolFileNativePDB.h:183

[Lldb-commits] [PATCH] D61183: PostfixExpression: Introduce CFANode

amccarth accepted this revision. amccarth added a comment. LGTM, but I found one comment a bit confusing for me. Comment at: source/Symbol/PostfixExpression.cpp:150 + /// InitialValueNodes in our input expression, we assume the initial stack + /// will contain their value (he

[Lldb-commits] [PATCH] D61311: PostfixExpression: Use signed integers in IntegerNode

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. This looks fine to me, and the rationale sounds right. I'll be curious to see if any other reviewers see a potential problem with this. CHANGES SINCE LAST ACTION https://reviews.llvm.o

[Lldb-commits] [PATCH] D61686: Enable lldb-server on Windows

amccarth added inline comments. Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:220 +// In most cases the missing notifications do not affect lldb-server +// so we are temporarily relaxing the following for Windows. +#if !defined(_WIN32

[Lldb-commits] [PATCH] D61686: Enable lldb-server on Windows

amccarth marked an inline comment as done. amccarth added inline comments. Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:220 +// In most cases the missing notifications do not affect lldb-server +// so we are temporarily relaxing the

[Lldb-commits] [PATCH] D61733: Breakpad: Generate unwind plans from STACK CFI records

amccarth accepted this revision. amccarth added a comment. LGTM once you double-check the return value in the error case at the end of `SymbolFileBreakpad::ParseUnwindRow`. Comment at: source/Plugins/SymbolFile/Breakpad/SymbolFileBreakpad.cpp:472 + if (!unwind_rules.empty())

[Lldb-commits] [PATCH] D62759: Fix lit tests on Windows related to CR

amccarth created this revision. amccarth added a reviewer: jasonmolenda. Problem discovered in the breakpoint lit test, but probably exists in others. lldb-test splits lines on LF. Input files that are CR+LF separated (as is common on Windows) then resulted in commands being sent to LLDB that e

[Lldb-commits] [PATCH] D62759: Fix lit tests on Windows related to CR

This revision was automatically updated to reflect the committed changes. Closed by commit rL362844: Fix lit tests on Windows related to CR+LF (authored by amccarth, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.l

[Lldb-commits] [PATCH] D63165: Initial support for native debugging of x86/x64 Windows processes

amccarth added a comment. Sorry for the stupid question, but ... What exactly is meant here by "Native"? How is a NativeProcessWindows different from ProcessWindows? Comment at: source/Plugins/Process/Windows/Common/NativeProcessWindows.h:16 +#include "IDebugDelegate.h" +#in

[Lldb-commits] [PATCH] D63166: Move common functionality from processwindows into processdebugger

amccarth added a comment. It's been a while for me, so I'm not super-familiar with the code being changed, but I'm okay with factoring out common code. I agree with labath's open points and will try to look at it in more detail tomorrow. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION h

[Lldb-commits] [PATCH] D63166: Move common functionality from processwindows into processdebugger

amccarth added a comment. I'm OK with moving common stuff out for now, and I like the separation of ProcessWindows and ProcessDebugger. I agree that we don't want to live too long in a state with a regular plugin and a remote plugin, I still think there's advantage to having common Windows stu

[Lldb-commits] [PATCH] D62183: [Windows] Fix race condition between state changes

amccarth added a comment. zturner is not a regular here anymore. I think he pops in from time-to-time, but I wouldn't depend on just him for a review. Are you still wanting to land this? If you rebase and add me as a reviewer, I'd be happy to take a look. Repository: rLLDB LLDB CHANGES S

<    1   2   3   >