[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] 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] 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] 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] 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 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] 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] 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/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. 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] 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. 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] 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] 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] 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.

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 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 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 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 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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. 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] 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 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] 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] 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] 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] 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: 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. 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] 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] 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] 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] 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 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] 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] D146659: [LLDB] Fix for D139955 Summary:

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Must be relocations causing the stuff to not match up to the llvm-dwarfdump output. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146659/new

[Lldb-commits] [PATCH] D146659: [LLDB] Fix for D139955 Summary:

clayborg added a comment. FYI: This might be because we are using a .o file and relocations are being applied internally!? Comment at: lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s:11 +# CHECK: 0x006e: adding range [0x-0x001f) +# CH

[Lldb-commits] [PATCH] D146659: [LLDB] Fix for D139955 Summary:

clayborg added inline comments. Comment at: lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s:11 +# CHECK: 0x006e: adding range [0x-0x001f) +# CHECK-SAME: which has a base that is less than the function's low PC 0x0021. +# CHE

[Lldb-commits] [PATCH] D146659: [LLDB] Fix for D139955 Summary:

clayborg added a comment. Fix the error message to check for the exact error message and this is good to go. Comment at: lldb/test/Shell/SymbolFile/DWARF/range-lower-then-low-pc.s:10 + +# CHECK: which has a base that is less than the function's low PC + might

[Lldb-commits] [PATCH] D146659: [LLDB] Fix for D139955 Summary:

clayborg added a comment. Is there no test case catching this? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146659/new/ https://reviews.llvm.org/D146659 ___ lldb-commits mailing list lldb-commits@lists.

[Lldb-commits] [PATCH] D145624: [lldb] Make MemoryCache::Read more resilient

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

[Lldb-commits] [PATCH] D145624: [lldb] Make MemoryCache::Read more resilient

clayborg added inline comments. Comment at: lldb/source/Target/Memory.cpp:133-135 + if (pos != m_L2_cache.end()) { +return pos->second; + } remove braces for single line if statement per llvm coding guidelines Comment at: lldb/source/Targ

[Lldb-commits] [PATCH] D145624: [lldb] Make MemoryCache::Read more resilient

clayborg added a comment. I would suggest checking the google stadia patch for the L1 and L2 caches: https://github.com/googlestadia/vsi-lldb/blob/master/patches/llvm-project/0019-lldb-Fix-incorrect-L1-inferior-memory-cache-flushing.patch Just to see how they did t

[Lldb-commits] [PATCH] D145624: [lldb] Make MemoryCache::Read more resilient

clayborg added a comment. One other optimization we can do is if we read from the process memory and it returns that is read zero bytes, right now we add the range we were trying to read into the m_invalid_ranges member variable. So lets say we were trying to read the range [0x1000-0x2000) on a

[Lldb-commits] [PATCH] D145624: [lldb] Make MemoryCache::Read more resilient

clayborg added a comment. There was a fix that was never submitted for Google Stadia for the memory cache here: https://github.com/googlestadia/vsi-lldb/tree/master/patches/llvm-project Might be worth checking what they did to ensure we have all of the same abilities. Comme

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

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Thanks for the changes! LGTM. Just one missed EXPECT_THAT_EXPECTED, but accepted. Comment at: lldb/unittests/Symbol/JSONSymbolTest.cpp:144-145 + Expected symbol = Symbo

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

clayborg added inline comments. Comment at: lldb/include/lldb/Core/Debugger.h:594 lldb::TargetSP m_dummy_target_sp; + Diagnostics::CallbackID m_diagnostics_callback_id; I am guessing these changes are in Debugger.h and Debugger.cpp are not related to this

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

