[Lldb-commits] [PATCH] D67018: [lldb][NFC] Add basic test for GUI command

clayborg added a comment. Seems like revision 370054 sets the screen size quite wide. Just checking that this change will be used for this test? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67018/new/ https://reviews.llvm.org/D67018 ___

[Lldb-commits] [PATCH] D67022: Skip getting declarations for repeated DIEs (WIP)

clayborg added a comment. In D67022#1653248 , @guiandrade wrote: > Hey guys, > > This change is more for me to get to know what you think. I've noticed that > the GetDeclForUIDFromDWARF() calls inside > SymbolFileDWARF::ParseDeclsForContext >

[Lldb-commits] [PATCH] D66546: Extend FindTypes w/ CompilerContext to allow filtering by language

clayborg added a comment. Looks good overall. Just a question of it we want to return "const LanguageSet &" to avoid copies. Also switch static functions that currently return "LanguageSet" over to use static variables and llvm::once to init them and then return "const LanguageSet &".

[Lldb-commits] [PATCH] D65952: SymbolVendor: Have plugins return symbol files directly

clayborg added a comment. In D65952#1624799 , @labath wrote: > In D65952#1623297 , @clayborg wrote: > > > So I am confused. Are we keeping SymbolVendor around for locating symbols > > files or are we getting rid

[Lldb-commits] [PATCH] D66546: Extend FindTypes w/ CompilerContext to allow filtering by language

clayborg added a comment. Sounds good about LanguageSet being cheap to pass by value. Are there any paths that will call this over and over where we still will be calculating the LangaugeSet over and over in a type system? We might benefit from using llvm::once and a static LanguageSet in the

[Lldb-commits] [PATCH] D66357: Fix GetDIEForDeclContext so it only returns entries matching the provided context

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Now that obj2yaml is a library, we might be able to make some real DWARF to exercise this? I can custom make any DWARF you want as I have a DWARF generator. Though this test does verify

[Lldb-commits] [PATCH] D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance

clayborg added a comment. In D67022#1656874 , @labath wrote: > Thanks for the clarification Greg. > > Functionally, this patch seems fine, but I am still wondering if we shouldn't > make this more efficient. std::set is not the most memory-efficient >

[Lldb-commits] [PATCH] D67347: [Windows] Use information from the PE32 exceptions directory to construct unwind plans

clayborg added a comment. My opinion is we need to be able to ask both the SymbolFile and ObjectFile for unwind plans in the API, but we can always just ask the SymbolFile for the unwind plan and the default SymbolFile virtual function can just defer to the ObjectFile. We also need to say "I

[Lldb-commits] [PATCH] D67347: [Windows] Use information from the PE32 exceptions directory to construct unwind plans

clayborg added a comment. Added Jason Molenda who owns the unwind stuff so he might be able to comment and help us with this patch. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67347/new/ https://reviews.llvm.org/D67347

[Lldb-commits] [PATCH] D67472: [Target] Move InferiorCall to Process

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

