[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

mgorny added a comment. This is the final version that was pushed, correct? (the commits don't link the revision) I've raised some concerns about the resulting API in https://reviews.llvm.org/D156270#4557902. I'd appreciate if you could take a look. Repository: rG LLVM Github Monorepo CHA

[Lldb-commits] [PATCH] D156270: [lldb][NFCI] Change logic to find clang resource dir in standalone builds

mgorny added a comment. Yeah, that's why I CC-ed the two people who committed that API, as I don't really understand how it's supposed to work (and don't really have the capacity to figure it out). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156

[Lldb-commits] [PATCH] D156270: [lldb][NFCI] Change logic to find clang resource dir in standalone builds

mgorny added subscribers: paperchalice, tstellar, mgorny. mgorny added a comment. I'm not sure if it was really intended but the new API is kinda horrible, at least for us in Gentoo. Our install prefix is `/usr/lib/llvm/NN`, whereas clang resource dir is `/usr/lib/clang/NN`. If I don't overrid

[Lldb-commits] [PATCH] D150032: [lldb, NetBSD] getValue => operator* for Optional migration

This revision was automatically updated to reflect the committed changes. Closed by commit rGe5f0f1d3ee95: [lldb] [NetBSD] getValue => operator* for Optional migration (authored by nikicoon, committed by mgorny). Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Repository:

[Lldb-commits] [PATCH] D149397: Host: generalise `GetXcodeSDKPath`

mgorny added a comment. This change broke building with GCC: samu: job failed: /usr/lib/ccache/bin/x86_64-pc-linux-gnu-g++ -DHAVE_ROUND -D_DEBUG -D_GLIBCXX_ASSERTIONS -D_LIBCPP_ENABLE_ASSERTIONS -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/var/tmp/portage/dev-util

[Lldb-commits] [PATCH] D145964: [lld] Use installed llvm_gtest in standalone builds

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG56464ba865b7: [lld] Use installed llvm_gtest in standalone builds (authored by mgorny). Herald added a project: LLVM. Herald added a subscriber: llvm

[Lldb-commits] [PATCH] D145964: [lld] Use installed llvm_gtest in standalone builds

mgorny created this revision. mgorny added a reviewer: tstellar. Herald added a project: All. mgorny requested review of this revision. Use the installed llvm_gtest library instead of rebuilding it locally when standalone builds are used. This change is now required as otherwise the build fails d

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

mgorny added reviewers: MaskRay, thesamesam, tstellar. mgorny added a comment. Ok, this turned out to be surprisingly painless, at least within our packaging. However, I'd prefer if someone more experienced looked at the CMake changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST A

[Lldb-commits] [PATCH] D141907: [CMake] Ensure `CLANG_RESOURCE_DIR` is respected

mgorny added a comment. While the general direction seems sound, I suspect this may be a real PITA given that for us: -DCLANG_RESOURCE_DIR="../../../../lib/clang/${LLVM_MAJOR}" I'm going to try and see later throughout the week if it's possible to keep that working with this patch. Reposit

[Lldb-commits] [PATCH] D140671: [lldb] [utils] Fix linking lit-cpuid to LLVM dylib

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

[Lldb-commits] [PATCH] D140671: [lldb] [utils] Fix linking lit-cpuid to LLVM dylib

This revision was automatically updated to reflect the committed changes. Closed by commit rGdfc20708bcdf: [lldb] [utils] Fix linking lit-cpuid to LLVM dylib (authored by mgorny). Herald added a project: LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm

[Lldb-commits] [PATCH] D140671: [lldb] [utils] Fix linking lit-cpuid to LLVM dylib

mgorny created this revision. mgorny added reviewers: labath, sylvestre.ledru, JDevlieghere, MaskRay. Herald added subscribers: StephenFan, delcypher. Herald added a project: All. mgorny requested review of this revision. Use `LINK_COMPONENTS` instead of manual `target_link_libraries` to link lit-

[Lldb-commits] [PATCH] D137724: [CMake] Warn when the version is older than 3.20.0.

mgorny added inline comments. Comment at: clang/CMakeLists.txt:14 set(CLANG_BUILT_STANDALONE TRUE) + if ("${CMAKE_VERSION}" VERSION_LESS "3.20.0") +message(WARNING I wonder if we could move this to `CMakePolicy.cmake`, though admittedly you'd have to som

[Lldb-commits] [PATCH] D137337: Replace LLVM_LIBDIR_SUFFIX by CMAKE_INSTALL_LIBDIR

mgorny added inline comments. Comment at: bolt/lib/RuntimeLibs/RuntimeLibrary.cpp:32 SmallString<128> LibPath = llvm::sys::path::parent_path(Dir); - llvm::sys::path::append(LibPath, "lib" LLVM_LIBDIR_SUFFIX); + llvm::sys::path::append(LibPath, CMAKE_INSTALL_LIBDIR); if (!

[Lldb-commits] [PATCH] D128465: [llvm] add zstd to `llvm::compression` namespace

mgorny added a comment. Is anyone working on a solution that would work for people using a zstd install method that's actually supported upstream? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D128465/new/ https://reviews.llvm.org/D128465

[Lldb-commits] [PATCH] D138750: [lldb][JITLoaderGDB] Resolve __jit_debug_register_code as eSymbolTypeCode

mgorny accepted this revision. mgorny added a comment. This revision is now accepted and ready to land. Thanks. I can confirm that the two tests pass for me with this patch. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138750/new/ https://reviews.

[Lldb-commits] [PATCH] D134033: [lldb/Plugins] Improve error reporting when reading memory in Scripted Process

mgorny added a comment. Thanks, it builds fine for me now! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134033/new/ https://reviews.llvm.org/D134033 ___ lldb-commits mailing list lldb-commits@lists.llvm

[Lldb-commits] [PATCH] D134033: [lldb/Plugins] Improve error reporting when reading memory in Scripted Process

mgorny added a comment. I can confirm that it doesn't fail with clang. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134033/new/ https://reviews.llvm.org/D134033 ___ lldb-commits mailing list lldb-commit

[Lldb-commits] [PATCH] D134033: [lldb/Plugins] Improve error reporting when reading memory in Scripted Process

mgorny added a comment. In D134033#3939158 , @mib wrote: > @mgorny @dyung `error: too few template-parameter-lists` this seems to be a > `g++` error. Could you try building with clang to confirm my theory ? I'll > try to fix the issue but I have no way

[Lldb-commits] [PATCH] D134033: [lldb/Plugins] Improve error reporting when reading memory in Scripted Process

mgorny added a comment. LLDB no longer builds for me after this change (could be related to swig 4.1.0): [1/5] Building CXX object tools/lldb/source/Plugins/ScriptInterpre...ldbPluginScriptInterpreterPython.dir/ScriptedPythonInterface.cpp.o FAILED: tools/lldb/source/Plugins/ScriptInterprete

[Lldb-commits] [PATCH] D138237: [lldb] Restore default setting of LLDB_INCLUDE_TESTS in standalone builds

mgorny accepted this revision. mgorny added a comment. This revision is now accepted and ready to land. Thanks, this makes sense. I just wish we could get D137035 in and avoid this whole complexity in the first place. Repository: rG LLVM Github Monorepo CH

[Lldb-commits] [PATCH] D125860: [clang] Only use major version in resource dir

mgorny added reviewers: serge-sans-paille, sylvestre.ledru, phosek, MaskRay. mgorny added a comment. Herald added subscribers: Michael137, StephenFan. To be honest, I've never seen much purpose in the x.y.z directory naming and I feel like it's making packaging unnecessarily more complex, at leas

[Lldb-commits] [PATCH] D136572: Harmonize cmake_policy() across standalone builds of all projects

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG9dd01a5241dc: Harmonize cmake_policy() across standalone builds of all projects (authored by mgorny). Repository: rG LLVM Github Monorepo CHANGES

[Lldb-commits] [PATCH] D136572: Harmonize cmake_policy() across standalone builds of all projects

mgorny added a comment. In D136572#3889317 , @mstorsjo wrote: > Also for the record, I'd love to get rid of this symlink based setup, if I > could make cmake produce project files where caching works in the same way. > The main requirement for that woul

[Lldb-commits] [PATCH] D136572: Harmonize cmake_policy() across standalone builds of all projects

mgorny added a reviewer: mstorsjo. mgorny added a comment. @mstorsjo, could you check this version? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136572/new/ https://reviews.llvm.org/D136572 ___ lldb-commits mailing list lldb-commits@lists.ll

[Lldb-commits] [PATCH] D136572: Harmonize cmake_policy() across standalone builds of all projects

mgorny updated this revision to Diff 471173. mgorny added a comment. Move `LLVM_COMMON_CMAKE_UTILS` earlier and use it for CMakePolicy path. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136572/new/ https://reviews.llvm.org/D136572 Files: clang/CMakeLists.txt cmake/Modules/CMakePol

[Lldb-commits] [PATCH] D136572: Harmonize cmake_policy() across standalone builds of all projects

mgorny added a comment. I've reverted it right now, and I'll update the patch later today. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136572/new/ https://reviews.llvm.org/D136572 ___ lldb-commits mail

[Lldb-commits] [PATCH] D136572: Harmonize cmake_policy() across standalone builds of all projects

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG88d7508dc479: Harmonize cmake_policy() across standalone builds of all projects (authored by mgorny). Herald added projects: clang, LLDB, Flang. Rep

[Lldb-commits] [PATCH] D136572: Harmonize cmake_policy() across standalone builds of all projects

mgorny added a comment. Thanks. Since this is not urgent, I'll let it wait for others to chime in a while more. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136572/new/ https://reviews.llvm.org/D136572 ___ lldb-commits mailing list lldb-com

[Lldb-commits] [PATCH] D136572: Harmonize cmake_policy() across standalone builds of all projects

mgorny updated this revision to Diff 470225. mgorny edited the summary of this revision. mgorny added a comment. Herald added a project: LLVM. Use a shared policy file via `include()` instead. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136572/new/ https://reviews.llvm.org/D136572 Fi

[Lldb-commits] [PATCH] D136572: Harmonize cmake_policy() across standalone builds of all projects

mgorny added inline comments. Comment at: clang/CMakeLists.txt:6 if(CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR) + # Please keep policies in sync with llvm/CMakeLists.txt. + if(POLICY CMP0114) nickdesaulniers wrote: > Seems error prone. Does cmake have

[Lldb-commits] [PATCH] D136551: [lldb] Include gtest in standalone build only if LLDB_INCLUDE_TESTS

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

[Lldb-commits] [PATCH] D136551: [lldb] Include gtest in standalone build only if LLDB_INCLUDE_TESTS

This revision was automatically updated to reflect the committed changes. Closed by commit rG52f39853abd4: [lldb] Include gtest in standalone build only if LLDB_INCLUDE_TESTS (authored by mgorny). Herald added a project: LLDB. Changed prior to commit: https://reviews.llvm.org/D136551?vs=469980&

[Lldb-commits] [PATCH] D136572: Harmonize cmake_policy() across standalone builds of all projects

mgorny updated this revision to Diff 470108. mgorny retitled this revision from "[lld] Copy cmake_policy() to silence CMake warnings in standalone builds" to "Harmonize cmake_policy() across standalone builds of all projects". mgorny edited the summary of this revision. mgorny added reviewers: me

[Lldb-commits] [PATCH] D136552: [lldb] Add LLVM include dirs prior to gtest target in standalone build

This revision was automatically updated to reflect the committed changes. Closed by commit rGd15239388cca: [lldb] Add LLVM include dirs prior to gtest target in standalone build (authored by mgorny). Herald added a project: LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D136551: [lldb] Include gtest in standalone build only if LLDB_INCLUDE_TESTS

mgorny added inline comments. Comment at: lldb/CMakeLists.txt:20 +option(LLDB_INCLUDE_TESTS "Generate build targets for the LLDB unit tests." ${LLVM_INCLUDE_TESTS}) + labath wrote: > mgorny wrote: > > To be honest, I don't exactly like moving this `option`. Th

[Lldb-commits] [PATCH] D136551: [lldb] Include gtest in standalone build only if LLDB_INCLUDE_TESTS

mgorny added inline comments. Comment at: lldb/CMakeLists.txt:20 +option(LLDB_INCLUDE_TESTS "Generate build targets for the LLDB unit tests." ${LLVM_INCLUDE_TESTS}) + To be honest, I don't exactly like moving this `option`. The alternative I can think of is m

[Lldb-commits] [PATCH] D136552: [lldb] Add LLVM include dirs prior to gtest target in standalone build

mgorny updated this revision to Diff 469984. mgorny added a comment. Rebase to make it independent of D136551 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136552/new/ https://reviews.llvm.org/D136552 Files: lldb/cmake/modules/LLDBStandalone.cmake

[Lldb-commits] [PATCH] D136552: [lldb] Add LLVM include dirs prior to gtest target in standalone build

mgorny created this revision. mgorny added reviewers: labath, JDevlieghere, MaskRay, tstellar. Herald added a subscriber: StephenFan. Herald added a project: All. mgorny requested review of this revision. Move include_directories() declaration before gtest targets are created in standalone build.

[Lldb-commits] [PATCH] D136551: [lldb] Include gtest in standalone build only if LLDB_INCLUDE_TESTS

mgorny created this revision. mgorny added reviewers: labath, JDevlieghere, tstellar, MaskRay. Herald added a subscriber: StephenFan. Herald added a project: All. mgorny requested review of this revision. Build gtest targets when building standalone only if LLDB_INCLUDE_TESTS is true. Prior to th

[Lldb-commits] [PATCH] D135516: [lldb] [MainLoopPosix] Fix crash upon adding lots of pending callbacks

This revision was automatically updated to reflect the committed changes. mgorny marked an inline comment as done. Closed by commit rGe8ee0f121dbc: [lldb] [MainLoopPosix] Fix crash upon adding lots of pending callbacks (authored by mgorny). Herald added a project: LLDB. Changed prior to commit:

[Lldb-commits] [PATCH] D135516: [lldb] [MainLoopPosix] Fix crash upon adding lots of pending callbacks

mgorny marked an inline comment as done. mgorny added a comment. Thanks. I'll do a fresh test run on the updated version and push if it passes. Comment at: lldb/source/Host/posix/MainLoopPosix.cpp:399-402 + if (m_triggering) +return; + m_triggering = true; + -

[Lldb-commits] [PATCH] D135516: [lldb] [MainLoopPosix] Fix crash upon adding lots of pending callbacks

mgorny updated this revision to Diff 467834. mgorny added a comment. Move and rename the `bool`. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135516/new/ https://reviews.llvm.org/D135516 Files: lldb/include/lldb/Host/posix/MainLoopPosix.h lldb/source/Host/posix/MainLoopPosix.cpp

[Lldb-commits] [PATCH] D135516: [lldb] [MainLoopPosix] Fix crash upon adding lots of pending callbacks

mgorny added inline comments. Comment at: lldb/source/Host/posix/MainLoopPosix.cpp:408 assert(bytes_written == 1); + m_trigger_done = true; } labath wrote: > I /think/ this is not right. The other thread can wake up as soon as the > Write call is done, and

[Lldb-commits] [PATCH] D135516: [lldb] [MainLoopPosix] Fix crash upon adding lots of pending callbacks

mgorny added a comment. In D135516#3846182 , @labath wrote: > I never expected we would have so many callbacks that we'd have to worry > about this, but yes, this is one way to fix the problem. Another (slightly > simpler, but also less performant) woul

[Lldb-commits] [PATCH] D135031: [lldb] [llgs] Move client-server communication into a separate thread (WIP)

mgorny added a comment. For the record, I'm still hitting a lot of problems with this, including comm hanging sometimes (especially under load), some Python tests segfaulting with completely useless backtrace (pretty normal for Python backtraces), lots of pexpect-based test failures that also t

[Lldb-commits] [PATCH] D135516: [lldb] [MainLoopPosix] Fix crash upon adding lots of pending callbacks

mgorny created this revision. mgorny added reviewers: labath, krytarowski, emaste, jingham. Herald added a project: All. mgorny requested review of this revision. If lots of pending callbacks are added while the main loop has exited already, the trigger pipe buffer fills in, causing the write to f

[Lldb-commits] [PATCH] D135031: [lldb] [llgs] Move client-server communication into a separate thread (WIP)

mgorny added a comment. In D135031#3833661 , @labath wrote: > I think we should get some measurements (e.g. from the `process plugin packet > speed-test` cmd) of the overhead of this approach. The latency/RTT of the > connection is very important for so

[Lldb-commits] [PATCH] D135028: [lldb] [gdb-remote] Move ReadPacketWithOutputSupport() to client

This revision was automatically updated to reflect the committed changes. Closed by commit rGb6c24c161900: [lldb] [gdb-remote] Move ReadPacketWithOutputSupport() to client (authored by mgorny). Herald added a project: LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:

[Lldb-commits] [PATCH] D135029: [lldb] [gdb-remote] Isolate ClientBase from Communication

mgorny added inline comments. Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteClientBase.h:116-118 + GDBRemoteCommunication &GetCommunication() { +return m_comm; + } labath wrote: > Is the plan to make this private/protected at some point, or so

[Lldb-commits] [PATCH] D135031: [lldb] [llgs] Move client-server communication into a separate thread (WIP)

mgorny created this revision. mgorny added reviewers: krytarowski, emaste, labath, jingham. Herald added a subscriber: arichardson. Herald added a project: All. mgorny requested review of this revision. Perform all the communication between client and server in a dedicated thread that's started af

[Lldb-commits] [PATCH] D135030: [lldb] [gdb-remote] Abstract sending ^c packet into SendCtrlC() method

mgorny created this revision. mgorny added reviewers: labath, emaste, jingham, krytarowski. Herald added a subscriber: arichardson. Herald added a project: All. mgorny requested review of this revision. Abstract sending ^c packet into a dedicated GDBRemoteClientBase::SendCtrlC() method. This makes

[Lldb-commits] [PATCH] D135029: [lldb] [gdb-remote] Isolate ClientBase from Communication

mgorny created this revision. mgorny added reviewers: labath, jingham, krytarowski, emaste. Herald added a subscriber: arichardson. Herald added a project: All. mgorny requested review of this revision. Introduce an isolation layer between GDBRemoteClientBase and GDBRemoteCommunication. This make

[Lldb-commits] [PATCH] D135028: [lldb] [gdb-remote] Move ReadPacketWithOutputSupport() to client

mgorny created this revision. mgorny added reviewers: labath, emaste, krytarowski, jingham. Herald added a project: All. mgorny requested review of this revision. Move ReadPacketWithOutputSupport() from GDBRemoteCommunication to GDBRemoteClientBase. This function is client-specific and moving it

[Lldb-commits] [PATCH] D133427: [gdb-remote] Move broadcasting logic down to GDBRemoteClientBase

This revision was automatically updated to reflect the committed changes. Closed by commit rGbdb4468d3949: [gdb-remote] Move broadcasting logic down to GDBRemoteClientBase (authored by mgorny). Herald added a project: LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https:

[Lldb-commits] [PATCH] D133427: [gdb-remote] Move broadcasting logic down to GDBRemoteClientBase

mgorny created this revision. mgorny added reviewers: labath, krytarowski, emaste, jingham. Herald added a subscriber: arichardson. Herald added a project: All. mgorny requested review of this revision. Move the broadcasting support from GDBRemoteCommunication to GDBRemoteClientBase since this is

[Lldb-commits] [PATCH] D133410: [lldb] Fix ThreadedCommunication races

mgorny accepted this revision. mgorny added a comment. This revision is now accepted and ready to land. Thanks for noticing and fixing this. Comment at: lldb/source/Core/ThreadedCommunication.cpp:113 - if (event_type & eBroadcastBitReadThreadDidExit) { -// If the

[Lldb-commits] [PATCH] D133352: [lldb-server] Report launch error in vRun packets

mgorny added inline comments. Comment at: lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py:18 +args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"] +hex_args = [seven.hexlify(x) for x in args] + labath wrote: > mgorny wrote: > > la

[Lldb-commits] [PATCH] D133410: [lldb] Fix ThreadedCommunication races

mgorny added inline comments. Comment at: lldb/source/Core/ThreadedCommunication.cpp:113 - if (event_type & eBroadcastBitReadThreadDidExit) { -// If the thread exited of its own accord, it either means it -// hit an end-of-file condition or an error. -

[Lldb-commits] [PATCH] D133365: [lldb] Fix CommunicationKDP following D133251

mgorny added a comment. … and thanks a lot for doing this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133365/new/ https://reviews.llvm.org/D133365 ___ lldb-commits mailing list lldb-commits@lists.llvm

[Lldb-commits] [PATCH] D133365: [lldb] Fix CommunicationKDP following D133251

mgorny accepted this revision. mgorny added a comment. This revision is now accepted and ready to land. That's the same thing that I did for gdb-remote, so I suppose it is correct. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133365/new/ https://r

[Lldb-commits] [PATCH] D133352: [lldb-server] Report launch error in vRun packets

mgorny accepted this revision. mgorny added inline comments. This revision is now accepted and ready to land. Comment at: lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py:18 +args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"] +hex_args = [seven.h

[Lldb-commits] [PATCH] D133352: [lldb-server] Report launch error in vRun packets

mgorny added inline comments. Comment at: lldb/test/API/tools/lldb-server/TestGdbRemoteLaunch.py:18 +args = [exe_path, "stderr:arg1", "stderr:arg2", "stderr:arg3"] +hex_args = [seven.hexlify(x) for x in args] + Since we no longer support Python 2,

[Lldb-commits] [PATCH] D131160: [WIP][lldb] Add "event" capability to the MainLoop class

mgorny added a comment. In D131160#3771693 , @labath wrote: > Are you sure about that? > https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/46639 is just > running the commit now. I'd actually suspect the breakage is caused by > D133181

[Lldb-commits] [PATCH] D131160: [WIP][lldb] Add "event" capability to the MainLoop class

mgorny added a comment. I think this may have broken tests on Darwin. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131160/new/ https://reviews.llvm.org/D131160 ___ lldb-commits mailing list lldb-commits

[Lldb-commits] [PATCH] D133251: [lldb] [Core] Split read thread support into ThreadedCommunication

This revision was automatically updated to reflect the committed changes. mgorny marked an inline comment as done. Closed by commit rG9823d42557eb: [lldb] [Core] Split read thread support into ThreadedCommunication (authored by mgorny). Herald added a project: LLDB. Changed prior to commit: htt

[Lldb-commits] [PATCH] D132395: [lldb] [gdb-remote] Use Communication::WriteAll() over Write()

mgorny added a comment. Yeah, you're right. This is not a problem worth addressing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132395/new/ https://reviews.llvm.org/D132395 ___ lldb-commits mailing lis

[Lldb-commits] [PATCH] D132283: [lldb] [Core] Reimplement Communication::ReadThread using MainLoop

mgorny added inline comments. Comment at: lldb/source/Core/Communication.cpp:427 // Notify the read thread. - m_connection_sp->InterruptRead(); labath wrote: > mgorny wrote: > > labath wrote: > > > mgorny wrote: > > > > labath wrote: > > > > > Have you cons

[Lldb-commits] [PATCH] D132578: [lldb] [Core] Use thread for Communication::Write() as well

mgorny updated this revision to Diff 458017. mgorny added a comment. Rebase on top of D131160 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132578/new/ https://reviews.llvm.org/D132578 Files: lldb/include/lldb/Core/Communication.h lldb/source/Cor

[Lldb-commits] [PATCH] D132283: [lldb] [Core] Reimplement Communication::ReadThread using MainLoop

mgorny updated this revision to Diff 458016. mgorny added a comment. Rebase on top of D131160 . CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132283/new/ https://reviews.llvm.org/D132283 Files: lldb/include/lldb/Core/Communication.h lldb/source/Cor

[Lldb-commits] [PATCH] D131160: [WIP][lldb] Add "event" capability to the MainLoop class

mgorny accepted this revision. mgorny added a comment. This revision is now accepted and ready to land. LGTM. I've just rebased and tested my patches on top of this, and they seem to work. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131160/new/

[Lldb-commits] [PATCH] D133251: [lldb] [Core] Split read thread support into ThreadedCommunication

mgorny marked an inline comment as done. mgorny added inline comments. Comment at: lldb/source/Core/CMakeLists.txt:57 StreamFile.cpp + ThreadedCommunication.cpp UserSettingsController.cpp labath wrote: > I think you're missing this file. Indeed I were. Tha

[Lldb-commits] [PATCH] D133251: [lldb] [Core] Split read thread support into ThreadedCommunication

mgorny updated this revision to Diff 457984. mgorny added a comment. Add the missing file. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133251/new/ https://reviews.llvm.org/D133251 Files: lldb/include/lldb/API/SBCommunication.h lldb/include/lldb/Core/Communication.h lldb/include

[Lldb-commits] [PATCH] D132578: [lldb] [Core] Use thread for Communication::Write() as well

mgorny added a comment. In D132578#3770124 , @labath wrote: > Seems reasonable. Just be aware that the packet RTT is very important for > some, so we will either make sure that the extra thread is not used in the > base case (no processes running), or s

[Lldb-commits] [PATCH] D132578: [lldb] [Core] Use thread for Communication::Write() as well

mgorny added a comment. Ok, so I guess I'm back the original plan of using async thread to do the packet exchanges. Right now, the rough plan is to: 1. Try to clean up / streamline the comms API a bit. 2. Introduce a wrapper between the client and the actual comms to clearly split the public pa

[Lldb-commits] [PATCH] D133251: [lldb] [Core] Split read thread support into ThreadedCommunication

mgorny created this revision. mgorny added reviewers: labath, krytarowski, emaste, jingham. Herald added a subscriber: arichardson. Herald added a project: All. mgorny requested review of this revision. Split the read thread support from Communication into a dedicated ThreadedCommunication subclas

[Lldb-commits] [PATCH] D132578: [lldb] [Core] Use thread for Communication::Write() as well

mgorny added a comment. Well, my "baseline" idea was something like that: having a shared (e.g. via `shared_ptr`) `m_gdb_comm`, and running async thread as part of that. The thread would read packets from remote and split them into two groups: asynchronous events (i.e. stop reasons and related

[Lldb-commits] [PATCH] D131160: [WIP][lldb] Add "event" capability to the MainLoop class

mgorny added a comment. In D131160#3738805 , @labath wrote: > What's the reasoning behind `TriggerPendingCallbacks`? I was assuming that > the addition of a callback would cause it to run automatically... To be honest, I didn't think about it much. The

[Lldb-commits] [PATCH] D132577: [lldb] [Core] Pass error/status from Communication::ReadThread()

This revision was automatically updated to reflect the committed changes. Closed by commit rG39e0a87c26e0: [lldb] [Core] Pass error/status from Communication::ReadThread() (authored by mgorny). Herald added a project: LLDB. Changed prior to commit: https://reviews.llvm.org/D132577?vs=455950&id=

[Lldb-commits] [PATCH] D132283: [lldb] [Core] Reimplement Communication::ReadThread using MainLoop

mgorny added inline comments. Comment at: lldb/source/Core/Communication.cpp:427 // Notify the read thread. - m_connection_sp->InterruptRead(); labath wrote: > mgorny wrote: > > labath wrote: > > > Have you considered putting this code (some version of it)

[Lldb-commits] [PATCH] D132283: [lldb] [Core] Reimplement Communication::ReadThread using MainLoop

mgorny added inline comments. Comment at: lldb/source/Core/Communication.cpp:427 // Notify the read thread. - m_connection_sp->InterruptRead(); labath wrote: > Have you considered putting this code (some version of it) inside > `InterruptRead`? Basically r

[Lldb-commits] [PATCH] D132578: [lldb] [Core] Use thread for Communication::Write() as well

mgorny updated this revision to Diff 455952. mgorny added a comment. Rebase. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132578/new/ https://reviews.llvm.org/D132578 Files: lldb/include/lldb/Core/Communication.h lldb/source/Core/Communication.cpp lldb/unittests/Core/Communicati

[Lldb-commits] [PATCH] D132283: [lldb] [Core] Reimplement Communication::ReadThread using MainLoop

mgorny updated this revision to Diff 455951. mgorny added a comment. Rebase, move `StopReadThread()` test here. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132283/new/ https://reviews.llvm.org/D132283 Files: lldb/include/lldb/Core/Communication.h lldb/source/Core/Communication.cp

[Lldb-commits] [PATCH] D132577: [lldb] [Core] Pass error/status from Communication::ReadThread()

mgorny updated this revision to Diff 455950. mgorny added a comment. Rebase, move `StopReadThread()` test to the other patch. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132577/new/ https://reviews.llvm.org/D132577 Files: lldb/include/lldb/Core/Communication.h lldb/source/Core/Co

[Lldb-commits] [PATCH] D132578: [lldb] [Core] Use thread for Communication::Write() as well

mgorny added a comment. (with the final goal of using `StartReadThread()` inside gdb-remote comms to take care of reading the data for concurrent process plugin instances) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132578/new/ https://reviews.llvm.org/D132578 __

[Lldb-commits] [PATCH] D132578: [lldb] [Core] Use thread for Communication::Write() as well

mgorny added a comment. Well, back when working on the async thread, you've indicated that it's a bad idea to read from one thread while writing from another, so I've basically focused on getting to the point when all reads and writes are guaranteed to happen from a single thread. CHANGES SIN

[Lldb-commits] [PATCH] D132577: [lldb] [Core] Pass error/status from Communication::ReadThread()

mgorny added a comment. Hmm, I see you've updated the tests, so I need to update this patch anyway. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132577/new/ https://reviews.llvm.org/D132577 ___ lldb-commits mailing list lldb-commits@lists.ll

[Lldb-commits] [PATCH] D131160: [WIP][lldb] Add "event" capability to the MainLoop class

mgorny added a comment. In that case, is there something more I should do about this patch or are you going to take over from here? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D131160/new/ https://reviews.llvm.org/D131160 ___ lldb-commits m

[Lldb-commits] [PATCH] D132577: [lldb] [Core] Pass error/status from Communication::ReadThread()

mgorny added inline comments. Comment at: lldb/unittests/Core/CommunicationTest.cpp:140 + + // StopReadThread() can hang, so force an external timeout. + std::unique_lock lock{finished_mutex}; labath wrote: > I don't understand what is the purpose of this. Can

[Lldb-commits] [PATCH] D132578: [lldb] [Core] Use thread for Communication::Write() as well

mgorny updated this revision to Diff 455295. mgorny added a comment. Update to the version with error support. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132578/new/ https://reviews.llvm.org/D132578 Files: lldb/include/lldb/Core/Communication.h lldb/source/Core/Communication.cpp

[Lldb-commits] [PATCH] D132578: [lldb] [Core] Use thread for Communication::Write() as well

mgorny created this revision. mgorny added reviewers: labath, krytarowski, emaste, jingham. Herald added a subscriber: arichardson. Herald added a project: All. mgorny requested review of this revision. When read thread is enabled, perform all the writes in the thread as well. Most importantly, t

[Lldb-commits] [PATCH] D132283: [lldb] [Core] Reimplement Communication::ReadThread using MainLoop

mgorny updated this revision to Diff 455291. mgorny added a comment. Rebase/update for the new error passing mechanism. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132283/new/ https://reviews.llvm.org/D132283 Files: lldb/include/lldb/Core/Communication.h lldb/source/Core/Communic

[Lldb-commits] [PATCH] D132577: [lldb] [Core] Pass error/status from Communication::ReadThread()

mgorny created this revision. mgorny added reviewers: labath, emaste, krytarowski, jingham. Herald added a subscriber: arichardson. Herald added a project: All. mgorny requested review of this revision. Ensure that the ConnectionStatus and Status from Communication::ReadThread() is correctly passe

[Lldb-commits] [PATCH] D132395: [lldb] [gdb-remote] Use Communication::WriteAll() over Write()

mgorny added a comment. Yeah, I meant inside LLDB, since that's what we're talking about. Basically, I'm wondering if we should perhaps rename `WriteAll()` to `Write()`, remove the old `Write()` method and just use the looped variant everywhere (implying also via `SB*`). Repository: rG LLVM

[Lldb-commits] [PATCH] D132395: [lldb] [gdb-remote] Use Communication::WriteAll() over Write()

mgorny added a comment. Herald added a subscriber: JDevlieghere. In D132395#3742365 , @labath wrote: >> Ideally, we'd remove Write() from the public API. > > That might be a bit too extreme. Although I agree that the "All" version is > what one wants in

[Lldb-commits] [PATCH] D132395: [lldb] [gdb-remote] Use Communication::WriteAll() over Write()

This revision was automatically updated to reflect the committed changes. Closed by commit rG03b8f79048bf: [lldb] [gdb-remote] Use Communication::WriteAll() over Write() (authored by mgorny). Herald added a project: LLDB. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://

[Lldb-commits] [PATCH] D131159: [lldb] Use WSAEventSelect for MainLoop polling on windows

mgorny added a comment. In D131159#3742235 , @mstorsjo wrote: > (They're only used in asserts.) I guess we should add void casts to mark the > variables as used outside of asserts. Actually, there's a dedicated `UNUSED_IF_ASSERT_DISABLED` macro for that

[Lldb-commits] [PATCH] D132395: [lldb] [gdb-remote] Use Communication::WriteAll() over Write()

mgorny created this revision. mgorny added reviewers: labath, krytarowski, emaste, jingham. Herald added subscribers: kristof.beyls, arichardson. Herald added a project: All. mgorny requested review of this revision. Replace the uses of Communication::Write() with WriteAll() to avoid partial write

[Lldb-commits] [PATCH] D132283: [lldb] [Core] Reimplement Communication::ReadThread using MainLoop

mgorny updated this revision to Diff 454331. mgorny added a comment. Update `Communication::SynchronizeWithReadThread()`, fixing tests. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132283/new/ https://reviews.llvm.org/D132283 Files: lldb/include/lldb/Core/Communication.h lldb/sour

[Lldb-commits] [PATCH] D132283: [lldb] [Core] Reimplement Communication::ReadThread using MainLoop

mgorny planned changes to this revision. mgorny added a comment. Unfortunately, many tests are failing or timing out, so need to investigate more. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D132283/new/ https://reviews.llvm.org/D132283 ___

  1   2   3   4   5   6   7   8   9   10   >