Remove bit_offset from value objects

There are a couple of reasons that it was the wrong choice to have a
bit_offset for value objects:

1. When we store a buffer with a bit_offset, we're storing useless
   padding bits.
2. bit_offset describes a location, or in other words, part of an
   address. This makes sense for references, but not for values, which
   are just a bag of bytes.

Get rid of union drgn_value.bit_offset in libdrgn, make
Object.bit_offset None for value objects, and disallow passing
bit_offset to the Object() constructor when creating a value. bit_offset
can still be passed when creating an object from a buffer, but we'll
shift the bytes down as necessary to store the value with no offset.

Signed-off-by: Omar Sandoval <osandov@osandov.com>

_drgn.pyi

@@ -962,7 +962,6 @@ class Object:
         value: Union[IntegerLike, float, bool, Mapping[str, Any], Sequence[Any]],
         *,
         byteorder: Optional[str] = None,
-        bit_offset: Optional[IntegerLike] = None,
         bit_field_size: Optional[IntegerLike] = None,
     ) -> None:
         """
@@ -974,9 +973,6 @@ class Object:
         :param byteorder: Byte order of the object. This should be ``'little'``
             or ``'big'``. The default is ``None``, which indicates the program
             byte order. This must be ``None`` for primitive values.
-        :param bit_offset: Offset in bits from the object's address to the
-            beginning of the object. The default is ``None``, which means no
-            offset. This must be ``None`` for primitive values.
         :param bit_field_size: Size in bits of the object if it is a bit field.
             The default is ``None``, which means the object is not a bit field.
         """
@@ -1001,13 +997,15 @@ class Object:
         *,
         address: IntegerLike,
         byteorder: Optional[str] = None,
-        bit_offset: Optional[IntegerLike] = None,
+        bit_offset: IntegerLike = 0,
         bit_field_size: Optional[IntegerLike] = None,
     ) -> None:
         """
         Create a reference object.
 
         :param address: Address of the object in the program.
+        :param bit_offset: Offset in bits from *address* to the beginning of
+            the object.
         """
         ...
     @overload
@@ -1041,7 +1039,7 @@ class Object:
     bit_offset_: Optional[int]
     """
     Offset in bits from this object's address to the beginning of the object if
-    it is a reference or a non-primitive value, ``None`` otherwise.
+    it is a reference, ``None`` otherwise.
     """
 
     bit_field_size_: Optional[int]

libdrgn/arch_x86_64.c.in

@@ -164,7 +164,7 @@ pt_regs_set_initial_registers_x86_64(Dwfl_Thread *thread,
 		      (__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__));
 	return set_initial_registers_from_struct_x86_64(thread,
 							drgn_object_buffer(obj),
-							drgn_buffer_object_size(obj),
+							drgn_object_size(obj),
 							bswap);
 }
 

libdrgn/debug_info.c

