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

[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

[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] 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] 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

[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

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

[Lldb-commits] [PATCH] D89812: [lldb][PDB] Add ObjectFile PDB plugin

amccarth added a comment. This looks pretty good, both the patch and Pavel's insights. I don't see much to comment on that Pavel didn't already catch. Comment at: lldb/source/Plugins/ObjectFile/PDB/ObjectFilePDB.cpp:168 + + ArchSpec = module_spec.GetArchitecture(); +

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

[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

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

[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

[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] 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

[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

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

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

[Lldb-commits] [PATCH] D84070: [LLDB] [COFF] Fix handling of symbols with more than one aux symbol

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. LGTM. That was a nice catch on the other review. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D84070/new/ https://reviews.llvm.org/D84070

[Lldb-commits] [PATCH] D83881: [lldb/COFF] Remove strtab zeroing hack

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. Thanks @mstorsjo for clarifying for me. Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:642 DataExtractor symtab_data =

[Lldb-commits] [PATCH] D83881: [lldb/COFF] Remove strtab zeroing hack

amccarth added a comment. Yes, getting rid of this hack looks like a good idea. If it was actually necessary, there should have been a test on it, and the comments should have been clearer. See my inline comment, though. It looks like this might back out only part of the change.

[Lldb-commits] [PATCH] D81058: [lldb/Interpreter] Color "error:" and "warning:" in the CommandReturnObject output.

amccarth added a comment. I'm just responding to the questions @labath raised without really looking at the details of the code. I agree that there's nothing wrong with having the ability to output color codes to a file, but it's a surprising default and experience tells me it would break a

[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] 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

[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 *), 0,

[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-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] D74010: Fix BroadcasterManager::RemoveListener to really remove the listener

amccarth added a comment. Wow, cool bug. It's too bad the original code re-used an iterator variable instead of make a new name (which would have helped the compiler find the problem). Note that the one they used is shadowed just a couple lines later. It's too bad the original code feels

[Lldb-commits] [PATCH] D74001: Fix after c25938d

This revision was automatically updated to reflect the committed changes. Closed by commit rGfb0d2d455f56: Fix after c25938d (authored by amccarth). Herald added a project: LLDB. Changed prior to commit: https://reviews.llvm.org/D74001?vs=242440=242465#toc Repository: rG LLVM Github Monorepo

[Lldb-commits] [PATCH] D74001: Fix after c25938d

amccarth created this revision. amccarth added reviewers: jasonmolenda, clayborg. My refactor caused some changes in error reporting that TestAddDsymCommand.py was checking, so this restores some of the changes to preserve the old behavior. Putting this through review rather than committing

[Lldb-commits] [PATCH] D73589: Improve help text for (lldb) target symbols add

This revision was automatically updated to reflect the committed changes. Closed by commit rG0e362d82b97f: Improve help text for (lldb) target symbols add (authored by amccarth). Herald added a project: LLDB. Changed prior to commit: https://reviews.llvm.org/D73589?vs=241013=242189#toc

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

This revision was automatically updated to reflect the committed changes. Closed by commit rGc25938d57b1c: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols (authored by amccarth). Herald added a project: LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D73589: Improve help text for (lldb) target symbols add

amccarth added a comment. This is low risk and it did get positive comments from one non-reviewer, so I'm going to land this. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D73589/new/ https://reviews.llvm.org/D73589 ___ lldb-commits

[Lldb-commits] [PATCH] D73589: Improve help text for (lldb) target symbols add

amccarth added a comment. In D73589#1845971 , @jasonmolenda wrote: > Looks good, for the --shlib option I would get rid of the "full path or base > name" language and just say "name", my two cents. I agree just "name" seems fine. Done. CHANGES

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

amccarth marked an inline comment as done. amccarth added inline comments. Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4175 + +if (object_file->GetFileSpec() != symbol_fspec) { + result.AppendWarning("there is a discrepancy between the module file "

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

amccarth marked 2 inline comments as done. amccarth added inline comments. Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4175 + +if (object_file->GetFileSpec() != symbol_fspec) { + result.AppendWarning("there is a discrepancy between the module file "

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

amccarth marked an inline comment as done. amccarth added inline comments. Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4149 + +lldbassert(matching_modules.GetSize() == 1); +ModuleSP module_sp(matching_modules.GetModuleAtIndex(0));

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

amccarth marked 3 inline comments as done. amccarth added a comment. Thanks for the feedback. Obviously I'm confused about how LLDB handles split debug info, so I need more clarification about how to proceed. Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4103 +

[Lldb-commits] [PATCH] D73594: Refactor CommandObjectTargetSymbolsAdd::AddModuleSymbols

amccarth created this revision. amccarth added reviewers: clayborg, jasonmolenda. - [NFC] Renamed local `matching_module_list` to `matching_modules` for conciseness. - [NFC] Eliminated redundant local variable `num_matches` to reduce the risk that changes get it out of sync with

[Lldb-commits] [PATCH] D73589: Improve help text for (lldb) target symbols add

amccarth created this revision. amccarth added a reviewer: aaron.ballman. There were some missing words and awkward syntax. I think this is clearer. https://reviews.llvm.org/D73589 Files: lldb/source/Commands/CommandObjectTarget.cpp Index: lldb/source/Commands/CommandObjectTarget.cpp

[Lldb-commits] [PATCH] D70458: [NFC] Refactor and improve comments in CommandObjectTarget

amccarth closed this revision. amccarth added a comment. This landed in 3b69f0c5550a229dd6d39e361182cdd7cecc36a4 , but there was a typo in the patch description so the tools didn't automatically close this. CHANGES SINCE

[Lldb-commits] [PATCH] D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM

amccarth resigned from this revision. amccarth added a comment. I'm following along, but I don't think I have enough domain knowledge to qualify as a reviewer. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70840/new/ https://reviews.llvm.org/D70840

[Lldb-commits] [PATCH] D70778: [LLDB] [PECOFF] Factorize mapping section names to types using StringSwitch. NFCI.

amccarth added inline comments. Comment at: lldb/source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:864-866 +// If the StringSwitch above picked any type, including +// eSectionTypeOther, accept that instead of the generic mappings +// based on flags

[Lldb-commits] [PATCH] D70745: [LLDB] [PECOFF] Look for the truncated ".eh_fram" section name

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

[Lldb-commits] [PATCH] D70745: [LLDB] [PECOFF] Look for the truncated ".eh_fram" section name

amccarth added a comment. Is `.eh_frame` the only one that matters? Should this be more general and compare `const_sect_name` to the full name and the truncated name for any known section names? If the giant cascade of else-if were factored into a separate function, then a trivial unit test

[Lldb-commits] [PATCH] D70742: [LLDB] [Windows] Avoid using InitializeContext for allocating a CONTEXT. NFC.

amccarth accepted this revision. amccarth added a comment. This revision is now accepted and ready to land. That's fair. LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70742/new/ https://reviews.llvm.org/D70742

[Lldb-commits] [PATCH] D70742: [LLDB] [Windows] Avoid using InitializeContext for allocating a CONTEXT. NFC.

amccarth added a comment. I'm good with the change, but have a couple small requests. I hope to hear from others, too, as this area is outside my wheelhouse. Comment at: lldb/source/Plugins/Process/Windows/Common/RegisterContextWindows.cpp:158 + memset(_context, 0,

[Lldb-commits] [PATCH] D70458: [NFC] Refactor and improve comments in CommandObjectTarget

amccarth updated this revision to Diff 230303. amccarth marked an inline comment as done. amccarth added a comment. Reverted unintended change caught by reviewer. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70458/new/ https://reviews.llvm.org/D70458 Files:

[Lldb-commits] [PATCH] D70458: [NFC] Refactor and improve comments in CommandObjectTarget

amccarth marked 2 inline comments as done. amccarth added inline comments. Comment at: lldb/source/Commands/CommandObjectTarget.cpp:4059 if (!module_spec.GetFileSpec() && !module_spec.GetPlatformFileSpec()) - module_spec.GetFileSpec().GetFilename() =

[Lldb-commits] [PATCH] D70458: [NFC] Refactor and improve comments in CommandObjectTarget

amccarth created this revision. amccarth added a reviewer: labath. Made small improvements while debugging through CommandObjectTarget::AddModuleSymbols. 1. Refactored error case for an early out, reducing the indentation of the rest of this long function. 2. Clarified some comments by

[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

[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

[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

[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

[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

[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] 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

[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

[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] 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

[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

[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

[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

[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

[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

[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,

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

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

[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 ); + void ForEachEntries(const std::function ) override;

[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

[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] 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] 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

[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

[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

[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] 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:

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

[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] 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:

[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] 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

[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] 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

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

[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

[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

[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] 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] 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-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

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

  1   2   3   >