[Lldb-commits] [lldb] r338828 - Revert "Add support for ARM and ARM64 breakpad generated minidump files"
Author: labath Date: Fri Aug 3 01:47:22 2018 New Revision: 338828 URL: http://llvm.org/viewvc/llvm-project?rev=338828&view=rev Log: Revert "Add support for ARM and ARM64 breakpad generated minidump files" This reverts commit r338734 (and subsequent fixups in r338772 and r338746), because it breaks some minidump unit tests and introduces a lot of compiler warnings. Removed: lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/arm-linux.dmp lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/arm-macos.dmp lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/arm64-macos.dmp lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.cpp lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM.h lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp lldb/trunk/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.h Modified: lldb/trunk/include/lldb/Target/Target.h lldb/trunk/lldb.xcodeproj/project.pbxproj lldb/trunk/lldb.xcodeproj/xcshareddata/xcschemes/desktop.xcscheme lldb/trunk/packages/Python/lldbsuite/test/functionalities/postmortem/minidump-new/TestMiniDumpNew.py lldb/trunk/source/Plugins/Process/minidump/CMakeLists.txt lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.cpp lldb/trunk/source/Plugins/Process/minidump/MinidumpParser.h lldb/trunk/source/Plugins/Process/minidump/ProcessMinidump.cpp lldb/trunk/source/Plugins/Process/minidump/ThreadMinidump.cpp lldb/trunk/source/Target/Target.cpp Modified: lldb/trunk/include/lldb/Target/Target.h URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Target/Target.h?rev=338828&r1=338827&r2=338828&view=diff == --- lldb/trunk/include/lldb/Target/Target.h (original) +++ lldb/trunk/include/lldb/Target/Target.h Fri Aug 3 01:47:22 2018 @@ -913,30 +913,28 @@ public: /// Set the architecture for this target. /// /// If the current target has no Images read in, then this just sets the - /// architecture, which will be used to select the architecture of the - /// ExecutableModule when that is set. If the current target has an - /// ExecutableModule, then calling SetArchitecture with a different + /// architecture, which will + /// be used to select the architecture of the ExecutableModule when that is + /// set. + /// If the current target has an ExecutableModule, then calling + /// SetArchitecture with a different /// architecture from the currently selected one will reset the - /// ExecutableModule to that slice of the file backing the ExecutableModule. - /// If the file backing the ExecutableModule does not contain a fork of this - /// architecture, then this code will return false, and the architecture - /// won't be changed. If the input arch_spec is the same as the already set - /// architecture, this is a no-op. + /// ExecutableModule to that slice + /// of the file backing the ExecutableModule. If the file backing the + /// ExecutableModule does not + /// contain a fork of this architecture, then this code will return false, and + /// the architecture + /// won't be changed. + /// If the input arch_spec is the same as the already set architecture, this + /// is a no-op. /// /// @param[in] arch_spec /// The new architecture. /// - /// @param[in] set_platform - /// If \b true, then the platform will be adjusted if the currently - /// selected platform is not compatible with the archicture being set. - /// If \b false, then just the architecture will be set even if the - /// currently selected platform isn't compatible (in case it might be - /// manually set following this function call). - /// /// @return /// \b true if the architecture was successfully set, \bfalse otherwise. //-- - bool SetArchitecture(const ArchSpec &arch_spec, bool set_platform = false); + bool SetArchitecture(const ArchSpec &arch_spec); bool MergeArchitecture(const ArchSpec &arch_spec); Modified: lldb/trunk/lldb.xcodeproj/project.pbxproj URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/lldb.xcodeproj/project.pbxproj?rev=338828&r1=338827&r2=338828&view=diff == --- lldb/trunk/lldb.xcodeproj/project.pbxproj (original) +++ lldb/trunk/lldb.xcodeproj/project.pbxproj Fri Aug 3 01:47:22 2018 @@ -665,10 +665,6 @@ 26474CBE18D0CB2D0073DEBA /* RegisterContextMach_i386.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 26474CB818D0CB2D0073DEBA /* RegisterContextMach_i386.cpp */; }; 26474CC018D0CB2D0073DEBA /* RegisterContextMach_x86_64.cpp in Sources */ = {isa = PBXBuildFile; fileRef = 26474CBA18D0CB2D0073DEBA /* Registe
Re: [Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.
I've reverted this to keep the bots green until the issues pointed out by Stella and Raphael are resolved. Based on a quick inspection, it seems that the test issue is that `GetSystemInfo` call that has been added to MinidumpParser::Initialize is failing. I guess that's because the hand-crafted(?) minidumps for these tests don't contain the necessary data. For the -Wextended-offsetof issue, the way that other register contexts avoid those is by factoring this out into two offsetof expressions (offsetof(big_struct, small_struct_field) + offsetof(small_struct, actual_field)). On Fri, 3 Aug 2018 at 07:28, Raphael Isemann via Phabricator wrote: > > teemperor added a comment. > > I don't see this mentioned here yet, so: This patch also seems to introduce a > few hundred warnings with -Wextended-offsetof (which is enabled by default on > the macOS builds): > > > [...]llvm/tools/lldb/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp:510:5: > warning: using extended field designator is an extension > [-Wextended-offsetof] > DEF_S(20), > ^ > > [...]llvm/tools/lldb/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp:67:25: > note: expanded from macro 'DEF_S' > "s" #i, nullptr, 4, OFFSET(v[i * 16]), eEncodingVector, >\ > ^ > > [...]llvm/tools/lldb/source/Plugins/Process/minidump/RegisterContextMinidump_ARM64.cpp:29:20: > note: expanded from macro 'OFFSET' > #define OFFSET(r) (offsetof(RegisterContextMinidump_ARM64::Context, r)) > ^~~ > [...]stddef.h:120:24: note: expanded from macro 'offsetof' > #define offsetof(t, d) __builtin_offsetof(t, d) > ^ ~ > > (And the tests also fail on macOS, but they are probably fixed when the > Linux/Windows tests are fixed). > > > Repository: > rL LLVM > > https://reviews.llvm.org/D49750 > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49750: Add support for ARM and ARM64 breakpad generated minidump files.
labath added a subscriber: rovka. labath added a comment. I've reverted this to keep the bots green until the issues pointed out by Stella and Raphael are resolved. Based on a quick inspection, it seems that the test issue is that `GetSystemInfo` call that has been added to MinidumpParser::Initialize is failing. I guess that's because the hand-crafted(?) minidumps for these tests don't contain the necessary data. For the -Wextended-offsetof issue, the way that other register contexts avoid those is by factoring this out into two offsetof expressions (offsetof(big_struct, small_struct_field) + offsetof(small_struct, actual_field)). Repository: rL LLVM https://reviews.llvm.org/D49750 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D50161: Add raw_ostream wrapper to the Stream class
labath accepted this revision. labath added a comment. This revision is now accepted and ready to land. looks good to me. Thanks. Btw, have you looked at how hard would it be to actually fix the places that are copying these streams? https://reviews.llvm.org/D50161 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D50071: Use rich mangling information in Symtab::InitNameIndexes()
labath added a comment. It sounds like we have a plan then! Look forward to seeing the results. Comment at: source/Core/RichManglingInfo.cpp:36-40 +RichManglingContext::SetLegacyCxxParserInfo(const ConstString &mangled) { + m_info.ResetProvider(); + m_info.m_provider = RichManglingInfo::PluginCxxLanguage; + m_info.m_legacy_parser = new CPlusPlusLanguage::MethodName(mangled); + return &m_info; sgraenitz wrote: > labath wrote: > > sgraenitz wrote: > > > labath wrote: > > > > Is this really the **mangled** name? > > > Yes, it is. > > How is that possible? I see nothing in CPlusPlusLanguage::MethodName which > > would perform the demangling, and yet it clearly operates on demangled > > names. Also, the LHS of this patch creates MethodName instances like this > > `CPlusPlusLanguage::MethodName > > cxx_method(mangled.GetDemangledName(lldb::eLanguageTypeC_plus_plus));` > > > > > Oh right, I should revisit that. > > While we are here: It's a pity that only MSVC-mangled names trigger the > fallback case with the new implementation. Thus, my unit test has no coverage > for it and I didn't recognize the mistake. I thought about integrating > ObjCLanguage parsing here too. On the one hand this would improve test > coverage. On the other hand it would cause a semantic change in name indexing > (and my ObjC knowledge is quite limited). > > What do you think? I understand Zachary is working on an MSVC demangler as we speak. When that's finished, we should be able to add a couple of MSVC mangled names to your test case. (The `MethodName` class itself is tested elsewhere, so here we just need to test that things are wired up correctly.) As for the ObjC idea, I am not sure what exactly would that entail, so I don't have an opinion on it. But I can certainly believe that there is a lot of room for improvement here. https://reviews.llvm.org/D50071 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D50071: Use rich mangling information in Symtab::InitNameIndexes()
sgraenitz added inline comments. Comment at: include/lldb/Core/RichManglingInfo.h:36 + /// "a::b". + llvm::StringRef GetFunctionDeclContextName() const; + I realized that returning `llvm::StringRef` from here may not be the best idea. To the outside it appears to be an immutable string, but internally it will be reused and even realloc'ed. I guess returning a `ConstString` is not acceptable as an alternative performance-wise, as it adds one `memcpy` per query. I will sketch something here, but it's not easy as it needs to fit both, the IPD and C++ MethodName. Comment at: include/lldb/Core/RichManglingInfo.h:61 +assert(m_legacy_parser.hasValue()); +assert(llvm::any_isa(m_legacy_parser)); +return llvm::any_cast(m_legacy_parser); This must be `llvm::any_isa`. Found during fallback-test and fixed. Comment at: source/Core/RichManglingInfo.cpp:36-40 +RichManglingContext::SetLegacyCxxParserInfo(const ConstString &mangled) { + m_info.ResetProvider(); + m_info.m_provider = RichManglingInfo::PluginCxxLanguage; + m_info.m_legacy_parser = new CPlusPlusLanguage::MethodName(mangled); + return &m_info; labath wrote: > sgraenitz wrote: > > labath wrote: > > > sgraenitz wrote: > > > > labath wrote: > > > > > Is this really the **mangled** name? > > > > Yes, it is. > > > How is that possible? I see nothing in CPlusPlusLanguage::MethodName > > > which would perform the demangling, and yet it clearly operates on > > > demangled names. Also, the LHS of this patch creates MethodName instances > > > like this > > > `CPlusPlusLanguage::MethodName > > > cxx_method(mangled.GetDemangledName(lldb::eLanguageTypeC_plus_plus));` > > > > > > > > Oh right, I should revisit that. > > > > While we are here: It's a pity that only MSVC-mangled names trigger the > > fallback case with the new implementation. Thus, my unit test has no > > coverage for it and I didn't recognize the mistake. I thought about > > integrating ObjCLanguage parsing here too. On the one hand this would > > improve test coverage. On the other hand it would cause a semantic change > > in name indexing (and my ObjC knowledge is quite limited). > > > > What do you think? > I understand Zachary is working on an MSVC demangler as we speak. When that's > finished, we should be able to add a couple of MSVC mangled names to your > test case. (The `MethodName` class itself is tested elsewhere, so here we > just need to test that things are wired up correctly.) > > As for the ObjC idea, I am not sure what exactly would that entail, so I > don't have an opinion on it. But I can certainly believe that there is a lot > of room for improvement here. It's actually only a few lines to hack, so that the unit test covers the fallback case. I did it once now and found the `llvm::any_isa` bug. Otherwise it passes. https://reviews.llvm.org/D50071 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D50071: Use rich mangling information in Symtab::InitNameIndexes()
labath added inline comments. Comment at: include/lldb/Core/RichManglingInfo.h:36 + /// "a::b". + llvm::StringRef GetFunctionDeclContextName() const; + sgraenitz wrote: > I realized that returning `llvm::StringRef` from here may not be the best > idea. To the outside it appears to be an immutable string, but internally it > will be reused and even realloc'ed. I guess returning a `ConstString` is not > acceptable as an alternative performance-wise, as it adds one `memcpy` per > query. I will sketch something here, but it's not easy as it needs to fit > both, the IPD and C++ MethodName. That is a bit unfortunate, but I don't think things would be any better if you left this as `const char *`. AFAICT, the caller converts this to a ConstString anyway, so returning that type might not be such a bad choice, but I would be fine with just documenting that the returned string is valid until the next `GetXXX` call. Comment at: source/Core/RichManglingInfo.cpp:93 + case PluginCxxLanguage: +return get()->GetContext().data(); + } These `.data()` calls can now be removed. Comment at: source/Symbol/Symtab.cpp:397 + // The base name will be our entry's name. + entry.cstring = ConstString(base_name.data()); + remove `.data()` https://reviews.llvm.org/D50071 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49740: Move RegisterValue, Scalar, State from Core to Utility
labath added a comment. Does anyone have thoughts on this? (I have one more move like this lined up (for the Listener/Broadcaster classes), and then I should be done with these kinds of changes for a while.) https://reviews.llvm.org/D49740 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D50071: Use rich mangling information in Symtab::InitNameIndexes()
sgraenitz added inline comments. Comment at: source/Core/RichManglingInfo.cpp:69-80 +const char *RichManglingInfo::GetFunctionBaseName() const { + switch (m_provider) { + case ItaniumPartialDemangler: +if (auto buf = m_IPD->getFunctionBaseName(m_IPD_buf, &m_IPD_size)) { + m_IPD_buf = buf; + return buf; +} labath wrote: > Could these return `StringRef`? Am I correct in assuming that `m_IPD_size` > holds the size of the returned string? If thats the case then you could even > do this with no performance impact (or a positive one). While investigating the `StringRef` issue, I realized another problem here. The second param `N` to the IPD queries is used to pass the **buffer size in** and the **string length out**. With the current use of `m_IPD_size` I pass ever smaller buffer sizes in subsequent calls. That causes unnecessary reallocs. I should track the length out param separately. Comment at: source/Symbol/Symtab.cpp:397 + // The base name will be our entry's name. + entry.cstring = ConstString(base_name.data()); + labath wrote: > remove `.data()` Actually, this is dangerous as `StringRef` doesn't guarantee null termination. Will fix that. https://reviews.llvm.org/D50071 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49740: Move RegisterValue, Scalar, State from Core to Utility
zturner added a subscriber: labath. zturner added a comment. No objections here https://reviews.llvm.org/D49740 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D49740: Move RegisterValue, Scalar, State from Core to Utility
No objections here On Fri, Aug 3, 2018 at 3:24 AM Pavel Labath via Phabricator < revi...@reviews.llvm.org> wrote: > labath added a comment. > > Does anyone have thoughts on this? (I have one more move like this lined > up (for the Listener/Broadcaster classes), and then I should be done with > these kinds of changes for a while.) > > > https://reviews.llvm.org/D49740 > > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r338901 - Add raw_ostream wrapper to the Stream class
Author: teemperor Date: Fri Aug 3 09:56:33 2018 New Revision: 338901 URL: http://llvm.org/viewvc/llvm-project?rev=338901&view=rev Log: Add raw_ostream wrapper to the Stream class Summary: This wrapper will allow us in the future to reuse LLVM methods from within the Stream class. Currently no test as this is intended to be an internal class that shouldn't have any NFC. The test for this change will be the follow up patch that migrates LLDB's LEB128 implementation to the one from LLVM. This change also adds custom move/assignment methods to Stream, as LLVM raw_ostream doesn't support these. As our internal stream has anyway no state, we can just keep the same stream object around. Reviewers: davide, labath Reviewed By: labath Subscribers: xiaobai, labath, lldb-commits Differential Revision: https://reviews.llvm.org/D50161 Modified: lldb/trunk/include/lldb/Utility/Stream.h lldb/trunk/source/Utility/Stream.cpp Modified: lldb/trunk/include/lldb/Utility/Stream.h URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Utility/Stream.h?rev=338901&r1=338900&r2=338901&view=diff == --- lldb/trunk/include/lldb/Utility/Stream.h (original) +++ lldb/trunk/include/lldb/Utility/Stream.h Fri Aug 3 09:56:33 2018 @@ -15,6 +15,7 @@ #include "lldb/lldb-enumerations.h" // for ByteOrder::eByteOrderInvalid #include "llvm/ADT/StringRef.h" // for StringRef #include "llvm/Support/FormatVariadic.h" +#include "llvm/Support/raw_ostream.h" #include #include // for size_t @@ -52,6 +53,17 @@ public: //-- Stream(); + // FIXME: Streams should not be copyable. + Stream(const Stream &other) : m_forwarder(*this) { (*this) = other; } + + Stream &operator=(const Stream &rhs) { +m_flags = rhs.m_flags; +m_addr_size = rhs.m_addr_size; +m_byte_order = rhs.m_byte_order; +m_indent_level = rhs.m_indent_level; +return *this; + } + //-- /// Destructor //-- @@ -520,6 +532,13 @@ public: //-- size_t PutULEB128(uint64_t uval); + //-- + /// Returns a raw_ostream that forwards the data to this Stream object. + //-- + llvm::raw_ostream &AsRawOstream() { +return m_forwarder; + } + protected: //-- // Member variables @@ -548,6 +567,34 @@ protected: /// The number of bytes that were appended to the stream. //-- virtual size_t WriteImpl(const void *src, size_t src_len) = 0; + + //-- + /// @class RawOstreamForward Stream.h "lldb/Utility/Stream.h" + /// This is a wrapper class that exposes a raw_ostream interface that just + /// forwards to an LLDB stream, allowing to reuse LLVM algorithms that take + /// a raw_ostream within the LLDB code base. + //-- + class RawOstreamForward : public llvm::raw_ostream { +// Note: This stream must *not* maintain its own buffer, but instead +// directly write everything to the internal Stream class. Without this, +// we would run into the problem that the Stream written byte count would +// differ from the actually written bytes by the size of the internal +// raw_ostream buffer. + +Stream &m_target; +void write_impl(const char *Ptr, size_t Size) override { + m_target.Write(Ptr, Size); +} + +uint64_t current_pos() const override { + return m_target.GetWrittenBytes(); +} + + public: +RawOstreamForward(Stream &target) +: llvm::raw_ostream(/*unbuffered*/ true), m_target(target) {} + }; + RawOstreamForward m_forwarder; }; } // namespace lldb_private Modified: lldb/trunk/source/Utility/Stream.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/Stream.cpp?rev=338901&r1=338900&r2=338901&view=diff == --- lldb/trunk/source/Utility/Stream.cpp (original) +++ lldb/trunk/source/Utility/Stream.cpp Fri Aug 3 09:56:33 2018 @@ -23,11 +23,11 @@ using namespace lldb_private; Stream::Stream(uint32_t flags, uint32_t addr_size, ByteOrder byte_order) : m_flags(flags), m_addr_size(addr_size), m_byte_order(byte_order), - m_indent_level(0) {} + m_indent_level(0), m_forwarder(*this) {} Stream::Stream() : m_flags(0), m_addr_size(4), m_byte_order(endian::InlHostByteOrder()), - m_indent_level(0) {} + m_indent_level(0), m_fo
[Lldb-commits] [PATCH] D50161: Add raw_ostream wrapper to the Stream class
This revision was automatically updated to reflect the committed changes. Closed by commit rL338901: Add raw_ostream wrapper to the Stream class (authored by teemperor, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D50161?vs=158926&id=159032#toc Repository: rL LLVM https://reviews.llvm.org/D50161 Files: lldb/trunk/include/lldb/Utility/Stream.h lldb/trunk/source/Utility/Stream.cpp Index: lldb/trunk/source/Utility/Stream.cpp === --- lldb/trunk/source/Utility/Stream.cpp +++ lldb/trunk/source/Utility/Stream.cpp @@ -23,11 +23,11 @@ Stream::Stream(uint32_t flags, uint32_t addr_size, ByteOrder byte_order) : m_flags(flags), m_addr_size(addr_size), m_byte_order(byte_order), - m_indent_level(0) {} + m_indent_level(0), m_forwarder(*this) {} Stream::Stream() : m_flags(0), m_addr_size(4), m_byte_order(endian::InlHostByteOrder()), - m_indent_level(0) {} + m_indent_level(0), m_forwarder(*this) {} //-- // Destructor Index: lldb/trunk/include/lldb/Utility/Stream.h === --- lldb/trunk/include/lldb/Utility/Stream.h +++ lldb/trunk/include/lldb/Utility/Stream.h @@ -15,6 +15,7 @@ #include "lldb/lldb-enumerations.h" // for ByteOrder::eByteOrderInvalid #include "llvm/ADT/StringRef.h" // for StringRef #include "llvm/Support/FormatVariadic.h" +#include "llvm/Support/raw_ostream.h" #include #include // for size_t @@ -52,6 +53,17 @@ //-- Stream(); + // FIXME: Streams should not be copyable. + Stream(const Stream &other) : m_forwarder(*this) { (*this) = other; } + + Stream &operator=(const Stream &rhs) { +m_flags = rhs.m_flags; +m_addr_size = rhs.m_addr_size; +m_byte_order = rhs.m_byte_order; +m_indent_level = rhs.m_indent_level; +return *this; + } + //-- /// Destructor //-- @@ -520,6 +532,13 @@ //-- size_t PutULEB128(uint64_t uval); + //-- + /// Returns a raw_ostream that forwards the data to this Stream object. + //-- + llvm::raw_ostream &AsRawOstream() { +return m_forwarder; + } + protected: //-- // Member variables @@ -548,6 +567,34 @@ /// The number of bytes that were appended to the stream. //-- virtual size_t WriteImpl(const void *src, size_t src_len) = 0; + + //-- + /// @class RawOstreamForward Stream.h "lldb/Utility/Stream.h" + /// This is a wrapper class that exposes a raw_ostream interface that just + /// forwards to an LLDB stream, allowing to reuse LLVM algorithms that take + /// a raw_ostream within the LLDB code base. + //-- + class RawOstreamForward : public llvm::raw_ostream { +// Note: This stream must *not* maintain its own buffer, but instead +// directly write everything to the internal Stream class. Without this, +// we would run into the problem that the Stream written byte count would +// differ from the actually written bytes by the size of the internal +// raw_ostream buffer. + +Stream &m_target; +void write_impl(const char *Ptr, size_t Size) override { + m_target.Write(Ptr, Size); +} + +uint64_t current_pos() const override { + return m_target.GetWrittenBytes(); +} + + public: +RawOstreamForward(Stream &target) +: llvm::raw_ostream(/*unbuffered*/ true), m_target(target) {} + }; + RawOstreamForward m_forwarder; }; } // namespace lldb_private Index: lldb/trunk/source/Utility/Stream.cpp === --- lldb/trunk/source/Utility/Stream.cpp +++ lldb/trunk/source/Utility/Stream.cpp @@ -23,11 +23,11 @@ Stream::Stream(uint32_t flags, uint32_t addr_size, ByteOrder byte_order) : m_flags(flags), m_addr_size(addr_size), m_byte_order(byte_order), - m_indent_level(0) {} + m_indent_level(0), m_forwarder(*this) {} Stream::Stream() : m_flags(0), m_addr_size(4), m_byte_order(endian::InlHostByteOrder()), - m_indent_level(0) {} + m_indent_level(0), m_forwarder(*this) {} //-- // Destructor Index: lldb/trunk/include/lldb/Utility/Stream.h ==
[Lldb-commits] [PATCH] D50225: Use a DenseMap for looking up functions by UID in CompileUnit::FindFunctionByUID
vsk added a comment. Thanks for doing this :)! Comment at: source/Symbol/CompileUnit.cpp:111 // TODO: order these by address m_functions.push_back(funcSP); } Is m_functions used to do anything crucial? I see a use in Dump, but it seems like you could replace that use by copying the function map into a vector and sorting it. I see another use in GetFunctionAtIndex, but that API can be deleted entirely because its only user is lldb-test (via Module::ParseAllDebugSymbols). You'd just need to sink this block of code into CompileUnit: ``` for (size_t func_idx = 0; (sc.function = sc.comp_unit->GetFunctionAtIndex(func_idx).get()) != nullptr; ++func_idx) { symbols->ParseFunctionBlocks(sc); // Parse the variables for this function and all its blocks symbols->ParseVariablesForContext(sc); } ``` Repository: rLLDB LLDB https://reviews.llvm.org/D50225 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D50071: Use rich mangling information in Symtab::InitNameIndexes()
sgraenitz updated this revision to Diff 159060. sgraenitz added a comment. Merge RichManglingInfo into RichManglingContext in order to reduce complexity when avoiding unnecessary reallocs. https://reviews.llvm.org/D50071 Files: include/lldb/Core/Mangled.h include/lldb/Core/RichManglingInfo.h include/lldb/Symbol/Symtab.h include/lldb/Utility/ConstString.h include/lldb/lldb-forward.h source/Core/CMakeLists.txt source/Core/Mangled.cpp source/Core/RichManglingInfo.cpp source/Symbol/Symtab.cpp Index: source/Symbol/Symtab.cpp === --- source/Symbol/Symtab.cpp +++ source/Symbol/Symtab.cpp @@ -10,9 +10,10 @@ #include #include -#include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h" #include "Plugins/Language/ObjC/ObjCLanguage.h" + #include "lldb/Core/Module.h" +#include "lldb/Core/RichManglingInfo.h" #include "lldb/Core/STLUtils.h" #include "lldb/Core/Section.h" #include "lldb/Symbol/ObjectFile.h" @@ -23,6 +24,8 @@ #include "lldb/Utility/Stream.h" #include "lldb/Utility/Timer.h" +#include "llvm/ADT/StringRef.h" + using namespace lldb; using namespace lldb_private; @@ -215,6 +218,39 @@ //-- // InitNameIndexes //-- +static bool lldb_skip_name(llvm::StringRef mangled, + Mangled::ManglingScheme scheme) { + switch (scheme) { + case Mangled::eManglingSchemeItanium: { +if (mangled.size() < 3 || !mangled.startswith("_Z")) + return true; + +// Avoid the following types of symbols in the index. +switch (mangled[2]) { +case 'G': // guard variables +case 'T': // virtual tables, VTT structures, typeinfo structures + names +case 'Z': // named local entities (if we eventually handle + // eSymbolTypeData, we will want this back) + return true; + +default: + break; +} + +// Include this name in the index. +return false; + } + + // No filters for this scheme yet. Include all names in indexing. + case Mangled::eManglingSchemeMSVC: +return false; + + // Don't try and demangle things we can't categorize. + case Mangled::eManglingSchemeNone: +return true; + } +} + void Symtab::InitNameIndexes() { // Protected function, no need to lock mutex... if (!m_name_indexes_computed) { @@ -243,25 +279,30 @@ m_name_to_index.Reserve(actual_count); #endif -NameToIndexMap::Entry entry; - -// The "const char *" in "class_contexts" must come from a -// ConstString::GetCString() +// The "const char *" in "class_contexts" and backlog::value_type::second +// must come from a ConstString::GetCString() std::set class_contexts; -UniqueCStringMap mangled_name_to_index; -std::vector symbol_contexts(num_symbols, nullptr); +std::vector> backlog; +backlog.reserve(num_symbols / 2); + +// Instantiation of the demangler is expensive, so better use a single one +// for all entries during batch processing. +RichManglingContext MC; +NameToIndexMap::Entry entry; for (entry.value = 0; entry.value < num_symbols; ++entry.value) { - const Symbol *symbol = &m_symbols[entry.value]; + Symbol *symbol = &m_symbols[entry.value]; // Don't let trampolines get into the lookup by name map If we ever need // the trampoline symbols to be searchable by name we can remove this and // then possibly add a new bool to any of the Symtab functions that // lookup symbols by name to indicate if they want trampolines. if (symbol->IsTrampoline()) continue; - const Mangled &mangled = symbol->GetMangled(); + // If the symbol's name string matched a Mangled::ManglingScheme, it is + // stored in the mangled field. + Mangled &mangled = symbol->GetMangled(); entry.cstring = mangled.GetMangledName(); if (entry.cstring) { m_name_to_index.Append(entry); @@ -274,70 +315,15 @@ m_name_to_index.Append(entry); } -const SymbolType symbol_type = symbol->GetType(); -if (symbol_type == eSymbolTypeCode || -symbol_type == eSymbolTypeResolver) { - llvm::StringRef entry_ref(entry.cstring.GetStringRef()); - if (entry_ref[0] == '_' && entry_ref[1] == 'Z' && - (entry_ref[2] != 'T' && // avoid virtual table, VTT structure, - // typeinfo structure, and typeinfo - // name - entry_ref[2] != 'G' && // avoid guard variables - entry_ref[2] != 'Z')) // named local entities (if we - // eventually handle eSymbolTypeData, - // we will want this back) - { -CPlusPlusLanguage::MethodName cxx_method( -mangled.Ge
[Lldb-commits] [PATCH] D49740: Move RegisterValue, Scalar, State from Core to Utility
jingham added a comment. I worry a little bit about Utility getting kind of incoherent. Some of it is clearly utility, some is more there because that's where we are putting the code we're trying to reduce dependencies on so we can share then with lldb-server. RegisterValue doesn't for instance seem like a "Utility" class to me. But maybe this is something to revisit once you've gotten at least what you need for lldb-server done? https://reviews.llvm.org/D49740 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D50071: Use rich mangling information in Symtab::InitNameIndexes()
sgraenitz updated this revision to Diff 159065. sgraenitz added a comment. Polishing https://reviews.llvm.org/D50071 Files: include/lldb/Core/Mangled.h include/lldb/Core/RichManglingInfo.h include/lldb/Symbol/Symtab.h include/lldb/Utility/ConstString.h include/lldb/lldb-forward.h source/Core/CMakeLists.txt source/Core/Mangled.cpp source/Core/RichManglingInfo.cpp source/Symbol/Symtab.cpp Index: source/Symbol/Symtab.cpp === --- source/Symbol/Symtab.cpp +++ source/Symbol/Symtab.cpp @@ -10,9 +10,10 @@ #include #include -#include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h" #include "Plugins/Language/ObjC/ObjCLanguage.h" + #include "lldb/Core/Module.h" +#include "lldb/Core/RichManglingInfo.h" #include "lldb/Core/STLUtils.h" #include "lldb/Core/Section.h" #include "lldb/Symbol/ObjectFile.h" @@ -23,6 +24,8 @@ #include "lldb/Utility/Stream.h" #include "lldb/Utility/Timer.h" +#include "llvm/ADT/StringRef.h" + using namespace lldb; using namespace lldb_private; @@ -215,6 +218,39 @@ //-- // InitNameIndexes //-- +static bool lldb_skip_name(llvm::StringRef mangled, + Mangled::ManglingScheme scheme) { + switch (scheme) { + case Mangled::eManglingSchemeItanium: { +if (mangled.size() < 3 || !mangled.startswith("_Z")) + return true; + +// Avoid the following types of symbols in the index. +switch (mangled[2]) { +case 'G': // guard variables +case 'T': // virtual tables, VTT structures, typeinfo structures + names +case 'Z': // named local entities (if we eventually handle + // eSymbolTypeData, we will want this back) + return true; + +default: + break; +} + +// Include this name in the index. +return false; + } + + // No filters for this scheme yet. Include all names in indexing. + case Mangled::eManglingSchemeMSVC: +return false; + + // Don't try and demangle things we can't categorize. + case Mangled::eManglingSchemeNone: +return true; + } +} + void Symtab::InitNameIndexes() { // Protected function, no need to lock mutex... if (!m_name_indexes_computed) { @@ -243,25 +279,30 @@ m_name_to_index.Reserve(actual_count); #endif -NameToIndexMap::Entry entry; - -// The "const char *" in "class_contexts" must come from a -// ConstString::GetCString() +// The "const char *" in "class_contexts" and backlog::value_type::second +// must come from a ConstString::GetCString() std::set class_contexts; -UniqueCStringMap mangled_name_to_index; -std::vector symbol_contexts(num_symbols, nullptr); +std::vector> backlog; +backlog.reserve(num_symbols / 2); + +// Instantiation of the demangler is expensive, so better use a single one +// for all entries during batch processing. +RichManglingContext MC; +NameToIndexMap::Entry entry; for (entry.value = 0; entry.value < num_symbols; ++entry.value) { - const Symbol *symbol = &m_symbols[entry.value]; + Symbol *symbol = &m_symbols[entry.value]; // Don't let trampolines get into the lookup by name map If we ever need // the trampoline symbols to be searchable by name we can remove this and // then possibly add a new bool to any of the Symtab functions that // lookup symbols by name to indicate if they want trampolines. if (symbol->IsTrampoline()) continue; - const Mangled &mangled = symbol->GetMangled(); + // If the symbol's name string matched a Mangled::ManglingScheme, it is + // stored in the mangled field. + Mangled &mangled = symbol->GetMangled(); entry.cstring = mangled.GetMangledName(); if (entry.cstring) { m_name_to_index.Append(entry); @@ -274,70 +315,15 @@ m_name_to_index.Append(entry); } -const SymbolType symbol_type = symbol->GetType(); -if (symbol_type == eSymbolTypeCode || -symbol_type == eSymbolTypeResolver) { - llvm::StringRef entry_ref(entry.cstring.GetStringRef()); - if (entry_ref[0] == '_' && entry_ref[1] == 'Z' && - (entry_ref[2] != 'T' && // avoid virtual table, VTT structure, - // typeinfo structure, and typeinfo - // name - entry_ref[2] != 'G' && // avoid guard variables - entry_ref[2] != 'Z')) // named local entities (if we - // eventually handle eSymbolTypeData, - // we will want this back) - { -CPlusPlusLanguage::MethodName cxx_method( -mangled.GetDemangledName(lldb::eLanguageTypeC_plus_plus)); -entry.cstring = ConstString(cxx_method.GetB
[Lldb-commits] [PATCH] D50071: Use rich mangling information in Symtab::InitNameIndexes()
sgraenitz updated this revision to Diff 159066. sgraenitz added a comment. Polishing https://reviews.llvm.org/D50071 Files: include/lldb/Core/Mangled.h include/lldb/Core/RichManglingInfo.h include/lldb/Symbol/Symtab.h include/lldb/Utility/ConstString.h include/lldb/lldb-forward.h source/Core/CMakeLists.txt source/Core/Mangled.cpp source/Core/RichManglingInfo.cpp source/Symbol/Symtab.cpp Index: source/Symbol/Symtab.cpp === --- source/Symbol/Symtab.cpp +++ source/Symbol/Symtab.cpp @@ -10,9 +10,10 @@ #include #include -#include "Plugins/Language/CPlusPlus/CPlusPlusLanguage.h" #include "Plugins/Language/ObjC/ObjCLanguage.h" + #include "lldb/Core/Module.h" +#include "lldb/Core/RichManglingInfo.h" #include "lldb/Core/STLUtils.h" #include "lldb/Core/Section.h" #include "lldb/Symbol/ObjectFile.h" @@ -23,6 +24,8 @@ #include "lldb/Utility/Stream.h" #include "lldb/Utility/Timer.h" +#include "llvm/ADT/StringRef.h" + using namespace lldb; using namespace lldb_private; @@ -215,6 +218,39 @@ //-- // InitNameIndexes //-- +static bool lldb_skip_name(llvm::StringRef mangled, + Mangled::ManglingScheme scheme) { + switch (scheme) { + case Mangled::eManglingSchemeItanium: { +if (mangled.size() < 3 || !mangled.startswith("_Z")) + return true; + +// Avoid the following types of symbols in the index. +switch (mangled[2]) { +case 'G': // guard variables +case 'T': // virtual tables, VTT structures, typeinfo structures + names +case 'Z': // named local entities (if we eventually handle + // eSymbolTypeData, we will want this back) + return true; + +default: + break; +} + +// Include this name in the index. +return false; + } + + // No filters for this scheme yet. Include all names in indexing. + case Mangled::eManglingSchemeMSVC: +return false; + + // Don't try and demangle things we can't categorize. + case Mangled::eManglingSchemeNone: +return true; + } +} + void Symtab::InitNameIndexes() { // Protected function, no need to lock mutex... if (!m_name_indexes_computed) { @@ -243,25 +279,30 @@ m_name_to_index.Reserve(actual_count); #endif -NameToIndexMap::Entry entry; - -// The "const char *" in "class_contexts" must come from a -// ConstString::GetCString() +// The "const char *" in "class_contexts" and backlog::value_type::second +// must come from a ConstString::GetCString() std::set class_contexts; -UniqueCStringMap mangled_name_to_index; -std::vector symbol_contexts(num_symbols, nullptr); +std::vector> backlog; +backlog.reserve(num_symbols / 2); + +// Instantiation of the demangler is expensive, so better use a single one +// for all entries during batch processing. +RichManglingContext MC; +NameToIndexMap::Entry entry; for (entry.value = 0; entry.value < num_symbols; ++entry.value) { - const Symbol *symbol = &m_symbols[entry.value]; + Symbol *symbol = &m_symbols[entry.value]; // Don't let trampolines get into the lookup by name map If we ever need // the trampoline symbols to be searchable by name we can remove this and // then possibly add a new bool to any of the Symtab functions that // lookup symbols by name to indicate if they want trampolines. if (symbol->IsTrampoline()) continue; - const Mangled &mangled = symbol->GetMangled(); + // If the symbol's name string matched a Mangled::ManglingScheme, it is + // stored in the mangled field. + Mangled &mangled = symbol->GetMangled(); entry.cstring = mangled.GetMangledName(); if (entry.cstring) { m_name_to_index.Append(entry); @@ -274,70 +315,15 @@ m_name_to_index.Append(entry); } -const SymbolType symbol_type = symbol->GetType(); -if (symbol_type == eSymbolTypeCode || -symbol_type == eSymbolTypeResolver) { - llvm::StringRef entry_ref(entry.cstring.GetStringRef()); - if (entry_ref[0] == '_' && entry_ref[1] == 'Z' && - (entry_ref[2] != 'T' && // avoid virtual table, VTT structure, - // typeinfo structure, and typeinfo - // name - entry_ref[2] != 'G' && // avoid guard variables - entry_ref[2] != 'Z')) // named local entities (if we - // eventually handle eSymbolTypeData, - // we will want this back) - { -CPlusPlusLanguage::MethodName cxx_method( -mangled.GetDemangledName(lldb::eLanguageTypeC_plus_plus)); -entry.cstring = ConstString(cxx_method.GetB
[Lldb-commits] [PATCH] D50071: Use rich mangling information in Symtab::InitNameIndexes()
sgraenitz added a comment. I quickly realized, that it needs a bigger change to properly address the aforementioned issues. Here's the major changes. Some documentation is todo. **StringRef to reused buffer** addressed with a split into first parse then get buffer. It seems more natural that the buffer may be reused. Also we can ask for `const char *` or a `StringRef`, which avoids the conversion issue. MC.ParseFunctionBaseName(); llvm::StringRef base_name = MC.GetBufferRef(); [...] MC.ParseFunctionDeclContextName(); const char *decl_context_cstr = MC.GetBufferAsCString(); **buffer size in, string length out on success, buffer size out on fail** is addressed in `processIPDStrResult`. It handles fails, reallocs and the regular cases and now distinguishes between `m_IPD_buf_size` and `m_IPD_str_len`.`RichManglingContext` maintains a single buffer now that is reused for all string-queries for Itanium. The fallback case always uses `ConstString` now and it may be a bit slower than before, but IMHO that's fine. This change made the separation between `RichManglingInfo` and `RichManglingContext` unsustainable. So I merged them and now there are these ugly `None` cases, but otherwise it looks ok. I didn't adjust class and file names yet. I touched the bookkeeping in `Symtab::InitNamesIndexes()` and added `Symtab::RegisterBacklogEntry()`. I did some benchmarking. The LLDB integrated timers add so much overhead that the numbers are not really helpful. Xcode Instruments gave good results (I can do it with the old impl on Monday). This is from `lldb -b -- /path-to-ninja-build/relwithdebinfo/bin/clang`: 16.00 ms 2.7% Symtab::PreloadSymbols() 16.00 ms 2.7%Symtab::InitNameIndexes() 7.00 ms 1.1% Mangled::DemangleWithRichManglingInfo(RichManglingConte... 3.00 ms 0.5%ConstString::SetCStringWithMangledCounterpart(char co... 3.00 ms 0.5%Pool::GetConstCStringAndSetMangledCounterPart(char co... 2.00 ms 0.3%RichManglingContext::ParseFullName() const 1.00 ms 0.1%lldb_skip_name(llvm::StringRef, Mangled::ManglingScheme) 1.00 ms 0.1%RichManglingContext::FromItaniumName(ConstString const&) 4.00 ms 0.6% Mangled::GuessLanguage() const 4.00 ms 0.6%Mangled::GetDemangledName(lldb::LanguageType) const 2.00 ms 0.3% Symtab::RegisterMangledNameEntry(UniqueCStringMaphttps://reviews.llvm.org/D50071 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49740: Move RegisterValue, Scalar, State from Core to Utility
labath added a comment. In https://reviews.llvm.org/D49740#1187799, @jingham wrote: > I worry a little bit about Utility getting kind of incoherent. Some of it is > clearly utility, some is more there because that's where we are putting the > code we're trying to reduce dependencies on so we can share then with > lldb-server. RegisterValue doesn't for instance seem like a "Utility" class > to me. But maybe this is something to revisit once you've gotten at least > what you need for lldb-server done? I think being a mixed bag is kind of an inherent property of the lowest layers. LLVM's Support library is also a collection of utilities, which have very little in common, but can be combined in various interesting ways to do cool stuff. I am not opposed to opening the discussion of splitting and/or putting these three classes into a difference place right now. If you have an idea on how to organize it, I'd like to hear it. However, I have to say, that RegisterValue actually *does* seem like a good fit for Utility to me -- it's the kind of a thing (representing a "value" of a "register") that every conceivable debugger will need. So it seems good to have it in a place where everybody can use (Utilize :)) it. It's also not host-dependent, as we want to represent registers on remote machines too, so it does not seem like it belongs to Host (although putting it there would have been enough for my present needs). The question that I am not too clear on is what then becomes of "Core", which I think had a similar design goal in mind. Once I am finished here, it will become kind of empty. I guess my best current definition for it would be that it is the "core" of this single specific debugger called "liblldb", which ties all of the interesting components from other modules together, and which probably don't make sense to reuse in a different debugger (e.g. for Zachary's tracing debugger, he could conceivably find a use for RegisterValue or other stuff in Utility, but it's unlikely he would ever want to use the Debugger class). However, I am not really clear on which of our current classes would fit this description. Looking over the current contents of the Utility module, I see only a couple of things that I would not choose to put there: - CompletionRequest - this sounds like it could go next to the command interpreter - SafeMachO - ObjectFileMachO ? - SelectHelper - Host (probably somehow merged with the MainLoop class) However, I'm not too bothered by them being there either, and I doubt this would help with the issues that are troubling you. https://reviews.llvm.org/D49740 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D49740: Move RegisterValue, Scalar, State from Core to Utility
Several months ago when this came up, i hypothesized that Utility would eventually contain a mix of random stuff some of which may not make sense in the long term. But that in the meantime, it’s useful to have some sort of layering abstraction for “has no other dependencies”. Eventually when this reaches critical mass, a natural separation should present itself, because all the stuff that conceptually belongs together will be in Utility. So then we can break Utility up into a more logical separation On Fri, Aug 3, 2018 at 12:37 PM Pavel Labath via Phabricator < revi...@reviews.llvm.org> wrote: > labath added a comment. > > In https://reviews.llvm.org/D49740#1187799, @jingham wrote: > > > I worry a little bit about Utility getting kind of incoherent. Some of > it is clearly utility, some is more there because that's where we are > putting the code we're trying to reduce dependencies on so we can share > then with lldb-server. RegisterValue doesn't for instance seem like a > "Utility" class to me. But maybe this is something to revisit once you've > gotten at least what you need for lldb-server done? > > > I think being a mixed bag is kind of an inherent property of the lowest > layers. LLVM's Support library is also a collection of utilities, which > have very little in common, but can be combined in various interesting ways > to do cool stuff. > > I am not opposed to opening the discussion of splitting and/or putting > these three classes into a difference place right now. If you have an idea > on how to organize it, I'd like to hear it. > > However, I have to say, that RegisterValue actually *does* seem like a > good fit for Utility to me -- it's the kind of a thing (representing a > "value" of a "register") that every conceivable debugger will need. So it > seems good to have it in a place where everybody can use (Utilize :)) it. > It's also not host-dependent, as we want to represent registers on remote > machines too, so it does not seem like it belongs to Host (although putting > it there would have been enough for my present needs). > > The question that I am not too clear on is what then becomes of "Core", > which I think had a similar design goal in mind. Once I am finished here, > it will become kind of empty. I guess my best current definition for it > would be that it is the "core" of this single specific debugger called > "liblldb", which ties all of the interesting components from other modules > together, and which probably don't make sense to reuse in a different > debugger (e.g. for Zachary's tracing debugger, he could conceivably find a > use for RegisterValue or other stuff in Utility, but it's unlikely he would > ever want to use the Debugger class). However, I am not really clear on > which of our current classes would fit this description. > > Looking over the current contents of the Utility module, I see only a > couple of things that I would not choose to put there: > > - CompletionRequest - this sounds like it could go next to the command > interpreter > - SafeMachO - ObjectFileMachO ? > - SelectHelper - Host (probably somehow merged with the MainLoop class) > > However, I'm not too bothered by them being there either, and I doubt this > would help with the issues that are troubling you. > > > https://reviews.llvm.org/D49740 > > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49740: Move RegisterValue, Scalar, State from Core to Utility
zturner added a comment. Several months ago when this came up, i hypothesized that Utility would eventually contain a mix of random stuff some of which may not make sense in the long term. But that in the meantime, it’s useful to have some sort of layering abstraction for “has no other dependencies”. Eventually when this reaches critical mass, a natural separation should present itself, because all the stuff that conceptually belongs together will be in Utility. So then we can break Utility up into a more logical separation https://reviews.llvm.org/D49740 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49963: Preliminary patch to support prompt interpolation
nealsid accepted this revision. nealsid added inline comments. This revision is now accepted and ready to land. Comment at: include/lldb/Host/Editline.h:187 + /// Register a callback to retrieve the prompt. + void SetPromptCallback(PromptCallbackType callback, void *baton); + zturner wrote: > I'd love to stop using the `baton` idiom if possible. can you make this > function take an `llvm::function_ref` instead? Then, > in the class, store a `std::function`. When you call > `SetPromptCallback`, write `SetPromptCallback([this](EditLine* L) { return > this->PromptCallback(L); });` Sounds good to me. I'll split up the changes and sent a patch that migrates to std::function & removes baton from all callbacks in Editline first and then rebase this one on top of that. Repository: rL LLVM https://reviews.llvm.org/D49963 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r338920 - Replace LLDB's LEB128 implementation with the one from LLVM
Author: teemperor Date: Fri Aug 3 13:51:31 2018 New Revision: 338920 URL: http://llvm.org/viewvc/llvm-project?rev=338920&view=rev Log: Replace LLDB's LEB128 implementation with the one from LLVM Reviewers: davide, labath Reviewed By: labath Subscribers: lldb-commits Differential Revision: https://reviews.llvm.org/D50162 Modified: lldb/trunk/source/Utility/Stream.cpp Modified: lldb/trunk/source/Utility/Stream.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/Stream.cpp?rev=338920&r1=338919&r2=338920&view=diff == --- lldb/trunk/source/Utility/Stream.cpp (original) +++ lldb/trunk/source/Utility/Stream.cpp Fri Aug 3 13:51:31 2018 @@ -12,6 +12,7 @@ #include "lldb/Utility/Endian.h" #include "lldb/Utility/VASPrintf.h" #include "llvm/ADT/SmallString.h" // for SmallString +#include "llvm/Support/LEB128.h" #include @@ -49,47 +50,20 @@ void Stream::Offset(uint32_t uval, const // Put an SLEB128 "uval" out to the stream using the printf format in "format". //-- size_t Stream::PutSLEB128(int64_t sval) { - size_t bytes_written = 0; - if (m_flags.Test(eBinary)) { -bool more = true; -while (more) { - uint8_t byte = sval & 0x7fu; - sval >>= 7; - /* sign bit of byte is 2nd high order bit (0x40) */ - if ((sval == 0 && !(byte & 0x40)) || (sval == -1 && (byte & 0x40))) -more = false; - else -// more bytes to come -byte |= 0x80u; - bytes_written += Write(&byte, 1); -} - } else { -bytes_written = Printf("0x%" PRIi64, sval); - } - - return bytes_written; + if (m_flags.Test(eBinary)) +return llvm::encodeSLEB128(sval, m_forwarder); + else +return Printf("0x%" PRIi64, sval); } //-- // Put an ULEB128 "uval" out to the stream using the printf format in "format". //-- size_t Stream::PutULEB128(uint64_t uval) { - size_t bytes_written = 0; - if (m_flags.Test(eBinary)) { -do { - - uint8_t byte = uval & 0x7fu; - uval >>= 7; - if (uval != 0) { -// more bytes to come -byte |= 0x80u; - } - bytes_written += Write(&byte, 1); -} while (uval != 0); - } else { -bytes_written = Printf("0x%" PRIx64, uval); - } - return bytes_written; + if (m_flags.Test(eBinary)) +return llvm::encodeULEB128(uval, m_forwarder); + else +return Printf("0x%" PRIx64, uval); } //-- ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D50162: Replace LLDB's LEB128 implementation with the one from LLVM
This revision was automatically updated to reflect the committed changes. Closed by commit rL338920: Replace LLDB's LEB128 implementation with the one from LLVM (authored by teemperor, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D50162?vs=158930&id=159082#toc Repository: rL LLVM https://reviews.llvm.org/D50162 Files: lldb/trunk/source/Utility/Stream.cpp Index: lldb/trunk/source/Utility/Stream.cpp === --- lldb/trunk/source/Utility/Stream.cpp +++ lldb/trunk/source/Utility/Stream.cpp @@ -12,6 +12,7 @@ #include "lldb/Utility/Endian.h" #include "lldb/Utility/VASPrintf.h" #include "llvm/ADT/SmallString.h" // for SmallString +#include "llvm/Support/LEB128.h" #include @@ -49,47 +50,20 @@ // Put an SLEB128 "uval" out to the stream using the printf format in "format". //-- size_t Stream::PutSLEB128(int64_t sval) { - size_t bytes_written = 0; - if (m_flags.Test(eBinary)) { -bool more = true; -while (more) { - uint8_t byte = sval & 0x7fu; - sval >>= 7; - /* sign bit of byte is 2nd high order bit (0x40) */ - if ((sval == 0 && !(byte & 0x40)) || (sval == -1 && (byte & 0x40))) -more = false; - else -// more bytes to come -byte |= 0x80u; - bytes_written += Write(&byte, 1); -} - } else { -bytes_written = Printf("0x%" PRIi64, sval); - } - - return bytes_written; + if (m_flags.Test(eBinary)) +return llvm::encodeSLEB128(sval, m_forwarder); + else +return Printf("0x%" PRIi64, sval); } //-- // Put an ULEB128 "uval" out to the stream using the printf format in "format". //-- size_t Stream::PutULEB128(uint64_t uval) { - size_t bytes_written = 0; - if (m_flags.Test(eBinary)) { -do { - - uint8_t byte = uval & 0x7fu; - uval >>= 7; - if (uval != 0) { -// more bytes to come -byte |= 0x80u; - } - bytes_written += Write(&byte, 1); -} while (uval != 0); - } else { -bytes_written = Printf("0x%" PRIx64, uval); - } - return bytes_written; + if (m_flags.Test(eBinary)) +return llvm::encodeULEB128(uval, m_forwarder); + else +return Printf("0x%" PRIx64, uval); } //-- Index: lldb/trunk/source/Utility/Stream.cpp === --- lldb/trunk/source/Utility/Stream.cpp +++ lldb/trunk/source/Utility/Stream.cpp @@ -12,6 +12,7 @@ #include "lldb/Utility/Endian.h" #include "lldb/Utility/VASPrintf.h" #include "llvm/ADT/SmallString.h" // for SmallString +#include "llvm/Support/LEB128.h" #include @@ -49,47 +50,20 @@ // Put an SLEB128 "uval" out to the stream using the printf format in "format". //-- size_t Stream::PutSLEB128(int64_t sval) { - size_t bytes_written = 0; - if (m_flags.Test(eBinary)) { -bool more = true; -while (more) { - uint8_t byte = sval & 0x7fu; - sval >>= 7; - /* sign bit of byte is 2nd high order bit (0x40) */ - if ((sval == 0 && !(byte & 0x40)) || (sval == -1 && (byte & 0x40))) -more = false; - else -// more bytes to come -byte |= 0x80u; - bytes_written += Write(&byte, 1); -} - } else { -bytes_written = Printf("0x%" PRIi64, sval); - } - - return bytes_written; + if (m_flags.Test(eBinary)) +return llvm::encodeSLEB128(sval, m_forwarder); + else +return Printf("0x%" PRIi64, sval); } //-- // Put an ULEB128 "uval" out to the stream using the printf format in "format". //-- size_t Stream::PutULEB128(uint64_t uval) { - size_t bytes_written = 0; - if (m_flags.Test(eBinary)) { -do { - - uint8_t byte = uval & 0x7fu; - uval >>= 7; - if (uval != 0) { -// more bytes to come -byte |= 0x80u; - } - bytes_written += Write(&byte, 1); -} while (uval != 0); - } else { -bytes_written = Printf("0x%" PRIx64, uval); - } - return bytes_written; + if (m_flags.Test(eBinary)) +return llvm::encodeULEB128(uval, m_forwarder); + else +return Printf("0x%" PRIx64, uval); } //-- ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D50161: Add raw_ostream wrapper to the Stream class
teemperor added a comment. StreamTee is copying it, which is expected to be copyable when we copy CommandObjectResult around. And then I just added the copy-constructor as CommandObjectResult refactoring sound time-expensive. Repository: rL LLVM https://reviews.llvm.org/D50161 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D50271: [IRMemoryMap] Shrink Allocation by sizeof(addr_t) (NFC)
vsk created this revision. vsk added reviewers: teemperor, lhames. Profiling data show that Allocation::operator= is hot, see: https://teemperor.de/lldb-bench/data/arithmetic.svg Reorder a few fields within Allocation to avoid implicit structure padding and shrink the structure. This should make copies cheaper and reduce lldb's RSS. There should be more low-hanging fruit here for performance optimization: we don't need std::map, we can make Allocation move-only, etc. https://reviews.llvm.org/D50271 Files: lldb/include/lldb/Expression/IRMemoryMap.h lldb/source/Expression/IRMemoryMap.cpp Index: lldb/source/Expression/IRMemoryMap.cpp === --- lldb/source/Expression/IRMemoryMap.cpp +++ lldb/source/Expression/IRMemoryMap.cpp @@ -272,8 +272,8 @@ uint32_t permissions, uint8_t alignment, AllocationPolicy policy) : m_process_alloc(process_alloc), m_process_start(process_start), - m_size(size), m_permissions(permissions), m_alignment(alignment), - m_policy(policy), m_leak(false) { + m_size(size), m_policy(policy), m_leak(false), m_permissions(permissions), + m_alignment(alignment) { switch (policy) { default: assert(0 && "We cannot reach this!"); Index: lldb/include/lldb/Expression/IRMemoryMap.h === --- lldb/include/lldb/Expression/IRMemoryMap.h +++ lldb/include/lldb/Expression/IRMemoryMap.h @@ -39,7 +39,7 @@ IRMemoryMap(lldb::TargetSP target_sp); ~IRMemoryMap(); - enum AllocationPolicy { + enum AllocationPolicy : uint8_t { eAllocationPolicyInvalid = 0, ///< It is an error for an allocation to have this policy. eAllocationPolicyHostOnly, ///< This allocation was created in the host and @@ -91,32 +91,36 @@ private: struct Allocation { lldb::addr_t -m_process_alloc; ///< The (unaligned) base for the remote allocation +m_process_alloc; ///< The (unaligned) base for the remote allocation. lldb::addr_t -m_process_start; ///< The base address of the allocation in the process -size_t m_size; ///< The size of the requested allocation -uint32_t m_permissions; ///< The access permissions on the memory in the -///process. In the host, the memory is always -///read/write. -uint8_t m_alignment;///< The alignment of the requested allocation +m_process_start; ///< The base address of the allocation in the process. +size_t m_size; ///< The size of the requested allocation. DataBufferHeap m_data; -///< Flags +/// Flags. Keep these grouped together to avoid structure padding. AllocationPolicy m_policy; bool m_leak; +uint8_t m_permissions; ///< The access permissions on the memory in the + /// process. In the host, the memory is always + /// read/write. +uint8_t m_alignment; ///< The alignment of the requested allocation. public: Allocation(lldb::addr_t process_alloc, lldb::addr_t process_start, size_t size, uint32_t permissions, uint8_t alignment, AllocationPolicy m_policy); Allocation() : m_process_alloc(LLDB_INVALID_ADDRESS), - m_process_start(LLDB_INVALID_ADDRESS), m_size(0), m_permissions(0), - m_alignment(0), m_data(), m_policy(eAllocationPolicyInvalid), - m_leak(false) {} + m_process_start(LLDB_INVALID_ADDRESS), m_size(0), m_data(), + m_policy(eAllocationPolicyInvalid), m_leak(false), m_permissions(0), + m_alignment(0) {} }; + static_assert(sizeof(Allocation) <= +(4 * sizeof(lldb::addr_t)) + sizeof(DataBufferHeap), +"IRMemoryMap::Allocation is larger than expected"); + lldb::ProcessWP m_process_wp; lldb::TargetWP m_target_wp; typedef std::map AllocationMap; Index: lldb/source/Expression/IRMemoryMap.cpp === --- lldb/source/Expression/IRMemoryMap.cpp +++ lldb/source/Expression/IRMemoryMap.cpp @@ -272,8 +272,8 @@ uint32_t permissions, uint8_t alignment, AllocationPolicy policy) : m_process_alloc(process_alloc), m_process_start(process_start), - m_size(size), m_permissions(permissions), m_alignment(alignment), - m_policy(policy), m_leak(false) { + m_size(size), m_policy(policy), m_leak(false), m_permissions(permissions), + m_alignment(alignment) { switch (policy) { default: assert(0 && "We cannot reach this!"); Index: lldb/include/lldb/Expression/IRMemoryMap.h === --- lldb/include/lldb/Expression/IRMemoryMap.h +++ lldb/include/lldb/Expressio
[Lldb-commits] [lldb] r338923 - Modify lldb_suite.py to enable python debugging
Author: xiaobai Date: Fri Aug 3 14:37:01 2018 New Revision: 338923 URL: http://llvm.org/viewvc/llvm-project?rev=338923&view=rev Log: Modify lldb_suite.py to enable python debugging Summary: pudb and pdb interfere with the behavior of the inspect module. calling `inspect.getfile(inspect.currentframe())` returns a different result depending on whether or not you're in a debugger. Calling `os.path.abspath` on the result of `inspect.getfile(...)` normalizes the result between the two environments. Patch by Nathan Lanza Differential Revision: https://reviews.llvm.org/D49620 Modified: lldb/trunk/test/use_lldb_suite.py Modified: lldb/trunk/test/use_lldb_suite.py URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/test/use_lldb_suite.py?rev=338923&r1=338922&r2=338923&view=diff == --- lldb/trunk/test/use_lldb_suite.py (original) +++ lldb/trunk/test/use_lldb_suite.py Fri Aug 3 14:37:01 2018 @@ -4,7 +4,9 @@ import sys def find_lldb_root(): -lldb_root = os.path.dirname(inspect.getfile(inspect.currentframe())) +lldb_root = os.path.dirname( +os.path.abspath(inspect.getfile(inspect.currentframe())) +) while True: lldb_root = os.path.dirname(lldb_root) if lldb_root is None: ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49740: Move RegisterValue, Scalar, State from Core to Utility
jingham added a comment. First off, I'm fine with Zachary's suggestion that we let the dust settle a bit before we try to organize things better. We'll probably do a better job then. But just to use these cases, for instance, Scalar is the base of how we realize values in the debugger (ValueObject lives on top of this). It would be good if its landing place was such that if I was looking at how we represent values it was logically placed for that. State is part of how we present the Process current state, so it seems like it should be grouped with other process related entities. And RegisterValue belongs with the other parts of lldb that take apart the machine level state of a process. It will probably go away, but FastDemangle really belongs around the language handling code. I agree that Utility is an odd place for CompletionRequest... SafeMachO is weird. It gets used in Host - both common & macosx, and we're trying not to include out of Plugins so the MachO object file plugin directory doesn't seem right. Maybe Host is a better place, it's reason for being is so you can include both llvm/Support/MachO.h and mach/machine.h, so even though it's not directly a host functionality it look a bit like that. Not sure. There's also tension between "things that belong together functionally" and "things that need to be separated because we want to layer one strictly on top of the other, since we seem to be treating directories as a representation of dependencies. Do we want to have a Values directory with ValueObject at the top level, and Scalar in a no-dependency subdirectory? , if I need to find a file these days, I either Cmd-Click on a type to go to its definition, or type Cmd-Shift-O and start typing some bits of the file name and Xcode finds it for me. I'm pretty familiar with the code, so I don't need a higher level skeleton to help me figure out what all is in this project. I think at this point many of us are in this state... But once you get past the build issues, the real audience for this level of organization is folks coming new to the project. It would be good to hear from some of the newer folks what they found confusing or what would have helped them navigate around the project as they are starting out. https://reviews.llvm.org/D49740 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49740: Move RegisterValue, Scalar, State from Core to Utility
zturner added a comment. For the Register stuff, for example, I think it could make sense for it to be in a project such as `HAL` (Hardware Abstraction Layer) or something similarly named. Everything that describes properties of specific CPUs could go there, perhaps even including `ArchSpec`. For the Scalar stuff, perhaps a target called `Visualization` (or even just `Value`). Once more and more stuff starts ending up in Utility, I think it makes sense to try something like this. https://reviews.llvm.org/D49740 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49740: Move RegisterValue, Scalar, State from Core to Utility
teemperor added a comment. > CompletionRequest - this sounds like it could go next to the command > interpreter Yeah, makes sense. Even though Utility classes then can either not offer completion methods (currently only ArchSpec is doing that) or work around that with a forward decl. But I think that's not a big issue. https://reviews.llvm.org/D49740 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D50274: Misc module/dwarf logging improvements
lemo created this revision. lemo added reviewers: labath, clayborg. lemo added a project: LLDB. Herald added subscribers: JDevlieghere, arichardson, aprantl, emaste. Herald added a reviewer: espindola. This change improves the logging for the lldb.module category to note a few interesting cases: 1. Local object file found, but specs not matching 2. Local object file not found, using a placeholder module The logging for failing to load compressed dwarf symbols is also improved. Repository: rLLDB LLDB https://reviews.llvm.org/D50274 Files: source/Core/Module.cpp source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp source/Plugins/Process/minidump/ProcessMinidump.cpp source/Target/Process.cpp Index: source/Target/Process.cpp === --- source/Target/Process.cpp +++ source/Target/Process.cpp @@ -5842,7 +5842,7 @@ // that loaded. // Iterate over a copy of this language runtime list in case the language - // runtime ModulesDidLoad somehow causes the language riuntime to be + // runtime ModulesDidLoad somehow causes the language runtime to be // unloaded. LanguageRuntimeCollection language_runtimes(m_language_runtimes); for (const auto &pair : language_runtimes) { @@ -6095,15 +6095,15 @@ // For each StructuredDataPlugin, if the plugin handles any of the types in // the supported_type_names, map that type name to that plugin. Stop when // we've consumed all the type names. - // FIXME: should we return an error if there are type names nobody + // FIXME: should we return an error if there are type names nobody // supports? for (uint32_t plugin_index = 0; !const_type_names.empty(); plugin_index++) { auto create_instance = PluginManager::GetStructuredDataPluginCreateCallbackAtIndex( plugin_index); if (!create_instance) break; - + // Create the plugin. StructuredDataPluginSP plugin_sp = (*create_instance)(*this); if (!plugin_sp) { Index: source/Plugins/Process/minidump/ProcessMinidump.cpp === --- source/Plugins/Process/minidump/ProcessMinidump.cpp +++ source/Plugins/Process/minidump/ProcessMinidump.cpp @@ -357,6 +357,12 @@ // This enables most LLDB functionality involving address-to-module // translations (ex. identifing the module for a stack frame PC) and // modules/sections commands (ex. target modules list, ...) + if (log) { +log->Printf("Unable to locate the matching object file, creating a " +"placeholder module for: %s", +name.getValue().c_str()); + } + auto placeholder_module = std::make_shared(module_spec); placeholder_module->CreateImageSection(module, GetTarget()); Index: source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp === --- source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp +++ source/Plugins/ObjectFile/ELF/ObjectFileELF.cpp @@ -1390,7 +1390,7 @@ arch_spec.GetTriple().getOS() == llvm::Triple::OSType::UnknownOS) // In case of MIPSR6, the LLDB_NT_OWNER_GNU note is missing for some // cases (e.g. compile with -nostdlib) Hence set OS to Linux - arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux); + arch_spec.GetTriple().setOS(llvm::Triple::OSType::Linux); } } @@ -1494,7 +1494,7 @@ const uint32_t sub_type = subTypeFromElfHeader(header); arch_spec.SetArchitecture(eArchTypeELF, header.e_machine, sub_type, header.e_ident[EI_OSABI]); - + // Validate if it is ok to remove GetOsFromOSABI. Note, that now the OS is // determined based on EI_OSABI flag and the info extracted from ELF notes // (see RefineModuleDetailsFromNote). However in some cases that still @@ -3397,19 +3397,20 @@ size_t(section_data.GetByteSize())}, GetByteOrder() == eByteOrderLittle, GetAddressByteSize() == 8); if (!Decompressor) { -LLDB_LOG_ERROR(log, Decompressor.takeError(), - "Unable to initialize decompressor for section {0}", - section->GetName()); -return result; +GetModule()->ReportWarning( +"Unable to initialize decompressor for section '%s'", +section->GetName().GetCString()); +return 0; } auto buffer_sp = std::make_shared(Decompressor->getDecompressedSize(), 0); if (auto Error = Decompressor->decompress( {reinterpret_cast(buffer_sp->GetBytes()), size_t(buffer_sp->GetByteSize())})) { -LLDB_LOG_ERROR(log, std::move(Error), "Decompression of section {0} failed", - section->GetName()); -return result; +GetModule()->ReportWarning( +"Decompression of section '%s' failed", +section->GetName().GetCString()); +return 0; } section_data.SetDa
[Lldb-commits] [PATCH] D50290: Fix a bug in VMRange
lemo created this revision. lemo added reviewers: zturner, labath, teemperor. lemo added a project: LLDB. I noticed a suspicious failure: [ RUN ] VMRange.CollectionContains llvm/src/tools/lldb/unittests/Utility/VMRangeTest.cpp:146: Failure Value of: VMRange::ContainsRange(collection, VMRange(0x100, 0x104)) Actual: false Expected: true Looking at the code, it is a very real bug: class RangeInRangeUnaryPredicate { public: RangeInRangeUnaryPredicate(VMRange range) : _range(range) {} // note that _range binds to a temporary! bool operator()(const VMRange &range) const { return range.Contains(_range); } const VMRange &_range; }; This change fixes the bug. Repository: rLLDB LLDB https://reviews.llvm.org/D50290 Files: include/lldb/Utility/VMRange.h source/Utility/VMRange.cpp Index: source/Utility/VMRange.cpp === --- source/Utility/VMRange.cpp +++ source/Utility/VMRange.cpp @@ -24,14 +24,16 @@ bool VMRange::ContainsValue(const VMRange::collection &coll, lldb::addr_t value) { - ValueInRangeUnaryPredicate in_range_predicate(value); - return llvm::find_if(coll, in_range_predicate) != coll.end(); + return llvm::find_if(coll, [&](const VMRange &r) { + return r.Contains(value); + }) != coll.end(); } bool VMRange::ContainsRange(const VMRange::collection &coll, const VMRange &range) { - RangeInRangeUnaryPredicate in_range_predicate(range); - return llvm::find_if(coll, in_range_predicate) != coll.end(); + return llvm::find_if(coll, [&](const VMRange &r) { + return r.Contains(range); + }) != coll.end(); } void VMRange::Dump(Stream *s, lldb::addr_t offset, uint32_t addr_width) const { Index: include/lldb/Utility/VMRange.h === --- include/lldb/Utility/VMRange.h +++ include/lldb/Utility/VMRange.h @@ -87,24 +87,6 @@ void Dump(Stream *s, lldb::addr_t base_addr = 0, uint32_t addr_width = 8) const; - class ValueInRangeUnaryPredicate { - public: -ValueInRangeUnaryPredicate(lldb::addr_t value) : _value(value) {} -bool operator()(const VMRange &range) const { - return range.Contains(_value); -} -lldb::addr_t _value; - }; - - class RangeInRangeUnaryPredicate { - public: -RangeInRangeUnaryPredicate(VMRange range) : _range(range) {} -bool operator()(const VMRange &range) const { - return range.Contains(_range); -} -const VMRange &_range; - }; - static bool ContainsValue(const VMRange::collection &coll, lldb::addr_t value); Index: source/Utility/VMRange.cpp === --- source/Utility/VMRange.cpp +++ source/Utility/VMRange.cpp @@ -24,14 +24,16 @@ bool VMRange::ContainsValue(const VMRange::collection &coll, lldb::addr_t value) { - ValueInRangeUnaryPredicate in_range_predicate(value); - return llvm::find_if(coll, in_range_predicate) != coll.end(); + return llvm::find_if(coll, [&](const VMRange &r) { + return r.Contains(value); + }) != coll.end(); } bool VMRange::ContainsRange(const VMRange::collection &coll, const VMRange &range) { - RangeInRangeUnaryPredicate in_range_predicate(range); - return llvm::find_if(coll, in_range_predicate) != coll.end(); + return llvm::find_if(coll, [&](const VMRange &r) { + return r.Contains(range); + }) != coll.end(); } void VMRange::Dump(Stream *s, lldb::addr_t offset, uint32_t addr_width) const { Index: include/lldb/Utility/VMRange.h === --- include/lldb/Utility/VMRange.h +++ include/lldb/Utility/VMRange.h @@ -87,24 +87,6 @@ void Dump(Stream *s, lldb::addr_t base_addr = 0, uint32_t addr_width = 8) const; - class ValueInRangeUnaryPredicate { - public: -ValueInRangeUnaryPredicate(lldb::addr_t value) : _value(value) {} -bool operator()(const VMRange &range) const { - return range.Contains(_value); -} -lldb::addr_t _value; - }; - - class RangeInRangeUnaryPredicate { - public: -RangeInRangeUnaryPredicate(VMRange range) : _range(range) {} -bool operator()(const VMRange &range) const { - return range.Contains(_range); -} -const VMRange &_range; - }; - static bool ContainsValue(const VMRange::collection &coll, lldb::addr_t value); ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D49415: Add unit tests for VMRange
The new test cases did catch a real bug: [ RUN ] VMRange.CollectionContains llvm/src/tools/lldb/unittests/Utility/VMRangeTest.cpp:146: Failure Value of: VMRange::ContainsRange(collection, VMRange(0x100, 0x104)) Actual: false Expected: true class RangeInRangeUnaryPredicate { public: RangeInRangeUnaryPredicate(VMRange range) : _range(range) {} // note that _range binds to a temporary! bool operator()(const VMRange &range) const { return range.Contains(_range); } const VMRange &_range; }; I just sent out a review for the fix (https://reviews.llvm.org/D50290) On Tue, Jul 24, 2018 at 4:53 PM, Raphael Isemann via Phabricator via lldb-commits wrote: > This revision was not accepted when it landed; it landed in state "Needs > Review". > This revision was automatically updated to reflect the committed changes. > Closed by commit rL337873: Add unit tests for VMRange (authored by > teemperor, committed by ). > Herald added a subscriber: llvm-commits. > > Changed prior to commit: > https://reviews.llvm.org/D49415?vs=157172&id=157173#toc > > Repository: > rL LLVM > > https://reviews.llvm.org/D49415 > > Files: > lldb/trunk/unittests/Utility/CMakeLists.txt > lldb/trunk/unittests/Utility/VMRangeTest.cpp > > > ___ > lldb-commits mailing list > lldb-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D49415: Add unit tests for VMRange
lemo added subscribers: teemperor, lemo. lemo added a comment. The new test cases did catch a real bug: [ RUN ] VMRange.CollectionContains llvm/src/tools/lldb/unittests/Utility/VMRangeTest.cpp:146: Failure Value of: VMRange::ContainsRange(collection, VMRange(0x100, 0x104)) Actual: false Expected: true class RangeInRangeUnaryPredicate { public: RangeInRangeUnaryPredicate(VMRange range) : _range(range) {} // note that _range binds to a temporary! bool operator()(const VMRange &range) const { return range.Contains(_range); } const VMRange &_range; }; I just sent out a review for the fix (https://reviews.llvm.org/D50290) Repository: rL LLVM https://reviews.llvm.org/D49415 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D50290: Fix a bug in VMRange
I think we have llvm::contains() which would allow you to make this slightly better. Other than that, good find! On Fri, Aug 3, 2018 at 5:49 PM Leonard Mosescu via Phabricator < revi...@reviews.llvm.org> wrote: > lemo created this revision. > lemo added reviewers: zturner, labath, teemperor. > lemo added a project: LLDB. > > I noticed a suspicious failure: > > [ RUN ] VMRange.CollectionContains > llvm/src/tools/lldb/unittests/Utility/VMRangeTest.cpp:146: Failure > Value of: VMRange::ContainsRange(collection, VMRange(0x100, 0x104)) > > Actual: false > > Expected: true > > Looking at the code, it is a very real bug: > > class RangeInRangeUnaryPredicate { > public: > RangeInRangeUnaryPredicate(VMRange range) : _range(range) {} // note > that _range binds to a temporary! > bool operator()(const VMRange &range) const { > return range.Contains(_range); > } > const VMRange &_range; > }; > > This change fixes the bug. > > > Repository: > rLLDB LLDB > > https://reviews.llvm.org/D50290 > > Files: > include/lldb/Utility/VMRange.h > source/Utility/VMRange.cpp > > > Index: source/Utility/VMRange.cpp > === > --- source/Utility/VMRange.cpp > +++ source/Utility/VMRange.cpp > @@ -24,14 +24,16 @@ > > bool VMRange::ContainsValue(const VMRange::collection &coll, > lldb::addr_t value) { > - ValueInRangeUnaryPredicate in_range_predicate(value); > - return llvm::find_if(coll, in_range_predicate) != coll.end(); > + return llvm::find_if(coll, [&](const VMRange &r) { > + return r.Contains(value); > + }) != coll.end(); > } > > bool VMRange::ContainsRange(const VMRange::collection &coll, > const VMRange &range) { > - RangeInRangeUnaryPredicate in_range_predicate(range); > - return llvm::find_if(coll, in_range_predicate) != coll.end(); > + return llvm::find_if(coll, [&](const VMRange &r) { > + return r.Contains(range); > + }) != coll.end(); > } > > void VMRange::Dump(Stream *s, lldb::addr_t offset, uint32_t addr_width) > const { > Index: include/lldb/Utility/VMRange.h > === > --- include/lldb/Utility/VMRange.h > +++ include/lldb/Utility/VMRange.h > @@ -87,24 +87,6 @@ >void Dump(Stream *s, lldb::addr_t base_addr = 0, > uint32_t addr_width = 8) const; > > - class ValueInRangeUnaryPredicate { > - public: > -ValueInRangeUnaryPredicate(lldb::addr_t value) : _value(value) {} > -bool operator()(const VMRange &range) const { > - return range.Contains(_value); > -} > -lldb::addr_t _value; > - }; > - > - class RangeInRangeUnaryPredicate { > - public: > -RangeInRangeUnaryPredicate(VMRange range) : _range(range) {} > -bool operator()(const VMRange &range) const { > - return range.Contains(_range); > -} > -const VMRange &_range; > - }; > - >static bool ContainsValue(const VMRange::collection &coll, > lldb::addr_t value); > > > > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D50290: Fix a bug in VMRange
zturner added a subscriber: lemo. zturner added a comment. I think we have llvm::contains() which would allow you to make this slightly better. Other than that, good find! Repository: rLLDB LLDB https://reviews.llvm.org/D50290 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D50290: Fix a bug in VMRange
Thanks Zach. I can't find llvm::contains() though, any pointers to it? On Fri, Aug 3, 2018 at 5:58 PM, Zachary Turner via Phabricator via lldb-commits wrote: > zturner added a subscriber: lemo. > zturner added a comment. > > I think we have llvm::contains() which would allow you to make this > slightly better. Other than that, good find! > > > Repository: > rLLDB LLDB > > https://reviews.llvm.org/D50290 > > > > ___ > lldb-commits mailing list > lldb-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D50290: Fix a bug in VMRange
lemo added subscribers: bgianfo, labath, penryu, teemperor, zturner. lemo added a comment. Thanks Zach. I can't find llvm::contains() though, any pointers to it? Repository: rLLDB LLDB https://reviews.llvm.org/D50290 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
Re: [Lldb-commits] [PATCH] D50290: Fix a bug in VMRange
Looks like i was wrong, nevermind! On Fri, Aug 3, 2018 at 6:23 PM Leonard Mosescu via Phabricator via lldb-commits wrote: > lemo added subscribers: bgianfo, labath, penryu, teemperor, zturner. > lemo added a comment. > > Thanks Zach. I can't find llvm::contains() though, any pointers to it? > > > Repository: > rLLDB LLDB > > https://reviews.llvm.org/D50290 > > > > ___ > lldb-commits mailing list > lldb-commits@lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits > ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D50290: Fix a bug in VMRange
zturner added a comment. Looks like i was wrong, nevermind! Repository: rLLDB LLDB https://reviews.llvm.org/D50290 ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D50290: Fix a bug in VMRange
This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rL338949: Fix a bug in VMRange (authored by lemo, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D50290?vs=159147&id=159156#toc Repository: rL LLVM https://reviews.llvm.org/D50290 Files: lldb/trunk/include/lldb/Utility/VMRange.h lldb/trunk/source/Utility/VMRange.cpp Index: lldb/trunk/source/Utility/VMRange.cpp === --- lldb/trunk/source/Utility/VMRange.cpp +++ lldb/trunk/source/Utility/VMRange.cpp @@ -24,14 +24,16 @@ bool VMRange::ContainsValue(const VMRange::collection &coll, lldb::addr_t value) { - ValueInRangeUnaryPredicate in_range_predicate(value); - return llvm::find_if(coll, in_range_predicate) != coll.end(); + return llvm::find_if(coll, [&](const VMRange &r) { + return r.Contains(value); + }) != coll.end(); } bool VMRange::ContainsRange(const VMRange::collection &coll, const VMRange &range) { - RangeInRangeUnaryPredicate in_range_predicate(range); - return llvm::find_if(coll, in_range_predicate) != coll.end(); + return llvm::find_if(coll, [&](const VMRange &r) { + return r.Contains(range); + }) != coll.end(); } void VMRange::Dump(Stream *s, lldb::addr_t offset, uint32_t addr_width) const { Index: lldb/trunk/include/lldb/Utility/VMRange.h === --- lldb/trunk/include/lldb/Utility/VMRange.h +++ lldb/trunk/include/lldb/Utility/VMRange.h @@ -87,24 +87,6 @@ void Dump(Stream *s, lldb::addr_t base_addr = 0, uint32_t addr_width = 8) const; - class ValueInRangeUnaryPredicate { - public: -ValueInRangeUnaryPredicate(lldb::addr_t value) : _value(value) {} -bool operator()(const VMRange &range) const { - return range.Contains(_value); -} -lldb::addr_t _value; - }; - - class RangeInRangeUnaryPredicate { - public: -RangeInRangeUnaryPredicate(VMRange range) : _range(range) {} -bool operator()(const VMRange &range) const { - return range.Contains(_range); -} -const VMRange &_range; - }; - static bool ContainsValue(const VMRange::collection &coll, lldb::addr_t value); Index: lldb/trunk/source/Utility/VMRange.cpp === --- lldb/trunk/source/Utility/VMRange.cpp +++ lldb/trunk/source/Utility/VMRange.cpp @@ -24,14 +24,16 @@ bool VMRange::ContainsValue(const VMRange::collection &coll, lldb::addr_t value) { - ValueInRangeUnaryPredicate in_range_predicate(value); - return llvm::find_if(coll, in_range_predicate) != coll.end(); + return llvm::find_if(coll, [&](const VMRange &r) { + return r.Contains(value); + }) != coll.end(); } bool VMRange::ContainsRange(const VMRange::collection &coll, const VMRange &range) { - RangeInRangeUnaryPredicate in_range_predicate(range); - return llvm::find_if(coll, in_range_predicate) != coll.end(); + return llvm::find_if(coll, [&](const VMRange &r) { + return r.Contains(range); + }) != coll.end(); } void VMRange::Dump(Stream *s, lldb::addr_t offset, uint32_t addr_width) const { Index: lldb/trunk/include/lldb/Utility/VMRange.h === --- lldb/trunk/include/lldb/Utility/VMRange.h +++ lldb/trunk/include/lldb/Utility/VMRange.h @@ -87,24 +87,6 @@ void Dump(Stream *s, lldb::addr_t base_addr = 0, uint32_t addr_width = 8) const; - class ValueInRangeUnaryPredicate { - public: -ValueInRangeUnaryPredicate(lldb::addr_t value) : _value(value) {} -bool operator()(const VMRange &range) const { - return range.Contains(_value); -} -lldb::addr_t _value; - }; - - class RangeInRangeUnaryPredicate { - public: -RangeInRangeUnaryPredicate(VMRange range) : _range(range) {} -bool operator()(const VMRange &range) const { - return range.Contains(_range); -} -const VMRange &_range; - }; - static bool ContainsValue(const VMRange::collection &coll, lldb::addr_t value); ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r338949 - Fix a bug in VMRange
Author: lemo Date: Fri Aug 3 19:15:26 2018 New Revision: 338949 URL: http://llvm.org/viewvc/llvm-project?rev=338949&view=rev Log: Fix a bug in VMRange I noticed a suspicious failure: [ RUN ] VMRange.CollectionContains llvm/src/tools/lldb/unittests/Utility/VMRangeTest.cpp:146: Failure Value of: VMRange::ContainsRange(collection, VMRange(0x100, 0x104)) Actual: false Expected: true Looking at the code, it is a very real bug: class RangeInRangeUnaryPredicate { public: RangeInRangeUnaryPredicate(VMRange range) : _range(range) {} // note that _range binds to a temporary! bool operator()(const VMRange &range) const { return range.Contains(_range); } const VMRange &_range; }; This change fixes the bug. Differential Revision: https://reviews.llvm.org/D50290 Modified: lldb/trunk/include/lldb/Utility/VMRange.h lldb/trunk/source/Utility/VMRange.cpp Modified: lldb/trunk/include/lldb/Utility/VMRange.h URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Utility/VMRange.h?rev=338949&r1=338948&r2=338949&view=diff == --- lldb/trunk/include/lldb/Utility/VMRange.h (original) +++ lldb/trunk/include/lldb/Utility/VMRange.h Fri Aug 3 19:15:26 2018 @@ -87,24 +87,6 @@ public: void Dump(Stream *s, lldb::addr_t base_addr = 0, uint32_t addr_width = 8) const; - class ValueInRangeUnaryPredicate { - public: -ValueInRangeUnaryPredicate(lldb::addr_t value) : _value(value) {} -bool operator()(const VMRange &range) const { - return range.Contains(_value); -} -lldb::addr_t _value; - }; - - class RangeInRangeUnaryPredicate { - public: -RangeInRangeUnaryPredicate(VMRange range) : _range(range) {} -bool operator()(const VMRange &range) const { - return range.Contains(_range); -} -const VMRange &_range; - }; - static bool ContainsValue(const VMRange::collection &coll, lldb::addr_t value); Modified: lldb/trunk/source/Utility/VMRange.cpp URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/source/Utility/VMRange.cpp?rev=338949&r1=338948&r2=338949&view=diff == --- lldb/trunk/source/Utility/VMRange.cpp (original) +++ lldb/trunk/source/Utility/VMRange.cpp Fri Aug 3 19:15:26 2018 @@ -24,14 +24,16 @@ using namespace lldb_private; bool VMRange::ContainsValue(const VMRange::collection &coll, lldb::addr_t value) { - ValueInRangeUnaryPredicate in_range_predicate(value); - return llvm::find_if(coll, in_range_predicate) != coll.end(); + return llvm::find_if(coll, [&](const VMRange &r) { + return r.Contains(value); + }) != coll.end(); } bool VMRange::ContainsRange(const VMRange::collection &coll, const VMRange &range) { - RangeInRangeUnaryPredicate in_range_predicate(range); - return llvm::find_if(coll, in_range_predicate) != coll.end(); + return llvm::find_if(coll, [&](const VMRange &r) { + return r.Contains(range); + }) != coll.end(); } void VMRange::Dump(Stream *s, lldb::addr_t offset, uint32_t addr_width) const { ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [lldb] r338952 - Fixed header of StringLexer.h
Author: teemperor Date: Fri Aug 3 22:53:07 2018 New Revision: 338952 URL: http://llvm.org/viewvc/llvm-project?rev=338952&view=rev Log: Fixed header of StringLexer.h Modified: lldb/trunk/include/lldb/Utility/StringLexer.h Modified: lldb/trunk/include/lldb/Utility/StringLexer.h URL: http://llvm.org/viewvc/llvm-project/lldb/trunk/include/lldb/Utility/StringLexer.h?rev=338952&r1=338951&r2=338952&view=diff == --- lldb/trunk/include/lldb/Utility/StringLexer.h (original) +++ lldb/trunk/include/lldb/Utility/StringLexer.h Fri Aug 3 22:53:07 2018 @@ -1,5 +1,4 @@ -//===- StringLexer.h -*- C++ -//-*-===// +//===- StringLexer.h *- C++ -*-===// // // The LLVM Compiler Infrastructure // ___ lldb-commits mailing list lldb-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits