[Lldb-commits] [PATCH] D63790: [dotest] Add the ability to set environment variables for the inferior.

amccarth added inline comments. Comment at: lldb/packages/Python/lldbsuite/test/lldbtest.py:1861 +if lldbtest_config.inferior_env: +self.runCmd('env {}'.format(lldbtest_config.inferior_env)) + `env` or `settings set target.env-vars`? On Windo

[Lldb-commits] [PATCH] D63790: [dotest] Add the ability to set environment variables for the inferior.

amccarth added a comment. The LGTM, but I wasn't nominated as a reviewer, and I was mostly looking at it from the point of Windows compatibility. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63790/new/ https://reviews.llvm.org/D63790 ___

[Lldb-commits] [PATCH] D64444: Add Python 3.6 and 3.7 to the version list

amccarth added a comment. Be aware that Swig 3.0.12 has a compatibility problem with Python 3.7 that causes hundreds of LLDB tests to fail an assertion (at least on Windows). http://lists.llvm.org/pipermail/lldb-dev/2019-June/015086.html I don't think that should block the patch or whether it e

[Lldb-commits] [PATCH] D64821: [CMake] Remove duplicated logic to find Python when doing a standalone build

amccarth added a comment. Actually, right now I'm trying to figure out where the interpreter is found because cmake is finding different, incompatible versions of the interpreter (2.7) and the libs (3.6). On Windows, it looks like LLDBConfig.cmake doesn't explicitly look for the interpreter, o

[Lldb-commits] [PATCH] D64824: [CMake] Move standalone check so we don't have to reconfigure LLDB

amccarth added inline comments. Comment at: lldb/CMakeLists.txt:14 -include(LLDBStandalone) +# If we are not building as a part of LLVM, build LLDB as an +# standalone project, using LLVM as an external library: Nit: s/an/a/ Repository: rLLDB LLDB CHANGES

[Lldb-commits] [PATCH] D64812: [CMake] Fail when Python interpreter doesn't match Python libraries version

amccarth added a comment. In D64812#1588061 , @rnk wrote: > In D64812#1588055 , @rnk wrote: > > > You broke my build. =/ I got this output: > > > > CMake Error at > > C:/src/llvm-project/lldb/cmake/modules/LLDBCo

[Lldb-commits] [PATCH] D64812: [CMake] Fail when Python interpreter doesn't match Python libraries version

amccarth added a comment. OK, the only way I was able to make this work was to remove all traces of Python 2.x from my machine. As long as the older Python existed on my machine CMake would find that one, regardless of which one was in my PATH or indicated by PYTHON_HOME_DIR. Of course, it re

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

amccarth added a comment. This change looks fine to me, but I wish there was a reliable way to test it. Is it a true race condition (e.g., because the unpredictability of thread scheduling), or is there a way to write a test that would always fail without this fix? CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D64881: [Cmake] Use the modern way to find Python when possible

amccarth added a comment. An aside ... I'm still trying to get back to a buildable state the earlier changes, like the one that tries to enforce version consistency between the libs and the interpreter. I'm currently bisecting to figure out what I hope is the final blocker. For me, the find_

[Lldb-commits] [PATCH] D64881: [Cmake] Use the modern way to find Python when possible

amccarth added a comment. In D64881#1590252 , @JDevlieghere wrote: > In D64881#1590204 , @amccarth wrote: > > > An aside ... > > > > I'm still trying to get back to a buildable state the earlier changes, like > >

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

amccarth requested changes to this revision. amccarth added a comment. This revision now requires changes to proceed. In D63165#1541944 , @clayborg wrote: > In D63165#1540606 , @Hui wrote: > > > > In D63165#1539118

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

amccarth added a comment. Yes, I can submit it for you, probably in the next hour or two. Thanks for the patch! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D62183/new/ https://reviews.llvm.org/D62183 ___ lldb-commits mailing list lldb-co

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

This revision was automatically updated to reflect the committed changes. Closed by commit rL366703: [Windows] Fix race condition between state changes (authored by amccarth, committed by ). Herald added a project: LLVM. Herald added a subscriber: llvm-commits. Changed prior to commit: https://

[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 &spec = module_spec.GetArchitecture(); +

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

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

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

[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() = symbol_fspec.

[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/source/Commands/Comm

[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(&m_context, 0, sizeof

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

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

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

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

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

[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: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 3 inline comments as done. amccarth added a comment. I backed out the TODO and the warn-and-continue behavior to expedite this patch which is just supposed to be a refactor for readability. We can revisit the issue of debug symbol file names not matching the object file names in

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

amccarth updated this revision to Diff 241510. amccarth marked an inline comment as done. amccarth added a comment. - Backed out the warn-and-continue behavior change. It should be addressed later. - Changed lldb_assert to assert. - Improved comment in place of the proposed TODO. CHANGES SINCE

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

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

[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 https://revi

[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&id=242189#toc Repo

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

[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&id=242465#toc Repository: rG LLVM Github Monore

[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 it's

<    1   2   3