|
From: Nicholas N. <nj...@cs...> - 2005-03-28 14:35:38
|
On Mon, 28 Mar 2005, Julian Seward wrote:
>> One problem with the dictionary approach is that you are exposing the
>> internals of the struct to tools. I avoided this approach with the
>> 'needs' and 'details' structs for just this reason.
>
> Confused. Each tool would assemble a struct and pass a pointer
> to it to the core as the return value of the tool's initialisation
> function. So inevitably the internals of the struct are visible
> to the tool anyway since it created the struct.
In the current approach the struct is defined and allocated within the
core. It's called VG_(tool_interface), it has type "VgToolInterface.
The tool doesn't know what this struct looks like.
> How does having a struct as part of the interface make it fragile?
> I am thinking of a very simple struct which contains only function
> pointers; no other data.
I think the concern is if (when) fields get added or removed. Doing it
via functions can help, eg. if a field is removed you can make the
function that sets that field just do nothing. Just a general
abstract-types-are-nice thing. Although in this case it doesn't help that
much, as I agree below.
> If you are really worried about version mismatches in the struct,
> make the first field (viz, offset 0) be an int which carries a
> version identifier. Then we can safely say
>
> "if (struct->version != ...) barf()"
>
> This at least allows detection of ABI-level mismatches at startup
> time, providing the version number is bumped each time the API changes.
I don't like version numbers much, they're easy to forget to change.
Although I guess if you tied it into the existing MAJOR_VERSION_NUMBER (or
whatever it's called) that would help.
> ------
>
> The registration-based scheme feels cluttered to me, because there are
> more names and storage locations (global state) to keep track of. For
> example, the tool makes a registration call:
>
> + VG_(basic_tool_funcs) (TL_(post_clo_init),
> + TL_(instrument),
> + TL_(fini));
>
> Now in the core, if I want to call one of these, first I have to find the
> global variables that those pointers got stored in -- probably using
> grep.
Global variables? There are no global variables, the function pointers
will all get stored in the single VgToolInterface struct.
> Then I have to be convinced that they've been initialised, and only then
> can I make the call.
I don't see how this would be different if the tool does the struct
initialisation? (More below.)
> Also I have to figure out precisely which groups of functions make up
> the interface, and whether I've forgotten to consider any.
>
> By comparison, if I have a struct "tdict" from the tool, then there are
> no banks of global variables to be found,
As I've done it, we still have the single struct, it's just on the core
side. There are no banks of global variables. Perhaps it's confusing
because the names above TL_(post_clo_init)(), TL_(instrument)(), etc,
still have the TL_ prefix (which here means "vgTool_"). They don't need
to, I just haven't changed that in the patch. They could be eg.
mc_post_clo_init(), mc_instrument(), etc.
> no questions about initialisation,
> and if I want assurance I'm seeing the complete interface, I only have to
> read the struct definition -- nothing else.
That's true of how I've done it -- just look at VgToolInterface.
(Actually, it was already like that, I don't mean to take credit for it.)
> Then I can do:
>
> assert(tdict->instrument); // we're hosed if this fails
> bbi = tdict->instrument(bb);
>
> or
>
> if (tdict->some_optional_function)
> some_optional_function(args);
That's how it is with my patch. VG_(tool_interface) is the name of the
"tdict" struct. A shorter name might be better.
Another confusing aspect currently is that vg_toolint.c defines lots of
TL_ functions like this:
void TL_(fini)(Int exitcode)
{
return (*VG_(tool_interface).tool_fini)(exitcode);
}
where TL_ here means "vgToolInternal_" and so is different to the TL_
within the tools. So this is just a short-cut for your "tdict->fini(x)"
example which the core can call. We could get rid of these, and just
always use "tdict->fini(x)" to make it explicit. And then there are the
VG_(defined_*) and VG_(missing_*) functions which further clutter things,
and could possibly be removed.
> ------
>
> Re-reading that, my real point is that the struct scheme is easier for
> human readers of the code to read and reason about. And as Jeremy says,
> it's not really any less robust against API changes relative to the
> registration-scheme.
I think the patch is a lot closer to what you described and want than you
realise. Perhaps it's unclear because it's only a first step and there's
more cruft to remove (eg. the double-meaning of TL_). But it has a single
struct, which I think is the main thing you want.
As for the robustness against API changes, I agree that the way I've done
it isn't any more robust against API changes. But the advantage it does
have over a naked struct is robustness against one possible tool screwup:
the cases where the tool has to provide multiple functions together, as I
explained in my other response to Jeremy's comments.
--
Thanks for the feedback, really! I'm defending my patch, because there
are constraints involved that aren't obvious. But I won't refuse to
change it if the consensus is still against it after those constraints
have been made clearer. I'll be away until Thursday night, and I'm not
sure how much email contact I'll have, so if I don't reply it's not
because I'm sulking :)
N
|