clayborg added a comment. Looks good. Only questions is if we can add a C++ unit test for this file and test the new Symbol::FromJSON() and test all error conditions. Comment at: lldb/source/Symbol/Symbol.cpp:99-100 +llvm::Expected Symbol::FromJSON(const JSONSymbol &symbol,

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

clayborg added inline comments. Comment at: lldb/include/lldb/Symbol/Symbol.h:27 + std::optional id; + std::optional section; + std::optional type; Come to think of it, we might not need the section as a name as it adds no real value unless we want to add a "

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

clayborg added inline comments. Comment at: lldb/include/lldb/API/SBDebugger.h:203 + void CancelInterruptRequest(); + bool InterruptRequested(); bulbazord wrote: > Could this be marked `const` if the mutex was marked `mutable`? We don't tend to mark things in

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

clayborg added inline comments. Comment at: lldb/include/lldb/Symbol/Symbol.h:23 +struct JSONSymbol { + uint64_t value; + std::optional size; JDevlieghere wrote: > clayborg wrote: > > clayborg wrote: > > > JDevlieghere wrote: > > > > clayborg wrote: > > > > > D

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

clayborg added inline comments. Comment at: lldb/include/lldb/Symbol/Symbol.h:23 +struct JSONSymbol { + uint64_t value; + std::optional size; JDevlieghere wrote: > clayborg wrote: > > Do we something that says "value is an address"? Or are we inferring that >

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

clayborg added inline comments. Comment at: lldb/include/lldb/Symbol/Symbol.h:351-356 +bool fromJSON(const llvm::json::Value &value, lldb_private::JSONSymbol &symbol, + llvm::json::Path path); + +bool fromJSON(const llvm::json::Value &value, lldb::SymbolType &type, +

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

clayborg added a comment. If these files can be used as the only source of information (without a stripped executable), we really should include a serialized SectionList in the JSON that can be loaded into ObjectFileJSON. This would be very useful for easily creating unit tests.

[Lldb-commits] [PATCH] D143520: Add a new SBDebugger::SetDestroyCallback() API

clayborg added a comment. Just change HandleDestroryCallback to a member function and this is good to go. Comment at: lldb/source/Core/Debugger.cpp:688-694 +void Debugger::HandleDestroyCallback(const DebuggerSP &debugger_sp) { + if (debugger_sp->m_destroy_callback) { +debu

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

clayborg added inline comments. Comment at: lldb/include/lldb/Core/Debugger.h:397-398 + /// cooperative interruption. If this is non-zero, InterruptRequested will + /// return true. Interruptible operations are expected to query this + /// flag periodically, and interrupt wh

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

clayborg added a comment. I like this idea of this, but I would like to see this be a bit more complete. One idea is to remove the ObjectFileJSON::Symbol definition and just use lldb_private::Symbol objects and allow these objects to construct encode and decode from JSON. Then we have the abili

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

clayborg added a comment. Sorry for delay, I have been on vacation for the last two weeks. I will be back next Monday and get to this soon. Feel free to ping again next week if I don't get to it! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D14063

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

clayborg added a comment. In D142926#4116417 , @JDevlieghere wrote: > In D142926#4115980 , @clayborg > wrote: > >> In D142926#4115875 , @JDevlieghere >> wrote: >> >>> I

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

clayborg added a comment. In D142926#4115875 , @JDevlieghere wrote: > In D142926#4114215 , @bulbazord > wrote: > >> Not sure how useful it would be but I recorded the full list of methods get >> added with this

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

clayborg added a comment. Looks good to me. Pavel? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138618/new/ https://reviews.llvm.org/D138618 ___ lldb-commits mailing list lldb-commits@lists.llvm.org htt

[Lldb-commits] [PATCH] D143623: [lldb] Print an error for unsupported combinations of log options

clayborg added inline comments. Comment at: lldb/test/Shell/Log/TestHandlers.test:1-2 +# RUN: %lldb -o 'log enable -h os -f /tmp/foo gdb-remote packets' 2>&1 | FileCheck %s --check-prefix UNSUPPORTED-FILE +# RUN: %lldb -o 'log enable -h os -b 10 gdb-remote packets' 2>&1 | Fil

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

clayborg added a comment. In D142926#4111717 , @bulbazord wrote: > Potentially interesting: > > - SBData::GetDescription base_addr parameter has default value now > - SBInstructionList::GetInstructionsCount canSetBreakpoint has default value > now > - SB

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

clayborg added a comment. In D142926#4111717 , @bulbazord wrote: > In addition to what I wrote above, I also fixed several documentation bugs > (putting docstrings on the wrong method). > > I manually audited the generated code before and after this chan

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

clayborg added inline comments. Comment at: lldb/source/Plugins/SymbolFile/DWARF/DIERef.h:22-24 +/// - file_index: identifies the dwo file in the Module. If this field is not +/// set, +/// the DIERef references the main, dwo or .o file. Com

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

clayborg added a comment. Very clean patch now, just a few nits about asserts! Comment at: lldb/source/Plugins/SymbolFile/DWARF/DebugNamesDWARFIndex.cpp:129 DWARFUnit &cu, llvm::function_ref callback) { - lldbassert(!cu.GetSymbolFileDWARF().GetDwoNum()); + lldbassert(!cu

[Lldb-commits] [PATCH] D143520: Add a new SBDebugger::SetDestroyCallback() API

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. If I am reading the code for this patch correctly, we need the BatonSP stuff because we have differing callback bytes for public vs private APIs. If we switch to using a common c

[Lldb-commits] [PATCH] D143347: [lldb][DWARF] Infer no_unique_address attribute

clayborg added a comment. Changes look fine to me. I would like someone that specializes in the expression parser to give the final ok though. Comment at: lldb/source/Plugins/ExpressionParser/Clang/ClangASTImporter.cpp:551 const LayoutI

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

clayborg added inline comments. Comment at: lldb/include/lldb/API/SBAddress.h:14-16 +#ifdef SWIG +%include "interface/SBAddressDocstrings.i" +#endif bulbazord wrote: > clayborg wrote: > > Do we want two different .i files? Right now we have > > both"interface/SB

  1   2   3   4   5   6   7   8   9   10   >