Revert "Add ReadCStringFromMemory for faster string reads"

This reverts commit a733539.

It seems this is breaking a bunch of tests (https://reviews.llvm.org/D62503#1549874) so reverting until I find the time to repro and fix.

llvm-svn: 364355

Author: aadsm

lldb/include/lldb/Host/common/NativeProcessProtocol.h

@@ -84,31 +84,6 @@ class NativeProcessProtocol {
   Status ReadMemoryWithoutTrap(lldb::addr_t addr, void *buf, size_t size,
                                size_t &bytes_read);
 
-  /// Reads a null terminated string from memory.
-  ///
-  /// Reads up to \p max_size bytes of memory until it finds a '\0'.
-  /// If a '\0' is not found then it reads max_size-1 bytes as a string and a
-  /// '\0' is added as the last character of the \p buffer.
-  ///
-  /// \param[in] addr
-  ///     The address in memory to read from.
-  ///
-  /// \param[in] buffer
-  ///     An allocated buffer with at least \p max_size size.
-  ///
-  /// \param[in] max_size
-  ///     The maximum number of bytes to read from memory until it reads the
-  ///     string.
-  ///
-  /// \param[out] total_bytes_read
-  ///     The number of bytes read from memory into \p buffer.
-  ///
-  /// \return
-  ///     Returns a StringRef backed up by the \p buffer passed in.
-  llvm::Expected<llvm::StringRef>
-  ReadCStringFromMemory(lldb::addr_t addr, char *buffer, size_t max_size,
-                        size_t &total_bytes_read);
-
   virtual Status WriteMemory(lldb::addr_t addr, const void *buf, size_t size,
                              size_t &bytes_written) = 0;
 

lldb/source/Host/common/NativeProcessProtocol.cpp

@@ -16,8 +16,6 @@
 #include "lldb/Utility/State.h"
 #include "lldb/lldb-enumerations.h"
 
-#include "llvm/Support/Process.h"
-
 using namespace lldb;
 using namespace lldb_private;
 
@@ -661,58 +659,6 @@ Status NativeProcessProtocol::ReadMemoryWithoutTrap(lldb::addr_t addr,
   return Status();
 }
 
-llvm::Expected<llvm::StringRef>
-NativeProcessProtocol::ReadCStringFromMemory(lldb::addr_t addr, char *buffer,
-                                             size_t max_size,
-                                             size_t &total_bytes_read) {
-  static const size_t cache_line_size =
-      llvm::sys::Process::getPageSizeEstimate();
-  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 =
-        cache_line_size - (curr_addr % cache_line_size);
-    addr_t bytes_to_read = std::min<addr_t>(bytes_left, cache_line_bytes_left);
-    status = ReadMemory(curr_addr, reinterpret_cast<void *>(curr_buffer),
-                        bytes_to_read, bytes_read);
-
-    if (bytes_read == 0)
-      break;
-
-    void *str_end = std::memchr(curr_buffer, '\0', bytes_read);
-    if (str_end != nullptr) {
-      total_bytes_read =
-          (size_t)(reinterpret_cast<char *>(str_end) - buffer + 1);
-      status.Clear();
-      break;
-    }
-
-    total_bytes_read += bytes_read;
-    curr_buffer += bytes_read;
-    curr_addr += bytes_read;
-    bytes_left -= bytes_read;
-  }
-
-  string_size = total_bytes_read - 1;
-
-  // Make sure we return a null terminated string.
-  if (bytes_left == 0 && max_size > 0 && buffer[max_size - 1] != '\0') {
-    buffer[max_size - 1] = '\0';
-    total_bytes_read--;
-  }
-
-  if (!status.Success())
-    return status.ToError();
-
-  return llvm::StringRef(buffer, string_size);
-}
-
 lldb::StateType NativeProcessProtocol::GetState() const {
   std::lock_guard<std::recursive_mutex> guard(m_state_mutex);
   return m_state;

lldb/source/Plugins/Process/Linux/NativeProcessLinux.cpp

@@ -2076,4 +2076,4 @@ Status NativeProcessLinux::StopProcessorTracingOnThread(lldb::user_id_t traceid,
   m_processor_trace_monitor.erase(iter);
 
   return error;
-}
+}

lldb/source/Plugins/Process/POSIX/NativeProcessELF.cpp

@@ -118,13 +118,14 @@ NativeProcessELF::ReadSVR4LibraryInfo(lldb::addr_t link_map_addr) {
     return error.ToError();
 
   char name_buffer[PATH_MAX];
-  llvm::Expected<llvm::StringRef> string_or_error = ReadCStringFromMemory(
-      link_map.l_name, &name_buffer[0], sizeof(name_buffer), bytes_read);
-  if (!string_or_error)
-    return string_or_error.takeError();
+  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';
 
   SVR4LibraryInfo info;
-  info.name = string_or_error->str();
+  info.name = std::string(name_buffer);
   info.link_map = link_map_addr;
   info.base_addr = link_map.l_addr;
   info.ld_addr = link_map.l_ld;

lldb/unittests/Host/NativeProcessProtocolTest.cpp

@@ -9,7 +9,6 @@
 #include "TestingSupport/Host/NativeProcessTestUtils.h"
 
 #include "lldb/Host/common/NativeProcessProtocol.h"
-#include "llvm/Support/Process.h"
 #include "llvm/Testing/Support/Error.h"
 #include "gmock/gmock.h"
 
@@ -97,53 +96,3 @@ TEST(NativeProcessProtocolTest, ReadMemoryWithoutTrap) {
   EXPECT_THAT_EXPECTED(Process.ReadMemoryWithoutTrap(4, 2),
                        llvm::HasValue(std::vector<uint8_t>{4, 5}));
 }
-
-TEST(NativeProcessProtocolTest, ReadCStringFromMemory) {
-  NiceMock<MockDelegate> DummyDelegate;
-  MockProcess<NativeProcessProtocol> 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<MockDelegate> DummyDelegate;
-  MockProcess<NativeProcessProtocol> 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);
-}
-
-TEST(NativeProcessProtocolTest, ReadCStringFromMemory_CrossPageBoundary) {
-  NiceMock<MockDelegate> DummyDelegate;
-  MockProcess<NativeProcessProtocol> Process(DummyDelegate,
-                                             ArchSpec("aarch64-pc-linux"));
-  unsigned string_start = llvm::sys::Process::getPageSizeEstimate() - 3;
-  FakeMemory M({'h', 'e', 'l', 'l', 'o', 0, 'w', 'o'}, string_start);
-  EXPECT_CALL(Process, ReadMemory(_, _))
-      .WillRepeatedly(Invoke(&M, &FakeMemory::Read));
-
-  char string[1024];
-  size_t bytes_read;
-  EXPECT_THAT_EXPECTED(Process.ReadCStringFromMemory(string_start, &string[0],
-                                                     sizeof(string),
-                                                     bytes_read),
-                       llvm::HasValue(llvm::StringRef("hello")));
-  EXPECT_EQ(bytes_read, 6UL);
-}