[Lldb-commits] [PATCH] D39825: [lldb] Fix cu_offset for dwo/dwp used by DWARFCompileUnit::Index

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks fine to me as long as you got a clean test suite run. Be sure to keep an eye on the buildbots after this goes in. Repository: rL LLVM https://reviews.llvm.org/D39825 _

[Lldb-commits] [PATCH] D39825: [lldb] Fix cu_offset for dwo/dwp used by DWARFCompileUnit::Index

clayborg added a comment. If no tests currently fail due to this, then we need to add some. Repository: rL LLVM https://reviews.llvm.org/D39825 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listin

[Lldb-commits] [PATCH] D39844: CompilerType: Add ability to retrieve an integral template argument

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Just a few changes. - I would like the see the SBType returned for the integer template types as it is what I would expect to happen. - we should add default implementations for

[Lldb-commits] [PATCH] D39844: CompilerType: Add ability to retrieve an integral template argument

clayborg added inline comments. Comment at: include/lldb/Symbol/TypeSystem.h:356-360 + virtual CompilerType GetTypeTemplateArgument(lldb::opaque_compiler_type_t type, + size_t idx) = 0; + virtual std::pair + GetIntegralTemplateArgumen

[Lldb-commits] [PATCH] D39844: CompilerType: Add ability to retrieve an integral template argument

clayborg added a comment. I see your point. But if we do ask a template parameter for its type, I would like to be able to get it's type somehow. Seems wrong to leave this out. I know it doesn't mirror clang, but we should do the right thing here. At least at the SB API layer. I am fine with le

[Lldb-commits] [PATCH] D39896: Remove last Host usage from ArchSpec

clayborg added a comment. I just saw Jim's email for this, Please comment in this bug so we can all see the info in one location. Accepting pending the outcome of the discussion that Jim start in email that I am pasting below: > I'm not sure we have enough instances to decide on better organiza

[Lldb-commits] [PATCH] D39578: Fix a couple of self-assignments using memcpy.

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. First change looks good. Second one we can probably avoid doing anything in Value::AppendDataToHostBuffer and return 0. No need to copy data over itself. Comme

[Lldb-commits] [PATCH] D39967: Refactoring of MemoryWrite function

clayborg added a comment. I believe we normally just clang format the changes. This is done with: svn diff --diff-cmd=diff -x-U0 | $llvm_src/llvm/tools/clang/tools/clang-format/clang-format-diff.py -i -binary $llvm_build/bin/clang-format git diff -U0 HEAD^ | $llvm_src/llvm/tools/clang/tool

[Lldb-commits] [PATCH] D39969: Set error status in ObjectFile::LoadInMemory if it is not set

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Before working on a file for a diff you will submit, clang format it first, and then check it in. No need for review on clang format only changes. So please clang format first an

[Lldb-commits] [PATCH] D39967: Refactoring of MemoryWrite function

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Please clang format and check in without any changes to the functionality and then make a patch that only has your functional changes. Repository: rL LLVM https://reviews.llv

[Lldb-commits] [PATCH] D40022: Corrected two typos and removed unused variable

clayborg accepted this revision. clayborg added a comment. The "--set-pc-to-entry" change is fine. No need for review when removing unused variables or clang formatting, but it would be nice to do those commits separately. Repository: rL LLVM https://reviews.llvm.org/D40022 _

[Lldb-commits] [PATCH] D40079: Make sure DataBufferLLVM contents are writable

clayborg added a comment. I had suggested in https://reviews.llvm.org/D38142 that we have ObjectFileELF check the file type of the file and only map with write abilities if the ELF file is an object file since that is the only time we need relocations. If we can pass a flag through to indicate

[Lldb-commits] [PATCH] D40133: elf-core: Convert remaining register context to use register set maps

clayborg added inline comments. Comment at: source/Plugins/Process/elf-core/elf-core-enums.h:58 +enum class CoreRegset : uint8_t { GPR, FPR, PPC_VMX, PPC_VSX }; + Seems weird to have PPC_VMX and PPC_VSX define in a CoreRegSet? Do these need to be specific for

[Lldb-commits] [PATCH] D40133: elf-core: Convert remaining register context to use register set maps

clayborg added inline comments. Comment at: source/Plugins/Process/elf-core/elf-core-enums.h:58 +enum class CoreRegset : uint8_t { GPR, FPR, PPC_VMX, PPC_VSX }; + labath wrote: > clayborg wrote: > > Seems weird to have PPC_VMX and PPC_VSX define in a CoreRegSet

