[Lldb-commits] [PATCH] D50071: Use rich mangling information in Symtab::InitNameIndexes()

sgraenitz created this revision. sgraenitz added reviewers: labath, jingham, JDevlieghere, erik.pilkington. I set up a new review, because not all the code I touched was marked as a change in old one anymore. In preparation for this review, there were two earlier ones: -

[Lldb-commits] [PATCH] D49990: Use rich mangling information in Symtab::InitNameIndexes()

sgraenitz added inline comments. Comment at: include/lldb/Symbol/Symtab.h:81-83 + // No move + RichManglingInfo(RichManglingInfo &&) = delete; + RichManglingInfo =(RichManglingInfo &&) = delete; labath wrote: > sgraenitz wrote: > > labath wrote: > > > This is

[Lldb-commits] [PATCH] D49990: Use rich mangling information in Symtab::InitNameIndexes()

labath added a comment. I think there is still something wrong with the diff. I can't see any of the callers of e.g. `createItaniumInfo` but I can see the function on both LHS and RHS of the diff (which shouldn't be the case as it's a new function). It looks like you uploaded just an ammending

[Lldb-commits] [PATCH] D49909: Unit test for Symtab::InitNameIndexes

JDevlieghere accepted this revision. JDevlieghere added a comment. This revision is now accepted and ready to land. I agree with Pavel, but I also don't mind having this test in the meantime. LGTM if you add a FIXME with the direction we want to go. https://reviews.llvm.org/D49909

[Lldb-commits] [PATCH] D49990: Use rich mangling information in Symtab::InitNameIndexes()

sgraenitz updated this revision to Diff 158206. sgraenitz added a comment. Remove test code, that I added accidentally. Move RichManglingInfo and RichManglingSpec to their own header and cpp. https://reviews.llvm.org/D49990 Files: include/lldb/Core/RichManglingInfo.h

[Lldb-commits] [PATCH] D49990: Use rich mangling information in Symtab::InitNameIndexes()

sgraenitz marked 3 inline comments as done. sgraenitz added a comment. > The part I'm not sure about is whether the RichManglingInfo vs. > RichManglingSpec distinction brings any value. [...] So you might as well > just have one class, pass that to DemangleWithRichManglingInfo, and then >

[Lldb-commits] [PATCH] D49271: Adding libc++ formattors for std::optional

shafik updated this revision to Diff 158362. shafik added a comment. Fixing gcc skipIf to check for version as well https://reviews.llvm.org/D49271 Files: lldb.xcodeproj/project.pbxproj

[Lldb-commits] [PATCH] D50087: Add doxygen comments to the StackFrameList API (NFC)

vsk created this revision. vsk added reviewers: labath, jasonmolenda, tberghammer. Clarify how StackFrameList works by documenting its methods. Also, delete some dead code and insert some TODOs. https://reviews.llvm.org/D50087 Files: lldb/include/lldb/Target/StackFrameList.h

[Lldb-commits] [PATCH] D49271: Adding libc++ formattors for std::optional

