Re: [Valgrind-developers] Matching core and skin versions

[Nicholas N. <nj...@ca...>]

On some days, Jeremy Fitzhardinge wrote:

> I think a better scheme would be to have the core export a preprocessor
> define called something like VG_IF_VERSION.  The skins would then simply
> do:
>
>       details->core_interface = VG_IF_VERSION;
>
> If the core makes an incompatible change to its skin interface, then
> increment the IF_VERSION.  You could use a string in X.Y format, where
> changes in X are completely incompatible, but Y indicates a backwards
> compatible change.
>
> This way the association between skin binary and core is checked by the
> compiler and then built into the skin automatically.

Thinking about this some more, seems to me like every skin is going to
identically do:

      details->core_interface = VG_IF_VERSION;

In which case why not just put:

   const Char* core_interface = VG_IF_VERSION;

in vg_skin.h, which every skin #includes.  That way, the skin doesn't have
to mention anything about version numbers, and assuming we get the X.Y
binary (in)compatibility numbering right, everything will work as desired.

(Thinking some more:  it would be better to have VG_MINOR_IF_VERSION and
VG_MAJOR_IF_VERSION integers rather than a string, just to make
comparisons simpler.)

> Not exposing structures as part of the interface is a really good
> idea (OpenGL is a really nice example of an API which is extremely good
> at maintaining long-term binary compatibility, partly because it exposes
> no structures more complex than arrays of ints or floats).

Interesting.  Maybe I should change the way `details', `needs' and
`trackable events' are set, using functions instead?  Eg. change this:

   details->name             = "foo";
   details->version          = "1.1.1";
   details->description      = "blah blah blah";
   details->copyright_author =
      "Copyright (C) 2002, and GNU GPL'd, by Nicholas Nethercote.";
   details->bug_reports_to   = "nj...@ca...";

   needs->basic_block_discards = True;
   needs->command_line_options = True;

   track->new_mem_heap       = & my_new_mem_heap;
   track->die_mem_heap       = & my_die_mem_heap;

to this:

   VG_(details_name)("foo");
   VG_(details_version)("1.1.1");
   ...

   VG_(set_needs_basic_block_discards)();
   VG_(set_command_line_options)();

   /* could have corresponding VG_(unset_command_line_options)() etc */

   VG_(track_new_mem_heap)(& my_new_mem_heap);
   VG_(track_die_mem_heap)(& my_die_mem_heap);

And not expose the fields of the struct in vg_skin.h. That way even if the
structs were totally reordered in core, an old skin would still work fine
with it.

The only complication with this is that a skin could call these functions
at any time... but that could possibly be desirable (eg. start/stop
tracking new_mem_heap events if something happens) and it's also currently
possible anyway since VG_(needs) etc. are all global anyway.  By using
functions I could add some sanity checking, eg. complain if a skin tries
to set a need after startup which would screw things up (eg. shadow_regs,
which affects the baseBlock initialisation and thus can't be turned on
halfway through execution).

There are some other exposed structs in the interface... I might have a
look through and see how else it can be better future-proofed.

Thanks for the good suggestions.

N