Add support for reading methods via "members" array #72

jgkamat · 2020-10-01T23:29:10Z

This is the first step towards real "method" support (ie: reading methods like Class::methodname). It also adds support for instance.method right now, and adds the methods to the members array of object types.

Here are the biggest red flags (that I would like input on):

Right now the python side is completely unaware that parameters can be objects - this works because objects can be automagically evaluated to types. Is this fine for now, or would you like that to be implemented here as well.
Anywhere outside the core - parameters are converted to types for compatibility. Is there anywhere a dual pathway makes sense.
I am using the lazy_type and lazy_object variable names to signify the parameter must be a type/object at that point. I could split things a little more to remove this distinction if you wish.
I still need to work on adding tests.

If you have high-level suggestions for the architecture, I'm also happy to hear those as well.

osandov

The architecture all looks good here. There are a couple of bugs I spotted, but my comments are mostly around naming and commit organization.

osandov · 2020-10-05T20:59:15Z

libdrgn/debug_info.c

@@ -1026,6 +1026,7 @@ struct drgn_type_from_dwarf_thunk {
 	struct drgn_type_thunk thunk;
 	Dwarf_Die die;
 	bool can_be_incomplete_array;
+	uint64_t bias;


Nit, and hopefully it's not too much trouble to change this, but I think of the die, bias pair as one entity, so I'd prefer if bias was immediately after die here and in parameter lists where they're passed together.

libdrgn/debug_info.c

libdrgn/Makefile.am

libdrgn/python/drgnpy.h

osandov · 2020-10-05T21:49:08Z

libdrgn/object.h

+			      struct drgn_object *object)
+{
+	lazy_object->object = object;
+	if (object)


I don't see a good reason for this to be NULL. I'm assuming you did this because drgn_lazy_type_init_evaluated() says it can take a NULL type, but I think that documentation is wrong ;) it doesn't make sense for that to take NULL either as far as I can tell.

