[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

JDevlieghere added inline comments. Comment at: lldb/source/API/SBThread.cpp:853 +if (!location_spec) { + sb_error.SetErrorString( + llvm::toString(location_spec.takeError()).c_str()); mib wrote: > JDevlieghere wrote: > > Why not `sb_error =

[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

mib marked an inline comment as done. mib added inline comments. Comment at: lldb/source/API/SBThread.cpp:853 +if (!location_spec) { + sb_error.SetErrorString( + llvm::toString(location_spec.takeError()).c_str()); JDevlieghere wrote: > Why not

[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

JDevlieghere added inline comments. Comment at: lldb/source/API/SBThread.cpp:853 +if (!location_spec) { + sb_error.SetErrorString( + llvm::toString(location_spec.takeError()).c_str()); Why not `sb_error = location_spec.takeError()`?

[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

mib updated this revision to Diff 340211. mib added a comment. Fix test failures. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100965/new/ https://reviews.llvm.org/D100965 Files: lldb/include/lldb/Breakpoint/BreakpointResolverFileLine.h

[Lldb-commits] [PATCH] D100962: [lldb/Utility] Add SourceLocationSpec class (NFC)

mib updated this revision to Diff 340210. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100962/new/ https://reviews.llvm.org/D100962 Files: lldb/include/lldb/Utility/SourceLocationSpec.h lldb/source/Utility/CMakeLists.txt

[Lldb-commits] [PATCH] D100962: [lldb/Utility] Add SourceLocationSpec class (NFC)

mib updated this revision to Diff 340202. mib added a comment. Fix unit test failure. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100962/new/ https://reviews.llvm.org/D100962 Files: lldb/include/lldb/Utility/SourceLocationSpec.h

[Lldb-commits] [PATCH] D100208: [lldb] [Process/Linux] Report fork/vfork stop reason

mgorny updated this revision to Diff 340187. mgorny added a comment. Add `multiprocess` to supported features. Tests were moved to earlier commit. Now this commit only enables the relevant category. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100208/new/

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

mgorny updated this revision to Diff 340186. mgorny added a comment. Fix tests to include `multiprocess+` in `qSupported`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100191/new/ https://reviews.llvm.org/D100191 Files: lldb/include/lldb/Host/common/NativeProcessProtocol.h

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

mgorny updated this revision to Diff 340184. mgorny marked an inline comment as done. mgorny added a comment. Add the comment for mock stuff. Move tests from the later patch here and skip them based on category. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100191/new/

[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

mib updated this revision to Diff 340183. mib added a comment. Addressed @JDevlieghere comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100965/new/ https://reviews.llvm.org/D100965 Files:

[Lldb-commits] [PATCH] D100196: [lldb] [gdb-remote server] Introduce new stop reasons for fork and vfork

mgorny updated this revision to Diff 340180. mgorny added a comment. Actually, add asserts for fork/vfork extensions too. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100196/new/ https://reviews.llvm.org/D100196 Files: lldb/bindings/interface/SBThread.i

[Lldb-commits] [PATCH] D100196: [lldb] [gdb-remote server] Introduce new stop reasons for fork and vfork

mgorny updated this revision to Diff 340178. mgorny added a comment. Add an assert to make sure that process doesn't report fork/vfork stop reason without multiprocess support. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100196/new/ https://reviews.llvm.org/D100196 Files:

[Lldb-commits] [PATCH] D100153: [lldb] [Process] Introduce protocol extension support API

mgorny updated this revision to Diff 340177. mgorny added a comment. Added `Extension` bit for `multiprocess`. Made `fork-events` and `vfork-events` both depend on it — i.e. be implicitly disabled if `multiprocess` wasn't reported as supported. Simplify

[Lldb-commits] [PATCH] D100196: [lldb] [gdb-remote server] Introduce new stop reasons for fork and vfork

mgorny added inline comments. Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:946-948 +response.Printf("%s:p%" PRIx64 ".%" PRIx64 ";", reason_str, +tid_stop_info.details.fork.child_pid, +

[Lldb-commits] [PATCH] D101198: [lldb-vscode] Read requests asynchronously

wallace created this revision. wallace added reviewers: clayborg, kusmour. wallace requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Currently, lldb-vscode is reading and processing requests one at a time, which doesn't match exactly the

[Lldb-commits] [PATCH] D100208: [lldb] [Process/Linux] Report fork/vfork stop reason

mgorny added inline comments. Comment at: lldb/source/Plugins/Process/Linux/NativeThreadLinux.cpp:402-403 + is_vfork ? StopReason::eStopReasonVFork : StopReason::eStopReasonFork; + m_stop_info.details.fork.child_pid = child_pid; + m_stop_info.details.fork.child_tid =

[Lldb-commits] [PATCH] D101128: [lldb-vscode] only report long running progress events

clayborg requested changes to this revision. clayborg added inline comments. This revision now requires changes to proceed. Comment at: lldb/tools/lldb-vscode/ProgressEvent.cpp:89 + m_start_event_timestamp >= +

[Lldb-commits] [PATCH] D100208: [lldb] [Process/Linux] Report fork/vfork stop reason

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. In D100208#2705891 , @mgorny wrote: > In D100208#2705455 , @labath wrote: > >> This seems reasonable. >> >>

[Lldb-commits] [PATCH] D100962: [lldb/Utility] Add SourceLocationSpec class (NFC)

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100962/new/ https://reviews.llvm.org/D100962

[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

JDevlieghere added inline comments. Comment at: lldb/source/API/SBThread.cpp:853-856 + std::string err_msg = "Invalid SourceLocationSpec: "; + err_msg += llvm::toString(location_spec.takeError()); + sb_error.SetErrorString(err_msg.c_str()); + return sb_error;

[Lldb-commits] [PATCH] D100191: [lldb] [llgs] Support owning and detaching extra processes

labath accepted this revision. labath added inline comments. This revision is now accepted and ready to land. Comment at: lldb/unittests/TestingSupport/Host/NativeProcessTestUtils.h:31-34 + void NewSubprocess(NativeProcessProtocol *parent_process, +

[Lldb-commits] [PATCH] D100196: [lldb] [gdb-remote server] Introduce new stop reasons for fork and vfork

labath accepted this revision. labath added inline comments. This revision is now accepted and ready to land. Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.cpp:946-948 +response.Printf("%s:p%" PRIx64 ".%" PRIx64 ";", reason_str, +

[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

mib updated this revision to Diff 340128. mib marked 6 inline comments as done. mib added a comment. Addressed @JDevlieghere @shafik feedbacks. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100965/new/ https://reviews.llvm.org/D100965 Files:

[Lldb-commits] [PATCH] D100962: [lldb/Utility] Add SourceLocationSpec class (NFC)

mib updated this revision to Diff 340117. mib added a comment. Address @JDevlieghere comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D100962/new/ https://reviews.llvm.org/D100962 Files: lldb/include/lldb/Utility/SourceLocationSpec.h

[Lldb-commits] [PATCH] D101131: [lldb-vscode] Follow up of D99989 - store some strings more safely

wallace added inline comments. Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2934 // We first find out which variable names are duplicated -llvm::DenseMap variable_name_counts; +std::map variable_name_counts; for (auto i = start_idx; i < end_idx; ++i) {

[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

shafik added inline comments. Comment at: lldb/source/API/SBThread.cpp:852 +auto location_spec = SourceLocationSpec::Create( +step_file_spec, line, column, check_inlines, exact); +lldbassert(location_spec && "Invalid SourceLocationSpec."); ```

[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests

rupprecht added a comment. LGTM too, thanks for writing this up! Comment at: lldb/docs/resources/test.rst:264 +:: +self.expect("expr 1 - 1", substrs=["0"]) + shafik wrote: > Maybe some more examples with alternatives would be helpful here. Also mentioning

[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests

shafik added a comment. Thank you, this is awesome. Comment at: lldb/docs/resources/test.rst:206 + +**Don't unnecessarily launch the test executable.** +Launching a process and running to a breakpoint can often be the most While I agree with this, it also

Re: [Lldb-commits] [PATCH] D100898: [CMake][lldb] add_lldb_library's functions require default visibility

Yes, this IMO should eventually be addressed by explicitly exporting each meant-to-be-exported function with LLDB_ABI and then changing the line this patch adds to use `hidden` instead of `default`. This patch is meant to be a band-aid until that can be done. > On Apr 20, 2021, at 11:58 PM,

[Lldb-commits] [PATCH] D99484: Use `GNUInstallDirs` to support custom installation dirs.

Ericson2314 updated this revision to Diff 339639. Ericson2314 added a comment. Resplit on @LebedevRI's advice that it's OK if not all patches build alone Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99484/new/ https://reviews.llvm.org/D99484

[Lldb-commits] [PATCH] D99484: Use `GNUInstallDirs` to support custom installation dirs.

Ericson2314 updated this revision to Diff 339626. Ericson2314 added a comment. Recombind revisions, need to convert everything at once Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99484/new/ https://reviews.llvm.org/D99484 Files:

[Lldb-commits] [PATCH] D62732: [RISCV] Add SystemV ABI

jade added a comment. In D62732#2306055 , @labath wrote: > ABI plugins are one of the hardest things to test in lldb, particularly > without actual hardware. That's why we've let them be added in the past > without any accompanying tests. The situation

[Lldb-commits] [PATCH] D99484: Use `GNUInstallDirs` to support custom installation dirs.

Ericson2314 updated this revision to Diff 338697. Ericson2314 added a comment. Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99484/new/ https://reviews.llvm.org/D99484 Files: clang-tools-extra/clang-doc/tool/CMakeLists.txt

[Lldb-commits] [PATCH] D99812: [PowerPC] [GlobalISel] Implementation of formal arguments lowering in the IRTranslator for the PPC backend

arsenm added inline comments. Comment at: llvm/lib/Target/PowerPC/GISel/PPCCallLowering.cpp:46-58 +/** + * @brief Lower incoming arguments into generic MIR, this method is responsible + * for splitting aggregate arguments into multiple single value types as well + * as setting

[Lldb-commits] [PATCH] D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files.

jingham added a comment. Since you just mainly want to run this past the bots again, probably best to just resubmit. You could update the patch here or not at your convenience, but I don't see it would serve any purpose to wait on approval. Sorry for not noticing it was a shell test, BTW...

[Lldb-commits] [PATCH] D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files.

cmtice added a comment. Pavel's test case passed on my local machine (x86_64 linux workstation). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D97786/new/ https://reviews.llvm.org/D97786 ___ lldb-commits

[Lldb-commits] [PATCH] D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files.

cmtice added a comment. Hi Pavel, Thank you for both the detailed explanation and the updated test case (and thank you Jim for your recommendations as well!). I'm testing the updated test case now. Assuming the tests pass, is it ok for me to re-land this CL using the updated test case? and

[Lldb-commits] [PATCH] D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files.

labath added a comment. It looks like you've managed to find a bug in lldb. Congratulations. ;) I don't yet fully understand what's going on, but the rough idea is this: Lldb relies heavily on knowing the boundaries of every function. For this reason, our ELF code contains logic to parse the

[Lldb-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

davezarzycki added a comment. In D98179#2689079 , @lebedev.ri wrote: > In D98179#2689075 , @mstorsjo wrote: > >> Something related to the time recording seems to fail intermittently on >> buildbots:

[Lldb-commits] [PATCH] D97786: LLDB: Use path relative to binary, not relative to debugger CWD, for finding .dwo files.

rupprecht added a comment. In D97786#2693410 , @jingham wrote: > In D97786#2693381 , @cmtice wrote: > >> I had to revert this change because the test case broke the windows builder. >> What's the right way to

[Lldb-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

lebedev.ri added a comment. In D98179#2689075 , @mstorsjo wrote: > Something related to the time recording seems to fail intermittently on > buildbots: https://lab.llvm.org/buildbot#builders/93/builds/2697 I'll cut to the fix here: @davezarzycki is

[Lldb-commits] [PATCH] D98179: [lit] Sort test start times based on prior test timing data

mstorsjo added a comment. Something related to the time recording seems to fail intermittently on buildbots: https://lab.llvm.org/buildbot#builders/93/builds/2697 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D98179/new/

[Lldb-commits] [PATCH] D99812: [PowerPC] [GlobalISel] Implementation of formal arguments lowering in the IRTranslator for the PPC backend

gandhi21299 marked 2 inline comments as done. gandhi21299 added a comment. I am looking towards more feedback on this patch, please do follow up at your convenience. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D99812/new/

[Lldb-commits] [PATCH] D100962: [lldb/Utility] Add SourceLocationSpec class (NFC)

mib marked 7 inline comments as done. mib added inline comments. Comment at: lldb/source/Utility/SourceLocationSpec.cpp:59-61 +bool SourceLocationSpec::operator!=(const SourceLocationSpec ) const { + return !(*this == rhs); +} JDevlieghere wrote: > Isn't this

[Lldb-commits] [PATCH] D101131: [lldb-vscode] Follow up of D99989 - store some strings more safely

wallace added a comment. TIL. I'll give it a try Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101131/new/ https://reviews.llvm.org/D101131 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D101131: [lldb-vscode] Follow up of D99989 - store some strings more safely

JDevlieghere added inline comments. Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:2934 // We first find out which variable names are duplicated -llvm::DenseMap variable_name_counts; +std::map variable_name_counts; for (auto i = start_idx; i < end_idx; ++i)

[Lldb-commits] [PATCH] D100965: [lldb] Refactor argument group by SourceLocationSpec (NFCI)

JDevlieghere requested changes to this revision. JDevlieghere added a comment. This revision now requires changes to proceed. In several places you have `SourceLocationSpec::Create` followed by a `lldbassert`. That's not sufficient, because in a non-assert build, the lldbassert is going to

[Lldb-commits] [PATCH] D100962: [lldb/Utility] Add SourceLocationSpec class (NFC)

JDevlieghere added inline comments. Comment at: lldb/include/lldb/Utility/SourceLocationSpec.h:13-15 +#include "lldb/lldb-defines.h" + +#include "llvm/ADT/Optional.h" Comment at: lldb/include/lldb/Utility/SourceLocationSpec.h:108-138 + ///

[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests

JDevlieghere added a comment. Thanks for taking all the lore around API tests and writing it down. I think this is going to be very useful and make our lives easier during code review. This LGTM but I won't accept (yet) to give others a chance to see this show up in their review queue.

[Lldb-commits] [PATCH] D101157: [lldb] [test] Add tests for core dumps of multithreaded programs [WIP]

mgorny updated this revision to Diff 340035. mgorny edited the summary of this revision. mgorny added a comment. Complete set of tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101157/new/ https://reviews.llvm.org/D101157 Files:

[Lldb-commits] [PATCH] D101157: [lldb] [test] Add tests for core dumps of multithreaded programs [WIP]

mgorny updated this revision to Diff 340030. mgorny added a comment. Now with complete set of amd64 tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101157/new/ https://reviews.llvm.org/D101157 Files: lldb/test/Shell/Register/Core/Inputs/multithread.cpp

[Lldb-commits] [PATCH] D101157: [lldb] [test] Add tests for core dumps of multithreaded programs [WIP]

mgorny added inline comments. Comment at: lldb/test/Shell/Register/Core/x86-64-linux-multithread.test:20 +thread select 2 +# CHECK: (lldb) thread select 2 +register read --all Fun fact: it seems that `register read` outputs immediately while `thread select` can

[Lldb-commits] [PATCH] D101157: [lldb] [test] Add tests for core dumps of multithreaded programs [WIP]

mgorny updated this revision to Diff 340003. mgorny added a comment. Check for `thread select` instructions rather than output. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101157/new/ https://reviews.llvm.org/D101157 Files: lldb/test/Shell/Register/Core/Inputs/multithread.cpp

[Lldb-commits] [PATCH] D101157: [lldb] [test] Add tests for core dumps of multithreaded programs [WIP]

mgorny created this revision. mgorny added reviewers: labath, krytarowski, emaste. Herald added a subscriber: jfb. mgorny requested review of this revision. (adding for early review) https://reviews.llvm.org/D101157 Files: lldb/test/Shell/Register/Core/Inputs/multithread.cpp

[Lldb-commits] [lldb] f3e6f85 - [lldb][NFC] Remove a stray unicode character in the LLDB test docs

Author: Raphael Isemann Date: 2021-04-23T13:20:10+02:00 New Revision: f3e6f856c2905362f54acebc5ef500df10cb06fa URL: https://github.com/llvm/llvm-project/commit/f3e6f856c2905362f54acebc5ef500df10cb06fa DIFF:

[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests

teemperor added a comment. If anyone feels like any of the guidelines is actually controversial then let me know and I'll remove it from this review and split it out into its own patch. (I am also aware that the text directly above has some small overlap with the guidelines as it for example

[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests

teemperor updated this revision to Diff 339978. teemperor added a comment. - Improve some wording. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D101153/new/ https://reviews.llvm.org/D101153 Files: lldb/docs/resources/test.rst Index: lldb/docs/resources/test.rst

[Lldb-commits] [PATCH] D101153: [lldb][NFC] Specify guidelines for API tests

teemperor created this revision. teemperor added a reviewer: LLDB. teemperor added a project: LLDB. Herald added a subscriber: JDevlieghere. teemperor requested review of this revision. This patch specifies a few guidelines that our API tests should follow. The motivations for this are twofold:

[Lldb-commits] [lldb] f8f3fc1 - [lldb][NFC] Delete a checked-in build log in docs/testsuite

Author: Raphael Isemann Date: 2021-04-23T10:36:43+02:00 New Revision: f8f3fc1fbad608ee7c7e0c2a5af0637006e1ba0c URL: https://github.com/llvm/llvm-project/commit/f8f3fc1fbad608ee7c7e0c2a5af0637006e1ba0c DIFF:

[Lldb-commits] [PATCH] D96236: [lldb] DWZ 1/9: Pass main DWARFUnit * along DWARFDIEs

jankratochvil planned changes to this revision. jankratochvil added a comment. In D96236#2710376 , @clayborg wrote: > I would be fine with DWARFDie getting bigger to 24B. Great, so I can return back to 2019. > These objects are used temporarily and not