@@ -2387,13 +2387,14 @@ drgn_object_from_dwarf_constant(struct drgn_debug_info *dbinfo, Dwarf_Die *die,
 		err = dwarf_die_is_little_endian(die, true, &little_endian);
 		if (err)
 			return err;
-		if (block.length < drgn_value_size(bit_size, 0)) {
+		if (block.length < drgn_value_size(bit_size)) {
 			return drgn_error_create(DRGN_ERROR_OTHER,
 						 "DW_AT_const_value block is too small");
 		}
-		return drgn_object_set_buffer_internal(ret, &type, encoding,
+		return drgn_object_set_from_buffer_internal(ret, &type,
-						       bit_size, block.data, 0,
+							    encoding, bit_size,
-						       little_endian);
+							    block.data, 0,
+							    little_endian);
 	} else if (encoding == DRGN_OBJECT_ENCODING_SIGNED) {
 		Dwarf_Sword svalue;
 		if (dwarf_formsdata(attr, &svalue)) {

libdrgn/drgn.h.in

@@ -1590,12 +1590,6 @@ union drgn_value {
 			 */
 			char ibuf[8];
 		};
-		/**
-		 * Offset of the value from the beginning of the buffer.
-		 *
-		 * This is always less than 8, but usually 0.
-		 */
-		uint8_t bit_offset;
 		/** Whether the values within the buffer are little-endian. */
 		bool little_endian;
 	};
@@ -1608,36 +1602,24 @@ union drgn_value {
 };
 
 /**
- * Return the number of bytes needed to store a given number of bits starting at
+ * Return the number of bytes needed to store the given number of bits.
- * a given offset.
  *
- * This assumes that <tt>bit_size + bit_offset</tt> does not overflow a 64-bit
+ * @param[in] bits Number of bits.
- * integer, which is guaranteed to be true for object values.
- *
- * @param[in] bit_size Size in bits of the value.
- * @param[in] bit_offset Offset of the value from the beginning of the buffer.
  */
-static inline uint64_t drgn_value_size(uint64_t bit_size, uint64_t bit_offset)
+static inline uint64_t drgn_value_size(uint64_t bits)
 {
-	uint64_t bits = bit_size + bit_offset;
+	return bits / CHAR_BIT + (bits % CHAR_BIT ? 1 : 0);
-
-	return bits / 8 + (bits % 8 ? 1 : 0);
 }
 
 /**
- * Return whether a buffer value uses the inline buffer (@ref drgn_value::ibuf).
+ * Return whether the given number of bits can be stored in the inline buffer of
+ * a @ref drgn_value (@ref drgn_value::ibuf).
  *
- * This assumes that <tt>bit_size + bit_offset</tt> does not overflow a 64-bit
+ * @param[in] bits Number of bits.
- * integer, which is guaranteed to be true for object values.
- *
- * @param[in] bit_size Size in bits of the value.
- * @param[in] bit_offset Offset of the value from the beginning of the buffer.
  */
-static inline bool drgn_value_is_inline(uint64_t bit_size, uint64_t bit_offset)
+static inline bool drgn_value_is_inline(uint64_t bits)
 {
-	uint64_t bits = bit_size + bit_offset;
+	return bits <= CHAR_BIT * sizeof(((union drgn_value *)0)->ibuf);
-
-	return bits <= 8 * sizeof(((union drgn_value *)0)->ibuf);
 }
 
 /**
@@ -1694,33 +1676,25 @@ struct drgn_object {
 	};
 };
 
-/** Return the size of a buffer object in bytes. */
+/** Return the number of bytes needed to store an object's value. */
-static inline uint64_t drgn_buffer_object_size(const struct drgn_object *obj)
+static inline uint64_t drgn_object_size(const struct drgn_object *obj)
 {
-	return drgn_value_size(obj->bit_size, obj->value.bit_offset);
+	return drgn_value_size(obj->bit_size);
-}
-
-/** Return the referenced size of a reference object in bytes. */
-static inline uint64_t drgn_reference_object_size(const struct drgn_object *obj)
-{
-	return drgn_value_size(obj->bit_size, obj->reference.bit_offset);
 }
 
 /**
- * Return whether an object's value uses the inline buffer (@ref
+ * Return whether an object's value can be stored in the inline buffer of a @ref
- * drgn_value::ibuf).
+ * drgn_value (@ref drgn_value::ibuf).
  */
-static inline bool drgn_buffer_object_is_inline(const struct drgn_object *obj)
+static inline bool drgn_object_is_inline(const struct drgn_object *obj)
 {
-	return drgn_value_is_inline(obj->bit_size, obj->value.bit_offset);
+	return drgn_value_is_inline(obj->bit_size);
 }
 
 /** Return an object's buffer. */
-#define drgn_object_buffer(obj) ({					\
+#define drgn_object_buffer(obj) ({						\
-	__auto_type _obj = (obj);					\
+	__auto_type _obj = (obj);						\
-									\
+	drgn_object_is_inline(_obj) ? _obj->value.ibuf : _obj->value.bufp;	\
-	(drgn_buffer_object_is_inline(_obj) ? _obj->value.ibuf :	\
-	 _obj->value.bufp);						\
 })
 
 /** Get the type of a @ref drgn_object. */
