[Lldb-commits] [PATCH] D158034: [lldb] Fix data race in ThreadList

labath added a comment. After this patch, python_api/run_locker/TestRunLocker.py becomes flaky. https://lab.llvm.org/buildbot/#/builders/68/builds/58456 is the first such failure, but there have been about a dozen failures since then. The backtraces on the buildbot page are fairly useless, but

[Lldb-commits] [PATCH] D156949: [lldb] Update LLDB Code Ownership

labath accepted this revision. labath added a comment. I've suggested some additional/alternative/backup (choose your interpretation) owners for the components I'm listed as the only owner (I only wish I could find someone to take over android). If they accept, then take this as my endorsement

[Lldb-commits] [PATCH] D155117: Platform qemu-user: Build path to qemu automatically if not specified

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. In D155117#4510512 , @ted wrote: > In D155117#4505538 , @labath wrote: > >> I am wondering if we actually

[Lldb-commits] [PATCH] D155117: Platform qemu-user: Build path to qemu automatically if not specified

labath added a comment. I am wondering if we actually need the second step (the architecture setting) here. The main reason it exists is the usage in `GetSupportedArchitectures` (which is called before a target is created) it seems like the value derived from the target should always be more

[Lldb-commits] [PATCH] D153636: [LLDB] Fix the use of "platform process launch" with no extra arguments

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Thanks for fixing this. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153636/new/ https://reviews.llvm.org/D153636

[Lldb-commits] [PATCH] D152364: [lldb] Rate limit progress reports -- different approach [WIP-ish]

