On Mon, Feb 25, 2013 at 1:14 PM, Jonathan Wakely <jwakely....@gmail.com> wrote: > On 25 February 2013 19:52, Caroline Tice wrote: >> I got too excited about being done and forgot to attach the patch. :-( >> Sorry. Here it is. > > Some comments follow, mostly from reading the comments to understand > what this patch does, it's a really interesting feature! > > The generated files (configure, */Makefile.in) don't need to be in the > patch and their ChangeLog entry can be simply "Regenerated" > > Was configure regenerated of modified by hand? When regenerating it > with Autoconf 2.64 I get a different output. >
I modified the configure file by hand. I didn't realize it was auto-generated. :-( > I also get this warning when regenerating the configury bits: > src/Makefile.am:80: variable `libvtv___la_LIBADD' is defined but no program or > src/Makefile.am:80: library has `libvtv___la' as canonical name (possible > typo) I was trying to define them as empty (this was in the else clause, where "--enable-vtable-verify=yes" was not specified when doing the original configure. I should probably remove libvtv___la_LIBADD from the else clause altogether. > > The copyright dates should be updated to 2012-2013. > Ok. > vtv_add_to_log in libstdc++-v3/libsupc++/vtv_utils.cc uses va_start > but the matching call to va_end is missing. > Ok. > Am I right in thinking there's no danger of namespace pollution from > vtv_map.h etc. because those headers will never be included unless > explicitly requested by users? That should be correct. > > I'm not sure what the rules are regarding flexible array members in > C++ (as it's an extension) but it looks like insert_only_hash_map is a > non-POD (formally, it has non-trivial initialization) but no > constructor or destructor runs for it, so formally the object's > lifetime never begins or ends, it is just a block of memory that gets > allocated, some bytes are set, then the memory is deallocated again. > I will go back and look at this again. It's been a while since I looked at that code. > The comments in libstdc++-v3/libsupc++/vtv_init.cc have a typo: > > +/* This file contains all the definitions that go into the libvtv_init > + library, which is part of the vtable verification feature. This > + library should contain exactly two functionsa (__VLTunprotect and > s/functionsa/functions/ > I'll fix that. > And in the comments in libstdc++-v3/libsupc++/vtv_rts.cc > > + The actual set of valid vtable pointers for a virtual class, > Should that be "polymorphic class" instead of "virtual class"? > Ok. > Further on there are a few typos: > > + pointters for the class, so we wrote our own hashtable-based symbol > s/pointters/pointers/ > > + libvtv_init.so is built from vtv_init.cc. It is designed to hel[p > s/hel\[p/help/ > > > + __VLTVerifyVtablePoitner) with stub functions that do nothing. If > s/Poitner/Pointer/ > > + initialize any of these statics with a runtime call (for ex: > + sysconf. > (Unclosed parenthesis) > > + the secttion offset and size, in conjunction with the data in INFO > s/secttion/section/ > > + /* TODO: Meed to revisit this code for dlopen. It most probably > + is not unlocking the protected vtable vars after for a load > > s/Meed/Need/ > s/after for a load/after a load/ > > I see a few TODO comments in the code, I assume the plan is to address > them eventually as time permits, rather than this being a code-drop > that becomes abandonware :-) Yes. They are things that we are planning on addressing but did not seem worth holding up the patch for.
