[Lldb-commits] [PATCH] D157556: Replace the singleton "ShadowListener" with a primary and N secondary listeners

clayborg added inline comments. Comment at: lldb/source/Target/Process.cpp:372 +const int Process::g_all_event_bits = eBroadcastBitStateChanged + | eBroadcastBitInterrupt mib wrote: > nit: this could probably be `constexpr` yes, th

[Lldb-commits] [PATCH] D157609: [lldb] Search debug file paths when looking for split DWARF files

clayborg added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1737 + if (!dwo_file.IsRelative()) { +found = FileSystem::Instance().Exists(dwo_file); + } else { Maybe we can also check the debug info search paths to see

[Lldb-commits] [PATCH] D157609: [lldb] Search debug file paths when looking for split DWARF files

clayborg added a comment. We are also looking for a way to locate .dwo files at Meta. One idea we came up with is to add a setting that can be set: (lldb) settings set symbols.locate-dwo-script /path/to/locate-dwo.py Any specified scripts would be expect to contain a python function: def l

[Lldb-commits] [PATCH] D157960: [lldb][gui] Update TreeItem children's m_parent on move

clayborg accepted this revision. clayborg added inline comments. This revision is now accepted and ready to land. Comment at: lldb/test/API/commands/gui/spawn-threads/TestGuiSpawnThreads.py:38 +for i in range(5): +# Stopped at the breakpoit, continue over the

[Lldb-commits] [PATCH] D158182: [lldb] Try to find relative DWO files by file name only, as a last resort

clayborg requested changes to this revision. clayborg added inline comments. This revision now requires changes to proceed. Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1784-1804 + FileSpec spec_to_get_name(dwo_name); + llvm::StringRef filename_o

[Lldb-commits] [PATCH] D157609: [lldb] Search debug file paths when looking for split DWARF files

clayborg added a comment. Just one question if we should still try all of your new code if we have a full path to a DW_AT_comp_dir, but we don't find the .dwo file, should be allow your code to run and still try to just append the relative "dwo_name" (without comp dir) to each of the search pat

[Lldb-commits] [PATCH] D155905: lldb RFC: Exposing set/get address masks, Fix*Address methods in SBProcess

clayborg accepted this revision. clayborg added a comment. Looks good Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155905/new/ https://reviews.llvm.org/D155905 ___ lldb-commits mailing list lldb-commits

[Lldb-commits] [PATCH] D158583: Fix shared library loading when users define duplicate _r_debug structure.

clayborg created this revision. clayborg added reviewers: labath, JDevlieghere, GeorgeHuyubo, yinghuitan, kusmour. Herald added a project: All. clayborg requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. We ran into a case where shared librar

[Lldb-commits] [PATCH] D158583: Fix shared library loading when users define duplicate _r_debug structure.

clayborg added a comment. In D158583#4609320 , @DavidSpickett wrote: > I'm not familiar with this mechanism but just out of curiosity: `ld.so loads > the main executable and any dependent shared libraries and wants to update > the "_r_debug" structure,

[Lldb-commits] [PATCH] D158583: Fix shared library loading when users define duplicate _r_debug structure.

clayborg updated this revision to Diff 552966. clayborg added a comment. Address review comments: - Added documentation in DYLDRendezvous.h to explain how things are supposed to work - Updated the documentation in DYLDRendezvous::GetAction() to explain why we need to work around seeing two cons

[Lldb-commits] [PATCH] D158583: Fix shared library loading when users define duplicate _r_debug structure.

clayborg updated this revision to Diff 553259. clayborg added a comment. Fixed more header documentation and cleaned up the test even more. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158583/new/ https://reviews.llvm.org/D158583 Files: lldb/so

[Lldb-commits] [PATCH] D158583: Fix shared library loading when users define duplicate _r_debug structure.

clayborg added a comment. Anyone have any objections? Super easy to repro this bug with the test program. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D158583/new/ https://reviews.llvm.org/D158583 ___ ll

[Lldb-commits] [PATCH] D158583: Fix shared library loading when users define duplicate _r_debug structure.

clayborg added a comment. In D158583#4627644 , @DavidSpickett wrote: > The added context helps document what was already there so that's a nice > improvement. > > Something I'm not clear on mechanically. The original r_debug has eAdd set. > Then the se

[Lldb-commits] [PATCH] D158583: Fix shared library loading when users define duplicate _r_debug structure.

This revision was automatically updated to reflect the committed changes. Closed by commit rG07c215e8a8af: Fix shared library loading when users define duplicate _r_debug structure. (authored by clayborg). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org

[Lldb-commits] [PATCH] D159164: [lldb] Add assembly syntax highlighting

clayborg added a comment. We shouldn't need to pass in "bool use_color" to the Disassembler creation functions as the only reason to pass something to the creation function for any plug-in is if that data would exclude a disassembler if it didn't support color. But we always want to see some di

[Lldb-commits] [PATCH] D159164: [lldb] Add assembly syntax highlighting

clayborg added a comment. Yes, this simplifies the patch a lot. Looks good after we check the mnemonic and comment for no color when used through the SBInstruction APIs. Comment at: lldb/test/API/python_api/disassemble-raw-data/TestDisassembleRawData.py:69 +ci.Han

[Lldb-commits] [PATCH] D157609: [lldb] Add more ways to find split DWARF files

clayborg added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1801 +FileSpecList dwo_paths; +FileSpec spec_to_get_name(dwo_name); +llvm::StringRef filename_only = spec_to_get_name.GetFilename(); Rename to "dwo_na

[Lldb-commits] [PATCH] D159164: [lldb] Add assembly syntax highlighting

clayborg added a comment. lgtm. Is there any more things you want to add to this patch test wise, or is this complete? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159164/new/ https://reviews.llvm.org/D159164 ___ lldb-commits mailing list l

[Lldb-commits] [PATCH] D159164: [lldb] Add assembly syntax highlighting

clayborg added inline comments. Comment at: lldb/include/lldb/Core/Disassembler.h:158 bool show_bytes, bool show_control_flow_kind, -const ExecutionContext *exe_ctx, +bool show_color, const ExecutionContext *exe_ctx,

[Lldb-commits] [PATCH] D159164: [lldb] Add assembly syntax highlighting

clayborg added a comment. Just one last inline comment where we can't use the colorized string to calculate the column width and this is good to go. Comment at: lldb/source/Core/Disassembler.cpp:662-663 // consistent column spacing in these cases, unfortunately. - if (m_op

[Lldb-commits] [PATCH] D159164: [lldb] Add assembly syntax highlighting

clayborg added inline comments. Comment at: lldb/source/Core/Disassembler.cpp:662-663 // consistent column spacing in these cases, unfortunately. - if (m_opcode_name.length() >= opcode_column_width) { -opcode_column_width = m_opcode_name.length() + 1; + if (opcode_name.l

[Lldb-commits] [PATCH] D159408: Switch over to using the LLVM archive parser for BSD archives.

clayborg created this revision. clayborg added reviewers: labath, JDevlieghere, GeorgeHuyubo, yinghuitan, kusmour. Herald added a project: All. clayborg requested review of this revision. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Our LLDB parser didn't correctly handl

[Lldb-commits] [PATCH] D157609: [lldb] Add more ways to find split DWARF files

clayborg added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1747-1754 if (!comp_dir) { unit.SetDwoError(Status::createWithFormat( "unable to locate relative .dwo debug file \"{0}\" for " "skeleton DIE {1:x

[Lldb-commits] [PATCH] D159408: Switch over to using the LLVM archive parser for BSD archives.

clayborg updated this revision to Diff 555933. clayborg added a comment. Log any errors we encounter during archive object parsing. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159408/new/ https://reviews.llvm.org/D159408 Files: lldb/source/Plu

[Lldb-commits] [PATCH] D159408: Switch over to using the LLVM archive parser for BSD archives.

This revision was automatically updated to reflect the committed changes. Closed by commit rGd4a141ef932a: Switch over to using the LLVM archive parser for BSD archives. (authored by clayborg). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159408/new

[Lldb-commits] [PATCH] D157609: [lldb] Add more ways to find split DWARF files

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Thanks a lot for acting on all feedback and getting all of the edge cases! LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157609/new/ ht

[Lldb-commits] [PATCH] D157609: [lldb] Add more ways to find split DWARF files

clayborg added a comment. In D157609#4644194 , @DavidSpickett wrote: >> Any chance we could simplify this situation and have dwo searches use >> exactly the same/shared logic as source file searches? > > I'm not sure what exactly this means, can you cla

[Lldb-commits] [PATCH] D152855: [lldb][Android] Add PlatformAndroidTest

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Very easy fix for this as suggested in code changes and this will be good to go Comment at: lldb/source/Plugins/Platform/Android/PlatformAndroid.h:73 + typed

[Lldb-commits] [PATCH] D152933: [lldb][Android] Add platform.plugin.remote-android.run-as

clayborg added a comment. I am wondering if the current proposed setting name "platform.plugin.remote-android.run-as" should actually be "platform.plugin.remote-android. package-name" to be a bit more clear. Not sure if we can use the package name in other places? Repository: rG LLVM Github

[Lldb-commits] [PATCH] D152569: [lldb] Introduce a tool to quickly generate projects with an arbitrary number of sources

clayborg added inline comments. Comment at: lldb/scripts/generate-project.py:21 +def generate_c_header(directory: str, index: int) -> None: +header_path = f"{directory}/obj{index}.h" +with open(header_path, "w") as f: kastiglione wrote: > kastiglione wrot

[Lldb-commits] [PATCH] D152759: [lldb][Android] Support zip .so file

clayborg added inline comments. Comment at: lldb/source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerCommon.cpp:1336 + // "zip_path!/so_path". Resolve the zip file path, .so file offset and size. + ZipFileResolver::FileKind file_kind; + std::string file_path; --

[Lldb-commits] [PATCH] D152757: [lldb][ObjectFileELF] Set ModuleSpec file offset and size

clayborg added a comment. LGTM. Anyone else have any comments? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152757/new/ https://reviews.llvm.org/D152757 ___ lldb-commits mailing list lldb-commits@lists.

[Lldb-commits] [PATCH] D152933: [lldb][Android] Add platform.plugin.remote-android.package-name

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. lgtm now! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D152933/new/ https://reviews.llvm.org/D152933 __

[Lldb-commits] [PATCH] D152855: [lldb][Android] Add PlatformAndroidTest

clayborg added inline comments. Comment at: lldb/source/Plugins/Platform/Android/PlatformAndroid.cpp:204 // constraints - try "cat ..." as a fallback. - AdbClient adb(m_device_id); + AdbClientUP adb(GetAdbClient(error)); + if (error.Fail()) Do we want the P

[Lldb-commits] [PATCH] D152855: [lldb][Android] Add PlatformAndroidTest

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Lets get this patch in so we have testing. We can work on caching the AdbClient internally in an ivar of PlatformAndroid in follow up patches. Repository: rG LLVM Github Monorepo CHANG

[Lldb-commits] [PATCH] D151765: [lldb] Introduce the FileSpecBuilder abstraction

clayborg added a comment. The idea behind FileSpec containing two ConstStrings, one for the directory and one for he filename is for lookup performance when searching thousands of line tables for a given file and line when setting breakpoints. Currently we pass in a FileSpec + line number and w

[Lldb-commits] [PATCH] D153390: [lldb][Windows] Fix ZipFileResolver tests

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Can we test this? I will ok to unblock windows bots. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153390/new/ https://reviews.llvm.org/D153

[Lldb-commits] [PATCH] D153482: [lldb][NFCI] Remove use of ConstString from StructuredDataPlugin

clayborg added a comment. Looks fine to me. I will let the owner of StructuredDataDarwinLog do the final approval. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153482/new/ https://reviews.llvm.org/D153482

[Lldb-commits] [PATCH] D153900: [lldb] Deprecate SBHostOS threading functionality

clayborg added a comment. I think the SBHostOS::ThreadCreated() function used to just set the thread name for the current thread, but looks like that hasn't been hooked up for a while. Don't know who was using this before, but LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACT

[Lldb-commits] [PATCH] D153735: [lldb][TargetGetModuleCallback] Implement Python interface

clayborg added inline comments. Comment at: lldb/include/lldb/API/SBDefines.h:129 +typedef SBError (*SBTargetGetModuleCallback)(SBDebugger debugger, + SBModuleSpec &module_spec, If we are putting this into SBPlatform,

[Lldb-commits] [PATCH] D153734: [lldb][TargetGetModuleCallback] Call get module callback

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. So the "SetTargetGetModuleCallback" functions in this patch need to pass both a callback _and_ a callback_baton. That way people can register native callbacks that can be used. T

[Lldb-commits] [PATCH] D149213: [lldb] Add basic support to Rust enums in TypeSystemClang

clayborg added a comment. Overall this looks fine as a way to help rust users see rust enums without having to implement a new TypeSystem for rust. I would just ask that we check the language of the CU before calling the ParseVariantPart, so that this would only get enabled for Rust. ===

[Lldb-commits] [PATCH] D149213: [lldb] Add basic support to Rust enums in TypeSystemClang

clayborg added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:3184 +case DW_TAG_variant_part: + ParseVariantPart(die, parent_die, class_clang_type, default_accessibility, + layout_info); c

[Lldb-commits] [PATCH] D153734: [lldb][LocateModuleCallback] Call locate module callback

clayborg added inline comments. Comment at: lldb/include/lldb/Target/Platform.h:884-887 + typedef std::function + LocateModuleCallback; I think we still need a baton for the callback so clients can register a callback + void *. Comment a

[Lldb-commits] [PATCH] D153735: [lldb][LocateModuleCallback] Implement API, Python interface

clayborg added a comment. It would be nice to pass the baton down into the platform layer if possible so that internal users could use the callback + baton. See comments in the other diff and see if you agree Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.ll

[Lldb-commits] [PATCH] D149213: [lldb] Add basic support to Rust enums in TypeSystemClang

clayborg added a comment. In D149213#4491594 , @bulbazord wrote: > I'm curious to know why you don't try and implement some kind of > `TypeSystemRust`? I assume it's going to be a lengthy process, but eventually > you (or somebody else) is going to want

[Lldb-commits] [PATCH] D153735: [lldb][LocateModuleCallback] Implement API, Python interface

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Thanks for the detailed explanation. We don't want internal users to add a callback with a baton to Platform.h as this would interfere with the APIs, so it is fine to not have a baton in P

[Lldb-commits] [PATCH] D153734: [lldb][LocateModuleCallback] Call locate module callback

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. LGTM. Thanks for the detailed explanations, it all makes sense. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D153734/new/ https://reviews.ll

[Lldb-commits] [PATCH] D149213: [lldb] Add basic support to Rust enums in TypeSystemClang

clayborg accepted this revision. clayborg added a comment. LGTM Comment at: lldb/source/Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:3968 + layout_info.field_offsets.insert({inner_field, 0}); +} Add a newline CHANGES SINCE LAST ACTION https://reviews.l

[Lldb-commits] [PATCH] D155107: Add support for llvm::MCInstPrinter::setPrintBranchImmAsAddress

clayborg added a comment. Seeing some before and after disassembler dumps would be nice to see what is changing. We might also want a new lldb setting that could be set with "settings set disassembly-branch-offsets [relative|absolute]" where setting it to "relative" would keep things as they a

[Lldb-commits] [PATCH] D155256: Add fs_base/gs_base support for Linux

clayborg added a comment. LGTM. Anyone else have any issues? Comment at: lldb/source/Plugins/Process/Utility/RegisterInfos_x86_64_with_base_shared.cpp:1-2 +//===-- RegisterInfos_x86_64_with_base_shared.cpp +//--===// +// fix comment lin

[Lldb-commits] [PATCH] D155107: Add support for llvm::MCInstPrinter::setPrintBranchImmAsAddress

clayborg added a comment. In D155107#4510539 , @ted wrote: > In D155107#4504667 , @jasonmolenda > wrote: > >> Isn't it better to print branches within a function as an offset, given that >> our disassembly forma

[Lldb-commits] [PATCH] D155256: [lldb][x86_64] Add fs_base/gs_base support for Linux

clayborg accepted this revision. clayborg 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/D155256/new/ https://reviews.llvm.org/D155256 __

[Lldb-commits] [PATCH] D156066: [lldb][LocateModuleCallback] Call locate module callback in Platform too

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. lgtm! I like this being in platform better Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156066/new/ https://reviews.llvm.org/D156066 _

[Lldb-commits] [PATCH] D155198: [lldb] Consider OP_addrx in DWARFExpression::Update_DW_OP_addr

clayborg added a comment. Sorry for the late response, but I had verbally agreed to this approach when Felipe and I spoke about how to fix this. LGTM! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155198/new/ https://reviews.llvm.org/D155198

[Lldb-commits] [PATCH] D155905: lldb RFC: Exposing set/get address masks, Fix*Address methods in SBProcess

clayborg added inline comments. Comment at: lldb/include/lldb/API/SBProcess.h:415-416 + /// (unsigned long long) 0xfc00 + lldb::addr_t GetCodeAddressMask(); + lldb::addr_t GetDataAddressMask(); + Will there only ever be two variants of this for cod

[Lldb-commits] [PATCH] D155107: Add support for llvm::MCInstPrinter::setPrintBranchImmAsAddress

clayborg added a comment. So Ted, can you verify that nothing is missing in RISCV since this is already the way it works for x86/x86_64 and arm32/arm64? I am thinking like Jason is where I wonder if something isn't just hooked up right for RISCV somehow. Or if you can verify that nothing change

[Lldb-commits] [PATCH] D155905: lldb RFC: Exposing set/get address masks, Fix*Address methods in SBProcess

clayborg added a comment. In D155905#4536917 , @jasonmolenda wrote: > Also interesting to consider if there should be an "Any" define. e.g. > > enum AddressMaskType { > eTypeCode = 0, > eTypeData, > eTypeHighmemCode, > eTypeHighmemData

[Lldb-commits] [PATCH] D156375: Fix lldb-vscode frame id integer overflow

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. We ran into this with a process that had many threads. No easy way to test this without creating a process with 8K threads. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTIO

[Lldb-commits] [PATCH] D155905: lldb RFC: Exposing set/get address masks, Fix*Address methods in SBProcess

clayborg added a comment. Looks much better, just a few questions in comments. Comment at: lldb/include/lldb/API/SBProcess.h:425 + /// code/data masks. Each of these can be set, or + /// most commonly, eMaskTypeall can be set, when all masks are + /// identical.

[Lldb-commits] [PATCH] D156732: Display PC instead of for stack trace in vscode

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Adding a test case for this is a good idea. In order to get the PC somewhere bad, you can probably make a function pointer that points to data, then try to branch there. The PC w

[Lldb-commits] [PATCH] D156732: Display PC instead of for stack trace in vscode

clayborg added a comment. You can copy this folder to a new test folder and adjust the test to verify: lldb/test/API/tools/lldb-vscode/stackTrace Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156732/new/ https://reviews.llvm.org/D156732 ___

[Lldb-commits] [PATCH] D140630: [lldb-vscode] Add data breakpoint support

clayborg added a comment. Sorry this fell off my radar. I was on medical leave for a surgery for 3 months. Back now Comment at: lldb/tools/lldb-vscode/Watchpoint.cpp:72 + m_verified = true; + m_error = ""; +} Comment at: lldb/tools/lldb-v

[Lldb-commits] [PATCH] D155905: lldb RFC: Exposing set/get address masks, Fix*Address methods in SBProcess

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Thanks for the explanation. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155905/new/ https://reviews.llvm.org/D155905 ___

[Lldb-commits] [PATCH] D156732: Display PC instead of for stack trace in vscode

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks good! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156732/new/ https://reviews.llvm.org/D156732

[Lldb-commits] [PATCH] D156970: Fix crash in lldb-vscode when missing function name

clayborg added inline comments. Comment at: lldb/tools/lldb-vscode/JSONUtils.cpp:699-703 + // `function_name` can be a nullptr, which throws an error when assigned to an + // `std::string`. + const char *function_name = frame.GetDisplayFunctionName(); + std::string frame_nam

[Lldb-commits] [PATCH] D75711: [NFC] Have ThreadPlans hold onto the Process & TID, rather than the Thread

clayborg added a comment. As long as the keeping thread plans around for threads that aren't there is only enabled when OS plug-in is around, I am fine with this direction. One questions: do we currently only allow stepping on real threads or OS threads that are backed by real threads right now

[Lldb-commits] [PATCH] D75925: [lldb] reject `.debug_arange` sections with nonzero segment size

clayborg added a comment. Change looks good, just needs a test. Should be easy to take a simple binary that has a .debug_aranges, and run obj2yaml on it, and tweak the segment size as needed? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75925/new

[Lldb-commits] [PATCH] D75925: [lldb] reject `.debug_arange` sections with nonzero segment size

clayborg added a comment. lgtm once all formatting issues are taken care of. Pavel? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D75925/new/ https://reviews.llvm.org/D75925 ___ lldb-commits mailing lis

[Lldb-commits] [PATCH] D74636: [lldb-vscode] Add inheritEnvironment option

clayborg added a comment. In D74636#1916356 , @labath wrote: > There's a `target.inherit-env` setting in lldb (which I believe also works > correctly for remote launches). Could you use that instead of reimplementing > the feature in vscode? The real q

[Lldb-commits] [PATCH] D75711: [NFC] Have ThreadPlans hold onto the Process & TID, rather than the Thread

clayborg added a comment. Everything looks good, just a question in inlined comment about having a thread plan hold onto a pointer to a thread. Seems dangerous Comment at: lldb/include/lldb/Target/ThreadPlan.h:601 + Thread *m_thread; ThreadPlanKind m_kind; --

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

clayborg added inline comments. Comment at: lldb/bindings/interface/SBPlatform.i:197-198 +lldb::SBEnvironment +GetEnvironment(); + What does it mean to get the environment from a platform? Fetching it from the remote platform as what ever binary was us

[Lldb-commits] [PATCH] D76314: [lldb-vscode] stop read loop after termination

clayborg added a comment. Does this have something to do with a multiplexor that funnels traffic through one tunnel? I am curious as to why the ReadJSON() doesn't exit after the other side closes down? LGTM, but needs a test. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. We need to determine if the objects we return are copies (from SBPlatform and SBTarget), or if they are objects that will modify the environment for the platform and target respe

[Lldb-commits] [PATCH] D76314: [lldb-vscode] stop read loop after termination

clayborg added inline comments. Comment at: lldb/test/API/tools/lldb-vscode/launch/TestVSCode_launch.py:46-48 +program = self.getBuildArtifact("a.out") +self.build_and_launch(program, stopOnEntry=True) +self.vscode.request_disconnect(terminateDebuggee=True

[Lldb-commits] [PATCH] D76351: [lldb-vscode] Don't use SBLaunchInfo in request_attach

clayborg accepted this revision. clayborg added a comment. Yep, copy and paste error! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76351/new/ https://reviews.llvm.org/D76351 ___ lldb-commits mailing l

[Lldb-commits] [PATCH] D76216: Improve step over performance

clayborg added a comment. Jim is the one that really needs to mark this as accepted as the thread plans are one of his many areas of expertise. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76216/new/ https://reviews.llvm.org/D76216 ___

[Lldb-commits] [PATCH] D76314: [lldb-vscode] stop read loop after termination

clayborg accepted this revision. clayborg added a comment. lgtm. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76314/new/ https://reviews.llvm.org/D76314 ___ lldb-commits mailing list lldb-commits@list

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Just some header doc cleanup before we submit. Thanks for sticking with these changes! Comment at: lldb/include/lldb/API/SBEnvironment.h:24 + + const lldb::SB

[Lldb-commits] [PATCH] D76314: [lldb-vscode] stop read loop after termination

clayborg added a comment. Just watch the build bots carefully when you check this in! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76314/new/ https://reviews.llvm.org/D76314 ___ lldb-commits mailing l

[Lldb-commits] [PATCH] D76111: Create basic SBEnvironment class

clayborg added a comment. In D76111#1930783 , @labath wrote: > The new interface is ok for me, but there are two things I want to mention: > > - the iteration via `Get{Name,Value}AtIndex` is going to be slow because the > underlying map (like most maps) d

[Lldb-commits] [PATCH] D74636: [lldb-vscode] Add inheritEnvironment option

clayborg added inline comments. Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1414 +auto kv = llvm::StringRef(name_and_value).split("="); +env.Set(kv.first.str().c_str(), kv.second.str().c_str(), true); + } We want to have a function in SBEnvironmen

[Lldb-commits] [PATCH] D76529: [lldb-vscode] Add missing launchCommands entry in the package.json

clayborg requested changes to this revision. clayborg added inline comments. This revision now requires changes to proceed. Comment at: lldb/tools/lldb-vscode/package.json:150 + "type": "array", +

[Lldb-commits] [PATCH] D76593: [lldb-vscode] Convert g_vsc.launch_info to a local variable

clayborg accepted this revision. clayborg added a comment. Yes, this works now that we are creating the target in this function. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76593/new/ https://reviews.llvm.org/D76593 ___

[Lldb-commits] [PATCH] D74636: [lldb-vscode] Add inheritEnvironment option

clayborg added a comment. I would either add the option to SBLaunchInfo so it can be specified, or execute the command. If the target is created, it is setting a target specific setting. If had to pick I would add the API to SBLaunchInfo. Whenever I see something that can't be done through the

[Lldb-commits] [PATCH] D76758: Augment lldb's symbol table with external symbols in Mach-O's dyld trie

clayborg added a comment. So I know the mach-o symbol table parsing code is a mess already, but it seems like this patch can be simpler if we make a std::set at the top of ObjectFileMachO::ParseSymtab() and every time we add a symbol that has a valid address value, add the file addr to this set

[Lldb-commits] [PATCH] D76872: [intel-pt] Fix existing support in LLDB

clayborg added a comment. Looks fine to me, but the original author should probably approve this Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76872/new/ https://reviews.llvm.org/D76872 ___ lldb-commit

[Lldb-commits] [PATCH] D76891: [lldb-vscode] fix breakpoint result ordering

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. LGTM as long as all test are passing! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76891/new/ https://reviews.llvm.org/D76891 __

[Lldb-commits] [PATCH] D76964: Fix an issue where the IgnoreName function was not allowing "Class" to be looked up inside a namespace or other class.

clayborg created this revision. clayborg added reviewers: jingham, aprantl, JDevlieghere. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. Objective C may or may not be enabled when running expressions. There was code that was no allowing "Class" or "id" to be looked up in e

[Lldb-commits] [PATCH] D75750: [lldb] integrate debuginfod

clayborg added a comment. Currently we have a solution for macOS to locate symbol files in the "lldb/source/Symbol/LocateSymbolFile.cpp" file in the Symbols::LocateExecutableSymbolFile(...) function: static FileSpec Symbols::LocateExecutableSymbolFile(const ModuleSpec &module_spec, const Fil

[Lldb-commits] [PATCH] D76758: Augment lldb's symbol table with external symbols in Mach-O's dyld trie

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

[Lldb-commits] [PATCH] D75750: [lldb] integrate debuginfod

clayborg added a comment. In D75750#1949527 , @labath wrote: > In D75750#1948273 , @clayborg wrote: > > > Currently we have a solution for macOS to locate symbol files in the > > "lldb/source/Symbol/LocateSymbolFil

[Lldb-commits] [PATCH] D75750: [lldb] integrate debuginfod

clayborg added a comment. In D75750#1949678 , @fche2 wrote: > In D75750#1949527 , @labath wrote: > > > > > > > > > I am expecting that this feature will hook in very near to > > `DownloadObjectAndSymbolFile` for do

[Lldb-commits] [PATCH] D76968: [lldb-vscode] Correctly return source mapped breakpoints for setBreakpoints request

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. The description confused me a bit as I thought we were going to be doing some "CommandObjectSource::DumpLinesInSymbolContexts()" stuff somewhere. But this path is really just "re

[Lldb-commits] [PATCH] D76968: [lldb-vscode] Correctly return source mapped breakpoints for setBreakpoints request

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. After reviewing more, we should just re-use CreateBreakpoint and add a "llvm::Optional request_path" argument. Then all breakpoints use the same function and we avoid duplicated

[Lldb-commits] [PATCH] D76968: [lldb-vscode] Correctly return source mapped breakpoints for setBreakpoints request

clayborg added a comment. Looks good as long as with we have "llvm::Optional request_path = {}" in the arguments for a function, that "{}" will turn into llvm::None and not an empty StringRef? Unclear to me. As long as this is what is happening and as long is this coding convention is used else

[Lldb-commits] [PATCH] D77107: [intel-pt] Implement a basic test case

clayborg added a comment. I am worried if this test will be flaky on loaded machines. Not sure how we can ever guarantee we will see processor traces with our stuff in it if the machine is busy running many tests or even doing other things. Comment at: lldb/test/API/tools/in

[Lldb-commits] [PATCH] D76968: [lldb-vscode] Correctly return source mapped breakpoints for setBreakpoints request

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Just use "llvm::None" instead of "{}" and this is good to go. Comment at: lldb/tools/lldb-vscode/JSONUtils.h:237 +CreateBreakpoint(lldb::SBBreakpoint &bp, +

[Lldb-commits] [PATCH] D75750: [lldb] integrate debuginfod

clayborg added a comment. The main issue is that the symbol vendors currently are ELF, macOS and WASM. Right now we have one SymbolVendor for a triple, but I can see a SymbolVendor wanting to use multiple symbol servers to get information: one for the OS binaries (debuginfod or DebugSymbols.fra

[Lldb-commits] [PATCH] D77186: [source maps] Ensure all valid source maps are added instead of failing with the first invalid one

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. This will do what the user intends more of the time. Good catch Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D77186/new/ https://reviews.llv

  1   2   3   4   5   6   7   8   9   10   >