[Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

clayborg added a comment. The default return value of WillResume should be no error. Sorry for not catching this. The core file Process subclasses will need to override this manually. Repository: rL LLVM https://reviews.llvm.org/D37651 ___

[Lldb-commits] [PATCH] D37934: Fix compatibility with OpenOCD debug stub.

clayborg added a comment. I like the solution detailed above by tatyana-krasnukha Repository: rL LLVM https://reviews.llvm.org/D37934 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D38153: Inhibit global lookups for symbols in the IR dynamic checks

clayborg requested changes to this revision. clayborg added inline comments. This revision now requires changes to proceed. Comment at: source/Plugins/ExpressionParser/Clang/ClangASTSource.cpp:651-652 return; + if (name_unique_cstr[0] == '_' && name_unique_cstr[1] == '$')

[Lldb-commits] [PATCH] D37926: Fix the SIGINT handlers

clayborg added a comment. Looks fine to me. https://reviews.llvm.org/D37926 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

clayborg added inline comments. Comment at: source/Commands/CommandObjectTarget.cpp:2056-2058 +if (m_interpreter.WasInterrupted()) { + break; +} Remove braces Comment at:

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

clayborg added a comment. We should have a test. The test would need to use pexpect or something similar. If anyone has any pointer on how to make a test for this, please chime in. I was thinking just a pexpect based test. https://reviews.llvm.org/D37923

[Lldb-commits] [PATCH] D37934: Fix compatibility with OpenOCD debug stub.

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. As long as everyone agrees that no threads from qfThreadInfo means there is one thread whose thread ID is 1 then this is ok. Repository: rL LLVM https://reviews.llvm.org/D37934

[Lldb-commits] [PATCH] D37923: Implement interactive command interruption

clayborg added a comment. ok. Then back to the "can we use StringRef"? We might be able to check if the second.data() is NULL? It might be NULL if the string doesn't end with a newline. It it does, it might be empty but contain a pointer to the '\0' byte? https://reviews.llvm.org/D37923

[Lldb-commits] [PATCH] D37651: Fix for bug 34532 - A few rough corners related to post-mortem debugging (core/minidump)

clayborg added a comment. Looks fine. https://reviews.llvm.org/D37651 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D38394: Fix dumping of characters with non-standard sizes

clayborg added a comment. Looks good. Would be nice to add support for byte sizes of 3, 5 and 7 to the unchecked version as noted in inline comments, or remove the function if no one is using this function. Just a few quick fixes and this will be good to go. Comment at:

[Lldb-commits] [PATCH] D38568: [lldb] Enable using out-of-tree dwps

clayborg added inline comments. Comment at: source/Host/common/Symbols.cpp:288-294 + // FIXME: at the moment llvm-dwp doesn't output build ids, + // nor does binutils dwp. Thus in the case of DWPs + // we skip uuids check. This needs to be

[Lldb-commits] [PATCH] D38323: Enable breakpoints and read/write GPRs for ppc64le

clayborg added a comment. Sorry for the delay, I was out on vacation for a week. Repository: rL LLVM https://reviews.llvm.org/D38323 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D38568: [lldb] Enable using out-of-tree dwps

clayborg added a comment. (sorry for the delay, I was out on vacation for a week) Repository: rL LLVM https://reviews.llvm.org/D38568 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D38394: Fix dumping of characters with non-standard sizes

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

[Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process

clayborg added a comment. Who is holding the lock and then fork'ing? Seems like we should fix that? I thought all log calls should be "lock, log, unlock". Why is this causing problems? https://reviews.llvm.org/D38938 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process

clayborg added a comment. Never mind, re-reading your original comment made it clear. https://reviews.llvm.org/D38938 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D38897: Add specific ppc64le hardware watchpoint handler

clayborg added a comment. Feel free to add me as a reviewer. Zach as well. https://reviews.llvm.org/D38897 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D38897: Add specific ppc64le hardware watchpoint handler

clayborg added a comment. I agree with all of Zach's suggestions. Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h:98-99 +uint32_t refcount; // Serves as enable/disable and reference counter. +long slot; // Saves the

[Lldb-commits] [PATCH] D37295: [lldb] Adjust UpdateExternalModuleListIfNeeded method for the case of *.dwo

clayborg added a comment. Can you explain the issue with an example? Comment at: source/Plugins/SymbolFile/DWARF/SymbolFileDWARF.cpp:1644-1656 +// A *.dwo file itself can have DW_AT_GNU_dwo_name (but no +// DW_AT_comp_dir) (clang 4.0 generates such

[Lldb-commits] [PATCH] D37154: lldb-mi: -var-update can hang when traversing complex types with pointers

clayborg added inline comments. Comment at: tools/lldb-mi/MICmdCmdVar.cpp:522 + continue; + +// Handle composite types (i.e. struct or arrays) Or even cleaner: ``` bool CMICmdCmdVarUpdate::ExamineSBValueForChange(lldb::SBValue ,

[Lldb-commits] [PATCH] D37154: lldb-mi: -var-update can hang when traversing complex types with pointers

clayborg requested changes to this revision. clayborg added inline comments. This revision now requires changes to proceed. Comment at: tools/lldb-mi/MICmdCmdVar.cpp:518-522 +// skip pointers and references to avoid infinite loop +if (member.GetType().GetTypeFlags() & +

[Lldb-commits] [PATCH] D37154: lldb-mi: -var-update can hang when traversing complex types with pointers

clayborg added inline comments. Comment at: tools/lldb-mi/MICmdCmdVar.cpp:525-526 +// Don't go down into pointers or references, to avoid a loop +lldb::SBType valueType = member.GetType(); +if (!valueType.IsPointerType() && !valueType.IsReferenceType()) + if

[Lldb-commits] [PATCH] D37295: [lldb] Adjust UpdateExternalModuleListIfNeeded method for the case of *.dwo

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Just add a more descriptive comment that states this exact issue more clearly as I didn't understand the issue from the comment that is there now (the comment that starts on line 1644).

[Lldb-commits] [PATCH] D38153: Inhibit global lookups for symbols in the IR dynamic checks

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

[Lldb-commits] [PATCH] D38323: Enable breakpoints and read/write GPRs for ppc64le

clayborg added a comment. Looks fine. One main questions for new linux archs in particular: is linux using the lldb-server to debug these days even when debugging locally? If so, then this patch only needs to implement both a native register content and not the lldb_private::RegisterContext

[Lldb-commits] [PATCH] D38323: Enable breakpoints and read/write GPRs for ppc64le

clayborg added inline comments. Comment at: source/Plugins/Process/Linux/NativeRegisterContextLinux_ppc64le.h:71 + RegInfo m_reg_info; + Reg m_gpr_ppc64le[48]; // 64-bit general purpose registers. + Could we share the structure that backs all registers from

[Lldb-commits] [PATCH] D32366: Set "success" status correctly

clayborg added a comment. Yes: lldbassert would be fine for that since those get compiled out during release. Patch looks fine. If we already have a test that would trigger the new "lldbassert" you will add, then no need for a special test for this, else we need a test that triggers this.

[Lldb-commits] [PATCH] D32366: Set "success" status correctly

clayborg added a comment. This is sure to trigger things in the test suite. We will need to ensure a few things: - test suite runs cleanly in debug mode after the lldbassert is added - without changes to CommandObjectRegister.cpp that the lldbassert is triggered, and if not, add a test

[Lldb-commits] [PATCH] D36620: Fix crash on parsing gdb remote packets

clayborg added a comment. We need a test for this to ensure we don't regress. https://reviews.llvm.org/D36620 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D37154: lldb-mi: -var-update can hang when traversing complex types with pointers

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Makes sense even though this can be quite expensive. https://reviews.llvm.org/D37154 ___ lldb-commits mailing list

[Lldb-commits] [PATCH] D37295: [lldb] Adjust UpdateExternalModuleListIfNeeded method for the case of *.dwo

clayborg added a comment. Thanks for the info. Does this seem like it is a compiler bug? Seems like there should be a DW_AT_comp_dir if the DW_AT_GNU_dwo_name is relative? Is it written in the DWO spec that if the DW_AT_comp_dir is empty then it is referring to itself? I would rather not hack

[Lldb-commits] [PATCH] D38568: [lldb] Enable using out-of-tree dwps

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks good. Repository: rL LLVM https://reviews.llvm.org/D38568 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D38938: Logging: provide a way to safely disable logging in a forked process

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Much better. https://reviews.llvm.org/D38938 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D39199: [lldbtests] Handle errors instead of crashing

clayborg added inline comments. Comment at: packages/Python/lldbsuite/test/dotest.py:52 def is_exe(fpath): +"""Returns true if fpath is an executable. We could add a default parameter here like: ``` def is_exe(fpath, fatal=False): if fatal and not

[Lldb-commits] [PATCH] D38897: Add specific ppc64le hardware watchpoint handler

clayborg added a comment. Yeah, I meant to suggest to remove that. Remove and submit as needed. Thanks again. https://reviews.llvm.org/D38897 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[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] 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:

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

[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

[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

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

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

[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

[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 >

[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

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

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

[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

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

[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

[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

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

[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

[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

[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. 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 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).

[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

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

[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^ |

[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

[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

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

[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

[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) { -

[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, ); + Do we need to check anything after parsing a note here to ensure it parsed? Can offset end up not

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

[Lldb-commits] [PATCH] D39515: Remove TestMyFirstWatchpoint and TestStepOverWatchpoint from basic_process category

clayborg added a comment. No objections from me as I don't use these categories. https://reviews.llvm.org/D39515 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits

[Lldb-commits] [PATCH] D39681: Implement core dump debugging for PPC64le

clayborg added inline comments. Comment at: source/Plugins/Process/elf-core/ThreadElfCore.h:183 lldb_private::DataExtractor m_vregset_data; + lldb_private::DataExtractor m_vsregset_data; /* For PPC VSX registers. */ labath wrote: > gpregset and fpregset

[Lldb-commits] [PATCH] D39545: Support scoped enums in the DWARF AST parser

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks good. https://reviews.llvm.org/D39545 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D39689: Add a dependency from check-lldb on lld

clayborg accepted this revision. clayborg added a comment. Do we want to add an option to our build system to try LLD where it is supported? Doesn't need to be part of this patch, but it would be great to be able to try it out on ELF based systems. https://reviews.llvm.org/D39689

[Lldb-commits] [PATCH] D39692: Disable tests in lang/c/shared_lib on Windows

clayborg added a comment. Are we planning on getting shared libraries working on windows? Or this is just an expression parser with shared libraries bug? Be nice to file a bug and mention it if it is something we are planning on fixing. https://reviews.llvm.org/D39692

[Lldb-commits] [PATCH] D38829: Python: SetOutputFileHandle doesn't work with IOBase

clayborg added a comment. In https://reviews.llvm.org/D38829#914300, @zturner wrote: > Ok, I wasn't aware of the libedit problem. > > I still don't like this approach, because it causes the design of the `File` > class to be influenced by a limitation of a 3rd party library. > > What about

[Lldb-commits] [PATCH] D39733: Simplify NativeProcessProtocol::GetArchitecture/GetByteOrder

clayborg requested changes to this revision. clayborg added a comment. This revision now requires changes to proceed. Looks fine. Seems like you should use your a const reference in a few places and this will be good to go? Comment at:

[Lldb-commits] [PATCH] D39733: Simplify NativeProcessProtocol::GetArchitecture/GetByteOrder

clayborg added a comment. If it isn't expensive to copy, then we should probably just return by value. https://reviews.llvm.org/D39733 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

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

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

[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

[Lldb-commits] [PATCH] D39487: Add float/vector registers for ppc64le

clayborg accepted this revision. clayborg added a comment. This revision is now accepted and ready to land. Looks fine. https://reviews.llvm.org/D39487 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D39436: Add regex support to file (-f) and module (-s) breakpoint options.

clayborg added a comment. In https://reviews.llvm.org/D39436#912828, @zturner wrote: > In https://reviews.llvm.org/D39436#912810, @clayborg wrote: > > > I was unhappy when we went over two pointers for a FileSpec when m_syntax > > was added due to the extra size. Anything we can do to make this

[Lldb-commits] [PATCH] D39436: Add regex support to file (-f) and module (-s) breakpoint options.

clayborg added a comment. I was unhappy when we went over two pointers for a FileSpec when m_syntax was added due to the extra size. Anything we can do to make this smaller would be great, so the type on the enum would work, but as you say the alignment will nullify that. The two ConstString

[Lldb-commits] [PATCH] D39436: Add regex support to file (-f) and module (-s) breakpoint options.

clayborg added a comment. The main reason for two strings is for searching efficiency. Most people don't set breakpoints using full paths, they give the basename: (lldb) b main.c:12 When setting breakpoints is it very easy to search for matches by basename since this is what users usually

[Lldb-commits] [PATCH] D39436: Add regex support to file (-f) and module (-s) breakpoint options.

clayborg added a comment. And yes, the memory savings are quite large as well when sharing directories. https://reviews.llvm.org/D39436 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[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 ___

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