[Lldb-commits] [PATCH] D55653: [lldb-mi] Check raw pointers before passing them to std::string ctor/assignment
hintonda added inline comments. Comment at: lldb/trunk/unittests/tools/lldb-mi/utils/CMakeLists.txt:12 + +target_sources(LLDBMiUtilTests PRIVATE $) Just wanted to let you know that using `$` in anything other than `add_library` and `add_executable` wasn't added until cmake 3.9, so I'm unable to configure lldb using the officially supported cmake 3.4.3 version. Btw, I use cmake 3.4.3 locally so I don't unintentionally use new unsupported features to the clang/llvm cmake files. Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D55653/new/ https://reviews.llvm.org/D55653 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads
aadsm updated this revision to Diff 204977.
aadsm added a comment.
Herald added a subscriber: emaste.
- Make ReadCStringFromMemory return llvm::Expected>
- Add documentation
- Address other comments
- Created `static const size_t g_cache_line_size` to avoid calling
llvm::sys::Process::getPageSizeEstimate() all the time, that thing is expensive.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62503/new/
https://reviews.llvm.org/D62503
Files:
lldb/include/lldb/Host/common/NativeProcessProtocol.h
lldb/source/Host/common/NativeProcessProtocol.cpp
lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
lldb/source/Plugins/Process/POSIX/NativeProcessELF.cpp
lldb/unittests/Host/NativeProcessProtocolTest.cpp
Index: lldb/unittests/Host/NativeProcessProtocolTest.cpp
===
--- lldb/unittests/Host/NativeProcessProtocolTest.cpp
+++ lldb/unittests/Host/NativeProcessProtocolTest.cpp
@@ -96,3 +96,35 @@
EXPECT_THAT_EXPECTED(Process.ReadMemoryWithoutTrap(4, 2),
llvm::HasValue(std::vector{4, 5}));
}
+
+TEST(NativeProcessProtocolTest, ReadCStringFromMemory) {
+ NiceMock DummyDelegate;
+ MockProcess Process(DummyDelegate,
+ ArchSpec("aarch64-pc-linux"));
+ FakeMemory M{{'h', 'e', 'l', 'l', 'o', 0, 'w', 'o'}};
+ EXPECT_CALL(Process, ReadMemory(_, _))
+ .WillRepeatedly(Invoke(&M, &FakeMemory::Read));
+
+ char string[1024];
+ size_t bytes_read;
+ EXPECT_THAT_EXPECTED(Process.ReadCStringFromMemory(
+ 0x0, &string[0], sizeof(string), bytes_read),
+ llvm::HasValue(llvm::StringRef("hello")));
+ EXPECT_EQ(bytes_read, 6UL);
+}
+
+TEST(NativeProcessProtocolTest, ReadCStringFromMemory_MaxSize) {
+ NiceMock DummyDelegate;
+ MockProcess Process(DummyDelegate,
+ ArchSpec("aarch64-pc-linux"));
+ FakeMemory M{{'h', 'e', 'l', 'l', 'o', 0, 'w', 'o'}};
+ EXPECT_CALL(Process, ReadMemory(_, _))
+ .WillRepeatedly(Invoke(&M, &FakeMemory::Read));
+
+ char string[4];
+ size_t bytes_read;
+ EXPECT_THAT_EXPECTED(Process.ReadCStringFromMemory(
+ 0x0, &string[0], sizeof(string), bytes_read),
+ llvm::HasValue(llvm::StringRef("hel")));
+ EXPECT_EQ(bytes_read, 3UL);
+}
Index: lldb/source/Plugins/Process/POSIX/NativeProcessELF.cpp
===
--- lldb/source/Plugins/Process/POSIX/NativeProcessELF.cpp
+++ lldb/source/Plugins/Process/POSIX/NativeProcessELF.cpp
@@ -118,14 +118,13 @@
return error.ToError();
char name_buffer[PATH_MAX];
- error = ReadMemory(link_map.l_name, &name_buffer, sizeof(name_buffer),
- bytes_read);
- if (!error.Success())
-return error.ToError();
- name_buffer[PATH_MAX - 1] = '\0';
+ llvm::Expected string_or_error = ReadCStringFromMemory(
+ link_map.l_name, &name_buffer, sizeof(name_buffer), bytes_read);
+ if (!string_or_error)
+return string_or_error.takeError();
SVR4LibraryInfo info;
- info.name = std::string(name_buffer);
+ info.name = string_or_error->str();
info.link_map = link_map_addr;
info.base_addr = link_map.l_addr;
info.ld_addr = link_map.l_ld;
Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -2076,4 +2076,4 @@
m_processor_trace_monitor.erase(iter);
return error;
-}
\ No newline at end of file
+}
Index: lldb/source/Host/common/NativeProcessProtocol.cpp
===
--- lldb/source/Host/common/NativeProcessProtocol.cpp
+++ lldb/source/Host/common/NativeProcessProtocol.cpp
@@ -16,10 +16,14 @@
#include "lldb/Utility/State.h"
#include "lldb/lldb-enumerations.h"
+#include "llvm/Support/Process.h"
+
using namespace lldb;
using namespace lldb_private;
// NativeProcessProtocol Members
+const size_t NativeProcessProtocol::g_cache_line_size =
+llvm::sys::Process::getPageSizeEstimate();
NativeProcessProtocol::NativeProcessProtocol(lldb::pid_t pid, int terminal_fd,
NativeDelegate &delegate)
@@ -659,6 +663,57 @@
return Status();
}
+llvm::Expected
+NativeProcessProtocol::ReadCStringFromMemory(lldb::addr_t addr, char *buffer,
+ size_t max_size,
+ size_t &total_bytes_read) {
+ size_t bytes_read = 0;
+ size_t bytes_left = max_size;
+ addr_t curr_addr = addr;
+ size_t string_size;
+ char *curr_buffer = buffer;
+ total_bytes_read = 0;
+ Status status;
+
+ while (bytes_left > 0 && status.Success()) {
+addr_t cache_line_bytes_left =
+g_ca
[Lldb-commits] [PATCH] D62503: Add ReadCStringFromMemory for faster string reads
aadsm updated this revision to Diff 204979.
aadsm added a comment.
- Address missing comments
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D62503/new/
https://reviews.llvm.org/D62503
Files:
lldb/include/lldb/Host/common/NativeProcessProtocol.h
lldb/source/Host/common/NativeProcessProtocol.cpp
lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
lldb/source/Plugins/Process/POSIX/NativeProcessELF.cpp
lldb/unittests/Host/NativeProcessProtocolTest.cpp
Index: lldb/unittests/Host/NativeProcessProtocolTest.cpp
===
--- lldb/unittests/Host/NativeProcessProtocolTest.cpp
+++ lldb/unittests/Host/NativeProcessProtocolTest.cpp
@@ -96,3 +96,35 @@
EXPECT_THAT_EXPECTED(Process.ReadMemoryWithoutTrap(4, 2),
llvm::HasValue(std::vector{4, 5}));
}
+
+TEST(NativeProcessProtocolTest, ReadCStringFromMemory) {
+ NiceMock DummyDelegate;
+ MockProcess Process(DummyDelegate,
+ ArchSpec("aarch64-pc-linux"));
+ FakeMemory M{{'h', 'e', 'l', 'l', 'o', 0, 'w', 'o'}};
+ EXPECT_CALL(Process, ReadMemory(_, _))
+ .WillRepeatedly(Invoke(&M, &FakeMemory::Read));
+
+ char string[1024];
+ size_t bytes_read;
+ EXPECT_THAT_EXPECTED(Process.ReadCStringFromMemory(
+ 0x0, &string[0], sizeof(string), bytes_read),
+ llvm::HasValue(llvm::StringRef("hello")));
+ EXPECT_EQ(bytes_read, 6UL);
+}
+
+TEST(NativeProcessProtocolTest, ReadCStringFromMemory_MaxSize) {
+ NiceMock DummyDelegate;
+ MockProcess Process(DummyDelegate,
+ ArchSpec("aarch64-pc-linux"));
+ FakeMemory M{{'h', 'e', 'l', 'l', 'o', 0, 'w', 'o'}};
+ EXPECT_CALL(Process, ReadMemory(_, _))
+ .WillRepeatedly(Invoke(&M, &FakeMemory::Read));
+
+ char string[4];
+ size_t bytes_read;
+ EXPECT_THAT_EXPECTED(Process.ReadCStringFromMemory(
+ 0x0, &string[0], sizeof(string), bytes_read),
+ llvm::HasValue(llvm::StringRef("hel")));
+ EXPECT_EQ(bytes_read, 3UL);
+}
Index: lldb/source/Plugins/Process/POSIX/NativeProcessELF.cpp
===
--- lldb/source/Plugins/Process/POSIX/NativeProcessELF.cpp
+++ lldb/source/Plugins/Process/POSIX/NativeProcessELF.cpp
@@ -118,14 +118,13 @@
return error.ToError();
char name_buffer[PATH_MAX];
- error = ReadMemory(link_map.l_name, &name_buffer, sizeof(name_buffer),
- bytes_read);
- if (!error.Success())
-return error.ToError();
- name_buffer[PATH_MAX - 1] = '\0';
+ llvm::Expected string_or_error = ReadCStringFromMemory(
+ link_map.l_name, &name_buffer, sizeof(name_buffer), bytes_read);
+ if (!string_or_error)
+return string_or_error.takeError();
SVR4LibraryInfo info;
- info.name = std::string(name_buffer);
+ info.name = string_or_error->str();
info.link_map = link_map_addr;
info.base_addr = link_map.l_addr;
info.ld_addr = link_map.l_ld;
Index: lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
===
--- lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
+++ lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp
@@ -2076,4 +2076,4 @@
m_processor_trace_monitor.erase(iter);
return error;
-}
\ No newline at end of file
+}
Index: lldb/source/Host/common/NativeProcessProtocol.cpp
===
--- lldb/source/Host/common/NativeProcessProtocol.cpp
+++ lldb/source/Host/common/NativeProcessProtocol.cpp
@@ -16,10 +16,14 @@
#include "lldb/Utility/State.h"
#include "lldb/lldb-enumerations.h"
+#include "llvm/Support/Process.h"
+
using namespace lldb;
using namespace lldb_private;
// NativeProcessProtocol Members
+const size_t NativeProcessProtocol::g_cache_line_size =
+llvm::sys::Process::getPageSizeEstimate();
NativeProcessProtocol::NativeProcessProtocol(lldb::pid_t pid, int terminal_fd,
NativeDelegate &delegate)
@@ -659,6 +663,56 @@
return Status();
}
+llvm::Expected
+NativeProcessProtocol::ReadCStringFromMemory(lldb::addr_t addr, char *buffer,
+ size_t max_size,
+ size_t &total_bytes_read) {
+ size_t bytes_read = 0;
+ size_t bytes_left = max_size;
+ addr_t curr_addr = addr;
+ size_t string_size;
+ char *curr_buffer = buffer;
+ total_bytes_read = 0;
+ Status status;
+
+ while (bytes_left > 0 && status.Success()) {
+addr_t cache_line_bytes_left =
+g_cache_line_size - (curr_addr % g_cache_line_size);
+addr_t bytes_to_read = std::min(bytes_left, cache_line_bytes_left);
+status = ReadMemory(curr_addr, reinterpret_cast(curr_buffer),
+bytes_to_read, bytes_read);
+
+if (b
[Lldb-commits] [PATCH] D63363: [Signals] Create a plugin directory just for signals
labath added a comment. Although this is technically correct and pretty consistent with our other "plugins", I can't help but feel that it's incredibly wasteful. Each of the XXXSignals.cpp files is less than a 100 lines long (with the licence header and all bolierplate) and it's unlikely to ever grow beyond that. And essentially, all these files do is define a single enum. The reason they are this long is because the UnixSignals class is already over-engineered (e.g. I don't see why LinuxSignals needs to be a separate class, or why it needs to repeat the descriptions/default stop values for each of the signals). Making this a plugin would just add another chunk of boilerplate on top of that. I don't know about others, but I'd rather us move in a direction which reduces the amount of boilerplate instead of adding more of it. In my ideal world, each of these signal definitions would just be a bunch of (number, name) pairs. This doesn't have/need to be a class or a plugin; a single constexpr variable would suffice for that. Then we'd just cross-reference this mapping with another one which gives the default stop values and descriptions for each of the signals, and that's it. I know I am repeating myself, but each time I say this, it's because I find another reason for it: I think we should start a new library which I would roughly define as "utilities for describing and manipulating various low-level aspects of processes, but which is agnostic of any actual process class". The idea would be that we can shove here all classes which are shared between lldb-server liblldb. UnixSignals would be one candidate for it. AuxVector, MemoryRegionInfo are others. `Plugins/Process/Utility` (where most of the signal classes live) would be a pretty good place for it already, were it not for the "Plugins" part (it would be weird for non-plugin code to depend on something called a "plugin"). However, maybe we could just create a new top-level library called "ProcessUtil" (or whatever name we come up with) and move the relevant stuff there. Anyway, TL;DR: I think this should be handled differently. However, if others are fine with this approach, then feel free to ignore me. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63363/new/ https://reviews.llvm.org/D63363 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
[Lldb-commits] [PATCH] D63380: [lldb] [test] Skip watchpoint tests on NetBSD if userdbregs is disabled
labath added a comment. This definitely aren't all of our watchpoint tests. What's the reason for picking this particular bunch? Are they the only ones enabled on NetBSD right now? Anyway, given that all of the watchpoint tests are already annotated with the "watchpoint" category, I think it would be better to handle this similar to how we handle other category-based skips (debug info flavours, c++ library types, etc.). This would also enable us to get rid of the "expectedFailure(windows)" decorator on all of these tests by implementing the skip centrally (though you don't have to do that right now). The category-based skipping happens around here: https://github.com/llvm-mirror/lldb/blob/master/packages/Python/lldbsuite/test/dotest.py#L1143 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D63380/new/ https://reviews.llvm.org/D63380 ___ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
