libdrgn: improve C string reading efficiency

The current C string reading implementation is inefficient, especially for
low bandwidth remote targets, as it needs to do a separate segment read
(including a fresh page table lookup) for each character read. A more
efficient approach is to retain the page table between character reads,
only discarding it when we hit the null terminator.

Implement this approach by allowing segments to also specify a C string
reading callback. The callback for page tables will preserve the page
table iterator while reading characters.

Signed-off-by: Peter Collingbourne <pcc@google.com>

libdrgn/drgn.h.in

@@ -530,6 +530,30 @@ typedef struct drgn_error *(*drgn_memory_read_fn)(void *buf, uint64_t address,
 						  size_t count, uint64_t offset,
 						  void *arg, bool physical);
 
+struct string_builder;
+
+/**
+ * Callback implementing a memory read of a C string.
+ *
+ * @param[out] str String builder to read into.
+ * @param[out] done Whether we were able to read the whole string.
+ * @param[in] address Address which we are reading from.
+ * @param[in] limit Maximum number of bytes to read.
+ * @param[in] offset Offset in bytes of @p address from the beginning of the
+ * segment.
+ * @param[in] arg Argument passed to @ref drgn_program_add_memory_segment().
+ * @param[in] physical Whether @c address is physical.
+ * @return @c NULL on success, non-@c NULL on error.
+ */
+typedef struct drgn_error *(*drgn_memory_read_cstr_fn)(
+	struct string_builder *str, bool *done, uint64_t address, size_t limit,
+	uint64_t offset, void *arg, bool physical);
+
+struct drgn_memory_ops {
+	drgn_memory_read_fn read_fn;
+	drgn_memory_read_cstr_fn read_cstr_fn;
+};
+
 /**
  * Register a segment of memory in a @ref drgn_program.
  *
@@ -539,14 +563,14 @@ typedef struct drgn_error *(*drgn_memory_read_fn)(void *buf, uint64_t address,
  *
  * @param[in] address Address of the segment.
  * @param[in] size Size of the segment in bytes.
- * @param[in] read_fn Callback to read from segment.
+ * @param[in] ops Memory operations for this segment.
  * @param[in] arg Argument to pass to @p read_fn.
  * @param[in] physical Whether to add a physical memory segment.
  * @return @c NULL on success, non-@c NULL on error.
  */
 struct drgn_error *
 drgn_program_add_memory_segment(struct drgn_program *prog, uint64_t address,
-				uint64_t size, drgn_memory_read_fn read_fn,
+				uint64_t size, const struct drgn_memory_ops *ops,
 				void *arg, bool physical);
 
 /**
@@ -760,13 +784,14 @@ struct drgn_error *drgn_program_read_memory(struct drgn_program *prog,
  * drgn_program_read_memory().
  * @param[in] max_size Stop after this many bytes are read, not including the
  * null byte. A null byte is appended to @p ret in this case.
- * @param[out] ret Returned string. On success, it must be freed with @c free().
+ * @param[out] str String builder for the returned string. Owned by the caller.
  * On error, its contents are undefined.
  * @return @c NULL on success, non-@c NULL on error.
  */
 struct drgn_error *drgn_program_read_c_string(struct drgn_program *prog,
 					      uint64_t address, bool physical,
-					      size_t max_size, char **ret);
+					      size_t max_size,
+					      struct string_builder *str);
 
 struct drgn_error *drgn_program_read_u8(struct drgn_program *prog,
 					uint64_t address, bool physical,

libdrgn/helpers.h

@@ -28,6 +28,11 @@ struct drgn_error *linux_helper_read_vm(struct drgn_program *prog,
 					uint64_t pgtable, uint64_t virt_addr,
 					void *buf, size_t count);
 
+struct drgn_error *linux_helper_read_cstr(struct drgn_program *prog,
+					  uint64_t pgtable, uint64_t virt_addr,
+					  struct string_builder *str,
+					  bool *done, size_t count);
+
 struct drgn_error *linux_helper_follow_phys(struct drgn_program *prog,
 					    uint64_t pgtable,
 					    uint64_t virt_addr, uint64_t *ret);

libdrgn/language_c.c

@@ -644,24 +644,26 @@ c_format_string(struct drgn_program *prog, uint64_t address, uint64_t length,
 		struct string_builder *sb)
 {
 	struct drgn_error *err;
+	struct string_builder tmp = STRING_BUILDER_INIT;
 
-	if (!string_builder_appendc(sb, '"'))
+	err = drgn_program_read_c_string(prog, address, false, length, &tmp);
+	if (err) {
+		free(tmp.str);
+		return err;
+	}
+
+	if (!string_builder_appendc(sb, '"')) {
+		free(tmp.str);
 		return &drgn_enomem;
-	while (length) {
-		unsigned char c;
-		err = drgn_program_read_memory(prog, &c, address++, 1, false);
-		if (err)
+	}
+	for (size_t i = 0; i != tmp.len; ++i) {
+		err = c_format_character(tmp.str[i], false, true, sb);
+		if (err) {
+			free(tmp.str);
 			return err;
-
-		if (c == '\0') {
-			break;
-		} else {
-			err = c_format_character(c, false, true, sb);
-			if (err)
-				return err;
 		}
-		length--;
 	}
+	free(tmp.str);
 	if (!string_builder_appendc(sb, '"'))
 		return &drgn_enomem;
 	return NULL;

libdrgn/linux_kernel.c

@@ -32,15 +32,30 @@
 
 #include "drgn_program_parse_vmcoreinfo.inc"
 
-struct drgn_error *read_memory_via_pgtable(void *buf, uint64_t address,
-					   size_t count, uint64_t offset,
-					   void *arg, bool physical)
+static struct drgn_error *read_memory_via_pgtable(void *buf, uint64_t address,
+						  size_t count, uint64_t offset,
+						  void *arg, bool physical)
 {
 	struct drgn_program *prog = arg;
 	return linux_helper_read_vm(prog, prog->vmcoreinfo.swapper_pg_dir,
 				    address, buf, count);
 }
 
+struct drgn_error *read_cstr_via_pgtable(struct string_builder *str, bool *done,
+					 uint64_t address, size_t limit,
+					 uint64_t offset, void *arg,
+					 bool physical)
+{
+	struct drgn_program *prog = arg;
+	return linux_helper_read_cstr(prog, prog->vmcoreinfo.swapper_pg_dir,
+				      address, str, done, limit);
+}
+
+const struct drgn_memory_ops segment_pgtable_ops = {
+	.read_fn = read_memory_via_pgtable,
+	.read_cstr_fn = read_cstr_via_pgtable,
+};
+
 struct drgn_error *proc_kallsyms_symbol_addr(const char *name,
 					     unsigned long *ret)
 {

libdrgn/linux_kernel.h

@@ -8,9 +8,7 @@
 
 struct drgn_debug_info_load_state;
 
-struct drgn_error *read_memory_via_pgtable(void *buf, uint64_t address,
-					   size_t count, uint64_t offset,
-					   void *arg, bool physical);
+extern const struct drgn_memory_ops segment_pgtable_ops;
 
 struct drgn_error *drgn_program_parse_vmcoreinfo(struct drgn_program *prog,
 						 const char *desc,

libdrgn/linux_kernel_helpers.c

@@ -190,6 +190,57 @@ struct drgn_error *linux_helper_read_vm(struct drgn_program *prog,
 	return err;
 }
 
+struct drgn_error *linux_helper_read_cstr(struct drgn_program *prog,
+					  uint64_t pgtable, uint64_t virt_addr,
+					  struct string_builder *str,
+					  bool *done, size_t count)
+{
+	struct drgn_error *err;
+
+	err = begin_virtual_address_translation(prog, pgtable, virt_addr);
+	if (err)
+		return err;
+	if (!count) {
+		*done = false;
+		err = NULL;
+		goto out;
+	}
+
+	struct pgtable_iterator *it = prog->pgtable_it;
+	pgtable_iterator_next_fn *next =
+		prog->platform.arch->linux_kernel_pgtable_iterator_next;
+	uint64_t read_addr = 0;
+	size_t read_size = 0;
+	virt_addr = it->virt_addr;
+	do {
+		uint64_t start_virt_addr, start_phys_addr;
+		err = next(prog, it, &start_virt_addr, &start_phys_addr);
+		if (err)
+			break;
+		if (start_phys_addr == UINT64_MAX) {
+			err = drgn_error_create_fault("address is not mapped",
+						      virt_addr);
+			break;
+		}
+
+		uint64_t phys_addr =
+			start_phys_addr + (virt_addr - start_virt_addr);
+		size_t n = min(it->virt_addr - virt_addr, (uint64_t)count);
+		err = drgn_memory_reader_read_cstr(&prog->reader, str, done,
+						   phys_addr, n, true);
+		if (err)
+			break;
+		if (*done)
+			goto out;
+		virt_addr = it->virt_addr;
+		count -= n;
+	} while (count);
+
+out:
+	end_virtual_address_translation(prog);
+	return err;
+}
+
 struct drgn_error *linux_helper_follow_phys(struct drgn_program *prog,
 					    uint64_t pgtable,
 					    uint64_t virt_addr, uint64_t *ret)

libdrgn/memory_reader.c

@@ -9,6 +9,7 @@
 
 #include "memory_reader.h"
 #include "minmax.h"
+#include "string_builder.h"
 
 /** Memory segment in a @ref drgn_memory_reader. */
 struct drgn_memory_segment {
@@ -22,8 +23,7 @@ struct drgn_memory_segment {
 	 * drgn_memory_segment::min_address.
 	 */
 	uint64_t orig_min_address;
-	/** Read callback. */
-	drgn_memory_read_fn read_fn;
+	const struct drgn_memory_ops *ops;
 	/** Argument to pass to @ref drgn_memory_segment::read_fn. */
 	void *arg;
 };
@@ -72,7 +72,7 @@ bool drgn_memory_reader_empty(struct drgn_memory_reader *reader)
 struct drgn_error *
 drgn_memory_reader_add_segment(struct drgn_memory_reader *reader,
 			       uint64_t min_address, uint64_t max_address,
-			       drgn_memory_read_fn read_fn, void *arg,
+			       const struct drgn_memory_ops *ops, void *arg,
 			       bool physical)
 {
 	assert(min_address <= max_address);
@@ -128,7 +128,7 @@ drgn_memory_reader_add_segment(struct drgn_memory_reader *reader,
 			tail->min_address = max_address + 1;
 			tail->max_address = it.entry->max_address;
 			tail->orig_min_address = it.entry->orig_min_address;
-			tail->read_fn = it.entry->read_fn;
+			tail->ops = it.entry->ops;
 			tail->arg = it.entry->arg;
 
 			drgn_memory_segment_tree_insert(tree, tail, NULL);
@@ -227,7 +227,7 @@ drgn_memory_reader_add_segment(struct drgn_memory_reader *reader,
 		truncate_tail->max_address = min_address - 1;
 	segment->min_address = segment->orig_min_address = min_address;
 	segment->max_address = max_address;
-	segment->read_fn = read_fn;
+	segment->ops = ops;
 	segment->arg = arg;
 	/* If the segment is stolen, then it's already in the tree. */
 	if (!stolen)
@@ -257,9 +257,9 @@ struct drgn_error *drgn_memory_reader_read(struct drgn_memory_reader *reader,
 
 		size_t n = min((uint64_t)(count - 1),
 			       segment->max_address - address) + 1;
-		err = segment->read_fn(p, address, n,
-				       address - segment->orig_min_address,
-				       segment->arg, physical);
+		err = segment->ops->read_fn(p, address, n,
+					    address - segment->orig_min_address,
+					    segment->arg, physical);
 		if (err)
 			return err;
 		p += n;
@@ -269,9 +269,65 @@ struct drgn_error *drgn_memory_reader_read(struct drgn_memory_reader *reader,
 	return NULL;
 }
 
-struct drgn_error *drgn_read_memory_file(void *buf, uint64_t address,
-					 size_t count, uint64_t offset,
-					 void *arg, bool physical)
+struct drgn_error *
+drgn_memory_reader_read_cstr(struct drgn_memory_reader *reader,
+			     struct string_builder *buf, bool *done,
+			     uint64_t address, size_t count, bool physical)
+{
+	assert(count == 0 || count - 1 <= UINT64_MAX - address);
+
+	struct drgn_error *err;
+	struct drgn_memory_segment_tree *tree = (physical ?
+						 &reader->physical_segments :
+						 &reader->virtual_segments);
+	while (count > 0) {
+		struct drgn_memory_segment *segment =
+			drgn_memory_segment_tree_search_le(tree,
+							   &address).entry;
+		if (!segment || segment->max_address < address) {
+			return drgn_error_create_fault("could not find memory segment",
+						       address);
+		}
+
+		if (segment->ops->read_cstr_fn) {
+			size_t n = min((uint64_t)(count - 1),
+				       segment->max_address - address) + 1;
+			err = segment->ops->read_cstr_fn(
+				buf, done, address, n,
+				address - segment->orig_min_address,
+				segment->arg, physical);
+			if (err)
+				return err;
+			if (*done)
+				return NULL;
+			address += n;
+			count -= n;
+		} else {
+			while (count > 0 && address <= segment->max_address) {
+				char c;
+				err = segment->ops->read_fn(
+					&c, address, 1,
+					address - segment->orig_min_address,
+					segment->arg, physical);
+				if (err)
+					return err;
+				if (!c) {
+  					*done = true;
+					return NULL;
+				}
+				string_builder_appendc(buf, c);
+				address++;
+				count--;
+			}
+		}
+	}
+	*done = false;
+	return NULL;
+}
+
+static struct drgn_error *drgn_read_memory_file(void *buf, uint64_t address,
+						size_t count, uint64_t offset,
+						void *arg, bool physical)
 {
 	struct drgn_memory_file_segment *file_segment = arg;
 	size_t file_count;
@@ -312,3 +368,7 @@ struct drgn_error *drgn_read_memory_file(void *buf, uint64_t address,
 	memset(p, '\0', zero_count);
 	return NULL;
 }
+
+const struct drgn_memory_ops segment_file_ops = {
+	.read_fn = drgn_read_memory_file,
+};

libdrgn/memory_reader.h

@@ -74,7 +74,7 @@ bool drgn_memory_reader_empty(struct drgn_memory_reader *reader);
 struct drgn_error *
 drgn_memory_reader_add_segment(struct drgn_memory_reader *reader,
 			       uint64_t min_address, uint64_t max_address,
-			       drgn_memory_read_fn read_fn, void *arg,
+			       const struct drgn_memory_ops *ops, void *arg,
 			       bool physical);
 
 /**
@@ -92,6 +92,11 @@ struct drgn_error *drgn_memory_reader_read(struct drgn_memory_reader *reader,
 					   void *buf, uint64_t address,
 					   size_t count, bool physical);
 
+struct drgn_error *
+drgn_memory_reader_read_cstr(struct drgn_memory_reader *reader,
+			     struct string_builder *buf, bool *done,
+			     uint64_t address, size_t count, bool physical);
+
 /** Argument for @ref drgn_read_memory_file(). */
 struct drgn_memory_file_segment {
 	/** Offset in the file where the segment starts. */
@@ -116,10 +121,7 @@ struct drgn_memory_file_segment {
 	bool zerofill;
 };
 
-/** @ref drgn_memory_read_fn which reads from a file. */
-struct drgn_error *drgn_read_memory_file(void *buf, uint64_t address,
-					 size_t count, uint64_t offset,
-					 void *arg, bool physical);
+extern const struct drgn_memory_ops segment_file_ops;
 
 /** @} */