shafik added inline comments. Comment at: packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py:21 +@add_test_categories(["libc++"]) +@skipIf(oslist=no_match(["macosx"]), compiler="clang",

[Lldb-commits] [PATCH] D49271: Adding libc++ formattors for std::optional

shafik updated this revision to Diff 158342. shafik marked 2 inline comments as done. shafik added a comment. - Adding comments - Changing from exit to self.skipTest - Adding skip for gcc https://reviews.llvm.org/D49271 Files: lldb.xcodeproj/project.pbxproj

[Lldb-commits] [PATCH] D49963: Preliminary patch to support prompt interpolation

clayborg added a comment. This will be very cool. Biggest issues to watch out for is to make sure it doesn't return incorrect values when running for things like the thread count. If you switch to use the "m_debugger.GetCommandInterpreter().GetExecutionContext()" it should solve this by

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

aprantl added inline comments. Comment at: source/API/SBTarget.cpp:1467 + } + const ConstString csFrom(from), csTo(to); + if (csFrom && csTo) { apolyakov wrote: > aprantl wrote: > > personally I would write this as: > > ``` > > if (!csFrom) > > return

Re: [Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

> On Jul 30, 2018, at 12:17 PM, Leonard Mosescu wrote: > > FYI, Breakpad & Crashpad will start generating the Microsoft flavor of ARM > minidumps soon. How do we tell the difference? I am guessing the ProcessorArchitecture will both be set to "PROCESSOR_ARCHITECTURE_ARM". Are plug-ins

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

clayborg updated this revision to Diff 158321. clayborg added a comment. Herald added a subscriber: srhines. - Fixed inline comments - Moved platform setting into Target::SetArchitecture(...) instead of doing this manually in the core file code. - Added ARM64 w0-w31, d0-d31, s0-s31 and h0-h31

[Lldb-commits] [PATCH] D50038: Introduce install-lldb-framework target

xiaobai updated this revision to Diff 158329. xiaobai added a comment. Address comments https://reviews.llvm.org/D50038 Files: CMakeLists.txt cmake/modules/AddLLDB.cmake cmake/modules/LLDBFramework.cmake source/API/CMakeLists.txt Index: source/API/CMakeLists.txt

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

apolyakov added inline comments. Comment at: source/API/SBTarget.cpp:1467 + } + const ConstString csFrom(from), csTo(to); + if (csFrom && csTo) { aprantl wrote: > personally I would write this as: > ``` > if (!csFrom) > return error.SetErrorString(" path is

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

apolyakov updated this revision to Diff 158330. apolyakov added a comment. Made error handling more meaningful: added different error messages for cases - empty path and empty path. https://reviews.llvm.org/D49739 Files: include/lldb/API/SBTarget.h lit/lit.cfg

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

lemo added a comment. Thanks Greg, looks good to me (a couple of inline comments left at your discretion) Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:15 // Other libraries and framework includes +//#include "lldb/Core/Architecture.h" #include

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

aprantl added inline comments. Comment at: source/API/SBTarget.cpp:1467 + } + const ConstString csFrom(from), csTo(to); + if (csFrom && csTo) { personally I would write this as: ``` if (!csFrom) return error.SetErrorString(" path is empty"); if (!csTo)

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

apolyakov added inline comments. Comment at: source/API/SBTarget.cpp:1467 + } + const ConstString csFrom(from), csTo(to); + if (csFrom && csTo) { aprantl wrote: > apolyakov wrote: > > aprantl wrote: > > > personally I would write this as: > > > ``` > > > if

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

apolyakov added a comment. OK, thank you for respond. Then, I think, we should wait for review from @clayborg or @aprantl, and if they accept the patch I'll divide it into two parts: SB API part and target-select one. https://reviews.llvm.org/D49739

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

aprantl added a comment. Seems good otherwise. Comment at: source/API/SBTarget.cpp:1468 + } + if (from[0] && to[0]) { +Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API)); apolyakov wrote: > I didn't find nullptr check in other API

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

apolyakov updated this revision to Diff 158319. apolyakov added a comment. Added converting from `const char *` to `ConstString`, documented new API. https://reviews.llvm.org/D49739 Files: include/lldb/API/SBTarget.h lit/lit.cfg lit/tools/lldb-mi/target/inputs/main.c

[Lldb-commits] [PATCH] D49963: Preliminary patch to support prompt interpolation

zturner added reviewers: labath, jasonmolenda. zturner added inline comments. Comment at: include/lldb/Host/Editline.h:187 + /// Register a callback to retrieve the prompt. + void SetPromptCallback(PromptCallbackType callback, void *baton); + I'd love to

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

clayborg added inline comments. Comment at: source/API/SBTarget.cpp:1476-1477 + } else +error.SetErrorStringWithFormat(" can't be empty " + "(SBTarget::%s) failed", __FUNCTION__); +} check if csFrom or csTo is empty and

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

clayborg added inline comments. Comment at: source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp:192 + +static size_t k_num_reg_infos = llvm::array_lengthof(g_reg_infos); + lemo wrote: > constexpr? will do Comment at:

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

labath added a comment. Sorry for not responding, I've been a bit busy these days. I've wanted to make a proof-of-concept to show you that reading the port number from stdout is possible, but I haven't gotten around to it yet. However, I am happy with the current mechanism of choosing the port

[Lldb-commits] [PATCH] D50071: Use rich mangling information in Symtab::InitNameIndexes()

labath added a comment. Thank you for updating the patch. If I understand things correctly, we could avoid circular deps and untyped pointers (or `llvm::Any`, which is essentially the same thing), by moving `CPlusPlusLanguage::MethodName` to a separate file. Could we do that as a preparatory

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

aprantl added inline comments. Comment at: source/API/SBTarget.cpp:1468 + } + if (from[0] && to[0]) { +Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API)); apolyakov wrote: > aprantl wrote: > > apolyakov wrote: > > > I didn't find nullptr

