Re: [PATCH, updated] Vtable pointer verification, runtime library changes (patch 3 of 3)

[Dave Korn <dave.korn.cygwin@gmail.com>]

On 08/03/2013 00:11, Jonathan Wakely wrote:
> On 7 March 2013 23:53, Caroline Tice wrote:

>> I believe this patch addresses all of your comments; I modified the
>> configure.ac files to generate the configures, and I fixed the
>> spelling mistakes in the comments.  I still get the warnings when
>> generating the Makefile.in files from the Makefile.am files, but the
>> resulting files seem to be correct, and I don't know how to make the
>> warnings go away.
>>
>> Please review this patch and let me know if it will be ok to commit
>> when GCC opens up again.
> 
> I'd like to know if someone with better automake skills than I have
> can do anything about that warning, but otherwise that looks OK to me,
> thanks.

  Looks like libvtv___la_LIBADD is still being defined in the else clause.
I'm also getting warnings for src/c++11/Makefile.am about EXTRA_VTV_LDFLAGS,
which may need renaming to avoid confusing automake:

src/c++11/Makefile.am:70: variable `EXTRA_VTV_LDFLAGS' is defined but no
program or
src/c++11/Makefile.am:70: library has `EXTRA_VTV' as canonical name (possible
typo)
src/c++98/Makefile.am:152: variable `EXTRA_VTV_LDFLAGS' is defined but no
program or
src/c++98/Makefile.am:152: library has `EXTRA_VTV' as canonical name (possible
typo)

  Also, it should generally contain only .lo and .la object and libraries to
link against.  From the automake manual:

> 8.3.7 _LIBADD, _LDFLAGS, and _LIBTOOLFLAGS
> As shown in previous sections, the ‘library_LIBADD’ variable should be used
> to list extra libtool objects (‘.lo’ files) or libtool libraries (‘.la’)
> to add to library. The ‘library_LDFLAGS’ variable is the place to list
> additional libtool linking flags, such as ‘-version-info’, ‘-static’, and a
> lot more.

  So putting a dir path into LIBADD and then concatenating -L and including it
wholesale in some other flag variable seems incorrect.

  Also, both that and the top-level change looks dubious to me.  The libtool
subdir isn't necessarily called '.libs' on all platforms, and you should never
need to reach into it(*).  Likewise LIBVTV_FLAGS.  Instead of adding
-lvtv_init or -lvtv_stubs to the linker flags, I think you're supposed to add
the corresponding .la files to the relevant LIBADD variable.  And w.r.t. the
top-level change, I think it may turn out to be unneeded altogether once the
libtool stuff is correct.

  It also looks odd to me that linker flags are being added into a CXXFLAGS
variable (EXTRA_VTV_LDFLAGS via VTV_CXXFLAGS) rather than the corresponding
LDFLAGS.

  I'm not a libtool expert, I've just been told to avoid similar dubious
practices in patches I've submitted in the past.

  And one minor nit, it is conventional not to include the generated files
(Makefile.in, configure) in patches.  They should autogenerate exactly the
same everywhere and it saves clutter.

    cheers,
      DaveK
-- 
(*) - I see a couple of cases have slipped through already, I think we should
avoid adding any more.