|
From: Nicholas N. <n.n...@gm...> - 2009-02-26 00:10:15
|
Hi,
Back when Valgrind tools were built as shared objects, we had this
notion of the binary interface version, so you could tell if a tool
would work with a particular version of the Valgrind core.
Now that tools are statically linked with the core, this isn't
necessary. (IMHO this is excellent.) Turns out there was some
left-over cruft from this. The patch below gets rid of it. Any
objections? The main reason I'm asking is to make sure that Josef is
happy with removing the version-specific argv-handling code.
Nick
Index: callgrind/dump.c
===================================================================
--- callgrind/dump.c (revision 9264)
+++ callgrind/dump.c (working copy)
@@ -1640,7 +1640,6 @@
Int i,j,size = 0;
HChar* argv;
-#if VG_CORE_INTERFACE_VERSION > 8
if (VG_(args_the_exename))
size = VG_(sprintf)(cmdbuf, " %s", VG_(args_the_exename));
@@ -1651,15 +1650,6 @@
for(j=0;argv[j]!=0;j++)
if (size < BUF_LEN) cmdbuf[size++] = argv[j];
}
-#else
- for(i = 0; i < VG_(client_argc); i++) {
- argv = VG_(client_argv)[i];
- if (!argv) continue;
- if ((size>0) && (size < BUF_LEN)) cmdbuf[size++] = ' ';
- for(j=0;argv[j]!=0;j++)
- if (size < BUF_LEN) cmdbuf[size++] = argv[j];
- }
-#endif
if (size == BUF_LEN) size--;
cmdbuf[size] = 0;
Index: include/pub_tool_tooliface.h
===================================================================
--- include/pub_tool_tooliface.h (revision 9264)
+++ include/pub_tool_tooliface.h (working copy)
@@ -37,39 +37,23 @@
/* ------------------------------------------------------------------ */
/* The interface version */
-/* The version number indicates binary-incompatible changes to the
- interface; if the core and tool versions don't match, Valgrind
- will abort. */
-#define VG_CORE_INTERFACE_VERSION 11
+/* Initialise tool. Must do the following:
+ - initialise the `details' struct, via the VG_(details_*)() functions
+ - register the basic tool functions, via VG_(basic_tool_funcs)().
+ May do the following:
+ - initialise the `needs' struct to indicate certain requirements, via
+ the VG_(needs_*)() functions
+ - any other tool-specific initialisation
+*/
+extern void (*VG_(tl_pre_clo_init)) ( void );
-typedef struct _ToolInfo {
- Int sizeof_ToolInfo;
- Int interface_version;
+/* Every tool must include this macro somewhere, exactly once. The
+ interface version is no longer relevant, but we kept the same name
+ to avoid requiring changes to tools.
+*/
+#define VG_DETERMINE_INTERFACE_VERSION(pre_clo_init) \
+ void (*VG_(tl_pre_clo_init)) ( void ) = pre_clo_init;
- /* Initialise tool. Must do the following:
- - initialise the `details' struct, via the VG_(details_*)() functions
- - register any helpers called by generated code
-
- May do the following:
- - initialise the `needs' struct to indicate certain requirements, via
- the VG_(needs_*)() functions
- - initialize all the tool's entrypoints via the VG_(init_*)() functions
- - register any tool-specific profiling events
- - any other tool-specific initialisation
- */
- void (*tl_pre_clo_init) ( void );
-} ToolInfo;
-
-extern const ToolInfo VG_(tool_info);
-
-/* Every tool must include this macro somewhere, exactly once. */
-#define VG_DETERMINE_INTERFACE_VERSION(pre_clo_init) \
- const ToolInfo VG_(tool_info) = { \
- .sizeof_ToolInfo = sizeof(ToolInfo), \
- .interface_version = VG_CORE_INTERFACE_VERSION, \
- .tl_pre_clo_init = pre_clo_init, \
- };
-
/* ------------------------------------------------------------------ */
/* Basic tool functions */
Index: include/pub_tool_libcproc.h
===================================================================
--- include/pub_tool_libcproc.h (revision 9264)
+++ include/pub_tool_libcproc.h (working copy)
@@ -35,9 +35,7 @@
Command-line and environment stuff
------------------------------------------------------------------ */
-/* Client args and environment. Note that VG_(client_argv)[] can be written
- to by the client, so you should check each entry is non-NULL before
- printing. VG_(client_envp) can be inspected with VG_(getenv)(). */
+/* Client environment. */
extern Char** VG_(client_envp);
/* Looks up VG_(client_envp) */
Index: coregrind/m_main.c
===================================================================
--- coregrind/m_main.c (revision 9264)
+++ coregrind/m_main.c (working copy)
@@ -1585,7 +1585,7 @@
// p: setup_file_descriptors() [for 'VG_(fd_xxx_limit)']
//--------------------------------------------------------------
VG_(debugLog)(1, "main", "Initialise the tool part 1 (pre_clo_init)\n");
- (VG_(tool_info).tl_pre_clo_init)();
+ VG_(tl_pre_clo_init)();
//--------------------------------------------------------------
// If --tool and --help/--help-debug was given, now give the core+tool
|
|
From: Nicholas N. <n.n...@gm...> - 2009-02-26 02:43:44
|
On Thu, Feb 26, 2009 at 11:10 AM, Nicholas Nethercote <n.n...@gm...> wrote: > Hi, > > Back when Valgrind tools were built as shared objects, we had this > notion of the binary interface version, so you could tell if a tool > would work with a particular version of the Valgrind core. > > Now that tools are statically linked with the core, this isn't > necessary. (IMHO this is excellent.) Turns out there was some > left-over cruft from this. The patch below gets rid of it. Any > objections? The main reason I'm asking is to make sure that Josef is > happy with removing the version-specific argv-handling code. Nb: In case it wasn't clear, this is purely dead code removal. The patch just removes stuff that hasn't been used for several years now. Nick |
|
From: Josef W. <Jos...@gm...> - 2009-02-26 22:14:55
|
On Thursday 26 February 2009, Nicholas Nethercote wrote: > Hi, > > Back when Valgrind tools were built as shared objects, we had this > notion of the binary interface version, so you could tell if a tool > would work with a particular version of the Valgrind core. > > Now that tools are statically linked with the core, this isn't > necessary. (IMHO this is excellent.) Turns out there was some > left-over cruft from this. The patch below gets rid of it. Any > objections? The main reason I'm asking is to make sure that Josef is > happy with removing the version-specific argv-handling code. Yeah, no problem from my side. Just get rid of it, thanks. An externally distributed tool still would need to get an version number of the tool interface used in the static VG libraries, at build time. Of course, it just can look at the Valgrind release version (I assume with pkgconfig?). Still, would it be better to have an explicit tool interface version given in valgrind.pc? Josef |
|
From: Nicholas N. <n.n...@gm...> - 2009-02-27 01:20:51
|
On Fri, Feb 27, 2009 at 9:13 AM, Josef Weidendorfer <Jos...@gm...> wrote: > On Thursday 26 February 2009, Nicholas Nethercote wrote: > > An externally distributed tool still would need to get an version > number of the tool interface used in the static VG libraries, > at build time. Of course, it just can > look at the Valgrind release version (I assume with pkgconfig?). > Still, would it be better to have an explicit tool interface version > given in valgrind.pc? How would the external tool be distributed? If binary, it'll be compiled with some version of the core, so no problem there. If source, it'll presumably be copied into an existing valgrind source tree to be compiled. In that case, if the version is right it'll compile ok. If not, it'll fail to compile. I guess there's a possible middle case where the meaning of something has changed, but in a way that doesn't affect compilation. It seems pretty unlikely, however. Also, noticing when the interface version number needs updating is a problem... in between official releases, there could be a large number of interfaces due to lots of minor changes. So I'm inclined to use the Valgrind release version as the only number of note. N |
|
From: Josef W. <Jos...@gm...> - 2009-02-27 17:55:44
|
On Friday 27 February 2009, Nicholas Nethercote wrote: > On Fri, Feb 27, 2009 at 9:13 AM, Josef Weidendorfer > <Jos...@gm...> wrote: > > On Thursday 26 February 2009, Nicholas Nethercote wrote: > > > > An externally distributed tool still would need to get an version > > number of the tool interface used in the static VG libraries, > > at build time. Of course, it just can > > look at the Valgrind release version (I assume with pkgconfig?). > > Still, would it be better to have an explicit tool interface version > > given in valgrind.pc? > > How would the external tool be distributed? > > If binary, it'll be compiled with some version of the core, so no problem there. Sure. > If source, it'll presumably be copied into an existing valgrind source > tree to be compiled. IMHO the preferable way is to be distributed as "exp-" tool with a VG release, as then, any needed patches to VG core for the tool are already merged. The second preferable way would be for the external tool to not need any VG core changes, and be compilable with the headers/libraries installed from a regular VG installation. It should be the last option for an external tool to be copied into an existing VG source for compilation, as it needs patching Makefile.am/configure.in. Consequently, the tool probably will provide a patch for this. More problematic is that this way, any needed changes to VG core by the tool are easily distributed in this patch, and there is no motivation/reason at all for the tool author to try to push the core changes upstream. I can only assume that because of this we already missed quite some potentially interesting VG core patches, especially if the patches do not even show up on the VG mailing list. Any idea how to improve this situation? > In that case, if the version is right it'll > compile ok. If not, it'll fail to compile. I guess there's a > possible middle case where the meaning of something has changed, but > in a way that doesn't affect compilation. It seems pretty unlikely, > however. Also, noticing when the interface version number needs > updating is a problem... in between official releases, there could be > a large number of interfaces due to lots of minor changes. > > So I'm inclined to use the Valgrind release version as the only number of note. I am fine with this. Especially, as we experienced that incrementing the tool interface version on changes is easy to forget. Josef > > N > |