@@ -1822,27 +1796,28 @@ drgn_object_set_float(struct drgn_object *res,
 		      struct drgn_qualified_type qualified_type, double fvalue);
 
 /**
- * Set a @ref drgn_object to a buffer value.
+ * Set a @ref drgn_object from a buffer.
  *
  * @param[out] res Object to set.
  * @param[in] qualified_type Type to set to.
- * @param[in] buf Buffer to set to. It must be at least
+ * @param[in] buf Buffer to set to. It is copied, so it need not remain valid
- * <tt>bit_size + bit_offset</tt> bits large, where @c bit_size is @p
+ * after this function returns.
- * bit_field_size if non-zero and the size of @p qualified_type otherwise. It is
+ * @param[in] buf_size Size of @p buf, in bytes. `buf_size * 8` must be at least
- * copied, so it need not remain valid after this function returns.
+ * `bit_size + bit_offset`, where @c bit_size is @p bit_field_size if non-zero
- * @param[in] bit_offset Offset of the value from the beginning of the buffer.
+ * and the size of @p qualified_type in bits otherwise.
- * This must be less than 8 (and is usually 0).
+ * @param[in] bit_offset Offset of the value from the beginning of the buffer,
+ * in bits. This is usually 0.
  * @param[in] bit_field_size If the object should be a bit field, its size in
  * bits. Otherwise, 0.
  * @param[in] byte_order Byte order of the result.
  * @return @c NULL on success, non-@c NULL on error.
  */
 struct drgn_error *
-drgn_object_set_buffer(struct drgn_object *res,
+drgn_object_set_from_buffer(struct drgn_object *res,
-		       struct drgn_qualified_type qualified_type,
+			    struct drgn_qualified_type qualified_type,
-		       const void *buf, uint8_t bit_offset,
+			    const void *buf, size_t buf_size,
-		       uint64_t bit_field_size,
+			    uint64_t bit_offset, uint64_t bit_field_size,
-		       enum drgn_byte_order byte_order);
+			    enum drgn_byte_order byte_order);
 
 /**
  * Set a @ref drgn_object to a reference.
@@ -1850,8 +1825,8 @@ drgn_object_set_buffer(struct drgn_object *res,
  * @param[out] res Object to set.
  * @param[in] qualified_type Type to set to.
  * @param[in] address Address of the object.
- * @param[in] bit_offset Offset of the value from @p address. This may be
+ * @param[in] bit_offset Offset of the value from @p address, in bits. This is
- * greater than or equal to 8.
+ * usually 0.
  * @param[in] bit_field_size If the object should be a bit field, its size in
  * bits. Otherwise, 0.
  * @param[in] byte_order Byte order of the result.

libdrgn/language_c.c

@@ -1462,7 +1462,7 @@ c_format_array_object(const struct drgn_object *obj,
 			if (!string_builder_appendc(sb, '"'))
 				return &drgn_enomem;
 			buf = (const unsigned char *)drgn_object_buffer(obj);
-			size = drgn_buffer_object_size(obj);
+			size = drgn_object_size(obj);
 			for (i = 0; i < size; i++) {
 				if (buf[i] == '\0')
 					break;

libdrgn/linux_kernel.c

@@ -314,10 +314,10 @@ struct drgn_error *linux_kernel_object_find(const char *name, size_t name_len,
 			if (err)
 				return err;
 			qualified_type.qualifiers = 0;
-			return drgn_object_set_buffer(ret, qualified_type,
+			return drgn_object_set_from_buffer(ret, qualified_type,
-						      prog->vmcoreinfo.osrelease,
+							   prog->vmcoreinfo.osrelease,
-						      0, 0,
+							   len + 1, 0, 0,
-						      DRGN_PROGRAM_ENDIAN);
+							   DRGN_PROGRAM_ENDIAN);
 		} else if (name_len == strlen("vmemmap") &&
 			   memcmp(name, "vmemmap", name_len) == 0) {
 			if (prog->vmemmap.kind == DRGN_OBJECT_UNAVAILABLE) {

libdrgn/object.c

@@ -32,7 +32,7 @@ static void drgn_value_deinit(const struct drgn_object *obj,
 			      const union drgn_value *value)
 {
 	if (obj->encoding == DRGN_OBJECT_ENCODING_BUFFER &&
-	    !drgn_buffer_object_is_inline(obj))
+	    !drgn_object_is_inline(obj))
 		free(value->bufp);
 }
 
@@ -293,12 +293,52 @@ drgn_byte_order_to_little_endian(struct drgn_program *prog,
 	)
 }
 
+static void copy_bits(void *dst, const void *src, unsigned int bit_offset,
+		      uint64_t bit_size, bool little_endian)
+{
+	unsigned char *d = dst;
+	const unsigned char *s = src;
+	size_t bytes = bit_size / CHAR_BIT;
+	unsigned int bits = bit_size % CHAR_BIT;
+	if (little_endian) {
+		if (bit_offset) {
+			for (size_t i = 0; i < bytes; i++) {
+				d[i] = ((s[i] >> bit_offset) |
+					(s[i + 1] << (CHAR_BIT - bit_offset)));
+			}
+		} else if (d != s) {
+			memcpy(d, s, bytes);
+		}
+		if (bits) {
+			unsigned char c = s[bytes] >> bit_offset;
+			if (bits > CHAR_BIT - bit_offset)
+				c |= s[bytes + 1] << (CHAR_BIT - bit_offset);
+			d[bytes] = c & ((1U << bits) - 1);
+		}
+	} else {
+		if (bit_offset) {
+			for (size_t i = 0; i < bytes; i++) {
+				d[i] = ((s[i] << bit_offset) |
+					(s[i + 1] >> (CHAR_BIT - bit_offset)));
+			}
+		} else if (d != s) {
+			memcpy(d, s, bytes);
+		}
+		if (bits) {
+			unsigned char c = s[bytes] << bit_offset;
+			if (bits > CHAR_BIT - bit_offset)
+				c |= s[bytes + 1] >> (CHAR_BIT - bit_offset);
+			d[bytes] = c & (-1U << (CHAR_BIT - bits));
+		}
+	}
+}
+
 struct drgn_error *
-drgn_object_set_buffer_internal(struct drgn_object *res,
+drgn_object_set_from_buffer_internal(struct drgn_object *res,
-				const struct drgn_object_type *type,
+				     const struct drgn_object_type *type,
-				enum drgn_object_encoding encoding,
+				     enum drgn_object_encoding encoding,
-				uint64_t bit_size, const void *buf,
+				     uint64_t bit_size, const void *buf,
-				uint8_t bit_offset, bool little_endian)
+				     uint64_t bit_offset, bool little_endian)
 {
 	struct drgn_error *err;
 
@@ -310,66 +350,73 @@ drgn_object_set_buffer_internal(struct drgn_object *res,
 	err = sanity_check_object(encoding, type->bit_field_size, bit_size);
 	if (err)
 		return err;
-	if (bit_offset >= 8) {
-		return drgn_error_format(DRGN_ERROR_INVALID_ARGUMENT,
-					 "bit offset must be less than 8");
-	}
-	if (bit_size > UINT64_MAX - bit_offset) {
-		return drgn_error_format(DRGN_ERROR_OVERFLOW,
-					 "object is too large");
-	}
 
+	const char *p = (const char *)buf + (bit_offset / CHAR_BIT);
+	bit_offset %= CHAR_BIT;
 	if (encoding == DRGN_OBJECT_ENCODING_BUFFER) {
-		uint64_t size;
+		/*
-		char *dst;
+		 * `buf` may point inside of `drgn_object_buffer(res)`, so copy
+		 * to a temporary value before freeing or modifying the latter.
+		 */
+		union drgn_value value;
+		value.little_endian = little_endian;
 