[Lldb-commits] [PATCH] D40212: refactor: Unify+simplify DWARFCompileUnit ctor+Clear() into in-class initializers

clayborg added a comment. Good change in the header file. I am not sure I like the destruct this object in place and replace with new version... If this is commonly done and acceptable form of C++ I would be ok with it, but I agree with Pavel, it seems a little bit off the books. https://revi

[Lldb-commits] [PATCH] D40214: performance: Prevent needless DWARFCompileUnit::Clear() on freshly ctor-ed object

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. See inline comments. Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp:74 lldb::offset_t *offset_ptr) { - Clear(

[Lldb-commits] [PATCH] D40216: #if 0 for DWARFDebugInfo::Find() as it is unused

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Feel free to remove any unused code. No need for review on dead code removal. So just remove the code, don't add #if 0 https://reviews.llvm.org/D40216 ___

[Lldb-commits] [PATCH] D40212: refactor: Unify+simplify DWARFCompileUnit ctor+Clear() into in-class initializers

clayborg added a comment. In https://reviews.llvm.org/D40212#929740, @jankratochvil wrote: > In https://reviews.llvm.org/D40212#929716, @clayborg wrote: > > > Good change in the header file. > > > If you mean the in-class initializers they obviously cannot be used without > the in-place construc

[Lldb-commits] [PATCH] D40311: elf-core: Split up parsing code into os-specific functions

clayborg added inline comments. Comment at: source/Plugins/Process/elf-core/ProcessElfCore.cpp:488 +ELFNote note = ELFNote(); +note.Parse(segment, &offset); + Do we need to check anything after parsing a note here to ensure it parsed? Can offset end up n

[Lldb-commits] [PATCH] D40283: lldb: Use the DWARF linkage name when importing C++ methods

clayborg added a comment. Jim will need to ok this. A few concerns follow. Most of the time when we don't get the DWARF -> AST conversion right, it can mean that we might code gen incorrect code. Is there not enough information for the GNU abi_tag in the DWARF to actually re-create these classe

[Lldb-commits] [PATCH] D40212: DWZ 02/12: refactor: Unify+simplify DWARFCompileUnit ctor+Clear() into in-class initializers + Extract()

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Won't hold up the checkin for the cleaner while loop, but feel free to fix that and checkin if it works. Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp:2

[Lldb-commits] [PATCH] D40466: DWZ 03/12: DWARFCompileUnit split to DWARFCompileUnitData

clayborg added inline comments. Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.h:216 + DWARFCompileUnitData *m_data; + Is there a reason this is a member variable that I am not seeing? Seems we could have this class inherit from DWARFCompileUnit

[Lldb-commits] [PATCH] D40467: DWZ 04/12: Separate Offset also into FileOffset

clayborg added a comment. It would be much easier to read this patch if there were no renames from "*offset" to "*file_offset". Can we remove these extra renames where it isn't needed? Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.h:15 #include +#include ---

[Lldb-commits] [PATCH] D40468: DWZ 05/12: Support reading section ".gnu_debugaltlink"

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Just move the eSectionTypeDWARFGNUDebugAltLink enumerator to the end of the enum and this is good to go. Comment at: include/lldb/lldb-enumerations.h:647 eS

[Lldb-commits] [PATCH] D40469: DWZ 06/12: Mask DW_TAG_partial_unit as DW_TAG_compile_unit for Tag()

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Seems like it would be cleaner to leave DWARFDebugInfoEntry and DWARFDie alone and just make clients deal with accepting DW_TAG_partial_unit when and where they need to. I don't

[Lldb-commits] [PATCH] D40470: DWZ 07/12: Protect DWARFCompileUnit::m_die_array by a new mutex

clayborg requested changes to this revision. clayborg added inline comments. This revision now requires changes to proceed. Comment at: source/Plugins/SymbolFile/DWARF/DWARFCompileUnit.cpp:123 size_t DWARFCompileUnit::ExtractDIEsIfNeeded(bool cu_die_only) { - const size_t initi

[Lldb-commits] [PATCH] D40471: DWZ 08/12: DWARFCompileUnit::ExtractDIEsIfNeeded can now expand AllDiesButCuDieOnlyForPartialUnits

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. See inline comments. Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:2050-2052 +if (dwarf_cu->ExtractDIEsIfNeeded(DWARFCompileUnit:: +

[Lldb-commits] [PATCH] D40472: DWZ 09/12: Protect DWARFDebugInfo::m_compile_units by a new mutex

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. DWARFDebugInfo::ParseCompileUnitHeadersIfNeeded() seems broken, see inlined comments. Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp:114-119 +

[Lldb-commits] [PATCH] D40473: DWZ 10/12: Adjust existing code for the DWZ support.

clayborg requested changes to this revision. clayborg added inline comments. This revision now requires changes to proceed. Comment at: source/Plugins/SymbolFile/DWARF/DWARFDebugInfo.cpp:199-202 + if (die_ref.cu_offset == DW_INVALID_OFFSET + // FIXME: Workaround default cu_

[Lldb-commits] [PATCH] D40283: lldb: Use the DWARF linkage name when importing C++ methods

clayborg added a comment. ok, sounds like the abi_tag stuff is meant to just affect the mangled named with no code gen implications. Jim should ok this, but I am ok if he is. https://reviews.llvm.org/D40283 ___ lldb-commits mailing list lldb-commit

[Lldb-commits] [PATCH] D40539: Allow using the object file name for debug symbol resolution

clayborg added a comment. I am not sure I follow this patch. We are adding a FileSpec whose path is just the basename of the current ELF file? What do we do with that? Do we look in certain directories to try and find this file? How this this basename added to the list end up getting found in t

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. A better solution would be to initialize UUID::m_num_uuid_bytes with zero and only set it to a valid value if we set bytes into it. Then UUID::IsValid() becomes easy: bool UUI

[Lldb-commits] [PATCH] D40539: Allow using the object file name for debug symbol resolution

clayborg added a comment. In https://reviews.llvm.org/D40539#937854, @sas wrote: > Basically, if you have a `.debug` directory in the same directory where the > original object file is, you can have debug symbols there. For instance, you > can have: > > /my/project/myElf.exe > /my/project/.

[Lldb-commits] [PATCH] D40537: Simplify UUID::IsValid()

clayborg added a comment. No one is relying on the 16 bytes of zeroes that I know of. UUID::IsValid() is the test that everyone uses to tell if the UUID is valid or not. I still prefer to just set the length to zero as this does allow us to have a UUID of all zeroes if needed. https://reviews

[Lldb-commits] [PATCH] D40539: Allow using the object file name for debug symbol resolution

clayborg added a comment. ok, this is fine then. Just need to test somehow. https://reviews.llvm.org/D40539 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D40615: Fix assertion in ClangASTContext

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Few nits, but nothing that would hold up the patch. Looks good. Comment at: include/lldb/Symbol/CompilerType.h:294 + struct IntegralTemplateArgument; + ---

[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections

clayborg added a comment. Why would we text scrape when we can test the API? https://reviews.llvm.org/D40616 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D40466: DWZ 03/12: DWARFCompileUnit split to DWARFCompileUnitData

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Take a look how the LLVM DWARF parser handles its units. It makes a DWARFUnit base class that the compile unit inherits from. That can then be used for type units and compile uni

[Lldb-commits] [PATCH] D40467: DWZ 04/12: Separate Offset also into FileOffset

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Please undo all renaming from offset to file_offset. The "offset" you get from a DIE should always be the absolute .debug_info offset. No need to say file_offset. Patch will be

[Lldb-commits] [PATCH] D40745: Add a clang-ast subcommand to lldb-test

clayborg added a comment. One really nice way we can get a lot of testing of the DWARF to clang::ASTContext conversion is to: 1 - compile a source file with clang and dumps the AST for a specific type as the compiler knows it 2 - using the .o file with debug info from step 1, load it into LLDB a

[Lldb-commits] [PATCH] D40821: Fix const-correctness in RegisterContext methods, NFC

clayborg added a comment. Seems wrong to remove the const on structs that don't need to change in order to make the write happen. Can't we just quiet the warnings with a const_cast inside the function call? https://reviews.llvm.org/D40821 ___ lldb

[Lldb-commits] [PATCH] D40821: Fix const-correctness in RegisterContext methods, NFC

clayborg added a comment. In https://reviews.llvm.org/D40821#945379, @vsk wrote: > In https://reviews.llvm.org/D40821#945314, @clayborg wrote: > > > Seems wrong to remove the const on structs that don't need to change in > > order to make the write happen. Can't we just quiet the warnings with a

[Lldb-commits] [PATCH] D40869: Optimize fake ELF section lookup while parsing symbols in ObjectFileELF

clayborg added a comment. Looks fine to me, but a test would be nice as Zach suggested. I am guessing before we made way too many sections. Should be easy to conjure up a test and verify the same sections is used. https://reviews.llvm.org/D40869 _

[Lldb-commits] [PATCH] D41070: llgs: Propagate the environment when launching the inferior from command line

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Anything that launches a process should use the ProcessLaunchInfo. Nice patch. Comment at: source/Plugins/Process/gdb-remote/GDBRemoteCommunicationServerLLGS.h:46 - /

[Lldb-commits] [PATCH] D41069: NPL: Clean up handling of inferior exit

clayborg added a comment. Is the lldb_private::Process we have an exit status in the class itself. The first person to set the exit status wins and no one can set it twice. Doesn't look like what we are doing here. I am not able to tell what actually fixes things here? Comme

[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections

clayborg added a comment. I think GetFileSize() should remain the number of bytes of the section on disk and we should add new API if we need to figure out the decompressed size. Or maybe when we get bytes from a compressed section we are expected to always just get the raw bytes, then we check

[Lldb-commits] [PATCH] D41086: [lldb] Check that a regex is valid before searching by regex for a symbol in a pdb.

clayborg added a comment. We should have a dedicated API that actually searches for types using a lldb_private::RegularExpression. Why do we even try to create a regular expression in SymbolFilePDB::FindTypes()? Was it used in testing? No API in LLDB currently expects this and all other API in

[Lldb-commits] [PATCH] D41092: Enable more abilities in SymbolFilePDB

clayborg added inline comments. Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:127 + } + lldbassert(m_session_up.get()); + if (auto enum_tables_up = m_session_up->getEnumTables()) { I am assuming this assert won't fire if we give this an ELF or Ma

[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections

clayborg added a comment. Sounds good,. So the solution will be: - Section::GetFileSize() will return the size in bytes of the section data as it appears in the file - Section::GetByteSize() will return the size in bytes for when this section is loaded into process memory (we might consider ren

[Lldb-commits] [PATCH] D41070: llgs: Propagate the environment when launching the inferior from command line

clayborg added a comment. No worries then. No need to make a new enum if this is just two places and they aren't all setting the same flags. https://reviews.llvm.org/D41070 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm

[Lldb-commits] [PATCH] D41169: ObjectFile: remove ReadSectionData/MemoryMapSectionData mutual recursion

clayborg accepted this revision. clayborg added a comment. This was left over from before we mmap'ed the entire object file into memory. Removing it is fine as the backing DataBufferSP for the object file will be mmaped or not depending on where the file was loaded from and if the section isn't

[Lldb-commits] [PATCH] D40616: ObjectFileELF: Add support for compressed sections

clayborg accepted this revision. clayborg added a comment. Move #include of "llvm/Object/Decompressor.h" into CPP file and this is good to go. Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.h:24 #include "lldb/lldb-private.h" +#include "llvm/Object/Decompressor.h"

[Lldb-commits] [PATCH] D41245: Reduce x86 register context boilerplate.

clayborg accepted this revision. clayborg added a comment. Looks good https://reviews.llvm.org/D41245 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D41086: [lldb] Check that a regex is valid before searching by regex for a symbol in a pdb.

clayborg added inline comments. Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:393 // try really hard not to use a regex match. - bool is_regex = false; - if (name_str.find_first_of("[]?*.-+\\") != std::string::npos) { -// Trying to compile an invalid regex

[Lldb-commits] [PATCH] D41352: debugserver: Propagate environment in launch-mode (pr35671)

clayborg added a comment. We already have an option for this named "--forward-env". It might be better to fix this by adding the "--forward-env" argument to the debugserver launch during testing? When we launch through LLDB, it forwards all environment variables manually. Maybe we add a "--forw

[Lldb-commits] [PATCH] D41359: Add Utility/Environment class for handling... environments

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. ok as long as we don't want to return const reference when returning Environment values in getters and setters. Comment at: include/lldb/Target/Platform.h:636 - virtu

[Lldb-commits] [PATCH] D41352: debugserver: Propagate environment in launch-mode (pr35671)

clayborg added a comment. I am ok either way, but I do want to make sure Jason gets input before we do anything. He might be out for a few weeks over the break, so there might be a delay. https://reviews.llvm.org/D41352 ___ lldb-commits mailing li

[Lldb-commits] [PATCH] D41359: Add Utility/Environment class for handling... environments

clayborg added a comment. Thanks for the explanation. Good to go. https://reviews.llvm.org/D41359 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D41352: debugserver: Propagate environment in launch-mode (pr35671)

clayborg added a comment. So the main question is what do we expect to happen by default. I kind of agree that if we launch the inferior directly when launching I would expect the environment to be passed along. Jason: since we always just launch debugserver for Xcode in the mode that doesn't e

[Lldb-commits] [PATCH] D41086: [lldb] Check that a regex is valid before searching by regex for a symbol in a pdb.

clayborg added inline comments. Comment at: source/Plugins/SymbolFile/PDB/SymbolFilePDB.cpp:394 + if (name_str.find_first_of("[]?*.-+\\") != std::string::npos) +FindTypesByRegex(RegularExpression(name_str), max_matches, types); else You should make the Re

[Lldb-commits] [PATCH] D40079: Make sure DataBufferLLVM contents are writable

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. See inlined comment. Quick fix and this will be good to go. Comment at: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp:409-410 if (!data_sp) { -data_sp

[Lldb-commits] [PATCH] D41352: debugserver: Propagate environment in launch-mode (pr35671)

clayborg added a comment. The existing code for the "--forward-env" does this: case 'F': // Pass the current environment down to the process that gets launched { char **host_env = *_NSGetEnviron(); char *env_entry; size_t i; for (i = 0; (env_entry = host_env[i])

[Lldb-commits] [PATCH] D41352: debugserver: Propagate environment in launch-mode (pr35671)

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. So we can also specify extra environment variable using the "--env" option with debugserver and your current fix will break that. https://reviews.llvm.org/D41352

[Lldb-commits] [PATCH] D41352: debugserver: Propagate environment in launch-mode (pr35671)

clayborg added a comment. Environment vars might be set by using --env, so those need to go into "inferior_envp" first. If we are launching, we will copy only the host environment vars that haven't been already set manually (they don't already exist in the inferior_envp). https://reviews.llvm

[Lldb-commits] [PATCH] D41352: debugserver: Propagate environment in launch-mode (pr35671)

clayborg added a comment. This is exposing a bug where if we have options like: % debugserver --env USER=hello --forward-env -- /bin/ls -lAF We would set USER to hello, then "--forward-env" would clobber the setting... https://reviews.llvm.org/D41352 __

[Lldb-commits] [PATCH] D41352: debugserver: Propagate environment in launch-mode (pr35671)

clayborg added inline comments. Comment at: tools/debugserver/source/debugserver.cpp:1426 +for (i = 0; (env_entry = host_env[i]) != NULL; ++i) + remote->Context().PushEnvironment(env_entry); + } We need to check if the env var is already in the environm

[Lldb-commits] [PATCH] D40079: Make sure DataBufferLLVM contents are writable

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks good, thanks for the making the changes. This will make future tweaks to reading of file data in object files just a single change in ObjectFile.cpp. https://reviews.llvm.org/D40079

[Lldb-commits] [PATCH] D41086: [lldb] Check that a regex is valid before searching by regex for a symbol in a pdb.

clayborg added a comment. If you load a exe file that has a PDB file, people can currently run: (lldb) type lookup "char*" If no testing is using the regular expression stuff, then just pull it out. Do we have unit tests that depend on this working? If not, lets just pull it out from the Sy

[Lldb-commits] [PATCH] D41086: [lldb] Stop searching for a symbol in a pdb by regex

clayborg added a comment. Looks good! Repository: rL LLVM https://reviews.llvm.org/D41086 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D41352: debugserver: Propagate environment in launch-mode (pr35671)

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Thanks for all the revisions. Looks good. https://reviews.llvm.org/D41352 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://li

[Lldb-commits] [PATCH] D41745: Handle O reply packets during qRcmd

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. In https://reviews.llvm.org/D41745#970362, @owenpshaw wrote: > Updated patch with new function name suggested by @clayborg. Added unit test > and changed to llvm::function_ref as sugges

[Lldb-commits] [PATCH] D41533: Advanced guessing of rendezvous breakpoint

clayborg added a comment. Looks fine. Set the breakpoint using the list of names and delete the breakpoint if you get no locations and this will be good to go. Comment at: source/Plugins/DynamicLoader/POSIX-DYLD/DynamicLoaderPOSIXDYLD.cpp:365-368 +static const char *Debug

[Lldb-commits] [PATCH] D41584: Check existence of each required component during construction of LLVMCDisassembler.

clayborg requested changes to this revision. clayborg added inline comments. This revision now requires changes to proceed. Comment at: source/Plugins/Disassembler/llvm/DisassemblerLLVMC.cpp:77-83 + std::unique_ptr m_instr_info; + std::unique_ptr m_reg_info; + std::unique_ptr

[Lldb-commits] [PATCH] D41584: Check existence of each required component during construction of LLVMCDisassembler.

clayborg added a comment. I like the overall direction this patch is taking, just a few fixes. https://reviews.llvm.org/D41584 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D41584: Check existence of each required component during construction of LLVMCDisassembler.

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Fix comment spacing as mentioned in inline comments and this is good to go. Comment at: source/Plugins/Disassembler/llvm/DisassemblerLLVMC.h:98-101 + // Since we need to

[Lldb-commits] [PATCH] D41533: Advanced guessing of rendezvous breakpoint

clayborg added a comment. Looks good. Repository: rL LLVM https://reviews.llvm.org/D41533 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D41902: Remove Platform references from the Host module

clayborg added a comment. As long as: % lldb /path/to/Foo.app (lldb) r Still works, then I am fine with this. The resolve executable should find the executable down inside the app bundle (like "/path/to/Foo.app/Contents/MacOS/Foo" for desktop apps and "/path/to/Foo.app/Foo" for iOS apps).

[Lldb-commits] [PATCH] D41909: Fix deadlock in dwarf logging

clayborg added a comment. Tough to call on this one. Yes the function is dumping simple stuff, but it is using m_arch, m_file and m_objname. It could cause crashes in multi-threaded environments. Is the deadlock caused by an A/B lock issue? https://reviews.llvm.org/D41909 __

[Lldb-commits] [PATCH] D40283: lldb: Use the DWARF linkage name when importing C++ methods

clayborg added a comment. Sounds like finding a clang expert to clarify what Jim last asked for is the way forward. Do a source control "blame" command and see who worked on the code in the area of clang and maybe add them as reviewers so they can comment? I agree with Jim that this sounds like

[Lldb-commits] [PATCH] D40283: lldb: Use the DWARF linkage name when importing C++ methods

clayborg added a comment. Added Adrian Prantl as a reviewer in case he has any input. Adrian: is there any way that a DIE is marked up with an extra attribute when the asm name is explicitly set? It would be great to know this from the DWARF so we don't have to end up setting the ASM name for e

[Lldb-commits] [PATCH] D40283: lldb: Use the DWARF linkage name when importing C++ methods

clayborg added a comment. And I do agree with Jim that we don't want to have to mangle the typename to see if it matches, that is too much work. https://reviews.llvm.org/D40283 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.

[Lldb-commits] [PATCH] D41909: Fix deadlock in dwarf logging

clayborg added a comment. In https://reviews.llvm.org/D41909#973299, @tberghammer wrote: > Why do we need to lock the Module mutex in SymbolVendor::FindFunctions? I > think a better option would be to remove that lock and if it is needed then > lock it just for the calls where it necessary. The

[Lldb-commits] [PATCH] D41909: Fix deadlock in dwarf logging

clayborg added a comment. We will need to pass the mutex down into any call that needs to let the mutex go. At least that is the only way I could see this working... But then all functions that do anything lazily will need this same treatment. Lot of trouble for the logging. https://reviews.l

[Lldb-commits] [PATCH] D41909: Fix deadlock in dwarf logging

clayborg added a comment. Yeah, the threading was added later. It started out as single threaded and didn't have this problem. https://reviews.llvm.org/D41909 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/m

[Lldb-commits] [PATCH] D41997: Build virtual override tables in DWARFASTParserClang::CompleteTypeFromDWARF

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Very cool and close. It would be nice to function correctly without asserts, see inlined comment. Comment at: Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Very nice overall. See inlined comments. Big issues are: - GDBRemoteCommunication::WriteEscapedBinary() is not needed as StreamGDBRemote::PutEscapedBytes() does this already - Re

[Lldb-commits] [PATCH] D41702: Add SysV Abi for PPC64le

clayborg added a comment. Looks nice. Only nit is we probably don't need the m_endian member variable. See inlined comment. Comment at: source/Plugins/ABI/SysV-ppc64/ABISysV_ppc64.h:114 + + lldb::ByteOrder m_endian; }; Most other code uses "m_byte_order" as

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

clayborg added inline comments. Comment at: include/lldb/Target/Process.h:1916 + //-- + virtual bool BeginWriteMemoryBatch() { return true; } + owenpshaw wrote: > clayborg wrote: > > labath wrote:

[Lldb-commits] [PATCH] D42195: [lldb] Generic base for testing gdb-remote behavior

clayborg added a comment. Looks fine to me. Wait for Pavel to give it the OK since he did more of the lldb-server testing stuff. https://reviews.llvm.org/D42195 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin

[Lldb-commits] [PATCH] D42145: [lldb] Use vFlash commands when writing to target's flash memory regions

clayborg added a comment. My objection to the BeginWriteMemoryBatch and EndWriteMemoryBatch is users must know to emit these calls when they really just want to call process.WriteMemory() and not worry about how it is done. Much like writing to read only code when setting breakpoints, we don't

[Lldb-commits] [PATCH] D41997: Build virtual override tables in DWARFASTParserClang::CompleteTypeFromDWARF

clayborg accepted this revision. clayborg added inline comments. This revision is now accepted and ready to land. Comment at: Plugins/SymbolFile/DWARF/DWARFASTParserClang.cpp:2106 +// a vtable entry) from overloads (which require distinct entries). +static bool isOverload(clang::

[Lldb-commits] [PATCH] D42206: If kevent() is interrupted by signal (or is being debugged) and we get EINTR, retry

clayborg accepted this revision. clayborg added a comment. This is a very common unix thing. Repository: rLLD LLVM Linker https://reviews.llvm.org/D42206 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mail

[Lldb-commits] [PATCH] D42206: If kevent() is interrupted by signal (or is being debugged) and we get EINTR, retry

clayborg added a comment. Are we going to test each unix call that can fail with EINTR? Seems a bit overkill. Repository: rLLD LLVM Linker https://reviews.llvm.org/D42206 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llv

[Lldb-commits] [PATCH] D42281: WIP: compile the LLDB tests out-of-tree

clayborg added a comment. Looks like a good start. It might be nice to validate that after "clean" that we have no files that are untracked in the test directory? This could help us to verify that we don't leave artifacts around. Comment at: packages/Python/lldbsuite/test/dot

[Lldb-commits] [PATCH] D42280: Wrap all references to build artifacts in the LLDB testsuite in TestBase::getBuildArtifact()

clayborg accepted this revision. clayborg added a comment. Nice! https://reviews.llvm.org/D42280 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D40283: lldb: Use the DWARF linkage name when importing C++ methods

clayborg added a comment. I am fine with checking this. The only issue on my end is the extra memory that will be needed to store these often huge mangled names in every AST context for the 0.0001% of the time it is actually needed. https://reviews.llvm.org/D40283 __

[Lldb-commits] [PATCH] D40283: lldb: Use the DWARF linkage name when importing C++ methods

clayborg added a comment. In https://reviews.llvm.org/D40283#983997, @aprantl wrote: > As Greg pointed out C++ (and Swift) mangled names can be enormous, so it > would definitely have an impact on the memory footprint (unless we are only > passing `StringRef`s around. Are we?) Most strings se

[Lldb-commits] [PATCH] D39967: Refactoring of MemoryWrite function

clayborg added a comment. This will need a test, preferable covering all of the cases: - breakpoint right at start with extra data after - breakpoint right at start with no extra data after - breakpoint in middle of ranges with data before and after - breakpoint at end of range with no data after

[Lldb-commits] [PATCH] D39969: Set error status in ObjectFile::LoadInMemory if it is not set

clayborg added a comment. I am not sure we can say for sure that a breakpoint intersected with the write here? I would rather just have an error saying something like "only %u of %u bytes in section %s were written". Extra credit for checking if there are overlapping breakpoints and appending "

[Lldb-commits] [PATCH] D39969: Set error status in ObjectFile::LoadInMemory if it is not set

clayborg added a comment. And yes, if the write memory can only write fewer byte than requested, it won't be an error if at least some bytes were written https://reviews.llvm.org/D39969 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http

<    1   2   3   4   5   6   7   8   9   10   >