labath reopened this revision. labath added a comment. :( Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152364/new/ https://reviews.llvm.org/D152364 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D152364: [lldb] Rate limit progress reports -- different approach [WIP-ish]

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rGc30853460da7: [lldb] Rate limit progress reports -- different approach [WIP-ish] (authored by labath). Repository: rG

[Lldb-commits] [PATCH] D152364: [lldb] Rate limit progress reports -- different approach [WIP-ish]

labath added a comment. Actually, thinking about these "unique" events, I think it would be worth trying out sending the progress update events as "unique". Depending on where exactly in the pipeline the congestion happens, it might just be enough to fix the immediate problem. If the slow

[Lldb-commits] [PATCH] D152364: [lldb] Rate limit progress reports -- different approach [WIP-ish]

labath added a comment. In D152364#4406589 , @JDevlieghere wrote: > Continuing some of the discussion from D150805 > here as it mostly relates to this patch: > > In D150805#4402906

[Lldb-commits] [PATCH] D150805: Rate limit progress reporting

labath added a comment. I agree that we should have a rate limiting mechanism very close to the source, to avoid wasting work for events that aren't going to be used. This is particularly important for debug info parsing, where we have multiple threads working in parallel and taking a lock

[Lldb-commits] [PATCH] D152364: [lldb] Rate limit progress reports -- different approach [WIP-ish]

labath created this revision. labath added reviewers: JDevlieghere, saugustine, rupprecht. Herald added a project: All. labath requested review of this revision. Herald added a project: LLDB. Have the Progress class spawn a thread to periodically send progress reports. The reporting period could

[Lldb-commits] [PATCH] D149949: [lldb][TypeSystem] ForEach: Don't hold the TypeSystemMap lock across callback

labath added a comment. Just as a drive-by, since we're constructing a copy anyway it would be possible to merge this construction with the "uniqueing" behavior of the `visited` set. I.e., instead of making a literal copy of the map, just construct a `to_be_visited` set of /unique/ objects,

[Lldb-commits] [PATCH] D149697: [lldb] Remove distribution_id from ArchSpec

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. FWIW, I'd be very much in favour of removing distribution_id support altogether. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D149697/new/

[Lldb-commits] [PATCH] D146058: [lldb][gnustep] Add basic test and infrastructure for GNUstep ObjC runtime

labath added a comment. In D146058#4224001 , @sgraenitz wrote: > In D146058#4223575 , @labath wrote: > >> What kind of setup is necessary to make that work? If it's not too >> complicated, I think we could make

[Lldb-commits] [PATCH] D147831: [lldb-vscode] Implement RestartRequest

labath added a comment. Looks ok-ish, though I don't feel entirely comfortable approving lldb-vscode changes. @wallace, do you have any thoughts on this? Comment at: lldb/tools/lldb-vscode/VSCode.h:152 bool is_attach; + // The process event thread normally responds to

[Lldb-commits] [PATCH] D147642: [lldb][ObjectFileELF] Support AArch32 in ApplyRelocations

labath accepted this revision. labath added inline comments. This revision is now accepted and ready to land. Comment at: lldb/source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:2663 +// the actual range check below. +if (addend < 0 && static_cast(std::abs(addend)) > value)

[Lldb-commits] [PATCH] D149214: [lldb] Speed up DebugAbbrev parsing

labath added a comment. I guess the most efficient (performance- and memory-wise) approach would be to have a global (well, scoped to a DWARFUnit or something) array of DWARFAttribute objects, and have the individual abbreviations just store pointers/indexes to that. Repository: rG LLVM

[Lldb-commits] [PATCH] D148662: [lldb] Make the libcxx unique_ptr prettyprinter support custom deleters.

labath accepted this revision. labath added inline comments. This revision is now accepted and ready to land. Comment at: lldb/source/Plugins/Language/CPlusPlus/LibCxx.cpp:52 +ValueObject ) { + ValueObjectSP value = pair.GetChildAtIndex(0, true)->GetChildMemberWithName( +

[Lldb-commits] [PATCH] D148062: [lldb] Make ObjectFileJSON loadable as a module

labath added inline comments. Comment at: lldb/test/API/macosx/symbols/TestObjectFileJSON.py:82 +} +self.emitJSON(data, json_object_file) + The test was (95%!) flaky because the two generated files had the same timestamp (and so lldb considered

[Lldb-commits] [PATCH] D147606: [lldb] fix #61942, discard "empty" ranges in DWARF to better handle gcc binary

labath added a comment. In D147606#4249283 , @JDevlieghere wrote: > In D147606#4247462 , @jwnhy wrote: > >> In D147606#4246832 , @JDevlieghere >> wrote: >> >>> The

[Lldb-commits] [PATCH] D147045: [lldb] Drop RegisterInfoInterface::GetDynamicRegisterInfo

This revision was automatically updated to reflect the committed changes. Closed by commit rG933d3ee60007: [lldb] Drop RegisterInfoInterface::GetDynamicRegisterInfo (authored by labath). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147045/new/

[Lldb-commits] [PATCH] D141605: [lldb] Detach the child process when stepping over a fork

This revision was automatically updated to reflect the committed changes. Closed by commit rGaf9e1fa17843: [lldb] Detach the child process when stepping over a fork (authored by labath). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141605/new/

[Lldb-commits] [PATCH] D146977: [lldb-server/linux] Use waitpid(-1) to collect inferior events

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

[Lldb-commits] [PATCH] D146977: [lldb-server/linux] Use waitpid(-1) to collect inferior events

This revision was automatically updated to reflect the committed changes. Closed by commit rGe64cc756819d: [lldb-server/linux] Use waitpid(-1) to collect inferior events (authored by labath). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146977/new/

[Lldb-commits] [PATCH] D147084: [lldb] Move ObjectFileJIT to lldbExpression

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Makes sense to me. Thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D147084/new/ https://reviews.llvm.org/D147084

[Lldb-commits] [PATCH] D146668: [lldb-server] Use Platform plugin corresponding to the host

labath added a comment. Thank you for your understanding. Let me know if you need anything from me. In the long-term I'd like to factor this signal code such that there is a single common code which maps all signals (or at least the most common ones) to their default disposition, description

[Lldb-commits] [PATCH] D147045: [lldb] Drop RegisterInfoInterface::GetDynamicRegisterInfo

labath created this revision. labath added a reviewer: DavidSpickett. Herald added a subscriber: pengfei. Herald added a project: All. labath requested review of this revision. Herald added a project: LLDB. "Dynamic register info" is a very overloaded term, and this particular instance of it was

[Lldb-commits] [PATCH] D147009: [lldb] Remove lldbExpression's dependency on ObjectFileJIT

labath added a comment. I already regret getting involved in this, but here it goes... Do you actually expect we will have multiple ObjectFile subclasses implementing the `ObjectFileCreateInstanceWithDelegate` interface? Because if not, then I think this is just a very roundabout way of being

[Lldb-commits] [PATCH] D146977: [lldb-server/linux] Use waitpid(-1) to collect inferior events

labath marked 2 inline comments as done. labath added inline comments. Comment at: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp:376-386 +bool handled = llvm::any_of(m_processes, [&](NativeProcessLinux *process) { + return process->TryHandleWaitStatus(pid,

[Lldb-commits] [PATCH] D146977: [lldb-server/linux] Use waitpid(-1) to collect inferior events

labath updated this revision to Diff 508957. labath marked an inline comment as done. labath added a comment. Address review comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146977/new/ https://reviews.llvm.org/D146977 Files:

[Lldb-commits] [PATCH] D146977: [lldb-server/linux] Use waitpid(-1) to collect inferior events

labath created this revision. labath added reviewers: DavidSpickett, yinghuitan. Herald added a subscriber: emaste. Herald added a project: All. labath requested review of this revision. Herald added a project: LLDB. This is a follow-up to D116372 , which had a

[Lldb-commits] [PATCH] D146058: [lldb][gnustep] Add basic test and infrastructure for GNUstep ObjC runtime

labath added a comment. In D146058#4193594 , @sgraenitz wrote: >> Pavel runs the x86 debian bot and I think the x86 Windows bot may have been >> retired, or at least is offline at the moment >> (https://lab.llvm.org/buildbot/#/builders/83) > > Yes,

[Lldb-commits] [PATCH] D146590: [lldb] Update some uses of Python2 API in typemaps.

labath added a comment. In D146590#4220388 , @jgorbe wrote: > I believe (but I don't have any experience with SWIG typemaps so this is an > educated guess) that the call to `free` in the error path comes from the > `%typemap(freearg)` immediately after

[Lldb-commits] [PATCH] D144240: Clear read_fd_set if EINTR received

labath added a comment. Yes, that is the correct process. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144240/new/ https://reviews.llvm.org/D144240 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D146668: [lldb-server] Use Platform plugin corresponding to the host

labath added a comment. I'm sorry, but I don't think this is a good change (which I guess means the previous one is not good either, but this one drives the point). I really don't want lldb-server to depend on a platform plugins . I have two reasons for that: - lldb-server is always running on

[Lldb-commits] [PATCH] D145450: [lldb] Respect empty arguments in target.run-args

labath added inline comments. Comment at: lldb/source/Host/common/ProcessLaunchInfo.cpp:324-325 std::string safe_arg = Args::GetShellSafeArgument(m_shell, argv[i]); + if (safe_arg.empty()) +safe_arg = "\"\""; // Add a space to separate

[Lldb-commits] [PATCH] D146044: [lldb] Implement CrashReason using UnixSignals

labath added a comment. It's not exactly what I had in mind, but I kinda like it :) Comment at: lldb/include/lldb/Target/UnixSignals.h:127 + struct SignalCode { +ConstString m_description; +SignalCodePrintOption m_print_option; I think we should just

[Lldb-commits] [PATCH] D145955: Refactor ObjectFilePlaceholder for sharing

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

[Lldb-commits] [PATCH] D144392: [lldb] Skip signal handlers for ignored signals on lldb-server's side when stepping

labath added a comment. In D144392#4188940 , @kpdev42 wrote: > Gdb does this for example: > >> GDB optimizes for stepping the mainline code. If a signal that has handle >> nostop and handle pass set arrives while a stepping command (e.g., stepi, >>

[Lldb-commits] [PATCH] D145180: [lldb] Introduce new SymbolFileJSON and ObjectFileJSON

labath added inline comments. Comment at: lldb/source/Plugins/SymbolFile/JSON/SymbolFileJSON.cpp:112 +symtab.AddSymbol(Symbol( +/*symID*/ 0, Mangled(symbol.name), eSymbolTypeCode, +/*is_global*/ true, /*is_debug*/ false, clayborg wrote: >

[Lldb-commits] [PATCH] D145450: [lldb] Respect empty arguments in target.run-args

labath added inline comments. Comment at: lldb/source/Host/common/ProcessLaunchInfo.cpp:324-325 std::string safe_arg = Args::GetShellSafeArgument(m_shell, argv[i]); + if (safe_arg.empty()) +safe_arg = "\"\""; // Add a space to separate

[Lldb-commits] [PATCH] D145487: [lldb][test] TestDataFormatterCpp.py: add test-case for member function pointer format

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. In D145487#4174832 , @Michael137 wrote: > - Remove virtual function pointer test for now You can keep it in, I think. Just don't assert the value of

[Lldb-commits] [PATCH] D145487: [lldb][test] TestDataFormatterCpp.py: add test-case for member function pointer format

labath added inline comments. Comment at: lldb/test/API/functionalities/data-formatter/data-formatter-cpp/TestDataFormatterCpp.py:301 +"frame variable virt_member_func_ptr", +patterns=['virt_member_func_ptr = ([0-9]{2}\s)+01']) If [[

[Lldb-commits] [PATCH] D145242: [lldb][TypeSystemClang] Use the CXXFunctionPointerSummaryProvider for member-function pointers

labath added a comment. You may want to check that pointers to virtual functions get printed here. In particular, that `0x0001` (pointer to the first virtual function) does not get printed as `nullptr` or something like that (due to the truncation of the 128-bit

[Lldb-commits] [PATCH] D145136: Add a Debugger interruption mechanism in parallel to the Command Interpreter interruption

labath added a comment. I kinda like this. Comment at: lldb/include/lldb/Core/Debugger.h:563 + // the previous IOHandler thread. + const HostThread SetIOHandlerThread(HostThread _thread); `const` on a return value rarely makes sense. OTOH, `const` on the

[Lldb-commits] [PATCH] D145377: [LLDB] Show sub type of signals for ELF core files

labath added inline comments. Comment at: lldb/source/Plugins/Process/elf-core/ThreadElfCore.cpp:230 + + siginfo_t info; + info.si_signo = m_signo; I think this won't work on Windows (no siginfo_t type, nor constants to decode it) -- and it may return garbage

[Lldb-commits] [PATCH] D143104: [lldb/Plugins] Add Attach capabilities to ScriptedProcess

labath added inline comments. Comment at: lldb/source/Utility/ProcessInfo.cpp:11 +#include "lldb/Interpreter/ScriptedMetadata.h" #include "lldb/Utility/ArchSpec.h" This is a layering violation. Can we restructure this so that it this does not depend on code

[Lldb-commits] [PATCH] D144240: Clear read_fd_set if EINTR received

This revision was automatically updated to reflect the committed changes. Closed by commit rGd8bd179a1738: Clear read_fd_set if EINTR received (authored by emrekultursay, committed by labath). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D144240: Clear read_fd_set if EINTR received

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Nice catch. As discussed internally, it would be great if we could remove this android-specific code path (by removing support for OS versions which necessitated it). Repository: rG LLVM

[Lldb-commits] [PATCH] D144224: [lldb] Extend SWIG SBProcess interface with WriteMemoryAsCString method

labath added inline comments. Comment at: lldb/bindings/interface/SBProcessExtensions.i:11 +''' +if not str[-1] == '\0': +str += '\0' I think this will fail if the string is empty (`""`). Somewhat relatedly, what if this

[Lldb-commits] [PATCH] D142926: [lldb] Replace SB swig interfaces with API headers

labath added a comment. In D142926#4124297 , @bulbazord wrote: > In D142926#4123492 , @labath wrote: > >> Well... right now, there isn't anything else to do there. >> >> Anyway, I don't think this is particularly

[Lldb-commits] [PATCH] D144392: [lldb] Skip signal handlers for ignored signals on lldb-server's side when stepping

labath added subscribers: jingham, labath. labath requested changes to this revision. labath added a reviewer: jingham. labath added a comment. This revision now requires changes to proceed. I'm afraid you're fixing this at the wrong end. This kind of complex thread control does not belong

[Lldb-commits] [PATCH] D143698: Support Debugging TLS variable with lldb

labath added a comment. There's one thing that's not clear to me about this patch. What is the relationship between `qGetTLSAddr`, as implemented here, and its implementation in GDB? I guess they're not compatible since we're not sending or using the offset and link map fields, but it's not

[Lldb-commits] [PATCH] D142926: [lldb] Replace SB swig interfaces with API headers

labath added a comment. In D142926#4103185 , @bulbazord wrote: > In D142926#4102287 , @labath wrote: > >> I like this. >> >> I'm not sure what it would take, but I think it'd be nice if the >> de-swig-ification

[Lldb-commits] [PATCH] D143652: [lldb][DWARFASTParserClang] Attach linkage name to ctors/dtors if missing

labath added inline comments. Comment at: lldb/test/API/lang/cpp/external_ctor_dtor_lookup/TestExternalCtorDtorLookup.py:28 +# CHECK: |-CXXConstructorDecl {{.*}} Wrapper 'void ()' +# CHECK-NEXT: | `-AsmLabelAttr {{.*}} Implicit "_ZN7WrapperI3FooEC1B4testEv" +#

[Lldb-commits] [PATCH] D143652: [lldb][DWARFASTParserClang] Attach linkage name to ctors/dtors if missing

labath added inline comments. Comment at: lldb/test/API/lang/cpp/external_ctor_dtor_lookup/TestExternalCtorDtorLookup.py:28 +# CHECK: |-CXXConstructorDecl {{.*}} Wrapper 'void ()' +# CHECK-NEXT: | `-AsmLabelAttr {{.*}} Implicit "_ZN7WrapperI3FooEC1B4testEv" +#

[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

labath added a comment. Thank you for your patience. I'm really happy with the overall result here. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138618/new/ https://reviews.llvm.org/D138618 ___

[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

labath accepted this revision. labath added inline comments. This revision is now accepted and ready to land. Comment at: lldb/source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp:402 DWARFUnit , llvm::function_ref callback) { -

[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

labath added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DIERef.h:24 +/// set, +/// the DIERef references the main, dwo or .o file. /// - section: identifies the section of the debug info entry in the given file: I don't understand this

[Lldb-commits] [PATCH] D142926: [lldb][RFC] Replace SB swig interfaces with API headers

labath added a comment. I like this. I'm not sure what it would take, but I think it'd be nice if the de-swig-ification was not specific to the framework build. Ideally, I'd make it controlled by a separate cmake variable, (autodetected to on if the relevant tool is found, but also

[Lldb-commits] [PATCH] D142309: [LLDB][NFC] Fix a incorrect use of shared_ptr in RenderScriptRuntime.cpp

labath added a subscriber: srhines. labath added a comment. In D142309#4075745 , @JDevlieghere wrote: > LGTM, but I wonder when we'll be able to get rid of RenderScript completely > (CC @labath) I am not going to stand in your way. I usually redirect

[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

labath added a comment. In D138618#4094933 , @clayborg wrote: >> - My main source of frustration was that my concern is getting >> overlooked/ignored (not necessarily your fault -- I've been told I am not >> always sufficiently clear). I think that is

[Lldb-commits] [PATCH] D143127: [LLDB] Fix assertion failure by removing `CopyType` in `std::coroutine_handle` pretty printer

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. Well.. I don't believe we have any ASTImporter calls in any of the other pretty printers, so I think this change is good. And if it causes some other issues, then maybe we need to look at

[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

labath added a comment. In D138618#4092040 , @clayborg wrote: > How about we make DIERef constructor always take all the info that is needed > to construct the objects correctly: > > DIERef(DWARFDie die); > DIERef(SymbolFileDWARF *dwarf, dw_offset_t

[Lldb-commits] [PATCH] D142672: [lldb] Make SBSection::GetSectionData call Section::GetSectionData.

labath added a comment. I'm sorry Omair, my comment was meant for Jorge, not you. `@skipIfXml` is not the right decorator here, although it will probably kinda fix the failure (because the bot does not have xml support either). I *think* the compressed section support is controlled by presence

[Lldb-commits] [PATCH] D142672: [lldb] Make SBSection::GetSectionData call Section::GetSectionData.

labath added a comment. I'm pretty sure that's because that bot builds without zlib support. You'll need to add something like `@skipIfXmlSupportMissing` to this test (I think we don't have a decorator for zlib now, so you'll probably need to add one). While you're at it, you might as well

[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

labath added a comment. I'm sorry, but that patch does not fix the problem I am trying to point out. In fact, I think it makes things a lot worse. We clearly have some kind of a communication problem, but I am running out of ideas of what can I do about it. Let me try rephrasing it one more

[Lldb-commits] [PATCH] D142709: [lldb][Target] GetScratchTypeSystems: sort TypeSystems with strict weak ordering

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

[Lldb-commits] [PATCH] D139957: [LLDB] Change OSO to use DieRef

labath added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DIERef.h:32 - DIERef(std::optional dwo_num, Section section, + DIERef(std::optional dwo_oso_num, Section section, dw_offset_t die_offset) ayermolo wrote: > labath wrote:

[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

labath added a comment. In D138618#4083481 , @clayborg wrote: > We just need to create all DIERef objects using the GetID() from the symbol > file as the file index, and we should be able to remove the > SymbolFile::GetUID() function now. As long as

[Lldb-commits] [PATCH] D142266: [lldb] Add PlatformMetadata for ScriptedPlatform

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. I wouldn't exactly use the word happy, but I also don't have the time to come up with an alternative proposal. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

labath added a comment. In D138618#4077851 , @ayermolo wrote: > In D138618#4073329 , @labath wrote: > >> I think that the 1-based index thingy helps a lot here, but I haven't seen >> anything (in your reponse,

[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

labath added a comment. In D138618#4060565 , @clayborg wrote: > ... > Since the user IDs of SymbolFileDWARF plug-ins mean nothing to anyone else, > we can make them what we need them to be so they work for us. I would suggest > to remove the use of

[Lldb-commits] [PATCH] D139957: [LLDB] Change OSO to use DieRef

labath added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DIERef.h:32 - DIERef(std::optional dwo_num, Section section, + DIERef(std::optional dwo_oso_num, Section section, dw_offset_t die_offset) Where is this constructor being

[Lldb-commits] [PATCH] D141605: [lldb] Detach the child process when stepping over a fork

labath added a comment. Thanks for your response, Jim. In D141605#4066649 , @jingham wrote: > The part of handling the fork where we decide we're going to follow the child > and so we need to switch the process PID & TID does have to happen on event >

[Lldb-commits] [PATCH] D139249: [lldb] Add Debugger & ScriptedMetadata reference to Platform::CreateInstance

labath added a comment. In addition to that, it breaks tests with ubsan. :) I raised a similar concern in one of the other patches. I'm of the opinion that it's not worth pretending like the scripted processes (platforms, etc.) are plugins. I don't think that it can be achieved without some

[Lldb-commits] [PATCH] D68655: Trust the arange accelerator tables in dSYMs

labath added a comment. In D68655#4048009 , @jasonmolenda wrote: > In D68655#4047126 , @labath wrote: > >> In D68655#4045895 , @jasonmolenda >> wrote: >> >>> Both have to

[Lldb-commits] [PATCH] D141605: [lldb] Detach the child process when stepping over a fork

labath added a comment. (The fix seems to make LLDB do the right thing, although I can't claim to fully understand what is going on. From what I can tell, we're currently detaching from the fork child when the fork event gets pulled off the public event queue, and this code is preventing that

[Lldb-commits] [PATCH] D141605: [lldb] Detach the child process when stepping over a fork

labath created this revision. labath added reviewers: jingham, mgorny. Herald added a project: All. labath requested review of this revision. Herald added a project: LLDB. Step over thread plans were claiming to explain the fork stop reasons, which prevented the default fork logic (detaching from

[Lldb-commits] [PATCH] D139250: [lldb] Add ScriptedPlatform python implementation

labath accepted this revision. labath added inline comments. This revision is now accepted and ready to land. Comment at: lldb/examples/python/scripted_process/scripted_platform.py:31 +def list_processes(self): +""" Get a list of processes that can be ran on the

[Lldb-commits] [PATCH] D141330: [lldb] Limit 8b259fe573e1 to dSYMs

labath added a comment. Just to clarify: I was responding to the question about the ELF file usage, not questioning the overall approach. Given that MachO has a special code for dsym files, I think it makes sense to use it. In D141330#4043807 ,

[Lldb-commits] [PATCH] D141021: [lldb] Remove tools copied into LLDB.framework before install

labath added a comment. I don't dispute the value of determinism, but it seems like there ought to be a way to achieve this with corrupting the build tree (which in itself is not very 'deterministic'). What if we had two copies of the framework in the build-tree? One pristine copy, which would

[Lldb-commits] [PATCH] D68655: Trust the arange accelerator tables in dSYMs

labath added a comment. In D68655#4045895 , @jasonmolenda wrote: > Both have to be written by the dwarf linker to be correct, but only the > former is written ONLY by the dwarf linker. I don't think that's right: $ clang -c -x c - -o -

[Lldb-commits] [PATCH] D139250: [lldb] Add ScriptedPlatform python implementation

labath added inline comments. Comment at: lldb/examples/python/scripted_process/scripted_platform.py:64 +def launch_process(self, launch_info): +""" Launch a scripted process. + Does this really have to be a scripted process? Ideally, one could also

[Lldb-commits] [PATCH] D141021: [lldb] Remove tools copied into LLDB.framework before install

labath added a comment. Does this mean that the in-tree lldb will cease to be functional once someone "installs" it? (at least until the next rebuild) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D141021/new/ https://reviews.llvm.org/D141021

[Lldb-commits] [PATCH] D141330: [lldb] Limit 8b259fe573e1 to dSYMs

labath added a comment. In D141330#4037932 , @JDevlieghere wrote: > In D141330#4037925 , @aprantl wrote: > >> Should this be true for a fully linked ELF executable, too? > > Sounds reasonable, but adding @labath

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

labath accepted this revision. labath added a comment. I'm sorry for the delay, I was out for the holidays. Looks now, thanks for your patience. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139955/new/ https://reviews.llvm.org/D139955

[Lldb-commits] [PATCH] D138344: [test][lldb-vscode] Relax assertion to allow multiple compile units returned.

labath accepted this revision. labath 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/D138344/new/ https://reviews.llvm.org/D138344

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

labath added a comment. This is looking much better -- and focused. I still have comments though. :) Comment at: lldb/include/lldb/Core/Module.h:833 - void LogMessageVerboseBacktrace(Log *log, const char *format, ...) - __attribute__((format(printf, 3, 4))); + void

[Lldb-commits] [PATCH] D139945: [lldb] Add scripted process launch/attach option to platform commands

labath added a comment. In D139945#4009427 , @JDevlieghere wrote: > In D139945#3999351 , @labath wrote: > >> For a "plugin", the scripted process is sure getting a lot of special >> handling in generic code. (I

[Lldb-commits] [PATCH] D139250: [lldb] Add ScriptedPlatform python implementation

labath added inline comments. Comment at: lldb/examples/python/scripted_process/scripted_platform.py:31 +def list_processes(self): +""" Get a list of processes that can be ran on the platform. + mib wrote: > labath wrote: > > mib wrote: > > > labath

[Lldb-commits] [PATCH] D139250: [lldb] Add ScriptedPlatform python implementation

labath added inline comments. Comment at: lldb/examples/python/scripted_process/scripted_platform.py:31 +def list_processes(self): +""" Get a list of processes that can be ran on the platform. + mib wrote: > labath wrote: > > mib wrote: > > > mib

[Lldb-commits] [PATCH] D138618: [LLDB] Enable 64 bit debug/type offset

labath added a comment. I think that the main reason we've arrived at such different conclusions is that I treat the "user IDs of DIEs" and and "user IDs of symbol files" as essentially two different things (namespaces if you will). Obviously, one needs the symbol file ID in order to compute

[Lldb-commits] [PATCH] D140262: [lldb][TypeSystemClang][NFC] Introduce TemplateParameterInfos::ArgumentMetadata

labath added inline comments. Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h:334 +struct ArgumentMetadata { + char const *const name = nullptr; +}; Michael137 wrote: > Michael137 wrote: > > labath wrote: > > > This may depend on

[Lldb-commits] [PATCH] D140262: [lldb][TypeSystemClang][NFC] Introduce TemplateParameterInfos::ArgumentMetadata

labath accepted this revision. labath added inline comments. This revision is now accepted and ready to land. Comment at: lldb/source/Plugins/TypeSystem/Clang/TypeSystemClang.h:334 +struct ArgumentMetadata { + char const *const name = nullptr; +};

[Lldb-commits] [PATCH] D140030: [lldb][TypeSystemClang][NFC] Make TemplateParameterInfos members private

labath accepted this revision. labath added a comment. That looks great. I have just one small nit. I think the test would read better if you made the names of temporary `TemplateArgument` objects more descriptive -- i.e. encode the kind and value information in the name. For example.

[Lldb-commits] [PATCH] D139955: [LLDB] Change formatting to use llvm::formatv

labath added a comment. In D139955#3999381 , @ayermolo wrote: > I tried to modify all the places wehre getOffset() is used. Which will later > return 64bit instead of 32 bit. > > Sorry I guess there was miss communication. > I am not quite clear in what

[Lldb-commits] [PATCH] D139248: [lldb/Interpreter] Improve ScriptedPythonInterface::GetStatusFromMethod

labath added inline comments. Comment at: lldb/source/Plugins/ScriptInterpreter/Python/ScriptedPythonInterface.h:105 +Status error; +Dispatch(method_name, error, args...); + `std::forward(args)...` maybe? Repository: rG LLVM Github Monorepo CHANGES

[Lldb-commits] [PATCH] D139249: [lldb] Add Debugger & ScriptedMetadata reference to Platform::CreateInstance

labath added inline comments. Comment at: lldb/include/lldb/Interpreter/OptionGroupPlatform.h:73 bool m_include_platform_option; + OptionGroupPythonClassWithDict m_class_options; }; labath wrote: > These nested groups are fairly unusual? Couldn't

[Lldb-commits] [PATCH] D139249: [lldb] Add Debugger & ScriptedMetadata reference to Platform::CreateInstance

labath added inline comments. Comment at: lldb/include/lldb/Interpreter/OptionGroupPlatform.h:73 bool m_include_platform_option; + OptionGroupPythonClassWithDict m_class_options; }; These nested groups are fairly unusual? Couldn't

  1   2   3   4   5   6   7   8   9   10   >