Re: [Valgrind-developers] Bug 73655 fix

[Robert W. <rj...@du...>]

> I tried it;  that test case works on my, er, RH 9 box too.

Yea!  Now we know it's safe :-)

> What about the memcheck/tests/new_override.cpp test?  The expected output
> in the .exp file is that Valgrind's 'new' takes precedence, which is
> actually wrong here.  With your patch, I'm still getting V's new taking
> precedence.

The test is borked.  new Test[2] doesn't ever call the operator new
defined in that file because it's looking for operator new[] instead. 
I'll fix the test in the patch and make a new release of the patch later
tonight.

> Also, you're now generating vg_intercept.c and vg_replace_malloc.c, from
> files with .vgi extensions... we already have similar files with .base
> extensions (vg_skin.h.base)... should be consistent here.

Yes.  I'll do that, too.

> Finally, how does your patch actually work?  What did you change?

First, the problem: the problem was that Valgrind was always
intercepting operator new, even when your program defined it's own
version of it.  This is why new_override.cpp would never have worked
even if it had been overriding operator new[].

This was happening because the preloaded vg_inject.so had an exported
operator new symbol.  This was to work around the following issue:
vg_inject.so used an init segment to set up the intercept table. 
However, there was no guarantee that vg_inject.so would have it's init
segment run before any other shared object.  This meant that a shared
object that had an init section that called operator new might actually
have ended up using the glibc version instead of the Valgrind one if the
intercept table hadn't been set up yet, causing all sorts of confusion. 
However, exporting the operator new function in vg_inject.so worked
around this issue by ensuring that the Valgrind one was always used.  Of
course, this meant that the Valgrind one was always used, even if your
program defined it's own version.

The solution was to get rid of any symbol called "operator new" (or,
actually, _Znwj) from the vg_inject.so file.  This meant that a
program-defined operator new got resolved properly.  However, we needed
some way of setting up the intercept table before hand.  Jeremy and I
talked about this for a few hours and decided to go with the solution I
adopted.  We now mangle the symbols in vg_inject.so so that they can be
demangled to indicate exactly what library and function they are
overriding.  When a .so file is loaded, we slurp in the symbol table,
pick out those symbols that match the mangled syntax we defined,
demangle the symbol into a library name/function name pair and set up
the table that way.

In order to make the whole thing a bit more palatable, we use a
pre-processor to turn a human-readable version of the file (the .vgi
files) into the mangled version.

The above also goes for all other overridden symbols, not just operator
new.

Does that make sense?

Thanks for the feedback.

Regards,
 Robert.