[Lldb-commits] [PATCH] D50071: Use rich mangling information in Symtab::InitNameIndexes()

labath added inline comments. Comment at: include/lldb/Core/RichManglingInfo.h:83-84 +public: + RichManglingInfo *SetItaniumInfo(); + RichManglingInfo *SetLegacyCxxParserInfo(const ConstString ); + I find it odd that one of these functions is stateless

[Lldb-commits] [PATCH] D49990: Use rich mangling information in Symtab::InitNameIndexes()

sgraenitz updated this revision to Diff 158231. sgraenitz added a comment. Fix potential leak of m_IPD_buf. Use llvm::Any instead of void*. Rename: RichManglingSpec -> RichManglingContext, RichManglingContext::CreateXyInfo() -> RichManglingContext::SetXyInfo() https://reviews.llvm.org/D49990

[Lldb-commits] [PATCH] D49990: Use rich mangling information in Symtab::InitNameIndexes()

sgraenitz abandoned this revision. sgraenitz added a comment. >> I think there is still something wrong with the diff. I can't see any of the >> callers of e.g. createItaniumInfo > > Weird. The caller is here, but not shown as a change anymore.. I created a new review where all my changes are

[Lldb-commits] [PATCH] D49909: Unit test for Symtab::InitNameIndexes

sgraenitz added a comment. Nice. I created a ticket for me internally to turn this into a lit test. For now I'd merge it once https://reviews.llvm.org/D50071 is done. (But I want to keep that one open until Jim had a look.) https://reviews.llvm.org/D49909

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

apolyakov added a comment. So, do you have any thoughts about this approach letting the debugserver choose a tcp port and patch overall? https://reviews.llvm.org/D49739 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