-		size = drgn_value_size(bit_size, bit_offset);
+		uint64_t size = drgn_value_size(bit_size);
+		char *dst;
 		if (size <= sizeof(res->value.ibuf)) {
-			dst = res->value.ibuf;
+			dst = value.ibuf;
 		} else {
 			dst = malloc64(size);
 			if (!dst)
 				return &drgn_enomem;
+			value.bufp = dst;
 		}
+		copy_bits(dst, p, bit_offset, bit_size, little_endian);
+
 		drgn_object_reinit(res, type, DRGN_OBJECT_ENCODING_BUFFER,
 				   bit_size, DRGN_OBJECT_VALUE);
-		memcpy(dst, buf, size);
+		res->value = value;
-		if (dst != res->value.ibuf)
-			res->value.bufp = dst;
-		res->value.bit_offset = bit_offset;
-		res->value.little_endian = little_endian;
 	} else {
 		drgn_object_reinit(res, type, encoding, bit_size,
 				   DRGN_OBJECT_VALUE);
-		drgn_value_deserialize(&res->value, buf, bit_offset, encoding,
+		drgn_value_deserialize(&res->value, p, bit_offset, encoding,
 				       bit_size, little_endian);
 	}
 	return NULL;
 }
 
 LIBDRGN_PUBLIC struct drgn_error *
