From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 15850 invoked by alias); 25 Feb 2013 22:44:14 -0000 Received: (qmail 15836 invoked by uid 22791); 25 Feb 2013 22:44:13 -0000 X-SWARE-Spam-Status: No, hits=-5.2 required=5.0 tests=AWL,BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,KHOP_RCVD_TRUST,KHOP_THREADED,RCVD_IN_DNSWL_LOW,RCVD_IN_HOSTKARMA_YE,RP_MATCHES_RCVD X-Spam-Check-By: sourceware.org Received: from mail-we0-f176.google.com (HELO mail-we0-f176.google.com) (74.125.82.176) by sourceware.org (qpsmtpd/0.43rc1) with ESMTP; Mon, 25 Feb 2013 22:44:05 +0000 Received: by mail-we0-f176.google.com with SMTP id s43so2872489wey.7 for ; Mon, 25 Feb 2013 14:44:03 -0800 (PST) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=20120113; h=x-received:mime-version:in-reply-to:references:from:date:message-id :subject:to:cc:content-type:x-gm-message-state; bh=XnU4AZ5nIdU3GDOo3UHQusJqsd+jyEIChi7T7v9nRAI=; b=g3scQmX/bvhdknK5hn4SOQxjpTlZgJfOlQNBL4EpZkDoisXHQaIHVYxTHMBWZdOjPI zWMb0WJ/5JxDS9gTfPUDtKhYY/Id6/ns9LzxNjCQ/e0A22KO8HHlN6RLi+5KGHxyN+RX NoZcUV2UJU/cSMlpnrv2FhFk6JD8HUQgMuXSZ807tARpQQQ8+FkkKCYhbuz4ZlC49tAh dbjz1TlCD+hY1xIHmLO6tXwg7kWBEJvs0F3MNvQE9xjIOyRGq1xf8FgXN/dPY9KqeG26 h/HBh55uGqDq4DbCc/OV6aKLmvlVwBrhgCfNd+WI/e1CPv8bf6rl0TNA9I38W9vFu++I VMcw== X-Received: by 10.194.120.169 with SMTP id ld9mr22355324wjb.24.1361832243527; Mon, 25 Feb 2013 14:44:03 -0800 (PST) MIME-Version: 1.0 Received: by 10.194.151.232 with HTTP; Mon, 25 Feb 2013 14:43:43 -0800 (PST) In-Reply-To: References: From: Caroline Tice Date: Mon, 25 Feb 2013 22:44:00 -0000 Message-ID: Subject: Re: [PATCH, updated] Vtable pointer verification, runtime library changes (patch 3 of 3) To: Jonathan Wakely Cc: GCC Patches , libstdc++@gcc.gnu.org, Diego Novillo , Luis Lozano , Bhaskar Janakiraman Content-Type: text/plain; charset=ISO-8859-1 X-Gm-Message-State: ALoCoQnzqQxfOtSXFdI5wcpJC1u3ynr1ePziC4PxuXwCZLJF+dVrB+87Vlnh4vS17gtMssR0kYbPO24J6nEWOAS7rDUjEG4ZUfQbgHnK7STLDVDX2ld4esoZT6/L1ztDiOZXCY5PyiAUcYyF0TP76XdpaW9HZ9gQ281jgzckSk+KdQZWzMARfeoeYg1RGacgqxUKPYtRRQew X-IsSubscribed: yes Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org X-SW-Source: 2013-02/txt/msg01133.txt.bz2 On Mon, Feb 25, 2013 at 1:14 PM, Jonathan Wakely 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.