apolyakov added inline comments. Comment at: source/API/SBTarget.cpp:1468 + } + if (from[0] && to[0]) { +Log *log(lldb_private::GetLogIfAllCategoriesSet(LIBLLDB_LOG_API)); aprantl wrote: > apolyakov wrote: > > I didn't find nullptr check in other API

[Lldb-commits] [PATCH] D50038: Introduce install-lldb-framework target

xiaobai added a comment. In https://reviews.llvm.org/D50038#1181817, @labath wrote: > I am glad filing the cmake bug has paid off. :) Same! :) Comment at: cmake/modules/AddLLDB.cmake:81-87 + # install-liblldb{,-stripped} is the actual target that will install the + #

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

apolyakov updated this revision to Diff 158377. apolyakov added a comment. Changed the order of `if` statements to follow llvm coding standards. https://reviews.llvm.org/D49739 Files: include/lldb/API/SBTarget.h lit/lit.cfg lit/tools/lldb-mi/target/inputs/main.c

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

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

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

clayborg updated this revision to Diff 158371. clayborg added a comment. Removed unnecessary Xcode project changes and removed #include that wasn't needed. https://reviews.llvm.org/D49750 Files: include/lldb/Target/Target.h lldb.xcodeproj/project.pbxproj

[Lldb-commits] [PATCH] D49739: [WIP] Re-implement MI target-select command.

aprantl added inline comments. Comment at: source/API/SBTarget.cpp:1467 + } + const ConstString csFrom(from), csTo(to); + if (csFrom && csTo) { apolyakov wrote: > aprantl wrote: > > apolyakov wrote: > > > aprantl wrote: > > > > personally I would write this

[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.

clayborg added inline comments. Comment at: source/Plugins/Process/minidump/ProcessMinidump.cpp:15 // Other libraries and framework includes +//#include "lldb/Core/Architecture.h" #include "lldb/Core/Module.h" lemo wrote: > it this set for removal? Ah yes!

[Lldb-commits] [PATCH] D50027: Added initial unit test for LLDB's Stream class.

teemperor updated this revision to Diff 158411. teemperor marked an inline comment as done. teemperor added a comment. - Addressed Pavel's comments. Will merge once I have time to watch the build bots afterwards. https://reviews.llvm.org/D50027 Files: unittests/Utility/CMakeLists.txt

[Lldb-commits] [PATCH] D50027: Added initial unit test for LLDB's Stream class.

teemperor added inline comments. Comment at: unittests/Utility/StreamTest.cpp:38-41 +TEST_F(StreamTest, ChangingByteOrder) { + s.SetByteOrder(lldb::eByteOrderPDP); + EXPECT_EQ(lldb::eByteOrderPDP, s.GetByteOrder()); +} labath wrote: > I've been wondering for

[Lldb-commits] [PATCH] D50027: Added initial unit test for LLDB's Stream class.

probinson added inline comments. Comment at: unittests/Utility/StreamTest.cpp:38-41 +TEST_F(StreamTest, ChangingByteOrder) { + s.SetByteOrder(lldb::eByteOrderPDP); + EXPECT_EQ(lldb::eByteOrderPDP, s.GetByteOrder()); +} teemperor wrote: > labath wrote: > >

[Lldb-commits] [lldb] r338458 - Use UnknownVendor rather than UnknownArch since they're in two different enums

Author: echristo Date: Tue Jul 31 16:53:23 2018 New Revision: 338458 URL: http://llvm.org/viewvc/llvm-project?rev=338458=rev Log: Use UnknownVendor rather than UnknownArch since they're in two different enums and we're switching on vendor and not arch. Modified:

[Lldb-commits] [lldb] r338460 - Android is an environment and we were comparing the android triple

Author: echristo Date: Tue Jul 31 16:53:24 2018 New Revision: 338460 URL: http://llvm.org/viewvc/llvm-project?rev=338460=rev Log: Android is an environment and we were comparing the android triple against the OS rather than the environment. Also update other uses of OS when we meant environment

[Lldb-commits] [lldb] r338459 - Tidy up comment.

Author: echristo Date: Tue Jul 31 16:53:24 2018 New Revision: 338459 URL: http://llvm.org/viewvc/llvm-project?rev=338459=rev Log: Tidy up comment. Modified: lldb/trunk/source/Plugins/Platform/Android/PlatformAndroid.cpp Modified:

[Lldb-commits] [PATCH] D40475: DWZ 05/06: DWZ test mode

jankratochvil added a comment. There is no DWZ-tool wrapping script as discussed at: D48782 https://reviews.llvm.org/D40475 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D49368: Complete user-defined types from PDB symbol files

aleksandr.urakov abandoned this revision. aleksandr.urakov added a comment. Abandoned due to https://reviews.llvm.org/D49980 https://reviews.llvm.org/D49368 ___ lldb-commits mailing list lldb-commits@lists.llvm.org

[Lldb-commits] [PATCH] D50038: Introduce install-lldb-framework target

labath added a comment. I am glad filing the cmake bug has paid off. :) I just have one small question about this patch. Comment at: cmake/modules/AddLLDB.cmake:81-87 + # install-liblldb{,-stripped} is the actual target that will install the + # framework, so it must rely

[Lldb-commits] [PATCH] D48782: LLDB Test Suite: Provide an Option to run all tests with Dwarf Package Format (DWP).

jankratochvil added a comment. In https://reviews.llvm.org/D48782#1164786, @labath wrote: > For the purposes of integration testing I think it would be better to just > silently accept these so that all tests work "out of the box" with dwz. It > might be nice to hide these details in a wrapper

[Lldb-commits] [PATCH] D50027: Added initial unit test for LLDB's Stream class.

labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. This is really great. Thank you for doing this. I have some small ideas for improvement, but I don't think we have to go through another review cycle for that. Comment at:

[Lldb-commits] [PATCH] D49969: DWZ 04/06: ManualDWARFIndex::GetGlobalVariables for DIEs in PUs

jankratochvil updated this revision to Diff 158176. https://reviews.llvm.org/D49969 Files: source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.cpp source/Plugins/SymbolFile/DWARF/ManualDWARFIndex.h source/Plugins/SymbolFile/DWARF/NameToDIE.cpp source/Plugins/SymbolFile/DWARF/NameToDIE.h

[Lldb-commits] [PATCH] D49980: [PDB] Parse UDT symbols and pointers to members (combined patch)

aleksandr.urakov updated this revision to Diff 158194. aleksandr.urakov added a comment. Added support of a MSInheritance attribute. https://reviews.llvm.org/D49980 Files: include/lldb/Symbol/ClangASTContext.h lit/SymbolFile/PDB/Inputs/ClassLayoutTest.cpp

[Lldb-commits] [PATCH] D49271: Adding libc++ formattors for std::optional

labath added inline comments. Comment at: packages/Python/lldbsuite/test/functionalities/data-formatter/data-formatter-stl/libcxx/optional/TestDataFormatterLibcxxOptional.py:21 +@add_test_categories(["libc++"]) +@skipIf(oslist=no_match(["macosx"]), compiler="clang",