-drgn_object_set_buffer(struct drgn_object *res,
+drgn_object_set_from_buffer(struct drgn_object *res,
-		       struct drgn_qualified_type qualified_type,
+			    struct drgn_qualified_type qualified_type,
-		       const void *buf, uint8_t bit_offset,
+			    const void *buf, size_t buf_size,
-		       uint64_t bit_field_size, enum drgn_byte_order byte_order)
+			    uint64_t bit_offset, uint64_t bit_field_size,
+			    enum drgn_byte_order byte_order)
 {
 	struct drgn_error *err;
-	bool little_endian;
-	struct drgn_object_type type;
-	enum drgn_object_encoding encoding;
-	uint64_t bit_size;
 
+	bool little_endian;
 	err = drgn_byte_order_to_little_endian(drgn_object_program(res),
 					       byte_order, &little_endian);
 	if (err)
 		return err;
 
+	struct drgn_object_type type;
+	enum drgn_object_encoding encoding;
+	uint64_t bit_size;
 	err = drgn_object_set_common(qualified_type, bit_field_size, &type,
 				     &encoding, &bit_size);
 	if (err)
 		return err;
-	return drgn_object_set_buffer_internal(res, &type, encoding, bit_size,
+
-					       buf, bit_offset, little_endian);
+	if (bit_size > UINT64_MAX - bit_offset ||
+	    buf_size < drgn_value_size(bit_offset + bit_size)) {
+		return drgn_error_create(DRGN_ERROR_INVALID_ARGUMENT,
+					 "buffer is too small");
+	}
+
+	return drgn_object_set_from_buffer_internal(res, &type, encoding,
+						    bit_size, buf, bit_offset,
+						    little_endian);
 }
 
 static struct drgn_error *