[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

clayborg added a comment. In D67390#1671463 , @kwk wrote: > @clayborg what address is it exactly to store here `std::map ContString> SymbolMapType;`? As an example > `dc_symbol.GetAddress().GetFileAddress()` is something that would work but as > soon as

[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

clayborg added a comment. In D67390#1671128 , @kwk wrote: > In D67390#1671018 , @labath wrote: > > > In D67390#1667270 , @kwk wrote: > > > > > @labath how shall we go about

[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

clayborg added a comment. In D67390#1671018 , @labath wrote: > In D67390#1667270 , @kwk wrote: > > > @labath how shall we go about this? We do have the situation that when you > > lookup a symbol you might find it

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

clayborg added inline comments. Comment at: lldb/source/API/SBCommandInterpreter.cpp:165-172 +SBCommandReturnObject sb_return; +std::swap(result, SBCommandReturnObject_ref(sb_return)); SBCommandInterpreter sb_interpreter(_interpreter); SBDebugger

[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

clayborg added a comment. In D67390#1671463 , @kwk wrote: > @clayborg what address is it exactly to store here `std::map ContString> SymbolMapType;`? As an example > `dc_symbol.GetAddress().GetFileAddress()` is something that would work but as > soon as

[Lldb-commits] [PATCH] D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance

clayborg added a comment. LGTM. Test seems about as good as we can do. Pavel, you ok with this now? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67022/new/ https://reviews.llvm.org/D67022 ___

[Lldb-commits] [PATCH] D67347: [Windows] Use information from the PE32 exceptions directory to construct unwind plans

clayborg added a comment. At the object file level I would love to see much less of this specific unwind info making it into the ObjectFile interface. Why can't we just ask the ObjectFile to provide an UnwindPlan for a given address. There is so much complexity within the UnwindPlan where it

[Lldb-commits] [PATCH] D67370: Fix ELF core file memory reading for PT_LOAD program headers with no p_filesz

clayborg added a comment. $ git llvm push Pushing 1 monorepo commit: f54a49a8133 Fix ELF core file memory reading for PT_LOAD program headers with no p_filesz Sending lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/elf-core/thread_crash/TestLinuxCoreThreads.py

[Lldb-commits] [PATCH] D67370: Fix ELF core file memory reading for PT_LOAD program headers with no p_filesz

clayborg created this revision. clayborg added reviewers: labath, aprantl. Prior to this fix, ELF files might contain PT_LOAD program headers that had a valid p_vaddr, and a valid file p_offset, but the p_filesz would be zero. For example in

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

clayborg added inline comments. Comment at: lldb/include/lldb/API/SBCommandReturnObject.h:22-27 +namespace lldb_private { +// Wrap SBCommandReturnObject::ref() so that any LLDB internal function can +// call it but still no SB API users can call it. +CommandReturnObject &

[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

clayborg added a comment. In D67390#1672210 , @kwk wrote: > @clayborg thank you for this explanation. My patch for minidebuginfo is > already done in D66791 . But this one here > I was asked to pull out as a separate

[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

clayborg added a comment. In D67390#1672254 , @labath wrote: > In D67390#1672210 , @kwk wrote: > > > So my point of this whole question is: What makes a symbol unique in the > > sense that it shouldn't be added to

[Lldb-commits] [PATCH] D67589: Fix crash on SBCommandReturnObject & assignment

clayborg added a comment. In D67589#1673771 , @jankratochvil wrote: > In D67589#1673756 , @labath wrote: > > > We couldn't use PointerIntPair due to the our abi stability promise > >

[Lldb-commits] [PATCH] D66638: Unwind: Add a stack scanning mechanism to support win32 unwinding

clayborg added a comment. So made a "sbt" command in a Python module as a way to get a backtrace when our backtracing fails because Android almost never makes it back to the first frame for any stacks. This code just grabs the current SP, looks up the memory region using the SP, and then it

[Lldb-commits] [PATCH] D66638: Unwind: Add a stack scanning mechanism to support win32 unwinding

clayborg added inline comments. Comment at: source/Plugins/Process/Utility/RegisterContextLLDB.cpp:1875 + if (addr.SetLoadAddress(candidate, ()) && + addr.GetSection()->GetPermissions() & lldb::ePermissionsExecutable) { +address = candidate_addr;

[Lldb-commits] [PATCH] D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance

clayborg added a comment. This looks like a good approach to me. Pavel? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67022/new/ https://reviews.llvm.org/D67022 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D67022: Enhance SymbolFileDWARF::ParseDeclsForContext performance

clayborg added a comment. In D67022#1660945 , @guiandrade wrote: > In D67022#1660901 , @clayborg wrote: > > > This looks like a good approach to me. Pavel? > > > Even without tests? Just to clarify my last comment,

[Lldb-commits] [PATCH] D65864: Remove Module::GetSymbolVendor

clayborg accepted this revision. clayborg added a comment. Optional rename in inline comments, but looks good. Comment at: include/lldb/Core/Module.h:1006 std::atomic m_did_load_objfile{false}; - std::atomic m_did_load_symbol_vendor{false}; + std::atomic

[Lldb-commits] [PATCH] D65469: Remove `bugreport` command

clayborg added a comment. In D65469#1608631 , @JDevlieghere wrote: > In D65469#1608278 , @clayborg wrote: > > > IMHO: we should keep this command and expand its abilities to help report > > stepping issues,

[Lldb-commits] [PATCH] D64993: Fix PC adjustment in StackFrame::GetSymbolContext

clayborg added a comment. Code looks fine now. Now Jason will need to chime in with comments on the new unwind functionality. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64993/new/ https://reviews.llvm.org/D64993

[Lldb-commits] [PATCH] D65330: [lldb][docs] Update documentation for monorepo and CMake caches

clayborg added a comment. Can we add a section for "Building LLDB with Xcode"? We had a shell script patch that was going to be checked in, not sure if it made it. But it did a two part thing where it built LLVM and clang and then generated an Xcode project. I would rather just produce the

[Lldb-commits] [PATCH] D65329: SymbolVendor: Move locking into the Symbol Files

clayborg added a comment. We will need to watch future submissions closely as this increases the chance that someone will extend a SymbolFile by overriding a virtual function and forget to lock. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65329/new/ https://reviews.llvm.org/D65329

[Lldb-commits] [PATCH] D65401: SymbolVendor: Remove the object file member variable

clayborg added a comment. So previously the SymbolVendor would only make a strong reference to the object file if it didn't match the object file of the module. Now it always makes one. Might be ok since the Module owns the SymbolVendor and thus owns the SymbolFile. We should make sure we

[Lldb-commits] [PATCH] D65386: [lldb][NFC] Use an enum instead of chars when handling options [WIP]

clayborg added a comment. In D65386#1604927 , @jingham wrote: > It worries me a little bit that we are making it harder and harder to figure > out "where does the option for "-t" get stored once this CommandObject's > options have been parsed. Can you

[Lldb-commits] [PATCH] D65414: Fix ClangASTContext::CreateParameterDeclaration to not call addDecl

clayborg added a comment. Looks fine to me. Be nice to clarify the test with comments a bit. I can't really tell what the test is doing or how it is verifying the fix in ClangASTContext.cpp CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65414/new/ https://reviews.llvm.org/D65414

[Lldb-commits] [PATCH] D65611: [Driver] Expand the target in the driver.

clayborg added a comment. In D65611#1615018 , @JDevlieghere wrote: > In D65611#1613052 , @clayborg wrote: > > > Also be careful if the user uses a symlink to not resolve the link. If a > > user tries to debug

[Lldb-commits] [PATCH] D65611: [Driver] Expand the executable path in the target create output

clayborg added a comment. I am fine with full path as long as argv[0] still behaves correctly. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65611/new/ https://reviews.llvm.org/D65611 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D65952: SymbolVendor: Have plugins return symbol files directly

clayborg added a comment. So I am confused. Are we keeping SymbolVendor around for locating symbols files or are we getting rid of it entirely? Comment at: include/lldb/Core/Module.h:989-996 + /// A pointer to the symbol file for this module. + std::unique_ptr m_symfile_up;

[Lldb-commits] [PATCH] D65109: [LLDB] Remove the Xcode project

clayborg added a comment. I am fine with removing the Xcode project. Others should ok it too. > I've included a new script to streamline the process: it creates two build > directories in the current directory. The first directory is for LLVM and > Clang which uses Ninja to build. The second

[Lldb-commits] [PATCH] D64917: Add offsetof support to expression evaluator.

clayborg added a comment. nice! Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D64917/new/ https://reviews.llvm.org/D64917 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D62570: Use LLVM's debug line parser in LLDB

clayborg added a comment. We also need to test to ensure this doesn't regress parsing speed. Need a large C++ project, like LLDB with debug clang and debug llvm, and something that forces all line tables to be parsed (not just the prologues like when setting a breakpoint) and see how things

[Lldb-commits] [PATCH] D65163: Fix occasional hangs of VSCode testcases

clayborg added a comment. In D65163#1597902 , @labath wrote: > Thank you very much for tracking this down. Maybe wait a while to see if > @clayborg has any thoughts on this, but otherwise, this looks good to me. haha, I already accepted. So good to go.

[Lldb-commits] [PATCH] D65152: Fix issues with inferior stdout coming out of order

clayborg added inline comments. Comment at: include/lldb/Core/Debugger.h:350 + std::mutex m_output_flush_mutex; + void FlushProcessOutput(Process , bool is_stdout); What about making this function: ``` void Debugger::FlushProcessOutput(Process , bool

[Lldb-commits] [PATCH] D65163: Fix occasional hangs of VSCode testcases

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. If all tests pass, then I am good with this. Repository: rLLDB LLDB CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65163/new/ https://reviews.llvm.org/D65163

[Lldb-commits] [PATCH] D65208: SymbolVendor: Move Symtab construction into the SymbolFile

clayborg added a comment. So the original idea behind the symbol vendor was that if you have more than one binary: stripped installed binary, and unstripped debug info binary with symbols, that the symbol table could be generated by using one or more object files. It would be nice to ensure we

[Lldb-commits] [PATCH] D65207: LLGS: fix tracking execve on linux

clayborg added a comment. Is this not going to be racy on other platforms? Or do all platforms stop at entry point once a program has been exec'ed? I am worried we might miss the breakpoint. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D65207/new/ https://reviews.llvm.org/D65207

[Lldb-commits] [PATCH] D65171: [LLDB] Find debugserver in Command Line Tools as well

clayborg accepted this revision. clayborg added a comment. Find with me as long as we default to using the installed Xcode, and then check for the command line tools if that fails. Many people keep Xcode up to date, but might not always update the installed command line tools. Repository:

[Lldb-commits] [PATCH] D65282: ObjectFileELF: permit thread-local sections with overlapping file addresses

clayborg added inline comments. Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:1874-1875 -ConstString Name(("PT_LOAD[" + llvm::Twine(LoadID++) + "]").str()); +ConstString Name(llvm::formatv("{0}[{1}]", provider.GetSegmentName(), +

[Lldb-commits] [PATCH] D65089: SymbolVendor: Move compile unit handling into the SymbolFile class

clayborg added a comment. Looks good. Just one question about caching the calculated number of compile units. Comment at: source/Symbol/SymbolFile.cpp:174-182 +uint32_t SymbolFile::GetNumCompileUnits() { + std::lock_guard guard(GetModuleMutex()); + if

[Lldb-commits] [PATCH] D65025: [Symbol] Improve TypeSystemMap mutex safety

clayborg added a comment. I fear this will deadlock again now. The original reason for the m_clear_in_progress was: > r260624 | jingham | 2016-02-11 16:03:19 -0800 (Thu, 11 Feb 2016) | 14 lines > > When calling TypeSystemMap::Clear, objects being destroyed in the process of > clearing the

[Lldb-commits] [PATCH] D67390: [LLDB][ELF] Load both, .symtab and .dynsym sections

clayborg added a comment. So overall approach is good. See inline comments for issue and questions. Comment at: lldb/source/Plugins/ObjectFile/ELF/ELFHeader.cpp:371-373 + r.st_name = st_name; + return elf::ELFSymbol::operator==(r) && + st_name_string ==

[Lldb-commits] [PATCH] D68088: Set eRegisterKindEHFrame register numbers for 32 bit ARM register contexts in minidumps

clayborg created this revision. clayborg added reviewers: labath, aadsm, dvlahovski. Herald added a subscriber: kristof.beyls. Stack unwinding was sometimes failing when trying to unwind stacks in 32 bit ARM. I discovered this was because the EH frame register numbers were not set. This patch

[Lldb-commits] [PATCH] D67966: Use llvm for dumping DWARF expressions

clayborg added a comment. nice! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67966/new/ https://reviews.llvm.org/D67966 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D68048: [WIP][RFC] Improve fetching the process list on the android platform

clayborg added a comment. We were discussing things over here and were wondering if we even need the lldb-server in order to connect to the platform, or if we should just implement everything through command line commands like "adb", or link against a ADB shared library (is there one?), or

[Lldb-commits] [PATCH] D68106: Fix a crasher due to an assert when two files have the same UUID but different paths.

clayborg updated this revision to Diff 222032. clayborg added a comment. Remove printf that was left in. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68106/new/ https://reviews.llvm.org/D68106 Files:

[Lldb-commits] [PATCH] D68106: Fix a crasher due to an assert when two files have the same UUID but different paths.

clayborg created this revision. clayborg added reviewers: labath, aadsm, dvlahovski. Herald added a project: LLDB. Herald added a subscriber: lldb-commits. clayborg updated this revision to Diff 222032. clayborg added a comment. Remove printf that was left in. The PlaceholderObjectFile has an

[Lldb-commits] [PATCH] D66638: Unwind: Add a stack scanning mechanism to support win32 unwinding

clayborg added a comment. In D66638#1679195 , @labath wrote: > Thanks for the review and sorry for the delay (I was OOO). The idea to use > `Process::GetLoadAddressPermissions` makes sense, both from consistency and > correctness perspectives.

[Lldb-commits] [PATCH] D69873: [lldb-vscode] support the completion request

clayborg added a comment. Maybe limit the matches if posssible if that works. If you type "target variable " you can complete a list of all global variables everywhere which might be quite a few. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D70127: [lldb-vscode] Fix a race in test_extra_launch_commands

clayborg added a comment. --stop-at-entry only really works for Darwin since its posix_spawn has a flag that will stop it at the entry point before anything executes. On linux, does this flag do anything? Comment at:

[Lldb-commits] [PATCH] D70134: dotest: Add a way for the run_to_* helpers to register dylibs

clayborg added a comment. In D70134#1742647 , @jingham wrote: > This is fine. > > I wondered a bit about whether it would be generally useful to add the > 'dylibs that have to be copied' to the SBLaunchInfo? It has some other > platform'y like things.

[Lldb-commits] [PATCH] D70387: [LLDB] [Windows] Allow making subprocesses use the debugger's console with LLDB_INHERIT_CONSOLE=true

clayborg added a comment. > When running the lldb command line debugger in a console, it can be > convenient to get the output of the debuggee inline in the same console. agreed. It just did the default behavior of launching in a command.exe since that is what windows does by default. > The

[Lldb-commits] [PATCH] D70851: [lldb] s/FileSpec::Equal/FileSpec::Match

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

[Lldb-commits] [PATCH] D70532: [lldb] Improve/fix base address selection in location lists

clayborg added a comment. Sorry for the delay, lgtm Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70532/new/ https://reviews.llvm.org/D70532 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D70840: [LLDB] [DWARF] Strip out the thumb bit from addresses on ARM

clayborg added a comment. Sorry for the delay. I have had medical issues going on for the past month. The hard part about Architecture vs ArchSpec is the architecture of the target doesn't always exactly match the arch of each module. But I agree that we need to be able to do more with

[Lldb-commits] [PATCH] D71440: Extending step-over range past calls was causing deadlocks, fix that.

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. That must have been fun to find! LGTM. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71440/new/ https://reviews.llvm.org/D71440

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

clayborg requested changes to this revision. clayborg added inline comments. This revision now requires changes to proceed. Comment at: lldb/source/Target/ThreadPlanStepOut.cpp:127-129 +if (!return_address_section) { + LLDB_LOGF(log, "Return address had no section."); +

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

clayborg added a comment. I believe it is ok for permissions to succeed as long as they don't return invalid permissions. For any address outside any mapped ranges, we should have zero as the permissions. Looking up address mappings for zero will return [0x - 0x1) --- no

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

clayborg added a comment. In D71372#1782538 , @jingham wrote: > In D71372#1782536 , @clayborg wrote: > > > I believe it is ok for permissions to succeed as long as they don't return > > invalid permissions. For

[Lldb-commits] [PATCH] D71372: [lldb] Add additional validation on return address in 'thread step-out'

clayborg added a comment. In D71372#1782567 , @jingham wrote: > In D71372#1782549 , @clayborg wrote: > > > In D71372#1782538 , @jingham wrote: > > > > > In D71372#1782536

[Lldb-commits] [PATCH] D71003: [lldb/DWARF] Switch to llvm location list parser

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

[Lldb-commits] [PATCH] D70989: [TypeCategory] IsApplicable doesn't seem to apply.

clayborg added a comment. This seems like it will make a lot more work happen. Many C++ formatters are regex based and are quite expensive to compare against type names. Seems worthwhile to skip if the language doesn't match? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[Lldb-commits] [PATCH] D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs

clayborg added a comment. We could just add a new flag to lldb-vscode like "--no-lldb-init" and always pass that when we run our test suite? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70886/new/ https://reviews.llvm.org/D70886

[Lldb-commits] [PATCH] D70979: [lldb][NFC] Migrate to raw_ostream in ArchSpec::DumpTriple

clayborg added inline comments. Comment at: lldb/include/lldb/Utility/ArchSpec.h:436 - void DumpTriple(Stream ) const; + void DumpTriple(llvm::raw_ostream ) const; Should we just make a static operator << function for this instead so we can use in

[Lldb-commits] [PATCH] D70883: [vscode.py] Make read_packet only return None when EOF

clayborg added a comment. Although I am ok with this patch, it will only work if the unexpected output comes before we expect any packets or perfectly in between packets. Not sure we should do this. For example this would work: random outut Content Length:2 {} And this would work:

[Lldb-commits] [PATCH] D71487: [LLDB] Fix address computation for member function linked with lld

clayborg added a comment. Can you attach a paste of the DWARF that is emitted by lld and the symbol table and annotate which one should be picked. I am having trouble understanding what the right solution for this fix would be. It makes me nervous to require a symbol in the symbol table since

[Lldb-commits] [PATCH] D71498: Fix ARM32 inferior calls

clayborg added a comment. > In other places you're replacing a reinterpret_cast with two c casts. > I guess this was done because two c++ casts were too long (?). In these cases > the second cast is not really needed, as uintptr_t->addr_t should convert > automatically. I think I'd prefer that

[Lldb-commits] [PATCH] D71498: Fix ARM32 inferior calls

clayborg added a comment. In D71498#1786343 , @jankratochvil wrote: > In D71498#1786319 , @clayborg wrote: > > > For the printf style statement, we can't use just one cast to "uintptr_t" > > because on 32 bit

[Lldb-commits] [PATCH] D71498: Fix ARM32 inferior calls

clayborg added a comment. Seems like it might be nice to have a macro defined in a LLDB header file like lldb-defines.h. Something like: #define PTR_TO_U64(x) ((uint64_t)(uintptr_t)(x)) Then we can make a unit test to verify it works on all systems. Seem like this issue will pop up all over

[Lldb-commits] [PATCH] D71498: Fix ARM32 inferior calls

clayborg added a comment. As I am reading this, I just wanted to send out a note of something else that can cause crashes in ARM/Thumb code. For anyone working with ARM/Thumb on systems that don't use the ARM and Thumb BKPT instruction when setting software breakpoints (like all lldb linux and

[Lldb-commits] [PATCH] D71268: [lldb/DWARF] Add support for DW_AT_loclists_base_FORM_loclistx

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

[Lldb-commits] [PATCH] D69273: ValueObject: Fix a crash related to children address type computation

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks good! CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69273/new/ https://reviews.llvm.org/D69273 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D69210: [Disassembler] Simplify MCInst predicates

clayborg added a comment. In D69210#1715679 , @vsk wrote: > Hm, this patch is bugging me. > > It looks a bit like instructions are still decoded multiple times in > different ways (e.g. in the `Decode` and > `CalculateMnemonicOperandsAndComment`

[Lldb-commits] [PATCH] D69210: [Disassembler] Simplify MCInst predicates

clayborg added a comment. In D69210#1717042 , @vsk wrote: > In D69210#1716861 , @clayborg wrote: > > > In D69210#1715679 , @vsk wrote: > > > > > Hm, this patch is bugging

[Lldb-commits] [PATCH] D68541: Implement 'up' and 'down' shortcuts in lldb gui

clayborg requested changes to this revision. clayborg added inline comments. This revision now requires changes to proceed. Comment at: lldb/packages/Python/lldbsuite/test/commands/gui/basicdebug/TestGuiBasicDebug.py:32 +self.child.send("s") # step +

[Lldb-commits] [PATCH] D68968: [android/process info] Introduce bundle id

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

[Lldb-commits] [PATCH] D68541: Implement 'up' and 'down' shortcuts in lldb gui

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks good! With a little work, "gui" mode can really become great. I hope to see more patches? Happy to discuss next stops on the mailing list if you are interested! Repository:

[Lldb-commits] [PATCH] D68968: [android/process list] use arg0 as fallback for process name

clayborg added a comment. I think it would be nice to have a new member to ProcessInfo like: std::string m_bundle_id; This notion works for MacOS and for Android. When launching on iOS, we need to application bundle identifier to do the launching as the install of the app registers the app

[Lldb-commits] [PATCH] D68961: Add support for DW_AT_export_symbols for anonymous structs

clayborg added a comment. Have many compilers supported DW_AT_export_symbols for a while now? If not, are there any serious issues introduced here that would change debugger behavior if this attribute is not emitted by a compiler? Or is this a new fix in clang that was recently introduced in

[Lldb-commits] [PATCH] D68968: [android/process info] Introduce bundle id

clayborg added a comment. sounds like we have a plan! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D68968/new/ https://reviews.llvm.org/D68968 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D69106: MemoryRegion: Print "don't know" permission values as such

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Sounds good, thanks for doing this CHANGES SINCE LAST ACTION https://reviews.llvm.org/D69106/new/ https://reviews.llvm.org/D69106 ___

[Lldb-commits] [PATCH] D69106: MemoryRegion: Print "don't know" permission values as such

clayborg added a comment. Looks good. Do we maybe want to use "unknown" instead of "don't know" when printing out the long for of a MemoryRegionInfo::OptionalBool? Or maybe "???"? Comment at: test/Shell/Minidump/memory-region-from-module.yaml:22 # CHECK1:

[Lldb-commits] [PATCH] D69100: COFF: Create a separate "section" for the file header

clayborg added inline comments. Comment at: source/Plugins/ObjectFile/PECOFF/ObjectFilePECOFF.cpp:794 +SectionSP header_sp = std::make_shared( +module_sp, this, ~user_id_t(0), ConstString("header"), +eSectionTypeOther, m_coff_header_opt.image_base,

[Lldb-commits] [PATCH] D69105: minidump: Create memory regions from the sections of loaded modules

clayborg added inline comments. Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:348-349 + bool is_complete; + std::tie(*m_memory_regions, is_complete) = + m_minidump_parser->BuildMemoryRegions(); + Might be nice to just assign memory

[Lldb-commits] [PATCH] D70885: [lldb] Use explicit lldb commands on tests

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. We shouldn't be running the test suite that allows your ~/.lldbinit file to be run. This can really hose up many things, so we should change the lldb-vscode test suite to not

[Lldb-commits] [PATCH] D70884: [lldb] Fix TestFormattersSBAPI test

clayborg accepted this revision. clayborg added inline comments. This revision is now accepted and ready to land. Comment at: lldb/packages/Python/lldbsuite/test/python_api/formatters/TestFormattersSBAPI.py:71 -format.format = lldb.eFormatOctal +

[Lldb-commits] [PATCH] D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Would be good to write a test for this where the init file does "script print('hello')" which would hose up the connection Repository: rG LLVM Github Monorepo CHANGES SINCE

[Lldb-commits] [PATCH] D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs

clayborg added inline comments. Comment at: lldb/tools/lldb-vscode/lldb-vscode.cpp:1207-1210 + auto arguments = request.getObject("arguments"); + const auto skipInitFiles = GetBoolean(arguments, "skipInitFiles", false); + g_vsc.debugger = +

[Lldb-commits] [PATCH] D70882: Add skipInitFiles option to lldb-vscode initialize

clayborg added a comment. If MS VS Code IDE itself will never pass this to the "initialize" packet, then we might just want to use an env var instead? You should probably get rid of https://reviews.llvm.org/D70886 and just modify this patch? Repository: rG LLVM Github Monorepo CHANGES

[Lldb-commits] [PATCH] D70886: [lldb-vscode] capture the debuggers file handles before lldbinit runs

clayborg added a comment. Abandon this one and modify https://reviews.llvm.org/D70882? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D70886/new/ https://reviews.llvm.org/D70886 ___ lldb-commits mailing

[Lldb-commits] [PATCH] D70906: [lldb] Move register info "augmentation" from gdb-remote into ABI

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/D70906/new/ https://reviews.llvm.org/D70906

<    8   9   10   11   12   13   14   15   16   17   >