[Ltrace-devel] Getting prototypes from debug information
Petr Machata
pmachata at redhat.com
Fri Apr 18 13:19:28 UTC 2014
Petr Machata <pmachata at redhat.com> writes:
> What we are using is very close to what Linux kernel coding style
> looks like.
One more thing, opening curly braces should be placed at the end of the
line, except for functions. (This is the same that Linux kernel does.)
> - in getEnum, you assume that the underlying type is INT. That may not
> be the case. In C++11, you can actually choose the underlying enum
> type. DW_TAG_enumeration_type's DW_AT_type (if present) will point to
> that underlying type.
I looked into this, and GCC currently doesn't seem to emit DW_AT_type
for enums with specified base. Instead it denotes signedness by
encoding enumerators with DW_FORM_udata or DW_FORM_sdata. Size of the
underlying type is encoded in DW_TAG_enumeration_type's DW_AT_byte_size,
which might be important for enums in structs (I don't think it will
matter in naked parameters, unless in weird cases like __int128, which
ltrace doesn't handle anyway).
Next, about the memory management. That's rather spartan. All the
"own" values that you pass to all sorts of initializers determine
whether the initialized structure owns the passed-in pointer. Most of
the time they will, because their lifetimes are generally different, and
you end up cloning stuff anyway. So the own == 0 case is really only
useful for referencing static data, and is not terribly helpful.
I'm wondering about this idiom, that you use in a couple places:
+ case DW_TAG_pointer_type:
[...]
+ *info = calloc( 1, sizeof(struct arg_type_info) );
[...]
+ type_init_pointer(*info, NULL, 0);
+
+ complain(type_die, "Storing pointer type: %p", *info);
+ dict_insert( &type_hash, &die_offset, info );
+ return getType( &(*info)->u.ptr_info.info, &next_die );
What you end up inserting is an incomplete type. If getType later
fails, it will never properly finish initializing info. Later on, a
DW_TAG_const_type referencing that type will pick it up, incomplete
(presumably) and return a legitimately-looking arg_type_info that will
kill ltrace. I would prefer it written this way:
+ struct arg_type_info *ptr;
+ if (getType(&ptr, &next_die)) {
+ type_init_pointer(*info, ptr, 1);
+ complain(type_die, "Storing pointer type: %p", *info);
+ dict_insert( &type_hash, &die_offset, info );
+ return true;
+ } else {
+ free(*info);
+ return false;
+ }
Next, this:
+ case DW_TAG_typedef: ;
+ case DW_TAG_const_type: ;
+ case DW_TAG_volatile_type: ;
Presumably the semicolons here are so that you can declare bool res on
the next line. Just put the whole thing into scope braces. You do the
same in attr_numeric.
+ // no type. Use 'void'. Normally I'd think this is bogus, but stdio
+ // typedefs something to void
+ *info = type_get_simple( ARGTYPE_VOID );
+ complain(type_die, "Storing void type: %p", *info);
Wow, I'll be damned. DWARF4 actually permits typeless typedefs for
typedef declarations (or forward typedefs if you will--C doesn't have
this). I don't think it's correct to emit it like that in this case. I
filed a bug about it:
http://gcc.gnu.org/bugzilla/show_bug.cgi?id=60885
However this doesn't help us, we'll need to support what's out there.
So yeah, that code should stay in.
+ while(1) {
+ if( dwarf_tag(&arg_die) != DW_TAG_formal_parameter )
+ goto next_prototype_argument;
+
I don't think the goto is necessary or very convenient here. Just
reverse the condition and put what's between the goto and the label into
a block.
+ next_prototype_argument: ;
+ int res = dwarf_siblingof(&arg_die, &arg_die);
+ if( res == 0 ) continue; /* sibling exists */
+ if( res < 0 ) return false; /* error */
+ break; /* no sibling exists */
dwarf_siblingof promises to return -1/0/+1. So:
+ switch (dwarf_siblingof(&arg_die, &arg_die)) {
+ case 0:
+ /* Sibling exists. */
+ continue;
+ case -1:
+ /* Error. */
+ return false;
+ case 1:
+ /* We are done. */
+ return true;
+ }
Later in process_die_compileunit:
+ const char* function_name = dwarf_diename(&die);
This could concievably return NULL for DW_AT_name-less DIE.
+ struct prototype* proto =
+ protolib_lookup_prototype(plib, function_name, true );
+
+ if( proto != NULL )
+ {
+ complain(&die, "Prototype already exists. Skipping");
+ goto next_prototype;
+ }
+
+ if( !filter_matches_symbol(options.plt_filter, function_name, lib) &&
+ !filter_matches_symbol(options.static_filter, function_name, lib) &&
+ !filter_matches_symbol(options.export_filter, function_name, lib) )
+ {
+ complain(&die, "Prototype not requested by any filter");
+ goto next_prototype;
+ }
Same as above, please shun goto's if at all possible. Here, just make
it something like:
+ if (protolib_lookup_prototype(plib, function_name,
+ true) == NULL
+ && (filter_matches_symbol(options.plt_filter,
+ function_name, lib)
+ || filter_matches_symbol(options.static_filter,
+ function_name, lib)
+ || filter_matches_symbol(options.export_filter,
+ function_name, lib) {
+ malloc;
+ prototype_init;
+ etc.;
+ }
And down below do the same dwarf_siblingof dance as above.
+static bool import( struct protolib* plib, struct library* lib, Dwfl* dwfl )
+{
+ dict_init(&type_hash, sizeof(Dwarf_Off), sizeof(struct arg_type_info*),
+ dwarf_die_hash, dwarf_die_eq, NULL );
type_hash should be a local variable in import and the pointer should be
passed to other functions. Is there a reason for making it global?
+- I handle static functions now. Should I? Those do not have DW_AT_external==1
Hmm, good question. I think so. .symtab will contain static functions
as well. But it's not clear what to do with duplicates. I guess the
dynamic one should win, as that's the one likely being traced. Ideally
there would be a way to denote which of the static functions one wants
to trace... and really, ideally, symbol lookup would tie into Dwarf as
well--that will give us many many more symbols than what's in symbol
tables. For now, I'd keep them, but maybe remember which are static and
reject them when there is a conflict.
+- what do function pointers look like? I'm doing void*
We don't do anything fancy for those. It would make sense to invent a
lens that translates these to function names, but treating them like
void* is fine.
+- unions
Yeah, we don't really do those.
+- protolib_lookup_prototype should look for imports?
You are only looking for duplicate definitions, so no. You are only
interested what's in your own protolib, not in what some other protolib
defines.
+ if (dwfl == NULL)
+ fprintf(stderr,
"Couldn't initialize libdwfl unwinding "
"for process %d: %s\n", leader->pid,
dwfl_errmsg (-1));
- }
The message needs to be updated.
int r = dwfl_linux_proc_attach(dwfl,
- leader->pid,
- true);
+ leader->pid,
+ true);
Why this indentation change?
+ if( dwfl != NULL &&
+ ( filter_matches_library(options.plt_filter, lib ) ||
+ filter_matches_library(options.static_filter, lib ) ||
+ filter_matches_library(options.export_filter, lib ) ) )
+ {
+ import_DWARF_prototypes( lib->protolib, lib, dwfl );
I'm not sure where to place this best. It seems it should be together
with the rest of this logic in library_get_prototype in output.c. The
Dwarf related to this module could be stored at struct library
(contingent on availability of libdw), where library_get_prototype will
get it.
The integration with conf-based protolibs is kinda tricky. What we
currently do for somelib.so.1.2.3 is look for somelib.so.1.2.3.conf,
then somelib.so.1.2.conf, then somelib.so.1.conf, and then
somelib.so.conf. I think it's only there that we should fall back on
dwarf. So in pseudocode:
@@ -207,6 +207,10 @@ library_get_prototype(struct library *lib, const char *name)
&& lib->type == LT_LIBTYPE_DSO
&& snip_period(buf));
+ if (lib->protolib == NULL && lib->dw != NULL) {
+ lib->protolib = import from Dwarf, cache, etc.
+ }
+
if (lib->protolib == NULL)
lib->protolib = protolib_cache_default(&g_protocache,
buf, 0);
Thank you,
PM
More information about the Ltrace-devel
mailing list