Also, again because objects are immutable, this needs to allocate a new object and copy the passed object (which of course means that `drgn_lazy_parameter_get_object() should manually reinitialize the lazy parameter rather than calling this). I think this will only end up being needed for the Python bindings, in case someone constructs a member with an object.

libdrgn/object.h

osandov · 2020-10-05T22:23:13Z

libdrgn/debug_info.c

+		err = drgn_compound_type_builder_add_member(builder, member, name,
+							    0, bit_field_size);
+		if (err)
+			goto err;
+		return NULL;


Could you refactor this so you only need to call drgn_compound_type_builder_add_member() in one place?

osandov · 2020-10-05T22:24:05Z

libdrgn/dwarf_index.h

@@ -319,6 +319,8 @@ struct drgn_error *drgn_dwarf_index_get_die(struct drgn_dwarf_index_die *die,
 					    Dwarf_Die *die_ret,
 					    uint64_t *bias_ret);

+bool find_definition(struct drgn_dwarf_index *dindex, uintptr_t die_addr,


Since this is being made public, it should be renamed drgn_dwarf_index_find_definition() and given a comment.

osandov · 2020-10-05T22:24:47Z

libdrgn/type.c

-		return err;
+	struct drgn_error *err = NULL;
+	if (!parameter.is_object) {
+		err = drgn_lazy_type_check_prog(&parameter, builder->prog);


I think we need to check this in the is_object case, too, right?

Since types can have object members, they must keep track of the bias variable.

jgkamat · 2020-10-14T23:57:49Z

libdrgn/debug_info.c

+	if (!dwarf_flag(die, DW_AT_declaration, &decl) && decl) {
+		Dwfl_Module *module;
+		Dwarf_Off offset;
+		if (drgn_dwarf_index_find_definition(&dbinfo->dindex, (uintptr_t)die->addr, &module, &offset)) {


Note to self - if we aren't able to find a declaration here, the object returned is pretty odd and can probably cause a crash if mishandled. Not sure if we should worry about that at the moment - but I have been able to generate class functions with no declarations (usually if the function was not used or defined)

osandov

Didn't look at the final commit in much detail yet, but wanted to send you the comments on the lazy object handling.

osandov · 2020-10-22T09:00:48Z

libdrgn/lazy_parameter.c

+		drgn_type_thunk_free(lazy_parameter->thunk);
+}
+
+inline struct drgn_error *


I'd just drop the inline now that this has been moved to a .c file.

osandov · 2020-10-22T09:02:34Z

libdrgn/lazy_parameter.c

+	}
+	return NULL;
+}
+


Stray blank line.

osandov · 2020-10-22T09:04:14Z

libdrgn/lazy_parameter.h

+ * Validate a @ref drgn_program matches the one in a @ref drgn_lazy_parameter.
+ *
+ * @param[in] lazy_parameter Lazy parameter to validate.
+ * @param[in] prog The program to compare against..


osandov · 2020-10-22T09:06:11Z

libdrgn/lazy_parameter.c

+		else
+			free(lazy_parameter->object);


You also need to call drgn_object_deinit() first.

osandov · 2020-10-22T09:06:37Z

libdrgn/lazy_parameter.c

+	} else
+		if (!drgn_lazy_parameter_is_evaluated(lazy_parameter))


Squash these to else if.

osandov · 2020-10-22T09:12:54Z

libdrgn/lazy_parameter.c

+		drgn_lazy_parameter_init_object(lazy_parameter, &obj);
+		thunk.free_fn(thunk_ptr);
+	}
+	drgn_object_init(ret, drgn_object_program(lazy_parameter->object));


The convention in libdrgn is to assume that a passed object is already initialized. Rather than calling drgn_object_init() here, the caller should do it.

osandov · 2020-10-22T09:15:35Z

libdrgn/lazy_parameter.c

+	}
+	drgn_object_init(ret, drgn_object_program(lazy_parameter->object));
+	drgn_object_copy(ret, lazy_parameter->object);
+	*ret = *lazy_parameter->object;


drgn_object_copy() already effectively did this, so this shouldn't be necessary and isn't safe since the object may have an allocated buffer.

osandov · 2020-10-22T09:18:34Z

libdrgn/lazy_parameter.c

+		struct drgn_error *err = thunk.evaluate_fn(thunk_ptr, &obj);
+		if (err)
+			return err;
+		drgn_lazy_parameter_init_object(lazy_parameter, &obj);


Using drgn_lazy_parameter_init_object() means that you're creating a temporary object only to copy it and then discard it. I'd just malloc() the new object directly and reinitialize the lazy parameter here.

osandov · 2020-10-22T09:19:07Z

libdrgn/lazy_parameter.h

+	if (!new_obj)
+		return &drgn_enomem;
+	drgn_object_init(new_obj, drgn_object_program(object));
+	drgn_object_copy(new_obj, object);


This can fail, you need to handle errors.

osandov · 2020-10-22T09:21:56Z

libdrgn/debug_info.c

+	struct drgn_object_thunk thunk;
+	Dwarf_Die die;
+	uint64_t bias;
+	const char* name;


const char *name

osandov · 2021-10-21T21:08:26Z

This will be superseded by #123, so I'm closing this in favor of exposing methods that way instead.

osandov requested changes Oct 5, 2020

View reviewed changes

jgkamat force-pushed the jay/methods branch 3 times, most recently from 397751f to 5e6b5fd Compare October 10, 2020 00:06

jgkamat added 4 commits October 13, 2020 16:17

libdrgn: Add bias to all types

d5cc395

Since types can have object members, they must keep track of the bias variable.

libdrgn: Rename drgn_lazy_type to drgn_lazy_parameter

8d3d0ed

libdrgn: Add support for objects in lazy_parameters

9b01b59

libdrgn: Add support for parsing member objects

0718551

jgkamat force-pushed the jay/methods branch from 5e6b5fd to 0718551 Compare October 13, 2020 23:18

jgkamat mentioned this pull request Oct 13, 2020

Add support for parsing and reading C++ type templates #60

Merged

jgkamat commented Oct 14, 2020

View reviewed changes

osandov reviewed Oct 22, 2020

View reviewed changes

osandov mentioned this pull request Oct 13, 2021

Scopes API #123

Open

osandov closed this Oct 21, 2021

osandov mentioned this pull request Jul 5, 2023

Support debugging programs written in C++ #328

Open

25 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for reading methods via "members" array #72

Add support for reading methods via "members" array #72

jgkamat commented Oct 1, 2020

osandov left a comment

osandov Oct 5, 2020

osandov Oct 5, 2020

osandov Oct 5, 2020

osandov Oct 5, 2020

osandov Oct 5, 2020

jgkamat Oct 14, 2020

osandov left a comment

osandov Oct 22, 2020

osandov Oct 22, 2020

osandov Oct 22, 2020

osandov Oct 22, 2020

osandov Oct 22, 2020

osandov Oct 22, 2020

osandov Oct 22, 2020

osandov Oct 22, 2020

osandov Oct 22, 2020

osandov Oct 22, 2020

osandov commented Oct 21, 2021

		} else
		if (!drgn_lazy_parameter_is_evaluated(lazy_parameter))

Add support for reading methods via "members" array #72

Add support for reading methods via "members" array #72

Conversation

jgkamat commented Oct 1, 2020

osandov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osandov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

osandov commented Oct 21, 2021