@@ -467,16 +514,14 @@ drgn_object_copy(struct drgn_object *res, const struct drgn_object *obj)
 	SWITCH_ENUM(obj->kind,
 	case DRGN_OBJECT_VALUE:
 		if (obj->encoding == DRGN_OBJECT_ENCODING_BUFFER) {
-			uint64_t size;
+			size_t size = drgn_object_size(obj);
 			char *dst;
 			const char *src;
-
-			size = drgn_buffer_object_size(obj);
 			if (size <= sizeof(obj->value.ibuf)) {
 				dst = res->value.ibuf;
 				src = obj->value.ibuf;
 			} else {
-				dst = malloc64(size);
+				dst = malloc(size);
 				if (!dst)
 					return &drgn_enomem;
 				src = obj->value.bufp;
@@ -486,7 +531,6 @@ drgn_object_copy(struct drgn_object *res, const struct drgn_object *obj)
 			memcpy(dst, src, size);
 			if (dst != res->value.ibuf)
 				res->value.bufp = dst;
-			res->value.bit_offset = obj->value.bit_offset;
 			res->value.little_endian = obj->value.little_endian;
 		} else {
 			drgn_object_reinit_copy(res, obj);
@@ -516,26 +560,22 @@ drgn_object_slice_internal(struct drgn_object *res,
 {
 	SWITCH_ENUM(obj->kind,
 	case DRGN_OBJECT_VALUE: {
-		uint64_t bit_end;
-		const char *buf;
-
 		if (obj->encoding != DRGN_OBJECT_ENCODING_BUFFER) {
 			return drgn_error_create(DRGN_ERROR_TYPE,
 						 "not a buffer object");
 		}
 
+		uint64_t bit_end;
 		if (__builtin_add_overflow(bit_offset, bit_size, &bit_end) ||
 		    bit_end > obj->bit_size) {
 			return drgn_error_create(DRGN_ERROR_OUT_OF_BOUNDS,
 						 "out of bounds of value");
 		}
-		bit_offset += obj->value.bit_offset;
+		return drgn_object_set_from_buffer_internal(res, type, encoding,
-		buf = drgn_object_buffer(obj) + bit_offset / 8;
+							    bit_size,
-		bit_offset %= 8;
+							    drgn_object_buffer(obj),
-		return drgn_object_set_buffer_internal(res, type, encoding,
+							    bit_offset,
-						       bit_size, buf,
+							    obj->value.little_endian);
-						       bit_offset,
-						       obj->value.little_endian);
 	}
 	case DRGN_OBJECT_REFERENCE:
 		if (obj->encoding != DRGN_OBJECT_ENCODING_BUFFER &&
@@ -607,7 +647,6 @@ drgn_object_read_reference(const struct drgn_object *obj,
 				union drgn_value *value)
 {
 	struct drgn_error *err;
-	uint64_t size;
 
 	assert(obj->kind == DRGN_OBJECT_REFERENCE);
 
@@ -616,42 +655,56 @@ drgn_object_read_reference(const struct drgn_object *obj,
 						  obj->type);
 	}
 
-	size = drgn_reference_object_size(obj);
+	uint64_t bit_size = obj->bit_size;
-	if (obj->encoding == DRGN_OBJECT_ENCODING_BUFFER) {
+	uint8_t bit_offset = obj->reference.bit_offset;
-		char *buf;
+	uint64_t read_size;
+	if (bit_offset == 0)
+		read_size = drgn_object_size(obj);
+	else
+		read_size = drgn_value_size(bit_offset + bit_size);
 
-		if (size <= sizeof(value->ibuf)) {
+	if (obj->encoding == DRGN_OBJECT_ENCODING_BUFFER) {
+		char ibuf_offset[sizeof(value->ibuf) + 1];
+		char *buf, *read_buf;
+		if (read_size <= sizeof(value->ibuf)) {
+			buf = read_buf = value->ibuf;
+		} else if (drgn_object_is_inline(obj)) {
+			/*
+			 * The value fits inside of the inline buffer, but it is
+			 * offset, so we need to read an additional byte. Read
+			 * it into an on-stack buffer first.
+			 */
 			buf = value->ibuf;
+			read_buf = ibuf_offset;
 		} else {
-			buf = malloc64(size);
+			buf = read_buf = malloc64(read_size);
 			if (!buf)
 				return &drgn_enomem;
 		}
 		err = drgn_memory_reader_read(&drgn_object_program(obj)->reader,
-					      buf, obj->reference.address, size,
+					      read_buf, obj->reference.address,
-					      false);
+					      read_size, false);
 		if (err) {
 			if (buf != value->ibuf)
 				free(buf);
 			return err;
 		}
+		copy_bits(buf, read_buf, bit_offset, bit_size,
+			  obj->reference.little_endian);
 		if (buf != value->ibuf)
 			value->bufp = buf;
-		value->bit_offset = obj->reference.bit_offset;
 		value->little_endian = obj->reference.little_endian;
 		return NULL;
 	} else {
 		char buf[9];
-
+		assert(read_size <= sizeof(buf));
-		assert(size <= sizeof(buf));
 		err = drgn_memory_reader_read(&drgn_object_program(obj)->reader,
-					      buf, obj->reference.address, size,
+					      buf, obj->reference.address,
-					      false);
+					      read_size, false);
 		if (err)
 			return err;
-		drgn_value_deserialize(value, buf, obj->reference.bit_offset,
+		drgn_value_deserialize(value, buf, bit_offset, obj->encoding,
-				       obj->encoding, obj->bit_size,
+				       bit_size, obj->reference.little_endian);
-				       obj->reference.little_endian);
 		return NULL;
 	}
 }
@@ -841,7 +894,7 @@ drgn_object_read_c_string(const struct drgn_object *obj, char **ret)
 			char *p, *str;
 
 			buf = drgn_object_buffer(obj);
-			value_size = drgn_buffer_object_size(obj);
+			value_size = drgn_object_size(obj);
 			len = min(value_size, (uint64_t)max_size);
 			p = memchr(buf, 0, len);
 			if (p)

libdrgn/object.h

@@ -149,15 +149,15 @@ drgn_object_set_unsigned_internal(struct drgn_object *res,
 				  uint64_t bit_size, uint64_t uvalue);
 
 /**
- * Like @ref drgn_object_set_buffer() but @ref drgn_object_set_common() was
+ * Like @ref drgn_object_set_from_buffer() but @ref drgn_object_set_common() was
- * already called.
+ * already called and the bounds of the buffer have already been checked.
  */
 struct drgn_error *
-drgn_object_set_buffer_internal(struct drgn_object *res,
+drgn_object_set_from_buffer_internal(struct drgn_object *res,
-				const struct drgn_object_type *type,
+				     const struct drgn_object_type *type,
-				enum drgn_object_encoding encoding,
+				     enum drgn_object_encoding encoding,
-				uint64_t bit_size, const void *buf,
+				     uint64_t bit_size, const void *buf,
-				uint8_t bit_offset, bool little_endian);
+				     uint64_t bit_offset, bool little_endian);
 
 /** Convert a @ref drgn_byte_order to a boolean. */
 struct drgn_error *

libdrgn/python/object.c

@@ -305,16 +305,12 @@ static int serialize_py_object(struct drgn_program *prog, char *buf,
 
 static int buffer_object_from_value(struct drgn_object *res,
 				    struct drgn_qualified_type qualified_type,
-				    PyObject *value_obj, uint64_t bit_offset,
+				    PyObject *value_obj,
 				    enum drgn_byte_order byte_order)
 {
 	struct drgn_error *err;
-	union drgn_value value;
-	struct drgn_object_type type;
-	enum drgn_object_encoding encoding;
-	uint64_t bit_size, size;
-	char *buf;
 
+	union drgn_value value;
 	err = drgn_byte_order_to_little_endian(drgn_object_program(res),
 					       byte_order,
 					       &value.little_endian);
@@ -323,6 +319,9 @@ static int buffer_object_from_value(struct drgn_object *res,
 		return -1;
 	}
 
+	struct drgn_object_type type;
+	enum drgn_object_encoding encoding;
+	uint64_t bit_size;
 	err = drgn_object_set_common(qualified_type, 0, &type, &encoding,
 				     &bit_size);
 	if (err) {
@@ -330,25 +329,16 @@ static int buffer_object_from_value(struct drgn_object *res,
 		return -1;
 	}
 
-	if (bit_offset >= 8) {
+	uint64_t size = drgn_value_size(bit_size);
-		PyErr_SetString(PyExc_ValueError,
-				"bit offset must be less than 8");
-		return -1;
-	}
-	if (bit_size > UINT64_MAX - bit_offset) {
-		PyErr_SetString(PyExc_OverflowError, "object is too large");
-		return -1;
-	}
-
-	size = drgn_value_size(bit_size, bit_offset);
 	if (size > SIZE_MAX) {
 		PyErr_NoMemory();
 		return -1;
 	}
+	char *buf;
 	if (size <= sizeof(value.ibuf)) {
 		buf = value.ibuf;
 	} else {
-		buf = malloc(size);
+		buf = malloc64(size);
 		if (!buf) {
 			PyErr_NoMemory();
 			return -1;
@@ -356,11 +346,9 @@ static int buffer_object_from_value(struct drgn_object *res,
 		value.bufp = buf;
 	}
 	memset(buf, 0, size);
-	value.bit_offset = bit_offset;
 
-	if (serialize_py_object(drgn_object_program(res), buf,
+	if (serialize_py_object(drgn_object_program(res), buf, bit_size, 0,
-				bit_offset + bit_size, bit_offset, value_obj,
+				value_obj, &type, value.little_endian) == -1) {
-				&type, value.little_endian) == -1) {
 		if (buf != value.ibuf)
 			free(buf);
 		return -1;
@@ -459,9 +447,14 @@ static DrgnObject *DrgnObject_new(PyTypeObject *subtype, PyObject *args,
 		}
 		err = NULL;
 	} else if (value_obj != Py_None) {
-		enum drgn_object_encoding encoding;
+		if (!bit_offset.is_none) {
+			PyErr_SetString(PyExc_ValueError,
+					"value cannot have bit offset");
+			goto err;
+		}
 
-		encoding = drgn_type_object_encoding(qualified_type.type);
+		enum drgn_object_encoding encoding =
+			drgn_type_object_encoding(qualified_type.type);
 		if (!drgn_object_encoding_is_complete(encoding)) {
 			err = drgn_error_incomplete_type("cannot create value with %s type",
 							 qualified_type.type);
@@ -476,24 +469,16 @@ static DrgnObject *DrgnObject_new(PyTypeObject *subtype, PyObject *args,
 					"bit field must be integer");
 			goto err;
 		}
-		if (encoding != DRGN_OBJECT_ENCODING_BUFFER) {
+		if (encoding != DRGN_OBJECT_ENCODING_BUFFER && !byteorder.is_none) {
-			if (!byteorder.is_none) {
+			PyErr_SetString(PyExc_ValueError,
-				PyErr_SetString(PyExc_ValueError,
+					"primitive value cannot have byteorder");
-						"primitive value cannot have byteorder");
+			goto err;
-				goto err;
-			}
-			if (!bit_offset.is_none) {
-				PyErr_SetString(PyExc_ValueError,
-						"primitive value cannot have bit offset");
-				goto err;
-			}
 		}
 
 		switch (encoding) {
 		case DRGN_OBJECT_ENCODING_BUFFER:
 			if (buffer_object_from_value(&obj->obj, qualified_type,
 						     value_obj,
-						     bit_offset.uvalue,
 						     byteorder.value) == -1)
 				goto err;
 			err = NULL;
@@ -903,14 +888,10 @@ static PyObject *DrgnObject_repr(DrgnObject *self)
 			goto out;
 		}
 		Py_DECREF(tmp);
-		if (self->obj.encoding == DRGN_OBJECT_ENCODING_BUFFER) {
+		if (self->obj.encoding == DRGN_OBJECT_ENCODING_BUFFER &&
-			if (append_byte_order(parts,
+		    append_byte_order(parts, drgn_object_program(&self->obj),
-					      drgn_object_program(&self->obj),
+				      self->obj.value.little_endian) == -1)
-					      self->obj.value.little_endian) == -1 ||
+			goto out;
-			    append_bit_offset(parts,
-					      self->obj.value.bit_offset) == -1)
-				goto out;
-		}
 		break;
 	}
 	case DRGN_OBJECT_REFERENCE: {
@@ -1091,13 +1072,9 @@ static PyObject *DrgnObject_get_byteorder(DrgnObject *self, void *arg)
 static PyObject *DrgnObject_get_bit_offset(DrgnObject *self, void *arg)
 {
 	SWITCH_ENUM(self->obj.kind,
-	case DRGN_OBJECT_VALUE:
-		if (self->obj.encoding == DRGN_OBJECT_ENCODING_BUFFER)
-			return PyLong_FromLong(self->obj.value.bit_offset);
-		else
-			Py_RETURN_NONE;
 	case DRGN_OBJECT_REFERENCE:
 		return PyLong_FromLong(self->obj.reference.bit_offset);
+	case DRGN_OBJECT_VALUE:
 	case DRGN_OBJECT_UNAVAILABLE:
 		Py_RETURN_NONE;
 	)

tests/test_object.py

@@ -103,13 +103,22 @@ class TestInit(MockProgramTestCase):
     def test_bit_offset(self):
         self.assertRaisesRegex(
             ValueError,
-            "primitive value cannot have bit offset",
+            "value cannot have bit offset",
             Object,
             self.prog,
             "int",
             value=0,
             bit_offset=4,
         )
+        self.assertRaisesRegex(
+            ValueError,
+            "value cannot have bit offset",
+            Object,
+            self.prog,
+            self.point_type,
+            value={},
+            bit_offset=4,
+        )
         self.assertRaisesRegex(
             ValueError,
             "unavailable object cannot have bit offset",
@@ -272,6 +281,41 @@ class TestReference(MockProgramTestCase):
             obj.value_(), {"point": {"x": 99, "y": -1}, "bar": 12345, "baz": 0}
         )
 
+    def test_read_struct_bit_offset(self):
+        value = 12345678912345678989
+        for bit_size in range(1, 65):
+            for bit_offset in range(8):
+                size = (bit_size + bit_offset + 7) // 8
+                size_mask = (1 << (8 * size)) - 1
+                for byteorder in ["little", "big"]:
+                    if byteorder == "little":
+                        tmp = value << bit_offset
+                    else:
+                        tmp = value << (8 - bit_size - bit_offset) % 8
+                    tmp &= size_mask
+                    buf = tmp.to_bytes(size, byteorder) + b"\0"
+                    prog = mock_program(segments=[MockMemorySegment(buf, 0)])
+                    obj = Object(
+                        prog,
+                        prog.struct_type(
+                            None,
+                            (bit_size + 7) // 8,
+                            (
+                                TypeMember(
+                                    prog.type("unsigned long long"),
+                                    "x",
+                                    bit_field_size=bit_size,
+                                ),
+                            ),
+                        ),
+                        address=0,
+                        bit_offset=bit_offset,
+                        byteorder=byteorder,
+                    )
+                    self.assertEqual(
+                        obj.read_().x.value_(), value & ((1 << bit_size) - 1)
+                    )
+
     def test_array(self):
         segment = bytearray()
         for i in range(10):