Re: [PATCH v2] LoongArch: Libvtv add loongarch support.
On Tue, Oct 11, 2022 at 7:52 PM Lulu Cheng wrote:
>
> 在 2022/10/12 上午4:57, Caroline Tice 写道:
>
> I think that if VTV_PAGE_SIZE is not set to the actual size being used by
> the system, it could result in some unexpected failures. I believe the
> right thing to do in this case, since the size may vary, is to get the
> actual size being used by the system and use that in the definition of
> VTV_PAGE_SIZE. So in include/vtv-permission.h you would have something
> like:
>
> +#elif defined(__loongarch_lp64)
> +#define VTV_PAGE_SIZE sysconf(_SC_PAGE_SIZE)
>
> Then you would have the accurate, correct size for the current system, and
> there would be no need to update the
> check in vtv_malloc.cc at all.
>
>
> /* Page-aligned symbol to mark beginning of .vtable_map_vars section.
> */
> char _vtable_map_vars_start []
> __attribute__ ((__visibility__ ("protected"), used,
> aligned(VTV_PAGE_SIZE),
> section(".vtable_map_vars")))
> = { };
>
> The above code is in the libgcc/vtv_start.c file. Alignment (aligned
> (alignment) ) must be an
> integer constant power of 2. So setting VTV_PAGE_SIZE as a variable is not
> advisable.
>
>
> As xiruoyao notes, the default value for the LoongArch Linux kernel
> configuration is 16KB.
>
> So let's set VTV_PAGE_SIZE to 16KB first and I will indicate in the
> submission information
>
> that only 16KB pages are supported.
>
>
> OK, that would work if you want to go that way.
Re: [PATCH v2] LoongArch: Libvtv add loongarch support.
I think that if VTV_PAGE_SIZE is not set to the actual size being used by
the system, it could result in some unexpected failures. I believe the
right thing to do in this case, since the size may vary, is to get the
actual size being used by the system and use that in the definition of
VTV_PAGE_SIZE. So in include/vtv-permission.h you would have something
like:
+#elif defined(__loongarch_lp64)
+#define VTV_PAGE_SIZE sysconf(_SC_PAGE_SIZE)
Then you would have the accurate, correct size for the current system, and
there would be no need to update the
check in vtv_malloc.cc at all.
-- Caroline Tice
cmt...@google.com
On Tue, Sep 27, 2022 at 3:04 AM Lulu Cheng wrote:
>
> v1 - > v2:
>
> 1. When the macro __loongarch_lp64 is defined, the VTV_PAGE_SIZE is set to
> 64K.
> 2. In the vtv_malloc.cc file __vtv_malloc_init function, it does not check
>whether VTV_PAGE_SIZE is equal to the system page size, if the macro
>__loongarch_lp64 is defined.
>
> All regression tests of libvtv passed.
>
> === libvtv Summary ===
>
> # of expected passes176
>
> But I haven't tested the performance yet.
>
> ---
> Co-Authored-By: qijingwen
>
> include/ChangeLog:
>
> * vtv-change-permission.h (defined):
> (VTV_PAGE_SIZE): Under the loongarch64 architecture,
> set VTV_PAGE_SIZE to 64K.
>
> libvtv/ChangeLog:
>
> * configure.tgt: Add loongarch support.
> * vtv_malloc.cc (defined): If macro __loongarch_lp64 is
> defined, then don't check whether VTV_PAGE_SIZE is the
> same as the system page size.
> ---
> include/vtv-change-permission.h | 4
> libvtv/configure.tgt| 3 +++
> libvtv/vtv_malloc.cc| 5 +
> 3 files changed, 12 insertions(+)
>
> diff --git a/include/vtv-change-permission.h
> b/include/vtv-change-permission.h
> index 70bdad92bca..64e419c29d5 100644
> --- a/include/vtv-change-permission.h
> +++ b/include/vtv-change-permission.h
> @@ -48,6 +48,10 @@ extern void __VLTChangePermission (int);
> #else
> #if defined(__sun__) && defined(__svr4__) && defined(__sparc__)
> #define VTV_PAGE_SIZE 8192
> +/* LoongArch architecture 64-bit system supports 4k,16k and 64k
> + page size, which is set to the maximum value here. */
> +#elif defined(__loongarch_lp64)
> +#define VTV_PAGE_SIZE 65536
> #else
> #define VTV_PAGE_SIZE 4096
> #endif
> diff --git a/libvtv/configure.tgt b/libvtv/configure.tgt
> index aa2a3f675b8..6cdd1e97ab1 100644
> --- a/libvtv/configure.tgt
> +++ b/libvtv/configure.tgt
> @@ -50,6 +50,9 @@ case "${target}" in
> ;;
>x86_64-*-darwin[1]* | i?86-*-darwin[1]*)
> ;;
> + loongarch*-*-linux*)
> + VTV_SUPPORTED=yes
> + ;;
>*)
> ;;
> esac
> diff --git a/libvtv/vtv_malloc.cc b/libvtv/vtv_malloc.cc
> index 67c5de6d4e9..45804b8d7f8 100644
> --- a/libvtv/vtv_malloc.cc
> +++ b/libvtv/vtv_malloc.cc
> @@ -212,6 +212,11 @@ __vtv_malloc_init (void)
>
> #if defined (__CYGWIN__) || defined (__MINGW32__)
>if (VTV_PAGE_SIZE != sysconf_SC_PAGE_SIZE())
> +#elif defined (__loongarch_lp64)
> + /* I think that under the LoongArch 64-bit system, VTV_PAGE_SIZE is set
> + to the maximum value of 64K supported by the system, so there is no
> + need to judge here. */
> + if (false)
> #else
>if (VTV_PAGE_SIZE != sysconf (_SC_PAGE_SIZE))
> #endif
> --
> 2.31.1
>
>
Re: [PATCH v2] LoongArch: Libvtv add loongarch support.
On Tue, Sep 27, 2022 at 3:04 AM Lulu Cheng wrote:
>
> v1 - > v2:
>
> 1. When the macro __loongarch_lp64 is defined, the VTV_PAGE_SIZE is set to
> 64K.
> 2. In the vtv_malloc.cc file __vtv_malloc_init function, it does not check
>whether VTV_PAGE_SIZE is equal to the system page size, if the macro
>__loongarch_lp64 is defined.
>
> All regression tests of libvtv passed.
>
> === libvtv Summary ===
>
> # of expected passes176
>
> But I haven't tested the performance yet.
>
> ---
> Co-Authored-By: qijingwen
>
> include/ChangeLog:
>
> * vtv-change-permission.h (defined):
> (VTV_PAGE_SIZE): Under the loongarch64 architecture,
> set VTV_PAGE_SIZE to 64K.
>
> libvtv/ChangeLog:
>
> * configure.tgt: Add loongarch support.
> * vtv_malloc.cc (defined): If macro __loongarch_lp64 is
> defined, then don't check whether VTV_PAGE_SIZE is the
> same as the system page size.
> ---
> include/vtv-change-permission.h | 4
> libvtv/configure.tgt| 3 +++
> libvtv/vtv_malloc.cc| 5 +
> 3 files changed, 12 insertions(+)
>
> diff --git a/include/vtv-change-permission.h
> b/include/vtv-change-permission.h
> index 70bdad92bca..64e419c29d5 100644
> --- a/include/vtv-change-permission.h
> +++ b/include/vtv-change-permission.h
> @@ -48,6 +48,10 @@ extern void __VLTChangePermission (int);
> #else
> #if defined(__sun__) && defined(__svr4__) && defined(__sparc__)
> #define VTV_PAGE_SIZE 8192
> +/* LoongArch architecture 64-bit system supports 4k,16k and 64k
> + page size, which is set to the maximum value here. */
> +#elif defined(__loongarch_lp64)
> +#define VTV_PAGE_SIZE 65536
> #else
> #define VTV_PAGE_SIZE 4096
> #endif
> diff --git a/libvtv/configure.tgt b/libvtv/configure.tgt
> index aa2a3f675b8..6cdd1e97ab1 100644
> --- a/libvtv/configure.tgt
> +++ b/libvtv/configure.tgt
> @@ -50,6 +50,9 @@ case "${target}" in
> ;;
>x86_64-*-darwin[1]* | i?86-*-darwin[1]*)
> ;;
> + loongarch*-*-linux*)
> + VTV_SUPPORTED=yes
> + ;;
>*)
> ;;
> esac
> diff --git a/libvtv/vtv_malloc.cc b/libvtv/vtv_malloc.cc
> index 67c5de6d4e9..45804b8d7f8 100644
> --- a/libvtv/vtv_malloc.cc
> +++ b/libvtv/vtv_malloc.cc
> @@ -212,6 +212,11 @@ __vtv_malloc_init (void)
>
> #if defined (__CYGWIN__) || defined (__MINGW32__)
>if (VTV_PAGE_SIZE != sysconf_SC_PAGE_SIZE())
> +#elif defined (__loongarch_lp64)
> + /* I think that under the LoongArch 64-bit system, VTV_PAGE_SIZE is set
> + to the maximum value of 64K supported by the system, so there is no
> + need to judge here. */
> + if (false)
> #else
>if (VTV_PAGE_SIZE != sysconf (_SC_PAGE_SIZE))
>
Is "if (VTV_PAGE_SIZE != sysconf (_SC_PAGE_SIZE))" going to fail for
loongarch? If not, why do you need to insert anything here at all? If so,
perhaps you could write something similar to sysconf_SC_PAGE_SIZE for
loongarch (as was done for __CYGWIN__ & __MINGW32__)?
-- Caroline
cmt...@google.com
> #endif
> --
> 2.31.1
>
>
Re: [PATCH v1] libstdc++-v3: Update VTV vars for libtool link commands [PR99172]
I have updated the patch as you suggested, to filter the "-fvtable-verify=std" out of AM_CXXFLAGS, and I have verified that it passes the testsuite with no regressions and fixes the reported bug. Is this OK to commit now? -- Caroline Tice cmt...@google.com libstdc++-v3/ChangeLog 2021-03-12 Caroline Tice PR libstdc++/99172 * src/Makefile.am (AM_CXXFLAGS_PRE, AM_CXXFLAGS): Add AM_CXXFLAGS_PRE with the old definition of AM_CXXFLAGS; make AM_CXXFLAGS to be AM_CXXFLAGS_PRE with '-fvtable-verify=std' filtered out. * src/Makefile.in: Regenerate. -- Caroline cmt...@google.com On Thu, Mar 11, 2021 at 9:10 AM Jonathan Wakely wrote: > > On 11/03/21 17:46 +0100, Jakub Jelinek via Libstdc++ wrote: > >On Thu, Mar 11, 2021 at 04:31:51PM +, Jonathan Wakely via Gcc-patches > >wrote: > >> On 11/03/21 16:27 +, Jonathan Wakely wrote: > >> > That seems cleaner to me, rather than adding another variable with > >> > minor differences from the existing AM_CXXFLAGS. > >> > >> My specific concern is that AM_CXXFLAGS and AM_CXXFLAGS_LT will get > >> out of sync, i.e. we'll add something to the former and forget to add > >> it to the latter. > >> > >> If we keep using AM_CXXFLAGS but cancel out the -fvtable-verify=std > >> option, then there aren't two separate variables that can diverge. > >> > >> But I think it's too late in the gcc-11 process for that kind of > >> refactoring. > > > >I think $(filter-out -fvtable-verify=std,$(AM_CXXFLAGS)) should be fairly > >simple thing if that is all that needs to be done. > > Yes, we could do that now, and then in stage 1 look at the other > changes (like moving -Wl,-u options to the link flags not cxxflags). > > Using filter-out does assume that no target is going to add anything > different that should also be filtered out, but that's true as of > today. > v2-0001-libstdc-v3-Update-VTV-vars-for-libtool-link-comma.patch Description: Binary data
Re: [PATCH v1] libstdc++-v3: Update VTV vars for libtool link commands [PR99172]
Adding the libstdc++ mailing list to the patch. -- Caroline cmt...@google.com On Wed, Mar 10, 2021 at 8:50 PM Caroline Tice wrote: > > This patch is to fix PR 99172. > > Currently when GCC is configured with --enable-vtable-verify, the > libstdc++-v3 Makefiles add "-fvtable-verify=std > -Wl,-u_vtable_map_vars_start,-u_vtable_map_vars_end" to libtool link > commands. The "-fvtable-verify=std" piece causes alternate versions of > libtool (such as slibtool) to fail, unable to find "-lvtv" (GNU > libtool just removes that piece). > > This patch updates the libstdc++-v3 Makefiles to not pass > "-fvtable-verify=std" to the libtool link commands, while continuing > to pass the rest of the VTV flags (which are necessary for VTV to > work). > > I tested this by configuring with --enable-vtable-verify, boostrapping > the compiler, and running all the regression testsuites (including > libvtv & libstdc++) without any regressions. I only ran it on a linux > system, on an x86_64 machine. > > I also gave a copy of the patch to the person who reported the bug, > and they verified that the patch fixes their issue. > > Is this ok to commit? > > -- Caroline Tice > cmt...@google.com > > libstdc++-v3/ChangeLog > > 2021-03-10 Caroline Tice > > PR libstdc++/99172 > * Makefile.in: Regenerate. > * acinclude.m4: Add definitions for VTV_CXXFLAGS_LT. > * configure: Regenerate. > * doc/Makefile.in: Regenerate. > * include/Maefile.in: Regenerate. > * libsupc++/Makefile.in: Regenerate. > * po/Makefile.in: Regenerate. > * python/Makefile.in: Regenerate. > * src/Makefile.am (AM_CXXFLAGS_LT): New definition. > (CXXLINK): Update to use AM_CXXFLAGS_LT instead of AM_CXXFLAGS. > * src/Makefile.in: Regenerate. > * src/c++11/Makefile.in: Regenerate. > * src/c++17/Makefile.in: Regenerate. > * src/c++20/Makefile.in: Regenerate. > * src/c++98/Makefile.in: Regenerate. > * src/filesystem/Makefile.in: Regenerate. > * testsuite/Makefile.in: Regenerate.
[PATCH v1] libstdc++-v3: Update VTV vars for libtool link commands [PR99172]
This patch is to fix PR 99172. Currently when GCC is configured with --enable-vtable-verify, the libstdc++-v3 Makefiles add "-fvtable-verify=std -Wl,-u_vtable_map_vars_start,-u_vtable_map_vars_end" to libtool link commands. The "-fvtable-verify=std" piece causes alternate versions of libtool (such as slibtool) to fail, unable to find "-lvtv" (GNU libtool just removes that piece). This patch updates the libstdc++-v3 Makefiles to not pass "-fvtable-verify=std" to the libtool link commands, while continuing to pass the rest of the VTV flags (which are necessary for VTV to work). I tested this by configuring with --enable-vtable-verify, boostrapping the compiler, and running all the regression testsuites (including libvtv & libstdc++) without any regressions. I only ran it on a linux system, on an x86_64 machine. I also gave a copy of the patch to the person who reported the bug, and they verified that the patch fixes their issue. Is this ok to commit? -- Caroline Tice cmt...@google.com libstdc++-v3/ChangeLog 2021-03-10 Caroline Tice PR libstdc++/99172 * Makefile.in: Regenerate. * acinclude.m4: Add definitions for VTV_CXXFLAGS_LT. * configure: Regenerate. * doc/Makefile.in: Regenerate. * include/Maefile.in: Regenerate. * libsupc++/Makefile.in: Regenerate. * po/Makefile.in: Regenerate. * python/Makefile.in: Regenerate. * src/Makefile.am (AM_CXXFLAGS_LT): New definition. (CXXLINK): Update to use AM_CXXFLAGS_LT instead of AM_CXXFLAGS. * src/Makefile.in: Regenerate. * src/c++11/Makefile.in: Regenerate. * src/c++17/Makefile.in: Regenerate. * src/c++20/Makefile.in: Regenerate. * src/c++98/Makefile.in: Regenerate. * src/filesystem/Makefile.in: Regenerate. * testsuite/Makefile.in: Regenerate. v1-0001-libstdc-v3-Update-VTV-vars-for-libtool-link-comma.patch Description: Binary data
[PATCH v1] [include] Add codes for DWARF v5 .dwp sections to dwarf2.h
For DWARF v5 Dwarf Package Files (.dwp files), the section identifier encodings have changed. This patch updates dwarf2.h to contain the new encodings. (see http://dwarfstd.org/doc/DWARF5.pdf, section 7.3.5). This patch has already been committed in binutils, but it needs to go into GCC as well to avoid the binutils patch being overwritten/lost. I tested this by running the regression testsuite; there were no regressions. Is this ok to commit? -- Caroline Tice cmt...@google.com include/ChangeLog 2020-09-09 Caroline Tice * dwarf2.h (enum dwarf_sect_v5): A new enum section for the sections in a DWARF 5 DWP file (DWP version 5). v1-0001-Add-codes-for-DWARF-v5-.dwp-sections-to-dwarf2.h.gcc.patch Description: Binary data
[PATCH] Fix testcase to not use vtable verification with LTO
Yesterday I submitted a patch that disallows using vtable verfication
with LTO (they don't work
properly together), but I missed fixing the flags for one testcase.
This patch fixes that omission.
Testing: Tescase passes with this change.
Is this ok to commit?
-- Caroline
cmt...@google.com
ChangeLog entry (gcc/testsuite/ChangeLog):
2019-09-05 Caroline Tice
PR testsuite/91670
* g++.dg/ubsan/pr59415.C: Disable LTO, since test uses
-fvtable-verify, and the two options are no longer allowed
together.
Index: gcc/testsuite/g++.dg/ubsan/pr59415.C
===
--- gcc/testsuite/g++.dg/ubsan/pr59415.C (revision 275387)
+++ gcc/testsuite/g++.dg/ubsan/pr59415.C (working copy)
@@ -1,5 +1,5 @@
/* { dg-do compile } */
-/* { dg-options "-fsanitize=null -Wall -fvtable-verify=std" } */
+/* { dg-options "-fsanitize=null -Wall -fvtable-verify=std -fno-lto" } */
void
foo (void)
[PATCH] Disable vtable verification with LTO
Vtable verification currently does not work properly with LTO. Fixing
this will be non-trivial, and I currently do not have the time do
this. Until this can be fixed, we should not allow users to specify
both vtable verification and link-time optimization. The attached
patch checks to see if both options were specified and if so, tells
the user that this is not supported.
I have tested this by compiling hello world with each option
separately and then trying to specify them both; it behaves as
expected.
Is this patch OK to commit?
-- Caroline Tice
cmt...@google.com
ChangeLog entry:
2019-09-04 Caroline Tice
* opts.c (finish_options): Disallow -fvtable-verify and -flto to be
specified together;
Index: gcc/opts.c
===
--- gcc/opts.c (revision 275387)
+++ gcc/opts.c (working copy)
@@ -1226,6 +1226,10 @@
if (opts->x_flag_live_patching && opts->x_flag_lto)
sorry ("live patching is not supported with LTO");
+ /* Currently vtable verification is not supported for LTO */
+ if (opts->x_flag_vtable_verify && opts->x_flag_lto)
+sorry ("vtable verification is not supported with LTO");
+
/* Control IPA optimizations based on different -flive-patching level. */
if (opts->x_flag_live_patching)
{
Re: [PATCH] PR other/91396 Fix static link error with -fvtable-verify
The bootstrap succeeded.
On Mon, Aug 12, 2019 at 11:51 AM Caroline Tice wrote:
>
> Hi,
>
> This patch is to fix a bug where linking with -fvtable-verify and
> -static causes the linker to complain about multiple definitions of
> things in the vtv_end*.o files (once from the .o file and once from
> libvtv.a). (See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91396).
> The fix is for the GNU_USER_TARGET_ENDFILE_SPEC to only include the
> vtv_end*.o files if we're not doing static linking (and we are using
> -fvtable-verify flags).
>
> Tested this fix by verifying that the test case in the bug compiles
> both with and without -static, and that there were no new regressions
> in the libvtv testsuite. I'm in the process of doing a full bootstrap
> build with this change. Assuming the bootstrap passes, is this ok to
> commit?
>
> Index: gcc/config/gnu-user.h
> ===
> --- gcc/config/gnu-user.h (revision 274317)
> +++ gcc/config/gnu-user.h (working copy)
> @@ -73,9 +73,9 @@
> GNU userspace "finalizer" file, `crtn.o'. */
>
> #define GNU_USER_TARGET_ENDFILE_SPEC \
> - "%{fvtable-verify=none:%s; \
> + "%{!static:%{fvtable-verify=none:%s; \
> fvtable-verify=preinit:vtv_end_preinit.o%s; \
> - fvtable-verify=std:vtv_end.o%s} \
> + fvtable-verify=std:vtv_end.o%s}} \
> %{static:crtend.o%s; \
> shared|static-pie|" PIE_SPEC ":crtendS.o%s; \
> :crtend.o%s} " \
>
> ChangeLog entry:
>
> 2019-08-12 Caroline Tice
>
> PR other/91396
> * config/gnu-user.h (GNU_USER_TARGET_ENDFILE_SPEC): Only add the
> vtv_end.o or vtv_end_preinit.o files if !static.
[PATCH] PR other/91396 Fix static link error with -fvtable-verify
Hi,
This patch is to fix a bug where linking with -fvtable-verify and
-static causes the linker to complain about multiple definitions of
things in the vtv_end*.o files (once from the .o file and once from
libvtv.a). (See https://gcc.gnu.org/bugzilla/show_bug.cgi?id=91396).
The fix is for the GNU_USER_TARGET_ENDFILE_SPEC to only include the
vtv_end*.o files if we're not doing static linking (and we are using
-fvtable-verify flags).
Tested this fix by verifying that the test case in the bug compiles
both with and without -static, and that there were no new regressions
in the libvtv testsuite. I'm in the process of doing a full bootstrap
build with this change. Assuming the bootstrap passes, is this ok to
commit?
Index: gcc/config/gnu-user.h
===
--- gcc/config/gnu-user.h (revision 274317)
+++ gcc/config/gnu-user.h (working copy)
@@ -73,9 +73,9 @@
GNU userspace "finalizer" file, `crtn.o'. */
#define GNU_USER_TARGET_ENDFILE_SPEC \
- "%{fvtable-verify=none:%s; \
+ "%{!static:%{fvtable-verify=none:%s; \
fvtable-verify=preinit:vtv_end_preinit.o%s; \
- fvtable-verify=std:vtv_end.o%s} \
+ fvtable-verify=std:vtv_end.o%s}} \
%{static:crtend.o%s; \
shared|static-pie|" PIE_SPEC ":crtendS.o%s; \
:crtend.o%s} " \
ChangeLog entry:
2019-08-12 Caroline Tice
PR other/91396
* config/gnu-user.h (GNU_USER_TARGET_ENDFILE_SPEC): Only add the
vtv_end.o or vtv_end_preinit.o files if !static.
Re: [PATCH][C++] Fix ICE with VTV
I have managed to reproduce the issue now, and the patch does appear to fix the ICE. There is a second issue, which will need more investigation: When compiled with '-flto' the extra internal functions that VTV generates, and which are necessary for it to work correctly, do not get propagated into the final binary. You can see this compiling the bb_tests.cc test case in the libvtv testsuite, and grepping for 'GLOBAL' in the final output: $ /usr/local/google3/cmtice/gcc-fsf.root/usr/local/bin/g++ -o bb_tests bb_tests.cc -O1 -fvtable-verify=std // Compile without flto $ nm bb_tests | grep GLOBAL 00601000 d _GLOBAL_OFFSET_TABLE_ 00400930 t _GLOBAL__sub_I.00099__Z14get_cond_value $ /usr/local/google3/cmtice/gcc-fsf.root/usr/local/bin/g++ -o bb_tests_flto bb_tests.cc -O1 -flto -fvtable-verify=std // Compile with flto $ nm bb_tests_flto | grep GLOBAL 00601000 d _GLOBAL_OFFSET_TABLE_ But as said, that's a separate issue, which I will need to investigate (if anyone has any suggestions as to the proper way to propagate the functions through -flto, I would love to hear them). I approve Richard's patch for fixing the ICE. -- Caroline Tice cmt...@google.com On Wed, Feb 20, 2019 at 6:30 AM Richard Biener wrote: > > On Tue, 19 Feb 2019, Caroline Tice wrote: > > > On Tue, Feb 19, 2019 at 1:57 AM Richard Biener wrote: > > > > > > > > Looks like vtv_generate_init_routine calls symtab->process_new_functions > > > () which has seriously bad effects on GCCs internal memory state. > > > > > > VTV has zero testsuite coverage it seems and besides its initial > > > commit didn't receive any love. > > > > > > > I am puzzled by these statements, as neither of them is true. VTV does > > have a testsuite in the libvtv directory. Naturally you can only run them > > when vtv is enabled and only from the libvtv build tree, as they will all > > fail if VTV is not enabled. I have fixed various bugs in VTV since the > > initial commit, and I have also approved several patches, especially for > > people migrating it to other architectures, suggesting that interest in it > > is not zero. > > > > > > > > > > > > Can we rip it out please? > > > > > > Is the patch OK? (Pointless) bootstrap and regtest running on > > > x86_64-unknown-linux-gnu. > > > > > > Has anybody "recently" tried to enable the feature and tested the > > > result? > > > > > > > I have not tried building it recently, but will do so. I will review your > > patch to see if it makes sense. I would prefer that VTV not be 'ripped > > out' of GCC, and will do whatever I can, within reason, to try to fix its > > issues. > > Meanwhile the patch passed bootstrap and testing with > --enable-vtable-verify. > > Richard. > > > > > > > > > Thanks, > > > Richard. > > > > > > 2019-02-19 Richard Biener > > > > > > PR middle-end/89392 > > > cp/ > > > * vtable-class-hierarchy.c (vtv_generate_init_routine): Do not > > > make symtab process new functions here. > > > > > > Index: gcc/cp/vtable-class-hierarchy.c > > > === > > > --- gcc/cp/vtable-class-hierarchy.c (revision 269009) > > > +++ gcc/cp/vtable-class-hierarchy.c (working copy) > > > @@ -1191,8 +1191,6 @@ vtv_generate_init_routine (void) > > >gimplify_function_tree (vtv_fndecl); > > >cgraph_node::add_new_function (vtv_fndecl, false); > > > > > > - symtab->process_new_functions (); > > > - > > >if (flag_vtable_verify == VTV_PREINIT_PRIORITY && !TARGET_PECOFF) > > > assemble_vtv_preinit_initializer (vtv_fndecl); > > > > > > > > > > -- > Richard Biener > SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB > 21284 (AG Nuernberg)
[PATCH, libvtv] Fix testsuite issue.
One of the testsuite tests for libvtv is failing due to an incorrect
signature for the function
"main". This patch fixes that.
Testing: The libvtv testsuite failed 4 tests without this fix; it
passes all of them with it.
Ok to commit?
-- Caroline Tice
cmt...@google.com
Index: libvtv/ChangeLog
===
--- libvtv/ChangeLog (revision 269022)
+++ libvtv/ChangeLog (working copy)
@@ -1,3 +1,8 @@
+2019-02-19 Caroline Tice
+
+ Fix testsuite
+ * testsuite/libvtv.cc/const_vtable.cc (main): Fix function signature.
+
2019-01-01 Jakub Jelinek
Update copyright years.
Index: libvtv/testsuite/libvtv.cc/const_vtable.cc
===
--- libvtv/testsuite/libvtv.cc/const_vtable.cc (revision 269022)
+++ libvtv/testsuite/libvtv.cc/const_vtable.cc (working copy)
@@ -28,7 +28,7 @@
~D();
};
extern "C" int printf(const char *,...);
-main()
+int main(int argc, char**argv)
{
try {
D *d = new D;
Re: [PATCH][C++] Fix ICE with VTV
On Tue, Feb 19, 2019 at 1:57 AM Richard Biener wrote: > > Looks like vtv_generate_init_routine calls symtab->process_new_functions > () which has seriously bad effects on GCCs internal memory state. > > VTV has zero testsuite coverage it seems and besides its initial > commit didn't receive any love. > I am puzzled by these statements, as neither of them is true. VTV does have a testsuite in the libvtv directory. Naturally you can only run them when vtv is enabled and only from the libvtv build tree, as they will all fail if VTV is not enabled. I have fixed various bugs in VTV since the initial commit, and I have also approved several patches, especially for people migrating it to other architectures, suggesting that interest in it is not zero. > > Can we rip it out please? > > Is the patch OK? (Pointless) bootstrap and regtest running on > x86_64-unknown-linux-gnu. > > Has anybody "recently" tried to enable the feature and tested the > result? > I have not tried building it recently, but will do so. I will review your patch to see if it makes sense. I would prefer that VTV not be 'ripped out' of GCC, and will do whatever I can, within reason, to try to fix its issues. > > Thanks, > Richard. > > 2019-02-19 Richard Biener > > PR middle-end/89392 > cp/ > * vtable-class-hierarchy.c (vtv_generate_init_routine): Do not > make symtab process new functions here. > > Index: gcc/cp/vtable-class-hierarchy.c > === > --- gcc/cp/vtable-class-hierarchy.c (revision 269009) > +++ gcc/cp/vtable-class-hierarchy.c (working copy) > @@ -1191,8 +1191,6 @@ vtv_generate_init_routine (void) >gimplify_function_tree (vtv_fndecl); >cgraph_node::add_new_function (vtv_fndecl, false); > > - symtab->process_new_functions (); > - >if (flag_vtable_verify == VTV_PREINIT_PRIORITY && !TARGET_PECOFF) > assemble_vtv_preinit_initializer (vtv_fndecl); > >
[PATCH, GCC] Fix conflicting posix_memalign declaration error
The posix_memalign declaration in gcc/i386/config/pmm_malloc.h is decorated with 'throw ()', which occasionally causes declaration conflict errors (some header files, not part of GCC, that declare posix_memalign, do not have the throw decoration). An example of this can be seen at https://github.com/android-ndk/ndk/issues/91. It appears that the 'throw()' decoration comes from glibc header files. Adding ifdefs to check for __GLIBC__ before adding the 'throw()' fixed the github problem mentioned above. I have tested this patch by bootstrapping and by running test testsuite with no regressions. Is this ok to commit? -- Caroline Tice ctic...@gmail.com gcc/ChangeLog: 2016-10-27 Caroline Tice <cmt...@google.com> * config/i386/pmm_malloc.h (posix_memalign): Add ifdefs to only decorate the declaration with 'throw()' if __GLIBC__ is defined. Index: gcc/config/i386/pmm_malloc.h === --- gcc/config/i386/pmm_malloc.h (revision 241483) +++ gcc/config/i386/pmm_malloc.h (working copy) @@ -31,8 +31,12 @@ #ifndef __cplusplus extern int posix_memalign (void **, size_t, size_t); #else +#ifdef __GLIBC__ extern "C" int posix_memalign (void **, size_t, size_t) throw (); +#else +extern "C" int posix_memalign (void **, size_t, size_t); #endif +#endif static __inline void * _mm_malloc (size_t __size, size_t __alignment)
Re: Port libvtv to Solaris
(Trying this again; the mailer daemon rejected my first message)
All of the patch looks good to me, but I can only approve the libvtv
portion (which I do). Someone else will need to approve the rest.
-- Caroline Tice
cmt...@google.com
On Tue, Nov 24, 2015 at 2:24 AM, Rainer Orth
<r...@cebitec.uni-bielefeld.de> wrote:
> Rainer Orth <r...@cebitec.uni-bielefeld.de> writes:
>
>> Now that init priority support on Solaris is on mainline, porting libvtv
>> proved to be relatively easy, though it discovered a couple of quirks on
>> a non-gld non-x86 platform.
>
> This patch has now remained unreviewed for a week. With both Jeff and
> Richi fine with it going into mainline even this late, it would be nice
> if Caroline could find the time to review it.
>
> Linux/x86_64 testing found one minor issue, explained below. I'm
> attaching the updated patch.
>
>> A considerable part of the patch lives in Solaris-specific files and
>> thus doesn't need approval, though some changes require explanation:
>>
>> * In gcc.c (LINK_COMMAND_SPEC), VTABLE_VERIFICATION_SPEC was before
>> %{L*}. The spec includes -lvtv. Solaris ld, other than GNU ld, heeds
>> the relative order of -L and -l switches, so the libvtv testcases
>> wouldn't link manually, but did inside a testsuite run where
>> LD_LIBRARY_PATH points ld at the correct directory.
>>
>> * Solaris/SPARC uses an 8 kB page size, so a couple of cases where uses
>> of VTV_PAGE_SIZE had been replaced with hardcoded values of 4096 had
>> to be reverted.
>>
>> * Inside libgcc, the vtv_*.c files are compiled with
>> -finhibit-size-directive, whereas in libvtv that flag is absent. This
>> caused all testcases to fail due to a linker warning:
>>
>> FAIL: libvtv.cc/bb_tests.cc -O0 -fvtable-verify=std (test for excess errors)
>> Excess errors:
>> ld: warning: symbol '_vtable_map_vars_end' has differing sizes:
>> (file
>> /var/gcc/gcc-6.0.0-2015/12-gcc-vtv/sparc-sun-solaris2.12/./libvtv/.libs/libvtv.so
>> value=0x1000; file /var/gcc/gcc-6.0.0-2015/12-gcc-vtv/gcc/vtv_end.o
>> value=0x0);
>> /var/gcc/gcc-6.0.0-2015/12-gcc-vtv/gcc/vtv_end.o definition taken
>>
>> * Like Cygwin, Solaris has no obstack functions in libc, so I'm now
>> using a common conditional for that.
>>
>> * libvtv requires constructor priority support and dl_iterate_phdr. The
>> former needs either a recent (Solaris 12 only so far) ld or gld, the
>> latter came in Solaris 11 only.
>>
>> * Unlike glibc systems, Solaris has no __fortify_fail in libc; some of
>> this can probably provided using libbacktrace, which I haven't yet
>> done.
>>
>> * It also lacks program_invocation_name, but the functionality can be
>> provided via getexecname() instead.
>>
>> * On Solaris 12/SPARC with Solaris as, the .vtable_map_vars section
>> wouldn't be pagesize aligned (I'm still looking how to fix this; it
>> works out of the box for .bss), so the section length calclated in
>> read_section_offset_and_length would be negative, leading to all sorts
>> of havoc. For the moment, I'm using gas instead to avoid this.
>>
>> * The patch also fixes a number of typos noticed during testing.
>>
>> With those changes, I get almost clean libvtv test results on
>> i386-pc-solaris2.12 (as/ld and gas/gld) and sparc-sun-solaris2.12
>> (gas/ld):
>>
>> * i386-pc-solaris2.12:
>>
>> === libvtv tests ===
>>
>>
>> Running target unix
>>
>> === libvtv Summary for unix ===
>>
>> # of expected passes 176
>>
>> Running target unix/-m64
>> WARNING: program timed out.
>> FAIL: libvtv.mt.cc/register_set_pair_inserts_mt.cc -O0 -fvtable-verify=std
>> -lpthread execution test
>> WARNING: program timed out.
>> FAIL: libvtv.mt.cc/register_set_pair_inserts_mt.cc -O2 -fvtable-verify=std
>> -lpthread execution test
>>
>> === libvtv Summary for unix/-m64 ===
>>
>> # of expected passes 174
>> # of unexpected failures 2
>>
>> === libvtv Summary ===
>>
>> # of expected passes 350
>> # of unexpected failures 2
>>
>> * sparc-sun-solaris2.12:
>>
>> === libvtv tests ===
>>
>>
>> Running target unix
>> WARNING: program timed out.
>> FAIL: libvtv.mt.cc/register_set_pair_inserts_mt.cc -O0 -fvtable-verify=std
>> -lpthread execution test
>> WARNING: program timed out.
>> FAIL: libvtv.mt.cc
[PATCH, GCC 5 branch] Fix compile time regression caused by fix to PR64111
Here is my promised backport to the GCC 5 branch, for the patch below that went into ToT last week. As with the previous patch, I've verified that it fixes the problem, bootstraps and has no new regression test failures. Is this ok to commit to the gcc-5-branch? -- Caroline Tice cmt...@google.com On Fri, Oct 23, 2015 at 3:22 PM, Caroline Tice <cmt...@google.com> wrote: > This patch fixes a compile-time regression that was originally > introduced by the fix > for PR64111, in GCC 4.9.3.One of our user's encountered this problem with > a > particular file, where the compile time (on arm) went from 20 seconds > to 150 seconds. > > The fix in this patch was suggested by Richard Biener, who wrote the > original fix for > PR64111. I have verified that this patch fixes the compile time > regression; I have bootstrapped > the compiler with this patch; and I have run the regression testsuite > (no regressions). > Is this ok to commit to ToT? (I am also working on backports for > gcc-5_branch and gcc-4_9-branch). > > -- Caroline Tice > cmt...@google.com gcc/ChangeLog: 2015-10-26 Caroline Tice <cmt...@google.com> (from Richard Biener) * tree.c (int_cst_hasher::hash): Replace XOR with more efficient call to iterative_hash_host_wide_int. gcc-fsf-5.patch Description: Binary data
[PATCH, GCC 4.9 branch] Fix compile time regression caused by fix to PR64111
Here is my promised backport to the GCC 4.9 branch, for the patch below that went into ToT last week. As with the previous patch, I've verified that it fixes the problem, bootstraps and has no new regression test failures. Is this ok to commit to the gcc-4_9-branch? -- Caroline Tice cmt...@google.com gcc/ChangeLog: 2015-10-26 Caroline Tice <cmt...@google.com> (from Richard Biener) * tree.c (int_cst_hash_hash): Replace XORs with more efficient calls to iterative_hash_host_wide_int. gcc-fsf-4_9.patch Description: Binary data
[PATCH, GCC 5 branch] Fix compile time regression caused by fix to PR64111
Here is my promised backport to the GCC 5 branch, for the patch below that went into ToT last week. As with the previous patch, I've verified that it fixes the problem, bootstraps and has no new regression test failures. Is this ok to commit to the gcc-5-branch? -- Caroline Tice cmt...@google.com On Fri, Oct 23, 2015 at 3:22 PM, Caroline Tice <cmt...@google.com> wrote: > This patch fixes a compile-time regression that was originally > introduced by the fix > for PR64111, in GCC 4.9.3.One of our user's encountered this problem with > a > particular file, where the compile time (on arm) went from 20 seconds > to 150 seconds. > > The fix in this patch was suggested by Richard Biener, who wrote the > original fix for > PR64111. I have verified that this patch fixes the compile time > regression; I have bootstrapped > the compiler with this patch; and I have run the regression testsuite > (no regressions). > Is this ok to commit to ToT? (I am also working on backports for > gcc-5_branch and gcc-4_9-branch). > > -- Caroline Tice > cmt...@google.com > gcc/ChangeLog: 2015-10-26 Caroline Tice <cmt...@google.com> (from Richard Biener) * tree.c (int_cst_hasher::hash): Replace XOR with more efficient call to iterative_hash_host_wide_int. gcc-fsf-5.patch Description: Binary data
[PATCH, GCC 4.9 branch] Fix compile time regression caused by fix to PR64111
Here is my promised backport to the GCC 4.9 branch, for the patch below that went into ToT last week. As with the previous patch, I've verified that it fixes the problem, bootstraps and has no new regression test failures. Is this ok to commit to the gcc-4_9-branch? -- Caroline Tice cmt...@google.com On Fri, Oct 23, 2015 at 3:22 PM, Caroline Tice <cmt...@google.com> wrote: > This patch fixes a compile-time regression that was originally > introduced by the fix > for PR64111, in GCC 4.9.3.One of our user's encountered this problem with > a > particular file, where the compile time (on arm) went from 20 seconds > to 150 seconds. > > The fix in this patch was suggested by Richard Biener, who wrote the > original fix for > PR64111. I have verified that this patch fixes the compile time > regression; I have bootstrapped > the compiler with this patch; and I have run the regression testsuite > (no regressions). > Is this ok to commit to ToT? (I am also working on backports for > gcc-5_branch and gcc-4_9-branch). > > -- Caroline Tice > cmt...@google.com > gcc/ChangeLog: 2015-10-26 Caroline Tice <cmt...@google.com> (from Richard Biener) * tree.c (int_cst_hash_hash): Replace XORs with more efficient calls to iterative_hash_host_wide_int. gcc-fsf-4_9.patch Description: Binary data
[PATCH] Fix compile time regression caused by fix to PR64111
This patch fixes a compile-time regression that was originally introduced by the fix for PR64111, in GCC 4.9.3.One of our user's encountered this problem with a particular file, where the compile time (on arm) went from 20 seconds to 150 seconds. The fix in this patch was suggested by Richard Biener, who wrote the original fix for PR64111. I have verified that this patch fixes the compile time regression; I have bootstrapped the compiler with this patch; and I have run the regression testsuite (no regressions). Is this ok to commit to ToT? (I am also working on backports for gcc-5_branch and gcc-4_9-branch). -- Caroline Tice cmt...@google.com gcc/ChangeLog: 2015-10-23 Caroline Tice <cmt...@google.com> (from Richard Biener) * tree.c (int_cst_hasher::hash): Replace XOR with more efficient call to iterative_hash_host_wide_int. Index: gcc/tree.c === --- gcc/tree.c (revision 229270) +++ gcc/tree.c (working copy) @@ -1364,7 +1364,7 @@ int i; for (i = 0; i < TREE_INT_CST_NUNITS (t); i++) -code ^= TREE_INT_CST_ELT (t, i); +code = iterative_hash_host_wide_int (TREE_INT_CST_ELT(t, i), code); return code; }
Re: [RFC VTV] Fix VTV for targets that have section anchors.
Sending this again, as the mailer daemon rejected it last time It looks good to me, but I can only approve the files that go into libvtv. In (belated) response to your earlier question about debugging vtv problems, there's a fair amount of useful info for debugging in the User's Guide, off the wiki page (https://gcc.gnu.org/wiki/vtv). If you're already read that and have further questions, let me know... -- Caroline cmt...@google.com On Mon, Oct 19, 2015 at 4:39 PM, Caroline Tice <cmt...@google.com> wrote: > It looks good to me, but I can only approve the files that go into libvtv. > > In (belated) response to your earlier question about debugging vtv problems, > there's a fair amount of useful info for debugging in the User's Guide, off > the wiki page (https://gcc.gnu.org/wiki/vtv). If you're already read that > and have further questions, let me know... > > > -- Caroline > cmt...@google.com > > On Mon, Oct 19, 2015 at 1:54 AM, Ramana Radhakrishnan > <ramana@googlemail.com> wrote: >> >> On Tue, Oct 13, 2015 at 1:53 PM, Ramana Radhakrishnan >> <ramana.radhakrish...@foss.arm.com> wrote: >> > >> > >> > >> > On 12/10/15 21:44, Jeff Law wrote: >> >> On 10/09/2015 03:17 AM, Ramana Radhakrishnan wrote: >> >>> This started as a Friday afternoon project ... >> >>> >> >>> It turned out enabling VTV for AArch64 and ARM was a matter of fixing >> >>> PR67868 which essentially comes from building libvtv with section >> >>> anchors turned on. The problem was that the flow of control from >> >>> output_object_block through to switch_section did not have the same >> >>> special casing for the vtable section that exists in >> >>> assemble_variable. >> >> That's some ugly code. You might consider factoring that code into a >> >> function and just calling it from both places. Your version doesn't seem >> >> to >> >> handle PECOFF, so I'd probably refactor from assemble_variable. >> >> >> > >> > I was a bit lazy as I couldn't immediately think of a target that would >> > want PECOFF, section anchors and VTV. That combination seems to be quite >> > rare, anyway point taken on the refactor. >> > >> > Ok if no regressions ? >> >> Ping. >> >> Ramana >> >> > >> >>> >> >>> However both these failures also occur on x86_64 - so I'm content to >> >>> declare victory on AArch64 as far as basic enablement goes. >> >> Cool. >> >> >> >>> >> >>> 1. Are the generic changes to varasm.c ok ? 2. Can we take the >> >>> AArch64 support in now, given this amount of testing ? Marcus / >> >>> Caroline ? 3. Any suggestions / helpful debug hints for VTV debugging >> >>> (other than turning VTV_DEBUG on and inspecting trace) ? >> >> I think that with refactoring they'd be good to go. No opinions on the >> >> AArch64 specific question -- call for the AArch64 maintainers. >> >> >> >> Good to see someone hacking on vtv. It's in my queue to look at as >> >> well. >> > >> > Yeah figuring out more about vtv is also in my background queue. >> > >> > regards >> > Ramana >> > >> > PR other/67868 >> > >> > * varasm.c (assemble_variable): Move special vtv handling to.. >> > (handle_vtv_comdat_sections): .. here. New function. >> > (output_object_block): Handle vtv sections. >> > >> > libvtv/Changelog >> > >> > * configure.tgt: Support aarch64 and arm. > >
Fwd: [libvtv] Fix formatting errors
-- Forwarded message -- From: Caroline Tice cmt...@google.com Date: Wed, Aug 26, 2015 at 12:50 PM Subject: Re: [libvtv] Fix formatting errors To: Jeff Law l...@redhat.com Cc: Rainer Orth r...@cebitec.uni-bielefeld.de, GCC Patches gcc-patches@gcc.gnu.org As far as I know vtv is working just fine...is there something I don't know about? -- Caroline cmt...@google.com On Wed, Aug 26, 2015 at 12:47 PM, Jeff Law l...@redhat.com wrote: On 08/26/2015 07:30 AM, Rainer Orth wrote: While looking at libvtv for the Solaris port, I noticed all sorts of GNU Coding Standard violations: * ChangeLog entries attributed to the committer instead of the author and with misformatted PR references, entries only giving a vague rational instead of what changed * overlong lines * tons of whitespace errors (though I may be wrong in some cases: C++ code might have other rules) * code formatting that seems to have been done to be visually pleasing, completely different from what Emacs does * commented code fragments (#if 0 equivalent) * configure.tgt target list in no recognizable order * the Cygwin/MingW port is done in the worst possible way: tons of target-specific ifdefs instead of feature-specific conditionals or an interface that can wrap both Cygwin and Linux variants of the code The following patch (as yet not even compiled) fixes some of the most glaring errors. The Solaris port will fix a few of the latter ones. Do you think this is the right direction or did I get something wrong? Thanks. Rainer 2015-08-26 Rainer Orth r...@cebitec.uni-bielefeld.de Fix formatting errors. I'm more interested in the current state of vtv as I keep getting dragged into discussions about what we can/should be doing in the compiler world to close more security stuff. Vtables are an obvious candidate given we've got vtv. Jeff
[PATCH, PR 66521,part 2] Fix warnings on darwin when bootstrapping with vtable verification enabled
When bootstrapping on darwin with vtable verificaiton enabled,
libstdc++ is giving a lot of warnings due to a flag in
VTV_CXXLINKFLAGS that has a slightly different format for darwin than
for linux. This patch (actually contributed by Eric Gallager) fixes
the problem.
I have tested this patch by bootstrapping and running the entire
testsuite with no regressions. Is this OK to commit?
-- Caroline Tice
cmt...@google.com
Index: libstdc++-v3/acinclude.m4
===
--- libstdc++-v3/acinclude.m4 (revision 226760)
+++ libstdc++-v3/acinclude.m4 (working copy)
@@ -2325,14 +2325,19 @@
case ${target_os} in
cygwin*|mingw32*)
VTV_CXXFLAGS=-fvtable-verify=std -Wl,-lvtv,-u_vtable_map_vars_start,-u_vtable_map_vars_end
+VTV_CXXLINKFLAGS=-L${toplevel_builddir}/libvtv/.libs -Wl,--rpath -Wl,${toplevel_builddir}/libvtv/.libs
vtv_cygmin=yes
;;
+ darwin*)
+VTV_CXXFLAGS=-fvtable-verify=std -Wl,-u,_vtable_map_vars_start -Wl,-u,_vtable_map_vars_end
+VTV_CXXLINKFLAGS=-L${toplevel_builddir}/libvtv/.libs -Wl,-rpath,${toplevel_builddir}/libvtv/.libs
+;;
*)
VTV_CXXFLAGS=-fvtable-verify=std -Wl,-u_vtable_map_vars_start,-u_vtable_map_vars_end
+VTV_CXXLINKFLAGS=-L${toplevel_builddir}/libvtv/.libs -Wl,--rpath -Wl,${toplevel_builddir}/libvtv/.libs
;;
esac
VTV_PCH_CXXFLAGS=-fvtable-verify=std
-VTV_CXXLINKFLAGS=-L${toplevel_builddir}/libvtv/.libs -Wl,--rpath -Wl,${toplevel_builddir}/libvtv/.libs
else
VTV_CXXFLAGS=
VTV_PCH_CXXFLAGS=
Index: libstdc++-v3/configure
===
--- libstdc++-v3/configure (revision 226760)
+++ libstdc++-v3/configure (working copy)
@@ -17433,14 +17433,19 @@
case ${target_os} in
cygwin*|mingw32*)
VTV_CXXFLAGS=-fvtable-verify=std -Wl,-lvtv,-u_vtable_map_vars_start,-u_vtable_map_vars_end
+VTV_CXXLINKFLAGS=-L${toplevel_builddir}/libvtv/.libs -Wl,--rpath -Wl,${toplevel_builddir}/libvtv/.libs
vtv_cygmin=yes
;;
+ darwin*)
+VTV_CXXFLAGS=-fvtable-verify=std -Wl,-u,_vtable_map_vars_start -Wl,-u,_vtable_map_vars_end
+VTV_CXXLINKFLAGS=-L${toplevel_builddir}/libvtv/.libs -Wl,-rpath,${toplevel_builddir}/libvtv/.libs
+;;
*)
VTV_CXXFLAGS=-fvtable-verify=std -Wl,-u_vtable_map_vars_start,-u_vtable_map_vars_end
+VTV_CXXLINKFLAGS=-L${toplevel_builddir}/libvtv/.libs -Wl,--rpath -Wl,${toplevel_builddir}/libvtv/.libs
;;
esac
VTV_PCH_CXXFLAGS=-fvtable-verify=std
-VTV_CXXLINKFLAGS=-L${toplevel_builddir}/libvtv/.libs -Wl,--rpath -Wl,${toplevel_builddir}/libvtv/.libs
else
VTV_CXXFLAGS=
VTV_PCH_CXXFLAGS=
Re: [PATCH, PR 66521,part 2] Fix warnings on darwin when bootstrapping with vtable verification enabled
I forgot the ChangeLog enty; here it is: libstdc++-v3/ChangeLog: 2015-08-11 Caroline Tice cmt...@google.com PR 66521, Contributed by Eric Gallager * acinclude.m4 (VTV_CXXLINKFLAGS): Make this variable OS-specific, and fix the rpath flag to work properly for darwin. * configure: Regenerated. On Tue, Aug 11, 2015 at 12:08 PM, Caroline Tice cmt...@google.com wrote: When bootstrapping on darwin with vtable verificaiton enabled, libstdc++ is giving a lot of warnings due to a flag in VTV_CXXLINKFLAGS that has a slightly different format for darwin than for linux. This patch (actually contributed by Eric Gallager) fixes the problem. I have tested this patch by bootstrapping and running the entire testsuite with no regressions. Is this OK to commit? -- Caroline Tice cmt...@google.com
[PATCH, PR 66521] Fix bootstrap segfault with vtable verification enabled
I believe the following patch fixes a problem with bootstrap failures
on some architectures with vtable verification enabled. The problem
was related to a change in name mangling, where classes with anonymous
namespaces get anon as their DECL_ASSEMBLER_NAME, rather than the
real mangled name. This was causing multiple problems for vtable
verification, since not only do we use the mangled name to uniquely
identify the various classes (the anonymous classes were no longer
being properly 'uniqued'), but also the DECL_ASSEMBLER_NAME was being
incorporated into our variable names and ending up in the assembly
code and angle-brackets are not legal there.
This patch should fix those problems, as well as a few other minor
issues I found while working on this.
I have bootstrapped with this patch on an x85_64 linux system; I have
run all the testsuites with no regressions; and I have verified that
it fixes the problem. Is this ok to commit?
-- Caroline Tice
cmt...@google.com
ChangeLogs:
libvtv/ChangeLog
2015-07-28 Caroline Tice cmt...@google.com
PR 66521
* Makefile.am: Update to match latest tree.
* Makefile.in: Regenerate.
* testsuite/lib/libvtv: Brought up to date.
* vtv_malloc.cc (VTV_DEBUG): Update function call to match renamed
function (old bug!).
* vtv_rts.cc (debug_functions, debug_init, debug_verify_vtable): Update
initializations to work correctly with VTV_DEBUG defined.
gcc/ChangeLog:
2015-07-28 Caroline Tice cmt...@google.com
PR 66521
* vtable-verify.c (vtbl_mangled_name_types, vtbl_mangled_name_ids): New
global variables.
(vtbl_find_mangled_name): New function.
(vtbl_register_mangled_name): New function.
(vtbl_map_get_node): If DECL_ASSEMBLER_NAME is anon, look up
mangled name in mangled name vectors.
(find_or_create_vtbl_map_node): Ditto.
(var_is_used_for_virtual_call_p): Add recursion_depth parameter;
update recursion_depth on function entry; pass it to every recursive
call; automatically exit if depth 25 (give up looking at that point).
(verify_bb_vtables): Initialize recursion_depth and pass it to
var_is_used_for_virtual_call_p.
* vtable-verify.h (vtbl_mangbled_name_types, vtbl_mangled_name_ids): New
global variable decls.
(vtbl_register_mangled_name): New extern function decl.
gcc/cp/ChangeLog:
2015-07-28 Caroline Tice cmt...@google.com
PR 66521
* mangle.c : Add vtable-verify.h to include files.
(get_mangled_vtable_map_var_name): If the DECL_ASSEMBLER_NAME
is anon get the real mangled name for the class instead, and
also store the real mangled name in a vector for use later.
Index: gcc/cp/mangle.c
===
--- gcc/cp/mangle.c (revision 226275)
+++ gcc/cp/mangle.c (working copy)
@@ -62,6 +62,7 @@
#include function.h
#include cgraph.h
#include attribs.h
+#include vtable-verify.h
/* Debugging support. */
@@ -4034,6 +4035,13 @@
gcc_assert (TREE_CODE (class_type) == RECORD_TYPE);
tree class_id = DECL_ASSEMBLER_NAME (TYPE_NAME (class_type));
+
+ if (strstr (IDENTIFIER_POINTER (class_id), anon) != NULL)
+{
+ class_id = get_mangled_id (TYPE_NAME (class_type));
+ vtbl_register_mangled_name (TYPE_NAME (class_type), class_id);
+}
+
unsigned int len = strlen (IDENTIFIER_POINTER (class_id)) +
strlen (prefix) +
strlen (postfix) + 1;
Index: gcc/vtable-verify.c
===
--- gcc/vtable-verify.c (revision 226275)
+++ gcc/vtable-verify.c (working copy)
@@ -310,6 +310,70 @@
/* Vtable map variable nodes stored in a vector. */
vecstruct vtbl_map_node * vtbl_map_nodes_vec;
+/* Vector of mangled names for anonymous classes. */
+extern GTY(()) vectree, va_gc *vtbl_mangled_name_types;
+extern GTY(()) vectree, va_gc *vtbl_mangled_name_ids;
+vectree, va_gc *vtbl_mangled_name_types;
+vectree, va_gc *vtbl_mangled_name_ids;
+
+/* Look up class_type (a type decl for record types) in the vtbl_mangled_names_*
+ vectors. This is a linear lookup. Return the associated mangled name for
+ the class type. This is for handling types from anonymous namespaces, whose
+ DECL_ASSEMBLER_NAME ends up being anon, which is useless for our
+ purposes.
+
+ We use two vectors of trees to keep track of the mangled names: One is a
+ vector of class types and the other is a vector of the mangled names. The
+ assumption is that these two vectors are kept in perfect lock-step so that
+ vtbl_mangled_name_ids[i] is the mangled name for
+ vtbl_mangled_name_types[i]. */
+
+static tree
+vtbl_find_mangled_name (tree class_type)
+{
+ tree result = NULL_TREE;
+ unsigned i;
+
+ if (!vtbl_mangled_name_types or !vtbl_mangled_name_ids)
+return result;
+
+ if (vtbl_mangled_name_types-length() != vtbl_mangled_name_ids-length())
+return result;
+
+ for (i = 0; i vtbl_mangled_name_types-length(); ++i)
+if ((*vtbl_mangled_name_types
Re: [PATCH] Fix size type for cold partition names (hot-cold function partitioning)
What is the correct way to regenerate doc/tm.texi? -- Caroline On Thu, Apr 30, 2015 at 12:24 PM, H.J. Lu hjl.to...@gmail.com wrote: On Thu, Apr 30, 2015 at 10:38 AM, Richard Henderson r...@redhat.com wrote: On 04/30/2015 09:26 AM, Caroline Tice wrote: 2015-04-30 Caroline Tice cmt...@google.com PR 65929 * config/elfos.h (ASM_DECLARE_COLD_FUNCTION_NAME): New macro definition. (ASM_DECLARE_COLD_FUNCTION_SIZE): New macro definition. * doc/tm.texi.in (ASM_DECLARE_COLD_FUNCTION_NAME): Document new macro. This breaks bootstrap. You also need to regenerate and check in doc/tm.texi. -- H.J.
Re: [PATCH] Fix size type for cold partition names (hot-cold function partitioning)
Done. Here is the updated patch (with ChangeLog entries). Only
change was to update tm.texi.in.
The bootstrap passed. Is the patch ok to commit?
-- Caroline
cmt...@google.com
ChangeLog (gcc):
2015-04-30 Caroline Tice cmt...@google.com
PR 65929
* config/elfos.h (ASM_DECLARE_COLD_FUNCTION_NAME): New macro definition.
(ASM_DECLARE_COLD_FUNCTION_SIZE): New macro definition.
* doc/tm.texi.in (ASM_DECLARE_COLD_FUNCTION_NAME): Document new macro.
(ASM_DECLARE_COLD_FUNCTION_SIZE): Document new macro.
* final.c (final_scan_insn): Use ASM_DECLARE_COLD_FUNCTION_NAME
instead of ASM_DECLARE_FUNCTION_NAME for cold partition name.
* varasm.c (assemble_end_function): Use ASM_DECLARE_COLD_FUNCTION_SIZE
instead of ASM_DECLARE_FUNCTION_SIZE for cold partition size.
ChangeLog (gcc/testsuite):
2015-04-30 Caroline Tice cmt...@google.com
PR 65929
* gcc.dg/tree-prof/cold_partition_label.c: Only check for cold
partition size on certain targets.
On Wed, Apr 29, 2015 at 11:12 PM, Uros Bizjak ubiz...@gmail.com wrote:
On Wed, Apr 29, 2015 at 11:22 PM, Caroline Tice cmt...@google.com wrote:
Here is a new patch to update the cold name partition so that it will
only be treated like a function name and be given a size on the
architectures that specifically define macros for such.
I also updated the test case to try to only test on the appropriate
architectures. I am not sure I got the target triples correct for
this, so I would appreciate some extra attention to that in the
review. I have tested this new patch on my workstation and it works
as intended. I am in the process of bootstrapping with the new patch.
Assuming that the bootstrap passes, is this ok to commit?
-- Caroline Tice
cmt...@google.com
ChangeLog (gcc):
2015-04-29 Caroline Tice cmt...@google.com
PR 65929
* config/elfos.h (ASM_DECLARE_COLD_FUNCTION_NAME): New macro
definition.
(ASM_DECLARE_COLD_FUNCTION_SIZE): New macro definition.
* final.c (final_scan_insn): Use ASM_DECLARE_COLD_FUNCTION_NAME
instead of ASM_DECLARE_FUNCTION_NAME for cold partition name.
* varasm.c (assemble_end_function): Use
ASM_DECLARE_COLD_FUNCTION_SIZE
instead of ASM_DECLARE_FUNCTION_SIZE for cold partition size.
ChangeLog (testsuite):
2015-04-29 Caroline Tice cmt...@google.com
PR 65929
* gcc.dg/tree-prof/cold_partition_label.c: Only check for cold
partition size on certain targets.
Documentation for new macros is missing (please see doc/tm.texi.in).
Uros.
Index: gcc/config/elfos.h
===
--- gcc/config/elfos.h (revision 222635)
+++ gcc/config/elfos.h (working copy)
@@ -284,6 +284,22 @@
while (0)
#endif
+/* Write the extra assembler code needed to declare the name of a
+ cold function partition properly. Some svr4 assemblers need to also
+ have something extra said about the function's return value. We
+ allow for that here. */
+
+#ifndef ASM_DECLARE_COLD_FUNCTION_NAME
+#define ASM_DECLARE_COLD_FUNCTION_NAME(FILE, NAME, DECL) \
+ do\
+{\
+ ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, function); \
+ ASM_DECLARE_RESULT (FILE, DECL_RESULT (DECL)); \
+ ASM_OUTPUT_FUNCTION_LABEL (FILE, NAME, DECL); \
+}\
+ while (0)
+#endif
+
/* Write the extra assembler code needed to declare an object properly. */
#ifdef HAVE_GAS_GNU_UNIQUE_OBJECT
@@ -358,6 +374,17 @@
while (0)
#endif
+/* This is how to declare the size of a cold function partition. */
+#ifndef ASM_DECLARE_COLD_FUNCTION_SIZE
+#define ASM_DECLARE_COLD_FUNCTION_SIZE(FILE, FNAME, DECL) \
+ do\
+{\
+ if (!flag_inhibit_size_directive)\
+ ASM_OUTPUT_MEASURED_SIZE (FILE, FNAME); \
+}\
+ while (0)
+#endif
+
/* A table of bytes codes used by the ASM_OUTPUT_ASCII and
ASM_OUTPUT_LIMITED_STRING macros. Each byte in the table
corresponds to a particular byte value [0..255]. For any
Index: gcc/doc/tm.texi.in
===
--- gcc/doc/tm.texi.in (revision 222635)
+++ gcc/doc/tm.texi.in (working copy)
@@ -5574,6 +5574,34 @@
of this macro.
@end defmac
+@defmac ASM_DECLARE_COLD_FUNCTION_NAME (@var{stream}, @var{name}, @var{decl})
+A C statement (sans semicolon) to output to the stdio stream
+@var{stream} any text necessary for declaring the name @var{name} of a
+cold function partition which is being defined. This macro is responsible
+for outputting the label definition (perhaps using
+@code{ASM_OUTPUT_FUNCTION_LABEL}). The argument @var{decl} is the
+@code{FUNCTION_DECL} tree node representing the function.
+
+If this macro is not defined, then the cold partition name is defined in the
+usual manner as a label (by means of @code{ASM_OUTPUT_LABEL}).
+
+You may wish to use @code{ASM_OUTPUT_TYPE_DIRECTIVE} in the definition
+of this macro.
+@end defmac
+
+@defmac
Re: [PATCH] Fix size type for cold partition names (hot-cold function partitioning)
Thank you; I will work with your suggestions and try to get a new patch done soon. -- Caroline Tice cmt...@google.com On Wed, Apr 29, 2015 at 11:34 AM, Uros Bizjak ubiz...@gmail.com wrote: On Wed, Apr 29, 2015 at 7:47 PM, Uros Bizjak ubiz...@gmail.com wrote: On Wed, Apr 29, 2015 at 7:38 PM, Caroline Tice cmt...@google.com wrote: The attached patch can revert the previous patch, if that is the way we should proceed on this. If you want me to apply the reversion, please let me know. I would be happy to fix to the problem, rather than just reverting the patch, but I do not have expertise in assembly language on other platforms, so I would need some help, if anyone would be interested in helping me? How about adding ASM_DECLARE_COLD_FUNCTION_NAME and ASM_DECLARE_COLD_FUNCTION_SIZE? If these are defined, they can be used instead, and targets are free to define them in any way. Something like the attached prototype RFC patch. Using this patch, readelf -sW returns: Symbol table '.symtab' contains 18 entries: Num:Value Size TypeBind Vis Ndx Name 0: 0 NOTYPE LOCAL DEFAULT UND 1: 0 SECTION LOCAL DEFAULT1 2: 0 SECTION LOCAL DEFAULT3 3: 0 SECTION LOCAL DEFAULT4 4: 0 SECTION LOCAL DEFAULT5 5: 0 SECTION LOCAL DEFAULT6 6: 0 SECTION LOCAL DEFAULT8 7: 8 FUNCLOCAL DEFAULT6 main.cold.0 8: 0 SECTION LOCAL DEFAULT 10 9: 0 SECTION LOCAL DEFAULT 13 10: 0 SECTION LOCAL DEFAULT 12 11: 312 FUNCGLOBAL DEFAULT [other: 88] 8 main 12: 0008 160 OBJECT GLOBAL DEFAULT COM buf 13: 0 NOTYPE GLOBAL DEFAULT UND memset 14: 44 FUNCGLOBAL DEFAULT [other: 88] 1 sub2 15: 0 NOTYPE GLOBAL DEFAULT UND strcmp 16: 0 NOTYPE GLOBAL DEFAULT UND exit 17: 0 NOTYPE GLOBAL DEFAULT UND abort Uros.
Re: [PATCH] Fix size type for cold partition names (hot-cold function partitioning)
The attached patch can revert the previous patch, if that is the way
we should proceed on this. If you want me to apply the reversion,
please let me know.
I would be happy to fix to the problem, rather than just reverting the
patch, but I do not have expertise in assembly language on other
platforms, so I would need some help, if anyone would be interested in
helping me?
-- Caroline Tice
cmt...@google.com
ChangeLog (gcc):
2015-04-29 Caroline Tice cmt...@google.com
Revert patch from 2015-04-27
* final.c (final_scan_insn): Remove code to output
cold_function_name as a function type in assembly.
* varasm.c (cold_function_name): Remove global declaration.
(assemble_start_function): Remove code to re-set
cold_function_name.
(assemble_end_function): Do not output side of cold
partition.
ChangLog (gcc/testsuite):
2015-04-29 Caroline Tice cmt...@google.com
Revert patch from 2015-04-27
* gcc.dg/tree-prof/cold_partition_label.c: Do not check
for cold partition size.
On Wed, Apr 29, 2015 at 9:33 AM, Uros Bizjak ubiz...@gmail.com wrote:
Hello!
2015-03-27 Caroline Tice cmt...@google.com
* final.c (final_scan_insn): Change 'cold_function_name' to
'cold_partition_name' and make it a global variable; also output
assembly to give it a 'FUNC' type, if appropriate.
* varasm.c (cold_partition_name): Declare and initialize global
variable.
(assemble_start_function): Re-set value for cold_partition_name.
(assemble_end_function): Output assembly to calculate size of cold
partition, and associate size with name, if appropriate.
* varash.h (cold_partition_name): Add extern declaration for global
variable.
* testsuite/gcc.dg/tree-prof/cold_partition_label.c: Add dg-final-use
to scan assembly for cold partition name and size.
This patch caused PR 65929 [1].
Targets are (ab)using ASM_DECLARE_FUNCTION_NAME and
ASM_DECLARE_FUNCTION_SIZE for more things that their name suggests. As
shown in the PR, some directives that are generated through these
macros apply only to real functions, not to parts of function in the
middle of the function.
In the particular case in the PR, assembler doesn't tolerate nested
function declaration.
I suggest to revert the patch for now, until side effects of the patch
are resolved.
[1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65929
Uros.
Index: gcc/final.c
===
--- gcc/final.c (revision 222584)
+++ gcc/final.c (working copy)
@@ -2233,16 +2233,10 @@
suffixing cold to the original function's name. */
if (in_cold_section_p)
{
- cold_function_name
+ tree cold_function_name
= clone_function_name (current_function_decl, cold);
-#ifdef ASM_DECLARE_FUNCTION_NAME
- ASM_DECLARE_FUNCTION_NAME (asm_out_file,
- IDENTIFIER_POINTER (cold_function_name),
- current_function_decl);
-#else
ASM_OUTPUT_LABEL (asm_out_file,
IDENTIFIER_POINTER (cold_function_name));
-#endif
}
break;
Index: gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c
===
--- gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c (revision 222584)
+++ gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c (working copy)
@@ -35,6 +35,4 @@
return 0;
}
-/* { dg-final-use { scan-assembler foo\[._\]+cold\[\._\]+0 } } */
-/* { dg-final-use { scan-assembler size\[ \ta-zA-Z0-0\]+foo\[._\]+cold\[\._\]+0 } } */
/* { dg-final-use { cleanup-saved-temps } } */
Index: gcc/varasm.c
===
--- gcc/varasm.c (revision 222584)
+++ gcc/varasm.c (working copy)
@@ -187,13 +187,6 @@
at the cold section. */
bool in_cold_section_p;
-/* The following global holds the function name for the code in the
- cold section of a function, if hot/cold function splitting is enabled
- and there was actually code that went into the cold section. A
- pseudo function name is needed for the cold section of code for some
- debugging tools that perform symbolization. */
-tree cold_function_name = NULL_TREE;
-
/* A linked list of all the unnamed sections. */
static GTY(()) section *unnamed_sections;
@@ -1726,7 +1719,6 @@
ASM_GENERATE_INTERNAL_LABEL (tmp_label, LCOLDE, const_labelno);
crtl-subsections.cold_section_end_label = ggc_strdup (tmp_label);
const_labelno++;
- cold_function_name = NULL_TREE;
}
else
{
@@ -1864,12 +1856,6 @@
save_text_section = in_section;
switch_to_section (unlikely_text_section ());
-#ifdef ASM_DECLARE_FUNCTION_SIZE
- if (cold_function_name != NULL_TREE)
- ASM_DECLARE_FUNCTION_SIZE (asm_out_file,
- IDENTIFIER_POINTER (cold_function_name),
- decl);
-#endif
ASM_OUTPUT_LABEL
Re: [PATCH] Fix size type for cold partition names (hot-cold function partitioning)
Here is a new patch to update the cold name partition so that it will
only be treated like a function name and be given a size on the
architectures that specifically define macros for such.
I also updated the test case to try to only test on the appropriate
architectures. I am not sure I got the target triples correct for
this, so I would appreciate some extra attention to that in the
review. I have tested this new patch on my workstation and it works
as intended. I am in the process of bootstrapping with the new patch.
Assuming that the bootstrap passes, is this ok to commit?
-- Caroline Tice
cmt...@google.com
ChangeLog (gcc):
2015-04-29 Caroline Tice cmt...@google.com
PR 65929
* config/elfos.h (ASM_DECLARE_COLD_FUNCTION_NAME): New macro definition.
(ASM_DECLARE_COLD_FUNCTION_SIZE): New macro definition.
* final.c (final_scan_insn): Use ASM_DECLARE_COLD_FUNCTION_NAME
instead of ASM_DECLARE_FUNCTION_NAME for cold partition name.
* varasm.c (assemble_end_function): Use ASM_DECLARE_COLD_FUNCTION_SIZE
instead of ASM_DECLARE_FUNCTION_SIZE for cold partition size.
ChangeLog (testsuite):
2015-04-29 Caroline Tice cmt...@google.com
PR 65929
* gcc.dg/tree-prof/cold_partition_label.c: Only check for cold
partition size on certain targets.
On Wed, Apr 29, 2015 at 11:59 AM, Caroline Tice cmt...@google.com wrote:
Thank you; I will work with your suggestions and try to get a new
patch done soon.
-- Caroline Tice
cmt...@google.com
On Wed, Apr 29, 2015 at 11:34 AM, Uros Bizjak ubiz...@gmail.com wrote:
On Wed, Apr 29, 2015 at 7:47 PM, Uros Bizjak ubiz...@gmail.com wrote:
On Wed, Apr 29, 2015 at 7:38 PM, Caroline Tice cmt...@google.com wrote:
The attached patch can revert the previous patch, if that is the way
we should proceed on this. If you want me to apply the reversion,
please let me know.
I would be happy to fix to the problem, rather than just reverting the
patch, but I do not have expertise in assembly language on other
platforms, so I would need some help, if anyone would be interested in
helping me?
How about adding ASM_DECLARE_COLD_FUNCTION_NAME and
ASM_DECLARE_COLD_FUNCTION_SIZE? If these are defined, they can be used
instead, and targets are free to define them in any way.
Something like the attached prototype RFC patch. Using this patch,
readelf -sW returns:
Symbol table '.symtab' contains 18 entries:
Num:Value Size TypeBind Vis Ndx Name
0: 0 NOTYPE LOCAL DEFAULT UND
1: 0 SECTION LOCAL DEFAULT1
2: 0 SECTION LOCAL DEFAULT3
3: 0 SECTION LOCAL DEFAULT4
4: 0 SECTION LOCAL DEFAULT5
5: 0 SECTION LOCAL DEFAULT6
6: 0 SECTION LOCAL DEFAULT8
7: 8 FUNCLOCAL DEFAULT6 main.cold.0
8: 0 SECTION LOCAL DEFAULT 10
9: 0 SECTION LOCAL DEFAULT 13
10: 0 SECTION LOCAL DEFAULT 12
11: 312 FUNCGLOBAL DEFAULT [other: 88] 8
main
12: 0008 160 OBJECT GLOBAL DEFAULT COM buf
13: 0 NOTYPE GLOBAL DEFAULT UND memset
14: 44 FUNCGLOBAL DEFAULT [other: 88] 1
sub2
15: 0 NOTYPE GLOBAL DEFAULT UND strcmp
16: 0 NOTYPE GLOBAL DEFAULT UND exit
17: 0 NOTYPE GLOBAL DEFAULT UND abort
Uros.
Index: gcc/config/elfos.h
===
--- gcc/config/elfos.h (revision 222584)
+++ gcc/config/elfos.h (working copy)
@@ -284,6 +284,22 @@
while (0)
#endif
+/* Write the extra assembler code needed to declare the name of a
+ cold function partition properly. Some svr4 assemblers need to also
+ have something extra said about the function's return value. We
+ allow for that here. */
+
+#ifndef ASM_DECLARE_COLD_FUNCTION_NAME
+#define ASM_DECLARE_COLD_FUNCTION_NAME(FILE, NAME, DECL) \
+ do\
+{\
+ ASM_OUTPUT_TYPE_DIRECTIVE (FILE, NAME, function); \
+ ASM_DECLARE_RESULT (FILE, DECL_RESULT (DECL)); \
+ ASM_OUTPUT_FUNCTION_LABEL (FILE, NAME, DECL); \
+}\
+ while (0)
+#endif
+
/* Write the extra assembler code needed to declare an object properly. */
#ifdef HAVE_GAS_GNU_UNIQUE_OBJECT
@@ -358,6 +374,17 @@
while (0)
#endif
+/* This is how to declare the size of a cold function partition. */
+#ifndef ASM_DECLARE_COLD_FUNCTION_SIZE
+#define ASM_DECLARE_COLD_FUNCTION_SIZE(FILE, FNAME, DECL) \
+ do\
+{\
+ if (!flag_inhibit_size_directive)\
+ ASM_OUTPUT_MEASURED_SIZE (FILE, FNAME); \
+}\
+ while (0)
+#endif
Re: [PATCH] Fix size type for cold partition names (hot-cold function partitioning)
Yes, this is already mentioned in PR 65910 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65910). There is a patch mentioned in that PR that both appears to fix the problem and appears to have been approved. I thought the author of the patch would commit it, but that does not appear to have happened yet. Should I go ahead and commit that patch, or should I wait for the author to do that? -- Caroline Tice cmt...@google.clom On Tue, Apr 28, 2015 at 10:10 AM, David Edelsohn dje@gmail.com wrote: Caroline, Your patch has broken bootstrap on AIX and probably other platforms. /nasfarm/edelsohn/src/src/gcc/varasm.c: In function 'void assemble_end_function(tree, const char*)': /nasfarm/edelsohn/src/src/gcc/varasm.c:1870:12: error: 'ASM_DECLARE_FUNCTION_SIZE' was not declared in this scope decl); ^ You added a reference to ASM_DECLARE_FUNCTION_SIZE without guarding it, as was done only 10 lines higher in the same function. The macro is not declared for all targets. Also, the ChangeLog entry has numerous typos. Please fix ASAP. Thanks, David
Re: [PATCH] Fix size type for cold partition names (hot-cold function partitioning)
Thank you! -- Caroline Tice cmt...@google.com On Tue, Apr 28, 2015 at 10:16 AM, David Edelsohn dje@gmail.com wrote: I just committed the patch for Dominique. - David On Tue, Apr 28, 2015 at 1:12 PM, Caroline Tice cmt...@google.com wrote: Yes, this is already mentioned in PR 65910 (https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65910). There is a patch mentioned in that PR that both appears to fix the problem and appears to have been approved. I thought the author of the patch would commit it, but that does not appear to have happened yet. Should I go ahead and commit that patch, or should I wait for the author to do that? -- Caroline Tice cmt...@google.clom On Tue, Apr 28, 2015 at 10:10 AM, David Edelsohn dje@gmail.com wrote: Caroline, Your patch has broken bootstrap on AIX and probably other platforms. /nasfarm/edelsohn/src/src/gcc/varasm.c: In function 'void assemble_end_function(tree, const char*)': /nasfarm/edelsohn/src/src/gcc/varasm.c:1870:12: error: 'ASM_DECLARE_FUNCTION_SIZE' was not declared in this scope decl); ^ You added a reference to ASM_DECLARE_FUNCTION_SIZE without guarding it, as was done only 10 lines higher in the same function. The macro is not declared for all targets. Also, the ChangeLog entry has numerous typos. Please fix ASAP. Thanks, David
Re: [PATCH] Add vtable verification feature announcement to news on main page...
I would appreciate it if you would do it. -- Caroline Tice cmt...@google.com On Fri, Apr 10, 2015 at 4:55 PM, Gerald Pfeifer ger...@pfeifer.com wrote: On Sun, 8 Sep 2013, Caroline Tice wrote: I believe I have addressed all the issues with the Vtable Verification feature. I have updated the announcement patch, and have attached the updated patch. Is this OK to commit? Eeh, I just noticed nobody ever applied the patch below. :-( Sorry about that. It's too late for the main page now, but can you push this into the appropriate place of news.html (or let me know, and I'll do it)? Gerald Index: htdocs/index.html === RCS file: /cvs/gcc/wwwdocs/htdocs/index.html,v retrieving revision 1.890 diff -r1.890 index.html 55a56,64 dtspanThe Vtable Verification Feature is now in GCC/span span class=date[2013-09-08]/span/dt ddThe a href=http://gcc.gnu.org/wiki/vtv;vtable verification/a branch has been merged into trunk. This adds a new security feature for C++ programs, to detect/prevent vtable pointer corruption or attacks. For more details see the documentation on the feature wiki page. This work was contributed by Caroline Tice, Luis Lozano and Geoff Pike of Google, and Benjamin Kosnik of Red Hat./dd
Re: [PATCH] Fix size type for cold partition names (hot-cold function partitioning)
I am fine with waiting until stage 1. When that is likely to be? -- Caroline Tice cmt...@google.com On Mon, Mar 30, 2015 at 10:19 PM, Jeff Law l...@redhat.com wrote: On 03/27/2015 10:44 AM, Caroline Tice wrote: It took me a while to get a test case I'm happy with, so I'm re-submitting the whole patch for approval. 2015-03-27 Caroline Tice cmt...@google.com * final.c (final_scan_insn): Change 'cold_function_name' to 'cold_partition_name' and make it a global variable; also output assembly to give it a 'FUNC' type, if appropriate. * varasm.c (cold_partition_name): Declare and initialize global variable. (assemble_start_function): Re-set value for cold_partition_name. (assemble_end_function): Output assembly to calculate size of cold partition, and associate size with name, if appropriate. * varash.h (cold_partition_name): Add extern declaration for global variable. * testsuite/gcc.dg/tree-prof/cold_partition_label.c: Add dg-final-use to scan assembly for cold partition name and size. Given we're late in stage4, can this wait until stage1, where it should be considered pre-approved? I'd hate to mess up the PA or PTX ports at this stage in the release process. jeff
Re: [PATCH] Fix size type for cold partition names (hot-cold function partitioning)
It took me a while to get a test case I'm happy with, so I'm
re-submitting the whole patch for approval.
2015-03-27 Caroline Tice cmt...@google.com
* final.c (final_scan_insn): Change 'cold_function_name' to
'cold_partition_name' and make it a global variable; also output
assembly to give it a 'FUNC' type, if appropriate.
* varasm.c (cold_partition_name): Declare and initialize global
variable.
(assemble_start_function): Re-set value for cold_partition_name.
(assemble_end_function): Output assembly to calculate size of cold
partition, and associate size with name, if appropriate.
* varash.h (cold_partition_name): Add extern declaration for global
variable.
* testsuite/gcc.dg/tree-prof/cold_partition_label.c: Add dg-final-use
to scan assembly for cold partition name and size.
I've attached the whole patch, with the testcase (which is the only
thing that has changed since the previous patch).
-- Caroline Tice
cmt...@google.com
On Mon, Dec 8, 2014 at 1:06 PM, Jeff Law l...@redhat.com wrote:
On 12/05/14 15:41, Caroline Tice wrote:
When hot/cold function splitting occurs, a symbol is generated for the
cold partition, and gets output in the assembly debug info, but the
symbol currently gets a size of 0 and a type of NOTYPE, as in this
example (on x86_64-linux) from the cold_partition_label test in the
testsuite:
$ readelf -sW cold_partition_label.x02 | grep foo
36: 00400450 0 NOTYPE LOCAL DEFAULT 12 foo.cold.0
58: 0040049043 FUNCGLOBAL DEFAULT 12 foo
$
This patch fixes this by calculating the right size for the partition,
and outputing the size and type fo the cold partition symbol. After
applying this patch and looking at the same test, I get:
$ readelf -sW cold_partition_label.x02 | grep foo
36: 0040045029 FUNCLOCAL DEFAULT 12 foo.cold.0
58: 0040049043 FUNCGLOBAL DEFAULT 12 foo
$
This patch has been tested by bootstrapping the compiler, running the
dejagnu testsuite with no regressions, and checked as shown above that
it fixes the original problem. Is this patch OK to commit to ToT?
-- Caroline Tice
cmt...@google.com
2014-12-05 Caroline Tice cmt...@google.com
* final.c (final_scan_insn): Change 'cold_function_name' to
'cold_partition_name' and make it a global variable; also output
assembly to give it a 'FUNC' type, if appropriate.
* varasm.c (cold_partition_name): Declare and initialize global
variable.
(assemble_start_function): Re-set value for cold_partition_name.
(assemble_end_function): Output assembly to calculate size of
cold
partition, and associate size with name, if appropriate.
* varash.h (cold_partition_name): Add extern declaration for
global
variable.
OK for the trunk. I'm ever-so-slightly worried about the PA and PTX ports
are they're probably the most sensitive to types. But we'll deal with them
if they complain.
Adding a testcase would be helpful. I believe some of the LTO tests use
readelf, so there should be some infrastructure you can factor out and
reuse.
jeff
Index: gcc/final.c
===
--- gcc/final.c (revision 221669)
+++ gcc/final.c (working copy)
@@ -2236,10 +2236,16 @@
suffixing cold to the original function's name. */
if (in_cold_section_p)
{
- tree cold_function_name
+ cold_function_name
= clone_function_name (current_function_decl, cold);
+#ifdef ASM_DECLARE_FUNCTION_NAME
+ ASM_DECLARE_FUNCTION_NAME (asm_out_file,
+ IDENTIFIER_POINTER (cold_function_name),
+ current_function_decl);
+#else
ASM_OUTPUT_LABEL (asm_out_file,
IDENTIFIER_POINTER (cold_function_name));
+#endif
}
break;
Index: gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c
===
--- gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c (revision 221669)
+++ gcc/testsuite/gcc.dg/tree-prof/cold_partition_label.c (working copy)
@@ -35,4 +35,6 @@
return 0;
}
+/* { dg-final-use { scan-assembler foo\[._\]+cold\[\._\]+0 } } */
+/* { dg-final-use { scan-assembler size\[ \ta-zA-Z0-0\]+foo\[._\]+cold\[\._\]+0 } } */
/* { dg-final-use { cleanup-saved-temps } } */
Index: gcc/varasm.c
===
--- gcc/varasm.c (revision 221669)
+++ gcc/varasm.c (working copy)
@@ -187,6 +187,13 @@
at the cold section. */
bool in_cold_section_p;
+/* The following global holds the function name for the code in the
+ cold section of a function, if hot/cold function splitting is enabled
+ and there was actually code that went into the cold section. A
+ pseudo function name is needed for the cold section of code for some
+ debugging
Re: [Ping] Port of VTV for Cygwin and MinGW
I (sadly) committed this patch in two pieces, one last night and one
this morning. In the commit last night, I had forgotten to commit the
Makefile.in and configure files that got generated by autoconf and
automake. Did you sync your sources before or after the second
commit?
-- Caroline Tice
On Thu, Jan 29, 2015 at 9:48 AM, H.J. Lu hjl.to...@gmail.com wrote:
On Wed, Jan 28, 2015 at 2:58 PM, Caroline Tice cmt...@google.com wrote:
Since all the pieces of this patch have been approved, I will commit
it later today (since Patrick does not have commit privileges).
I got
configure: error: conditional VTV_CYGMIN was never defined.
Usually this means the macro was only invoked conditionally.
Makefile:12580: recipe for target 'configure-stage1-target-libstdc++-v3'
failed
make[5]: *** [configure-stage1-target-libstdc++-v3] Error 1
-- Caroline Tice
cmt...@google.com
On Wed, Jan 28, 2015 at 3:31 AM, Patrick Wollgast
patrick.wollg...@rub.de wrote:
Ping.
https://gcc.gnu.org/ml/gcc-patches/2015-01/msg01270.html
On 15.01.2015 22:50, Patrick Wollgast wrote:
On 15.01.2015 17:01, Ian Lance Taylor wrote:
On Wed, Jan 14, 2015 at 11:54 PM, Patrick Wollgast
patrick.wollg...@rub.de wrote:
Is there something I'm still supposed to do, since I don't have write
access and this was the last part missing an OK?
Somebody with write access will need to commit the patch for you. You
should send a new clean patch including all the changes, along with
updated ChangeLog entries.
Ian
For the clean patch I co'ed the latest version of the trunk again and
applied my patch. It applies correctly, except for two changes:
patching file libgcc/Makefile.in
Hunk #1 succeeded at 995 with fuzz 2 (offset 9 lines).
Hunk #2 succeeded at 1020 (offset 17 lines).
# This is a version of crtbegin for -static links.
crtbeginT$(objext): $(srcdir)/crtstuff.c
$(crt_compile) $(CRTSTUFF_T_CFLAGS) -c $ -DCRT_BEGIN
-DCRTSTUFFT_O
endif
ifeq ($(enable_vtable_verify),yes)
# These are used in vtable verification; see comments in source files
for
# more details.
I had to move the endif down, since something was added before
ifeq ($(enable_vtable_verify),yes) inside the if.
# This is a version of crtbegin for -static links.
crtbeginT$(objext): $(srcdir)/crtstuff.c
$(crt_compile) $(CRTSTUFF_T_CFLAGS) -c $ -DCRT_BEGIN
-DCRTSTUFFT_O
# crtoffloadbegin and crtoffloadend contain symbols, that mark the
begin and
# the end of tables with addresses, required for offloading.
crtoffloadbegin$(objext): $(srcdir)/offloadstuff.c
$(crt_compile) $(CRTSTUFF_T_CFLAGS) -c $ -DCRT_BEGIN
crtoffloadend$(objext): $(srcdir)/offloadstuff.c
$(crt_compile) $(CRTSTUFF_T_CFLAGS) -c $ -DCRT_END
endif
ifeq ($(enable_vtable_verify),yes)
# These are used in vtable verification; see comments in source files
for
# more details.
patching file libgcc/config.host
Hunk #1 succeeded at 621 (offset 6 lines).
Hunk #2 succeeded at 640 (offset 6 lines).
Hunk #3 succeeded at 660 (offset 6 lines).
Hunk #4 succeeded at 1198 with fuzz 2 (offset 495 lines).
tmake_file=${tmake_file} ${tmake_eh_file} ${tmake_dlldir_file}
i386/t-slibgcc-cygming i386/t-mingw32 t-dfprules i386/t-crtfm
i386/t-chkstk
extra_parts=$extra_parts crtfastmath.o
The last two lines were changed to the following two lines.
tmake_file=${tmake_file} ${tmake_eh_file} ${tmake_dlldir_file}
i386/t-slibgcc-cygming i386/t-cygming i386/t-mingw32 t-dfprules
i386/t-crtfm i386/t-chkstk
extra_parts=$extra_parts crtbegin.o crtend.o crtfastmath.o
And therefore Hunk #4, which follows these lines, wasn't applied correctly.
These two parts were corrected in vtv_cygmin_clean.patch. For
convenience I also added vtv_cygmin_unclean.patch, which is the patch
from my last mail.
Regards,
Patrick
--
H.J.
Re: [Ping] Port of VTV for Cygwin and MinGW
Since all the pieces of this patch have been approved, I will commit
it later today (since Patrick does not have commit privileges).
-- Caroline Tice
cmt...@google.com
On Wed, Jan 28, 2015 at 3:31 AM, Patrick Wollgast
patrick.wollg...@rub.de wrote:
Ping.
https://gcc.gnu.org/ml/gcc-patches/2015-01/msg01270.html
On 15.01.2015 22:50, Patrick Wollgast wrote:
On 15.01.2015 17:01, Ian Lance Taylor wrote:
On Wed, Jan 14, 2015 at 11:54 PM, Patrick Wollgast
patrick.wollg...@rub.de wrote:
Is there something I'm still supposed to do, since I don't have write
access and this was the last part missing an OK?
Somebody with write access will need to commit the patch for you. You
should send a new clean patch including all the changes, along with
updated ChangeLog entries.
Ian
For the clean patch I co'ed the latest version of the trunk again and
applied my patch. It applies correctly, except for two changes:
patching file libgcc/Makefile.in
Hunk #1 succeeded at 995 with fuzz 2 (offset 9 lines).
Hunk #2 succeeded at 1020 (offset 17 lines).
# This is a version of crtbegin for -static links.
crtbeginT$(objext): $(srcdir)/crtstuff.c
$(crt_compile) $(CRTSTUFF_T_CFLAGS) -c $ -DCRT_BEGIN
-DCRTSTUFFT_O
endif
ifeq ($(enable_vtable_verify),yes)
# These are used in vtable verification; see comments in source files
for
# more details.
I had to move the endif down, since something was added before
ifeq ($(enable_vtable_verify),yes) inside the if.
# This is a version of crtbegin for -static links.
crtbeginT$(objext): $(srcdir)/crtstuff.c
$(crt_compile) $(CRTSTUFF_T_CFLAGS) -c $ -DCRT_BEGIN
-DCRTSTUFFT_O
# crtoffloadbegin and crtoffloadend contain symbols, that mark the
begin and
# the end of tables with addresses, required for offloading.
crtoffloadbegin$(objext): $(srcdir)/offloadstuff.c
$(crt_compile) $(CRTSTUFF_T_CFLAGS) -c $ -DCRT_BEGIN
crtoffloadend$(objext): $(srcdir)/offloadstuff.c
$(crt_compile) $(CRTSTUFF_T_CFLAGS) -c $ -DCRT_END
endif
ifeq ($(enable_vtable_verify),yes)
# These are used in vtable verification; see comments in source files
for
# more details.
patching file libgcc/config.host
Hunk #1 succeeded at 621 (offset 6 lines).
Hunk #2 succeeded at 640 (offset 6 lines).
Hunk #3 succeeded at 660 (offset 6 lines).
Hunk #4 succeeded at 1198 with fuzz 2 (offset 495 lines).
tmake_file=${tmake_file} ${tmake_eh_file} ${tmake_dlldir_file}
i386/t-slibgcc-cygming i386/t-mingw32 t-dfprules i386/t-crtfm i386/t-chkstk
extra_parts=$extra_parts crtfastmath.o
The last two lines were changed to the following two lines.
tmake_file=${tmake_file} ${tmake_eh_file} ${tmake_dlldir_file}
i386/t-slibgcc-cygming i386/t-cygming i386/t-mingw32 t-dfprules
i386/t-crtfm i386/t-chkstk
extra_parts=$extra_parts crtbegin.o crtend.o crtfastmath.o
And therefore Hunk #4, which follows these lines, wasn't applied correctly.
These two parts were corrected in vtv_cygmin_clean.patch. For
convenience I also added vtv_cygmin_unclean.patch, which is the patch
from my last mail.
Regards,
Patrick
Re: [Ping] Port of VTV for Cygwin and MinGW
On Thu, Jan 8, 2015 at 12:33 PM, Patrick Wollgast patrick.wollg...@rub.de wrote: A short recap again: Latest patch, changelog and a test program (further information about the program in the mail): https://gcc.gnu.org/ml/gcc-patches/2014-11/msg03368.html Approved: * gcc/config/i386/* * libgcc/* * libstdc++-v3/* * libvtv/* (Some changes made to three of these files. Listed in 'Not approved'.) Not approved: For the following two files I added checks, if TARGET_PECOFF is defined ( https://gcc.gnu.org/ml/gcc-patches/2014-10/msg00815.html ) * gcc/cp/vtable-class-hierarchy.c * gcc/varasm.c The changes in gcc/cp/vtable-class-hierarchy.c and gcc/varasm.c both look good to me. However I am not authorized to approve stuff in this part of GCC, so I need someone with global approval rights to look at these changes and give the final OK. Reasons for changes in the following files stated in https://gcc.gnu.org/ml/gcc-patches/2014-10/msg00815.html and in the mail of the latest patch. Removed implementation of mprotect. * libvtv/vtv_malloc.cc Added extern C to the prototype of mprotect. * libvtv/vtv_malloc.h Exchanged call to TerminateProcess with call to abort in __fortify_fail. * libvtv/vtv_rts.cc The changes in libvtv all look good to me (approved). -- Caroline Tice cmt...@google.com Has been removed from the most recent patch. Just listed for completeness. * libiberty/obstack.c Regards, Patrick
[PATCH] Fix size type for cold partition names (hot-cold function partitioning)
When hot/cold function splitting occurs, a symbol is generated for the
cold partition, and gets output in the assembly debug info, but the
symbol currently gets a size of 0 and a type of NOTYPE, as in this
example (on x86_64-linux) from the cold_partition_label test in the
testsuite:
$ readelf -sW cold_partition_label.x02 | grep foo
36: 00400450 0 NOTYPE LOCAL DEFAULT 12 foo.cold.0
58: 0040049043 FUNCGLOBAL DEFAULT 12 foo
$
This patch fixes this by calculating the right size for the partition,
and outputing the size and type fo the cold partition symbol. After
applying this patch and looking at the same test, I get:
$ readelf -sW cold_partition_label.x02 | grep foo
36: 0040045029 FUNCLOCAL DEFAULT 12 foo.cold.0
58: 0040049043 FUNCGLOBAL DEFAULT 12 foo
$
This patch has been tested by bootstrapping the compiler, running the
dejagnu testsuite with no regressions, and checked as shown above that
it fixes the original problem. Is this patch OK to commit to ToT?
-- Caroline Tice
cmt...@google.com
2014-12-05 Caroline Tice cmt...@google.com
* final.c (final_scan_insn): Change 'cold_function_name' to
'cold_partition_name' and make it a global variable; also output
assembly to give it a 'FUNC' type, if appropriate.
* varasm.c (cold_partition_name): Declare and initialize global
variable.
(assemble_start_function): Re-set value for cold_partition_name.
(assemble_end_function): Output assembly to calculate size of cold
partition, and associate size with name, if appropriate.
* varash.h (cold_partition_name): Add extern declaration for global
variable.
Index: gcc/final.c
===
--- gcc/final.c (revision 218434)
+++ gcc/final.c (working copy)
@@ -2223,10 +2223,16 @@
suffixing cold to the original function's name. */
if (in_cold_section_p)
{
- tree cold_function_name
+ cold_partition_name
= clone_function_name (current_function_decl, cold);
+#ifdef ASM_DECLARE_FUNCTION_NAME
+ ASM_DECLARE_FUNCTION_NAME (asm_out_file,
+ IDENTIFIER_POINTER (cold_partition_name),
+ current_function_decl);
+#else
ASM_OUTPUT_LABEL (asm_out_file,
-IDENTIFIER_POINTER (cold_function_name));
+IDENTIFIER_POINTER (cold_partition_name));
+#endif
}
break;
Index: gcc/varasm.c
===
--- gcc/varasm.c (revision 218434)
+++ gcc/varasm.c (working copy)
@@ -171,6 +171,13 @@
at the cold section. */
bool in_cold_section_p;
+/* The following global holds the partition name for the code in the
+ cold section of a function, if hot/cold function splitting is enabled
+ and there was actually code that went into the cold section. A
+ pseudo function name is needed for the cold section of code for some
+ debugging tools that perform symbolization. */
+tree cold_partition_name = NULL_TREE;
+
/* A linked list of all the unnamed sections. */
static GTY(()) section *unnamed_sections;
@@ -1708,6 +1715,7 @@
ASM_GENERATE_INTERNAL_LABEL (tmp_label, LCOLDE, const_labelno);
crtl-subsections.cold_section_end_label = ggc_strdup (tmp_label);
const_labelno++;
+ cold_partition_name = NULL_TREE;
}
else
{
@@ -1843,6 +1851,10 @@
save_text_section = in_section;
switch_to_section (unlikely_text_section ());
+ if (cold_partition_name != NULL_TREE)
+ ASM_DECLARE_FUNCTION_SIZE (asm_out_file,
+ IDENTIFIER_POINTER (cold_partition_name),
+ decl);
ASM_OUTPUT_LABEL (asm_out_file, crtl-subsections.cold_section_end_label);
if (first_function_block_is_cold)
switch_to_section (text_section);
Index: gcc/varasm.h
===
--- gcc/varasm.h (revision 218434)
+++ gcc/varasm.h (working copy)
@@ -20,6 +20,13 @@
#ifndef GCC_VARASM_H
#define GCC_VARASM_H
+/* The following global holds the partition name for the code in the
+ cold section of a function, if hot/cold function splitting is enabled
+ and there was actually code that went into the cold section. A
+ pseudo function name is needed for the cold section of code for some
+ debugging tools that perform symbolization. */
+extern tree cold_partition_name;
+
extern tree tree_output_constant_def (tree);
extern void make_decl_rtl (tree);
extern rtx make_decl_rtl_for_debug (tree);
Re: [Ping] Port of VTV for Cygwin and MinGW
Ok, your patch looks OK to me, but I can only approve the libvtv file changes. The changes in the other files also seem ok to me, but someone else will have to approve the modifications in them: gcc/config/i386/cygwin.h gcc/config/i386/mingw-w64.h gcc/config/i386/mingw32.h gcc/cp/vtable-class-hierarchy.c gcc/varasm.c libgcc/Makefile.in libgcc/config.host libiberty/obstack.c libstdc++-v3/acinclude.m4 libstdc++-v3/libsupc++/Makefile.am libstdc++-v3/libsupc++/vtv_stubs.cc -- Caroline Tice cmt...@google.com On Thu, Sep 18, 2014 at 3:23 PM, Patrick Wollgast patrick.wollg...@rub.de wrote: Added Benjamin De Kosnik as a c++ runtime libs maintainer and Kai Tietz as Windows/Cygwin/MinGW maintainer. In changes to gcc/config/i386/cygwin.h mingw-w64.h and mingw32.h, you forgot to handle the fvtable-verify=preinit options. fvtable-veriy=preinit should cause vtv_start_preinit.o to be added to the STARTFILE_SPEC and vtv_end_preinit.o to be added to the ENDFILE_SPEC (as in gcc/config/gnu-user.h). I expect you will also need to add it to your LIB_SPEC definitions in those config files. Like discussed via email I set preinit=std. This required additional changes in gcc/cp/vtable-class-hierarchy.c. in libgcc/config.host, the indentation looks wrong on the line 660 (where you add the extra parts for vtable verification for the case i[34567]86-*-mingw*). It also looks wrong on line 709 (again, adding extra parts, for case x86_64-*-mingw*). The rest of the changes in that file look ok, but someone else will need to approve them. Indentation fixed. The changes in libgcc/Makefile.in and gcc/varasm.c look ok to me, but someone will will have to approve them since I don't have authority to approve changes there. in libstdc++-v3/libsupc++: Your change in Makefile.am looks ok to me; your changes in vtv_stubs.cc look ok, except that you appear to have messed up the indentations in the function headers (both for the declarations and the actual functions). Also there is a typo in your comment: 'build' should be 'built'. But content-wise, the change looks fine. Again, someone else will have to actually approve these changes since I do not have that authority. Typo and indentation fixed. in libstdc++-v3/src/Makefile.am also looks ok to me; someone else will have to give final approval. in libvtv/Makefile.am, you need to fix the indentation at line 64 (vtv_stubs.cc): vtv_stubs_sources = \ vtv_start.c \ vtv_stubs.cc \ vtv_end.c Indentation fixed. the rest of the changes in that file look good. Why did you make a copy of obstack.c in libvtv rather than using the one in libiberty? It would be better not to make a second copy of the source file if that can be avoided... I removed obstack.c from libvtv and added the changes from libvtv/obstack.c to libiberty/obstack.c with #ifdefs. in libvtv/vtv_malloc.cc: lines 207-213: Fix the indentation on the second line of call to VirtualAlloc. #if defined (__CYGWIN__) || defined (__MINGW32__) if ((allocated = VirtualAlloc(NULL, size, MEM_RESERVE|MEM_COMMIT, PAGE_READWRITE)) == 0) #else if ((allocated = mmap (NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0)) == 0) #endif Remove extra blank line at line 216. Line removed and indentation fixed. in libvtv/vtv_rts.cc: Your version of the function read_section_offset_and_length has several lines that exceed the 80 character limit; please fix that. Your function iterate_modules also has one or two lines that are too long, and it needs a function comment. Character per line are now correct and comment added. in libvtv/vtv_utils.cc: In the function __vtv_open_log, you seem to have the following code twice: #ifdef __MINGW32__ mkdir (logs_prefix); #else mkdir (logs_prefix, S_IRWXU); #endif was there a reason for this, or is this an accident (in which case the second occurrence should be removed)? Should have been 'log_dir' instead of 'logs_prefix' the 2nd time. Fixed now. regards Patrick
Re: [Ping] Port of VTV for Cygwin and MinGW
First attempt to send this failed. On Fri, Sep 12, 2014 at 3:41 PM, Caroline Tice cmt...@google.com wrote: Hi Patrick, Mostly your patch looks OK to me, though there are a couple of serious issues (mentioned below). Most of my comments are for formatting stuff. Once you have fixed these issues, let me know and I'll look at it again. But someone else will still have to approve the parts of this patch that are outside the libvtv directory (I believe). -- Caroline Tice cmt...@google.com In changes to gcc/config/i386/cygwin.h mingw-w64.h and mingw32.h, you forgot to handle the fvtable-verify=preinit options. fvtable-veriy=preinit should cause vtv_start_preinit.o to be added to the STARTFILE_SPEC and vtv_end_preinit.o to be added to the ENDFILE_SPEC (as in gcc/config/gnu-user.h). I expect you will also need to add it to your LIB_SPEC definitions in those config files. in libgcc/config.host, the indentation looks wrong on the line 660 (where you add the extra parts for vtable verification for the case i[34567]86-*-mingw*). It also looks wrong on line 709 (again, adding extra parts, for case x86_64-*-mingw*). The rest of the changes in that file look ok, but someone else will need to approve them. The changes in libgcc/Makefile.in and gcc/varasm.c look ok to me, but someone will will have to approve them since I don't have authority to approve changes there. in libstdc++-v3/libsupc++: Your change in Makefile.am looks ok to me; your changes in vtv_stubs.cc look ok, except that you appear to have messed up the indentations in the function headers (both for the declarations and the actual functions). Also there is a typo in your comment: 'build' should be 'built'. But content-wise, the change looks fine. Again, someone else will have to actually approve these changes since I do not have that authority. in libstdc++-v3/src/Makefile.am also looks ok to me; someone else will have to give final approval. in libvtv/Makefile.am, you need to fix the indentation at line 64 (vtv_stubs.cc): vtv_stubs_sources = \ vtv_start.c \ vtv_stubs.cc \ vtv_end.c the rest of the changes in that file look good. Why did you make a copy of obstack.c in libvtv rather than using the one in libiberty? It would be better not to make a second copy of the source file if that can be avoided... in libvtv/vtv_malloc.cc: lines 207-213: Fix the indentation on the second line of call to VirtualAlloc. #if defined (__CYGWIN__) || defined (__MINGW32__) if ((allocated = VirtualAlloc(NULL, size, MEM_RESERVE|MEM_COMMIT, PAGE_READWRITE)) == 0) #else if ((allocated = mmap (NULL, size, PROT_READ | PROT_WRITE, MAP_PRIVATE | MAP_ANONYMOUS, -1, 0)) == 0) #endif Remove extra blank line at line 216. in libvtv/vtv_rts.cc: Your version of the function read_section_offset_and_length has several lines that exceed the 80 character limit; please fix that. Your function iterate_modules also has one or two lines that are too long, and it needs a function comment. in libvtv/vtv_utils.cc: In the function __vtv_open_log, you seem to have the following code twice: #ifdef __MINGW32__ mkdir (logs_prefix); #else mkdir (logs_prefix, S_IRWXU); #endif was there a reason for this, or is this an accident (in which case the second occurrence should be removed)? On Wed, Sep 10, 2014 at 9:12 PM, Patrick Wollgast patrick.wollg...@rub.de wrote: Ping for https://gcc.gnu.org/ml/gcc-patches/2014-08/msg02559.html Also added Caroline Tice, as libvtv maintainer, to cc and attached virtual_func_test_min_UAF.cpp, which I forgot in the original mail. Patrick On 28.08.2014 13:03, Patrick Wollgast wrote: This patch contains a port of VTV -fvtable-verify=std for Cygwin and MinGW. Since weak symbols on Windows and Linux are implemented differently, and VTV should have the possibility to be switched on and off, the structure of the feature had to be modified. On Linux libstdc++ contains the weak stub functions of VTV. For Cygwin and MinGW they have been removed, due to the difference of weak symbols. On Linux and on Windows libstdc++ itself gets build with -fvtable-verify=std. Since libvtv gets build after libstdc++, and libstdc++ doesn't contain the stub functions any more, 'undefined reference' errors are thrown during linking of libstdc++. To prevent these errors during the linking process a libvtv-0.dll gets build from the stub functions before libstdc++-6.dll is linked. At the end of the build process two VTV dlls have been build. One is called libvtv-0.dll, containing the real functions, the other is called libvtv_stubs-0.dll, containing the stub functions. Depending on whether libvtv-0.dll is first found in the dll search path or libvtv_stubs-0.dll, renamed to libvtv-0.dll, the real functions or the stub functions
[PATCH] Fix small vtable verification bugs.
The attached patch fixes two small problems with the current vtable
verification code: it makes the libvtv function decls globally
visible, and it updates all uses of the verified vtable pointer with
the verification results, rather than just the first use.
I have bootstrapped the compiler with this patch, both with and
without vtable verification enabled, and have run the regression
testsuites, both with and without vtable verification enabled, with no
regressions. I have only tested this on Linux.
Is this patch OK to commit?
-- Caroline Tice
cmt...@google.com
gcc ChangeLog:
2013-12-06 Caroline Tice cmt...@google.com
Submitting patch from Stephen Checkoway, s...@cs.jhu.edu
* vtable-class-hierarchy.c (init_functions): Make the libvtv
function decls externally visible.
gcc/cp ChangeLog:
2013-12-06 Caroline Tice cmt...@google.com
Submitting patch from Stephen Checkoway, s...@cs.jhu.edu
* vtable-class-hierarchy.c (init_functions): Make the libvtv
function decls externally visible.
Index: gcc/cp/vtable-class-hierarchy.c
===
--- gcc/cp/vtable-class-hierarchy.c (revision 205756)
+++ gcc/cp/vtable-class-hierarchy.c (working copy)
@@ -258,6 +258,7 @@ init_functions (void)
DECL_ATTRIBUTES (vlt_register_set_fndecl) =
tree_cons (get_identifier (leaf), NULL,
DECL_ATTRIBUTES (vlt_register_set_fndecl));
+ DECL_EXTERNAL(vlt_register_set_fndecl) = 1;
TREE_PUBLIC (vlt_register_set_fndecl) = 1;
DECL_PRESERVE_P (vlt_register_set_fndecl) = 1;
SET_DECL_LANGUAGE (vlt_register_set_fndecl, lang_cplusplus);
@@ -301,6 +302,7 @@ init_functions (void)
DECL_ATTRIBUTES (vlt_register_pairs_fndecl) =
tree_cons (get_identifier (leaf), NULL,
DECL_ATTRIBUTES (vlt_register_pairs_fndecl));
+ DECL_EXTERNAL(vlt_register_pairs_fndecl) = 1;
TREE_PUBLIC (vlt_register_pairs_fndecl) = 1;
DECL_PRESERVE_P (vlt_register_pairs_fndecl) = 1;
SET_DECL_LANGUAGE (vlt_register_pairs_fndecl, lang_cplusplus);
Index: gcc/vtable-verify.c
===
--- gcc/vtable-verify.c (revision 205756)
+++ gcc/vtable-verify.c (working copy)
@@ -646,9 +646,6 @@ verify_bb_vtables (basic_block bb)
if (vtable_map_node vtable_map_node-vtbl_map_decl)
{
- use_operand_p use_p;
- ssa_op_iter iter;
-
vtable_map_node-is_used = true;
vtbl_var_decl = vtable_map_node-vtbl_map_decl;
@@ -695,35 +692,27 @@ verify_bb_vtables (basic_block bb)
gimple_call_set_lhs (call_stmt, tmp0);
update_stmt (call_stmt);
- /* Find the next stmt, after the vptr assignment
- statememt, which should use the result of the
- vptr assignment statement value. */
- gsi_next (gsi_vtbl_assign);
- gimple next_stmt = gsi_stmt (gsi_vtbl_assign);
-
- if (!next_stmt)
-return;
-
- /* Find any/all uses of 'lhs' in next_stmt, and
- replace them with 'tmp0'. */
+ /* Replace all uses of lhs with tmp0. */
found = false;
- FOR_EACH_PHI_OR_STMT_USE (use_p, next_stmt, iter,
-SSA_OP_ALL_USES)
+ imm_use_iterator iterator;
+ gimple use_stmt;
+ FOR_EACH_IMM_USE_STMT (use_stmt, iterator, lhs)
{
- tree op = USE_FROM_PTR (use_p);
- if (op == lhs)
-{
- SET_USE (use_p, tmp0);
- found = true;
-}
+ use_operand_p use_p;
+ if (use_stmt == call_stmt)
+continue;
+ FOR_EACH_IMM_USE_ON_STMT (use_p, iterator)
+SET_USE (use_p, tmp0);
+ update_stmt (use_stmt);
+ found = true;
}
- update_stmt (next_stmt);
+
gcc_assert (found);
/* Insert the new verification call just after the
statement that gets the vtable pointer out of the
object. */
- gsi_vtbl_assign = gsi_for_stmt (stmt);
+ gcc_assert (gsi_stmt (gsi_vtbl_assign) == stmt);
gsi_insert_after (gsi_vtbl_assign, call_stmt,
GSI_NEW_STMT);
[PATCH, PR 58441] Fix location where libvtv puts its headers
The following patch updates where libvtv installs its header files (fixing PR 58441). This is a trivial change, and affects only libvtv, so I am going to go ahead and commit it. -- Caroline Tice cmt...@google.com 2013-09-24 Caroline Tice cmt...@google.com * Makefile.am: Change libvtv_includedir to the directory used by the other libraries rather than the top include directory. * Makefile.in: Regenerated. libvtv-headers.patch Description: Binary data
Re: [PATCH, libvtv] Fix configure/testsuite issues with libvtv
(Putting into plain test for gcc-patches list).
On Mon, Sep 23, 2013 at 11:42 AM, Caroline Tice cmt...@google.com wrote:
Ping?
On Thu, Sep 12, 2013 at 10:44 AM, Caroline Tice cmt...@google.com wrote:
On Wed, Sep 11, 2013 at 12:35 PM, H.J. Lu hjl.to...@gmail.com wrote:
On Wed, Sep 11, 2013 at 12:27 PM, Caroline Tice cmt...@google.com
wrote:
2. Why does libvtv/configure.ac have
echo 'MULTISUBDIR =' $ac_file
ml_norecursion=yes
. ${multi_basedir}/config-ml.in
when AM_ENABLE_MULTILIB is used? I got
make[3]: Entering directory
`/export/build/gnu/gcc/build-x86_64-linux/x86_64-unknown-linux-gnu/libvtv'
Makefile:844: warning: overriding recipe for target `multi-do'
Makefile:775: warning: ignoring old recipe for target `multi-do'
Makefile:892: warning: overriding recipe for target `multi-clean'
Makefile:823: warning: ignoring old recipe for target `multi-clean'
I do not know exactly why it was done this way, but I see that the
configure.ac files for libsanitizer and libstdc++-v3 seem to do it
exactly this way as well.
They have
AC_CONFIG_FILES(AC_FOREACH([DIR], [tsan], [DIR/Makefile ])
libvtv has
AC_CONFIG_FILES(AC_FOREACH([DIR], [. testsuite], [DIR/Makefile ]),
^ Is
the extra . needed?
Apparently the extra . is not needed.
Are either of these issues actually causing you problems at the moment?
No.
--
H.J.
I am attaching an updated patch that updates the configure and make
files in libvtv to not attempt to
build/run the testsuite unless --enable-vtable-verify was passed. It
also removes the . that was resulting in the
warnings H.J. Lu mentioned. This new patch has been tested both with
and without --enable-vtable-verify. As I mentioned before, I would
appreciate someone else reviewing my patch, as a sanity check.
Thanks!
2013-09-12 Caroline Tice cmt...@google.com
* Makefile.am: Re-instante ENABLE_VTABLE_VERIFY checks, to make
sure testsuite is not run if libstdc++ and libgcc were not built
with vtable verification.
* Makefile.in: Regenerated.
* configure.ac: Re-instatate checks for --enable-vtable-verify
flag,
to make sure testsuite is not run if libstdc++ and libgcc were not
built with vtable verification.
* configure: Regenerated.
Re: [PATCH, libvtv] Fix configure/testsuite issues with libvtv
On Wed, Sep 11, 2013 at 12:35 PM, H.J. Lu hjl.to...@gmail.com wrote:
On Wed, Sep 11, 2013 at 12:27 PM, Caroline Tice cmt...@google.com wrote:
2. Why does libvtv/configure.ac have
echo 'MULTISUBDIR =' $ac_file
ml_norecursion=yes
. ${multi_basedir}/config-ml.in
when AM_ENABLE_MULTILIB is used? I got
make[3]: Entering directory
`/export/build/gnu/gcc/build-x86_64-linux/x86_64-unknown-linux-gnu/libvtv'
Makefile:844: warning: overriding recipe for target `multi-do'
Makefile:775: warning: ignoring old recipe for target `multi-do'
Makefile:892: warning: overriding recipe for target `multi-clean'
Makefile:823: warning: ignoring old recipe for target `multi-clean'
I do not know exactly why it was done this way, but I see that the
configure.ac files for libsanitizer and libstdc++-v3 seem to do it
exactly this way as well.
They have
AC_CONFIG_FILES(AC_FOREACH([DIR], [tsan], [DIR/Makefile ])
libvtv has
AC_CONFIG_FILES(AC_FOREACH([DIR], [. testsuite], [DIR/Makefile ]),
^ Is
the extra . needed?
Apparently the extra . is not needed.
Are either of these issues actually causing you problems at the moment?
No.
--
H.J.
I am attaching an updated patch that updates the configure and make
files in libvtv to not attempt to
build/run the testsuite unless --enable-vtable-verify was passed. It
also removes the . that was resulting in the
warnings H.J. Lu mentioned. This new patch has been tested both with
and without --enable-vtable-verify. As I mentioned before, I would
appreciate someone else reviewing my patch, as a sanity check.
Thanks!
2013-09-12 Caroline Tice cmt...@google.com
* Makefile.am: Re-instante ENABLE_VTABLE_VERIFY checks, to make
sure testsuite is not run if libstdc++ and libgcc were not built
with vtable verification.
* Makefile.in: Regenerated.
* configure.ac: Re-instatate checks for --enable-vtable-verify flag,
to make sure testsuite is not run if libstdc++ and libgcc were not
built with vtable verification.
* configure: Regenerated.
libvtv-configure.patch
Description: Binary data
Re: [libvtv] Remove Android from supported targets
Yes, that patch is ok.
-- Caroline Tice
cmt...@google.com
On Thu, Sep 12, 2013 at 3:56 AM, Alexander Ivchenko aivch...@gmail.com wrote:
Hi,
We currently have build problem in Android ndk for trunk:
toolchain/gcc/gcc-4.9/libvtv/vtv_rts.cc:124:22: fatal error:
execinfo.h: No such file or directory
#include execinfo.h
^
compilation terminated.
toolchain/gcc/gcc-4.9/libvtv/vtv_utils.cc:36:22: fatal error:
execinfo.h: No such file or directory
#include execinfo.h
^
compilation terminated.
make[4]: *** [vtv_rts.lo] Error 1
make[4]: *** Waiting for unfinished jobs
make[4]: *** [vtv_utils.lo] Error 1
There is no execinfo.h in Bionic.
Is following patch OK?
diff --git a/libvtv/ChangeLog b/libvtv/ChangeLog
index 3c344f9..f0a7471 100644
--- a/libvtv/ChangeLog
+++ b/libvtv/ChangeLog
@@ -1,3 +1,7 @@
+2013-09-12 Alexander Ivchenko alexander.ivche...@intel.com
+
+ * configure.tgt: Remove *-*-*android* from supported targets.
+
2013-09-09 H.J. Lu hongjiu...@intel.com
PR other/58374
diff --git a/libvtv/configure.tgt b/libvtv/configure.tgt
index 801d2f0..046b415 100644
--- a/libvtv/configure.tgt
+++ b/libvtv/configure.tgt
@@ -21,6 +21,8 @@
# Filter out unsupported systems.
VTV_SUPPORTED=no
case ${target} in
+ *-*-*android*)
+ ;;
x86_64-*-linux* | i?86-*-linux*)
VTV_SUPPORTED=yes
;;
thanks,
Alexander
[PATCH, libvtv] Fix configure/testsuite issues with libvtv
This patch should fix the issues that people were having with the libvtv testsuite being run (and failing) when GCC was not configured with --enable-vtable-verify. I am still in the process of running some tests, but at this point I am fairly confident that this patch works correctly. Even though I am the libvtv maintainer, I would appreciate someone else reviewing this patch too, as a sanity check. -- Caroline Tice cmt...@google.com 2013-09-11 Caroline Tice cmt...@google.com * Makefile.am: Re-instante ENABLE_VTABLE_VERIFY checks, to make sure testsuite is not run if libstdc++ and libgcc were not built with vtable verification. * Makefile.in: Regenerated. * configure.ac: Re-instatate checks for --enable-vtable-verify flag, to make sure testsuite is not run if libstdc++ and libgcc were not built with vtable verification. * configure: Regenerated. libvtv-configure.patch Description: Binary data
Re: [PATCH, libvtv] Fix configure/testsuite issues with libvtv
On Wed, Sep 11, 2013 at 12:07 PM, H.J. Lu hjl.to...@gmail.com wrote:
On Wed, Sep 11, 2013 at 11:22 AM, Caroline Tice cmt...@google.com wrote:
This patch should fix the issues that people were having with the
libvtv testsuite being run (and failing) when GCC was not configured
with --enable-vtable-verify. I am still in the process of running
some tests, but at this point I am fairly confident that this patch
works correctly. Even though I am the libvtv maintainer, I would
appreciate someone else reviewing this patch too, as a sanity check.
-- Caroline Tice
cmt...@google.com
2013-09-11 Caroline Tice cmt...@google.com
* Makefile.am: Re-instante ENABLE_VTABLE_VERIFY checks, to make
sure testsuite is not run if libstdc++ and libgcc were not built
with vtable verification.
* Makefile.in: Regenerated.
* configure.ac: Re-instatate checks for --enable-vtable-verify flag,
to make sure testsuite is not run if libstdc++ and libgcc were not
built with vtable verification.
* configure: Regenerated.
I tried it. I noticed 2 issues:
1. Correct if I am wrong. libvtv doesn't work correctly if
--enable-vtable-verify
isn't used. Why is libvtv built/installed at all when
--enable-vtable-verify isn't used.
The short answer is because Benjamin Kosnik thought we should do it
this way. Benajmin, who helped me write most of the original
configure and Makefile stuff for libvtv, seemed to feel that it was
best to build libvtv whenever the architecture would allow it. I do
not fully understand his reasoning, so it may be best to let him
answer this one.
2. Why does libvtv/configure.ac have
echo 'MULTISUBDIR =' $ac_file
ml_norecursion=yes
. ${multi_basedir}/config-ml.in
when AM_ENABLE_MULTILIB is used? I got
make[3]: Entering directory
`/export/build/gnu/gcc/build-x86_64-linux/x86_64-unknown-linux-gnu/libvtv'
Makefile:844: warning: overriding recipe for target `multi-do'
Makefile:775: warning: ignoring old recipe for target `multi-do'
Makefile:892: warning: overriding recipe for target `multi-clean'
Makefile:823: warning: ignoring old recipe for target `multi-clean'
I do not know exactly why it was done this way, but I see that the
configure.ac files for libsanitizer and libstdc++-v3 seem to do it
exactly this way as well.
Are either of these issues actually causing you problems at the moment?
-- Caroline
cmt...@google.com
--
H.J.
Re: PATCH: PR other/58374: Wrong target check in configure.ac in libvtv
On Tue, Sep 10, 2013 at 9:10 AM, Jakub Jelinek ja...@redhat.com wrote: On Tue, Sep 10, 2013 at 08:55:35AM -0700, Caroline Tice wrote: Based on the errors you are reporting, my guess is that you did not configure with --enable-vtable-verifiy. The testsuite in libvtv will I'm not, but it doesn't make sense if you don't configure gcc in certain non-default way, you get dozens of FAILs during the make check. If libvtv doesn't work at all without --enable-vtable-verify, perhaps it shouldn't be built, or at least make check shouldn't be run? I didn't realize the testsuite in libvtv would be run unless explicitly requested. I agree that there should be a test to see whether --enable-vtable-verify was set before attempting to run the libvtv testsuite. This is not my area of expertise and if someone could point me to an example or show me where/how to put such a conditional test, I would appreciate it. Does --enable-vtable-verify affect anything but the 4 extra objects built in libgcc subdir that aren't actually linked into libgcc / libgcc_s? --enable-vtable-verify also affects how libstdc++ is built. That is for sure what is causing the error you are seeing below. That particular test case (template-list-iostream) tests whether a particular libstdc++ class passes verification. If libstdc++ was not built with vtable verification, then it will fail exactly as shown below. I have never seen that; if you could forward the complaints/errors to me I would appreciate it. E.g. Running /usr/src/gcc/libvtv/testsuite/libvtv.cc/vtv.exp ... *** Potential vtable pointer corruption detected!! ***: ./template-list-iostream.exe terminated === Backtrace: = /lib64/libc.so.6(__fortify_fail+0x37)[0x7f2cda8134d7] /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/./libvtv/.libs/libvtv.so.0(+0x3289)[0x7f2cdb2dd289] /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/./libvtv/.libs/libvtv.so.0(+0x338f)[0x7f2cdb2dd38f] /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/./libvtv/.libs/libvtv.so.0(_Z24__VLTVerifyVtablePointerPPvPKv+0xb0)[0x7f2cdb2dd570] ./template-list-iostream.exe[0x4012c2] ./template-list-iostream.exe[0x401366] /lib64/libc.so.6(__libc_start_main+0xf5)[0x7f2cda72b735] ./template-list-iostream.exe[0x401131] === Memory map: 0040-00404000 r-xp fd:02 18150808 /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/libvtv/testsuite/template-list-iostream.exe 00603000-00604000 rw-p 3000 fd:02 18150808 /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/libvtv/testsuite/template-list-iostream.exe 00604000-00605000 r--p 4000 fd:02 18150808 /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/libvtv/testsuite/template-list-iostream.exe 00605000-00606000 rw-p 5000 fd:02 18150808 /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/libvtv/testsuite/template-list-iostream.exe 01867000-01888000 rw-p 00:00 0 [heap] 7f2cda70a000-7f2cda8b6000 r-xp fd:00 2884204 /usr/lib64/libc-2.15.so 7f2cda8b6000-7f2cdaab6000 ---p 001ac000 fd:00 2884204 /usr/lib64/libc-2.15.so 7f2cdaab6000-7f2cdaaba000 r--p 001ac000 fd:00 2884204 /usr/lib64/libc-2.15.so 7f2cdaaba000-7f2cdaabc000 rw-p 001b fd:00 2884204 /usr/lib64/libc-2.15.so 7f2cdaabc000-7f2cdaac1000 rw-p 00:00 0 7f2cdaac1000-7f2cdaad6000 r-xp fd:02 17643616 /usr/src/gcc/obj716/gcc/libgcc_s.so.1 7f2cdaad6000-7f2cdacd5000 ---p 00015000 fd:02 17643616 /usr/src/gcc/obj716/gcc/libgcc_s.so.1 7f2cdacd5000-7f2cdacd6000 rw-p 00014000 fd:02 17643616 /usr/src/gcc/obj716/gcc/libgcc_s.so.1 7f2cdacd6000-7f2cdadd r-xp fd:00 2901742 /usr/lib64/libm-2.15.so 7f2cdadd-7f2cdafcf000 ---p 000fa000 fd:00 2901742 /usr/lib64/libm-2.15.so 7f2cdafcf000-7f2cdafd r--p 000f9000 fd:00 2901742 /usr/lib64/libm-2.15.so 7f2cdafd-7f2cdafd1000 rw-p 000fa000 fd:00 2901742 /usr/lib64/libm-2.15.so 7f2cdafd1000-7f2cdb0bc000 r-xp fd:02 18130055 /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/libstdc++-v3/src/.libs/libstdc++.so.6.0.19 7f2cdb0bc000-7f2cdb2bb000 ---p 000eb000 fd:02 18130055 /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/libstdc++-v3/src/.libs/libstdc++.so.6.0.19 7f2cdb2bb000-7f2cdb2c3000 r--p 000ea000 fd:02 18130055 /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/libstdc++-v3/src/.libs/libstdc++.so.6.0.19 7f2cdb2c3000-7f2cdb2c5000 rw-p 000f2000 fd:02 18130055 /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/libstdc++-v3/src/.libs/libstdc++.so.6.0.19 7f2cdb2c5000-7f2cdb2da000 rw-p 00:00 0 7f2cdb2da000-7f2cdb2e2000 r-xp fd:02 18130772
Re: PATCH: PR other/58374: Wrong target check in configure.ac in libvtv
On Tue, Sep 10, 2013 at 3:17 AM, Jakub Jelinek ja...@redhat.com wrote: On Mon, Sep 09, 2013 at 01:56:29PM -0700, Caroline Tice wrote: The patch looks good to me, but somebody else needs to approve it. Why? You are listed as libvtv maintainer, the patch is fully contained in libvtv, thus you can review it and approve. There was a bit of communication confusion and I originally misunderstood things. I thought I was the libvtv maintainer, but so far i have only been proposed (am being proposed?) to the steering committee to be the libvtv maintainer.Until the steering committee officially approves that, I am not actually the maintainer, and I do not have the authority to approve patches. On an unrelated note, what kind of testing was performed on libvtv testsuite? I'm seeing very bad results on both x86_64-linux and i686-linux, like === libvtv Summary === # of expected passes92 # of unexpected failures44 # of unresolved testcases 40 Many of the errors seem to be related to missing vtv_start_preinit.o not being found (why do those actually live in libgcc rather than libvtv?), libvtv/Makefile.am has only rules for vtv_start.c etc., but not vtv_start_preinit.c. Actually I performed a lot of testing of this testsuite, but it was all on my linux x86_64 system.I consistently get it passing all the tests: === libvtv tests === Schedule of variations: unix Running target unix Using /usr/local/share/dejagnu/baseboards/unix.exp as board description file for target. Using /usr/local/share/dejagnu/config/unix.exp as generic interface file for target. Using /usr/local/google2/cmtice/gcc-fsf/libvtv/testsuite/config/default.exp as tool-and-target-specific interface file. Running /usr/local/google2/cmtice/gcc-fsf/libvtv/testsuite/libvtv.cc/vtv.exp ... Running /usr/local/google2/cmtice/gcc-fsf/libvtv/testsuite/libvtv.mempool.cc/mempool.exp ... Running /usr/local/google2/cmtice/gcc-fsf/libvtv/testsuite/libvtv.mt.cc/mt.exp ... === libvtv Summary === # of expected passes 176 make[1]: Leaving directory `/usr/local/google2/cmtice/gcc-fsf.obj/x86_64-unknown-linux-gnu/libvtv/testsuite' Based on the errors you are reporting, my guess is that you did not configure with --enable-vtable-verifiy. The testsuite in libvtv will not pass without vtable verification enabled. If you DID configure with --enable-vtable-verify, then please send me the rest of your configure command so that I can try to reproduce your failures. But I also see complains from glibc about corrupted malloc state in some tests, tests with such undefined behavior IMHO don't belong into the testsuite, unless you can prevent the corruption before it happens. I have never seen that; if you could forward the complaints/errors to me I would appreciate it. -- Caroline Tice cmt...@google.com Jakub
Re: PATCH: PR other/58374: Wrong target check in configure.ac in libvtv
I believe I know how to fix this problem; I am in the middle of testing my patch. I should be able to submit the patch either late today or early tomorrow. In the meantime, if this is impacting your ability to make progress, you can configure gcc with --disable-libvtv and that should remove the problem. -- Caroline cmt...@google.com On Tue, Sep 10, 2013 at 9:17 AM, Caroline Tice cmt...@google.com wrote: On Tue, Sep 10, 2013 at 9:10 AM, Jakub Jelinek ja...@redhat.com wrote: On Tue, Sep 10, 2013 at 08:55:35AM -0700, Caroline Tice wrote: Based on the errors you are reporting, my guess is that you did not configure with --enable-vtable-verifiy. The testsuite in libvtv will I'm not, but it doesn't make sense if you don't configure gcc in certain non-default way, you get dozens of FAILs during the make check. If libvtv doesn't work at all without --enable-vtable-verify, perhaps it shouldn't be built, or at least make check shouldn't be run? I didn't realize the testsuite in libvtv would be run unless explicitly requested. I agree that there should be a test to see whether --enable-vtable-verify was set before attempting to run the libvtv testsuite. This is not my area of expertise and if someone could point me to an example or show me where/how to put such a conditional test, I would appreciate it. Does --enable-vtable-verify affect anything but the 4 extra objects built in libgcc subdir that aren't actually linked into libgcc / libgcc_s? --enable-vtable-verify also affects how libstdc++ is built. That is for sure what is causing the error you are seeing below. That particular test case (template-list-iostream) tests whether a particular libstdc++ class passes verification. If libstdc++ was not built with vtable verification, then it will fail exactly as shown below. I have never seen that; if you could forward the complaints/errors to me I would appreciate it. E.g. Running /usr/src/gcc/libvtv/testsuite/libvtv.cc/vtv.exp ... *** Potential vtable pointer corruption detected!! ***: ./template-list-iostream.exe terminated === Backtrace: = /lib64/libc.so.6(__fortify_fail+0x37)[0x7f2cda8134d7] /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/./libvtv/.libs/libvtv.so.0(+0x3289)[0x7f2cdb2dd289] /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/./libvtv/.libs/libvtv.so.0(+0x338f)[0x7f2cdb2dd38f] /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/./libvtv/.libs/libvtv.so.0(_Z24__VLTVerifyVtablePointerPPvPKv+0xb0)[0x7f2cdb2dd570] ./template-list-iostream.exe[0x4012c2] ./template-list-iostream.exe[0x401366] /lib64/libc.so.6(__libc_start_main+0xf5)[0x7f2cda72b735] ./template-list-iostream.exe[0x401131] === Memory map: 0040-00404000 r-xp fd:02 18150808 /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/libvtv/testsuite/template-list-iostream.exe 00603000-00604000 rw-p 3000 fd:02 18150808 /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/libvtv/testsuite/template-list-iostream.exe 00604000-00605000 r--p 4000 fd:02 18150808 /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/libvtv/testsuite/template-list-iostream.exe 00605000-00606000 rw-p 5000 fd:02 18150808 /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/libvtv/testsuite/template-list-iostream.exe 01867000-01888000 rw-p 00:00 0 [heap] 7f2cda70a000-7f2cda8b6000 r-xp fd:00 2884204 /usr/lib64/libc-2.15.so 7f2cda8b6000-7f2cdaab6000 ---p 001ac000 fd:00 2884204 /usr/lib64/libc-2.15.so 7f2cdaab6000-7f2cdaaba000 r--p 001ac000 fd:00 2884204 /usr/lib64/libc-2.15.so 7f2cdaaba000-7f2cdaabc000 rw-p 001b fd:00 2884204 /usr/lib64/libc-2.15.so 7f2cdaabc000-7f2cdaac1000 rw-p 00:00 0 7f2cdaac1000-7f2cdaad6000 r-xp fd:02 17643616 /usr/src/gcc/obj716/gcc/libgcc_s.so.1 7f2cdaad6000-7f2cdacd5000 ---p 00015000 fd:02 17643616 /usr/src/gcc/obj716/gcc/libgcc_s.so.1 7f2cdacd5000-7f2cdacd6000 rw-p 00014000 fd:02 17643616 /usr/src/gcc/obj716/gcc/libgcc_s.so.1 7f2cdacd6000-7f2cdadd r-xp fd:00 2901742 /usr/lib64/libm-2.15.so 7f2cdadd-7f2cdafcf000 ---p 000fa000 fd:00 2901742 /usr/lib64/libm-2.15.so 7f2cdafcf000-7f2cdafd r--p 000f9000 fd:00 2901742 /usr/lib64/libm-2.15.so 7f2cdafd-7f2cdafd1000 rw-p 000fa000 fd:00 2901742 /usr/lib64/libm-2.15.so 7f2cdafd1000-7f2cdb0bc000 r-xp fd:02 18130055 /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/libstdc++-v3/src/.libs/libstdc++.so.6.0.19 7f2cdb0bc000-7f2cdb2bb000 ---p 000eb000 fd:02 18130055 /usr/src/gcc/obj716/x86_64-unknown-linux-gnu/libstdc++-v3/src/.libs/libstdc++.so.6.0.19 7f2cdb2bb000
Re: PATCH: PR other/58374: Wrong target check in configure.ac in libvtv
The patch looks good to me, but somebody else needs to approve it.
-- Caroline Tice
cmt...@google.com
On Mon, Sep 9, 2013 at 11:48 AM, H.J. Lu hongjiu...@intel.com wrote:
configure.tgt in libvtv only recognizes canonical targets. This patch
moves the VTV_SUPPORTED check after AC_CANONICAL_SYSTEM for targets
like i686-linux or x86_64-linux. OK to install?
Thanks.
H.J.
---
diff --git a/libvtv/ChangeLog b/libvtv/ChangeLog
index 20092f4..3c344f9 100644
--- a/libvtv/ChangeLog
+++ b/libvtv/ChangeLog
@@ -1,3 +1,9 @@
+2013-09-09 H.J. Lu hongjiu...@intel.com
+
+ PR other/58374
+ * configure.ac: Move VTV_SUPPORTED check after AC_CANONICAL_SYSTEM.
+ * configure: Regenerated.
+
2013-09-08 Caroline Tice cmt...@google.com
* testsuite/event-main.cc: Move to libvtv.cc subdirectory.
diff --git a/libvtv/configure b/libvtv/configure
index bb56cb0..0bd614a 100755
--- a/libvtv/configure
+++ b/libvtv/configure
@@ -2339,22 +2339,6 @@ fi
{ $as_echo $as_me:${as_lineno-$LINENO}: result: $version_specific_libs 5
$as_echo $version_specific_libs 6; }
-# See if supported.
-unset VTV_SUPPORTED
-{ $as_echo $as_me:${as_lineno-$LINENO}: checking for host support for
vtable verification 5
-$as_echo_n checking for host support for vtable verification... 6; }
-. ${srcdir}/configure.tgt
-{ $as_echo $as_me:${as_lineno-$LINENO}: result: $VTV_SUPPORTED 5
-$as_echo $VTV_SUPPORTED 6; }
-
-# Decide if it's usable.
-use_vtable_verify=no
-if test x$VTV_SUPPORTED = xyes; then
- use_vtable_verify=yes
- { $as_echo $as_me:${as_lineno-$LINENO}: using vtable verification 5
-$as_echo $as_me: using vtable verification 6;}
-fi
-
# Do not delete or change the following two lines. For why, see
# http://gcc.gnu.org/ml/libstdc++/2003-07/msg00451.html
ac_aux_dir=
@@ -2522,6 +2506,22 @@ esac
+# See if supported.
+unset VTV_SUPPORTED
+{ $as_echo $as_me:${as_lineno-$LINENO}: checking for host support for
vtable verification 5
+$as_echo_n checking for host support for vtable verification... 6; }
+. ${srcdir}/configure.tgt
+{ $as_echo $as_me:${as_lineno-$LINENO}: result: $VTV_SUPPORTED 5
+$as_echo $VTV_SUPPORTED 6; }
+
+# Decide if it's usable.
+use_vtable_verify=no
+if test x$VTV_SUPPORTED = xyes; then
+ use_vtable_verify=yes
+ { $as_echo $as_me:${as_lineno-$LINENO}: using vtable verification 5
+$as_echo $as_me: using vtable verification 6;}
+fi
+
am__api_version='1.11'
# Find a good install program. We prefer a C program (faster),
diff --git a/libvtv/configure.ac b/libvtv/configure.ac
index 6db97dc..e3fb92f 100644
--- a/libvtv/configure.ac
+++ b/libvtv/configure.ac
@@ -20,6 +20,13 @@ AC_ARG_ENABLE(version-specific-runtime-libs,
[version_specific_libs=no])
AC_MSG_RESULT($version_specific_libs)
+# Do not delete or change the following two lines. For why, see
+# http://gcc.gnu.org/ml/libstdc++/2003-07/msg00451.html
+AC_CANONICAL_SYSTEM
+target_alias=${target_alias-$host_alias}
+AC_SUBST(target_alias)
+GCC_LIBSTDCXX_RAW_CXX_FLAGS
+
# See if supported.
unset VTV_SUPPORTED
AC_MSG_CHECKING([for host support for vtable verification])
@@ -33,13 +40,6 @@ if test x$VTV_SUPPORTED = xyes; then
AC_MSG_NOTICE(using vtable verification)
fi
-# Do not delete or change the following two lines. For why, see
-# http://gcc.gnu.org/ml/libstdc++/2003-07/msg00451.html
-AC_CANONICAL_SYSTEM
-target_alias=${target_alias-$host_alias}
-AC_SUBST(target_alias)
-GCC_LIBSTDCXX_RAW_CXX_FLAGS
-
AM_INIT_AUTOMAKE(foreign no-dist)
AM_ENABLE_MULTILIB(, ..)
AM_MAINTAINER_MODE
Re: [PATCH] Add vtable verification feature announcement to news on main page...
I believe I have addressed all the issues with the Vtable Verification feature. I have updated the announcement patch, and have attached the updated patch. Is this OK to commit? -- Caroline Tice cmt...@google.com On Thu, Aug 8, 2013 at 9:00 AM, Richard Earnshaw rearn...@arm.com wrote: On 07/08/13 18:19, Caroline Tice wrote: As requested, here is the patch to announce the vtable verification feature on the main gcc.gnu.org web page. Is this ok to commit? Please don't put this in until the issues that the patch have introduced have been resolved. R. wwwdocs.v2.patch Description: Binary data
Re: [PATCH, vtv update] Add documentation for --enable-vtable-verify configure option...
On Fri, Sep 6, 2013 at 6:47 AM, Diego Novillo dnovi...@google.com wrote:
On 2013-08-28 17:15 , Caroline Tice wrote:
# Least ordering for dependencies mean linking w/o libstdc++ for as
# long as the development of libvtv does not absolutely require it.
Index: gcc/doc/install.texi
===
--- gcc/doc/install.texi(revision 202060)
+++ gcc/doc/install.texi(working copy)
@@ -1032,6 +1032,16 @@ and for cross builds configured with @op
More documentation about multiarch can be found at
@uref{http://wiki.debian.org/Multiarch}.
+@item --enable-vtable-verify
+Specify whether to enable or disable the vtable verification feature.
+Enabling this feature causes libstdc++ to be built with its virtual calls
+in verifiable mode. This means that, when linked with libvtv, every
+virtual call in libstdc++ will verify the vtable pointer through which
the
+call will be made before actually making the call. If not linked with
libvtv,
+the verifier will call stub functions (in libstdc++ itself) and do
nothing.
+If vtable verification is disabled, then libstdc++ is not built with its
+virutal calls in verifiable mode at all.
s/virutal/virtual/
Could you clarify in the docs whether --enable-vtable-verify is the default
behaviour or not?
It will not be the default, but yes I will clarify.
Why would I need --disable-vtv, if I can use --disable-vtable-verify?
--disable-vtable-verify prevents libstdc++ from being build with
verification turned on, but the libvtv library will still be built.
--disable-libvtv prevents the libvtv library from being built.
Obviously I need to add a comment like this to the documentation, so I
will.
OK with those changes.
Diego.
Re: [PATCH, vtv update] Add documentation for --enable-vtable-verify configure option...
Ping? Could somebody please review this for me? -- Caroline Tice cmt...@google.com On Wed, Aug 28, 2013 at 2:15 PM, Caroline Tice cmt...@google.com wrote: Discussion on this topic seems to have petered out. I have removed references to ENABLE_VTABLE_VERIFY from libvtv, as requested. I cannot remove them from libgcc, as they really need to be there. I have updated the documentation in install.texi, as requested. Please review the attached patch, which does all of this, and let me know if it is ok to commit. -- Caroline Tice cmt...@google.com gcc ChangeLog: 2013-08-28 Caroline Tice cmt...@google.com * doc/install.texi: Add documentation for the --enable-vtable-verify and the --disable-libvtv configure options. libvtv ChangeLog: 2013-08-28 Caroline Tice cmt...@google.com * Makefile.am: Remove #if ENABLE_VTABLE_VERIFY checks around definitions of SUBDIRS, libvtv_la_SOURCES and libvtv_include_HEADERS. * Makefile.in: Regenerate. * configure.ac: Remove checks and tests for --enable-vtable-verify. * configure: Regenerate. On Thu, Aug 8, 2013 at 3:11 PM, Joseph S. Myers jos...@codesourcery.com wrote: On Thu, 8 Aug 2013, Caroline Tice wrote: The attached patch adds documentation for the --enable-vtable-verify configure option to install.texi. Is this ok to commit? Could you please answer the questions I raised in the first four paragraphs of http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00427.html at greater length? This patch appears to document it just as a libstdc++ configure option, but as noted there it appears to affect more than libstdc++ (even though it would be more sensible if it *only* affected libstdc++, and even better if it just enabled extra multilibs). -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH, libvtv] Fix most of the testsuite.
Ping? Could somebody please review this for me? -- Caroline Tice cmt...@google.com On Fri, Aug 30, 2013 at 2:55 PM, Caroline Tice cmt...@google.com wrote: The attached patch fixes most of the libvtv testsuite tests to run in the dejagnu test harness. There are still a few tests that I haven't been able to get working properly under dejagnu yet; they have been moved to libvtv/testsuite/other-tests for now. As I figure out how to make them work properly, I will move them out of there and update the testsuite. This patch looks like it's big, but most of that is files being moved from one directory to another (so the entire file gets deleted and then the entire file gets added in again, and nothing in the file actually changed). I have verified that all the tests that are not in the other-tests subdirectory do run and pass under dejagnu. Since I am supposedly the libvtv maintainer, I am not sure how this patch is supposed to be reviewed...Guidance would be appreciated... -- Caroline Tice cmt...@google.com 2013-08-30 Caroline Tice cmt...@google.com * testsuite/event-main.cc: Move to libvtv.cc subdirectory. * testsuite/environment.cc: Ditto. * testsuite/template-list2.cc: Ditto. * testsuite/event.h: Ditto. * testsuite/dataentry.cc: Ditto. * testsuite/event-private.h: Ditto. * testsuite/virtual_inheritance.cc: Ditto. * testsuite/povray-derived.cc: Ditto. * testsuite/nested_vcall_test.cc: Ditto. * testsuite/template-list-iostream.cc: Ditto. * testsuite/parts-test-extra-parts-views.h: Ditto. * testsuite/virtfunc-test.cc: Ditto. * testsuite/parts-test-extra-parts.h: Ditto. * testsuite/const_vtable.cc: Ditto. * testsuite/template-list.cc: Ditto. * testsuite/dup_name.cc: Ditto. * testsuite/thunk.cc: Ditto. * testsuite/parts-test-main.h: Ditto. * testsuite/mul_inh.cc: Ditto. * testsuite/test1.cc: Ditto. * testsuite/bb_tests.cc: Ditto. * testsuite/v8-test-2.cc: Ditto. * testsuite/thunk_vtable_map_attack.cc: Ditto. * testsuite/xlan-test.cc: Ditto. * testsuite/parts-test-main.cpp: Move to libvtv.cc subdirectory and change file extension from .cc to .cpp. * testsuite/event-definitions.cpp: Ditto. * testsuite/event-main.cpp: Ditto. * testsuite/derived-main.cpp: Ditto. * testsuite/derived-lib.cpp: Ditto. * testsuite/event-private.cpp: Ditto. * testsuite/parts-test-extra-parts-views.cpp: Ditto. * testsuite/parts-test-extra-parts.cpp: Ditto. * testsuite/parts-test.list: Move to libvtv.cc subdirectory. Change file extensions inside file from .cc to .cpp. * testsuite/event.list: Ditto. * testsuite/derived.list: Ditto. * testsuite/register_pair.cc: Move to libvtv.cc; rename file to register_set_pair.cc; include stdlib.h, stdio.h stdint.h string.h (KEY_TYPE_FIXED_SIZE): New define. (key_buffer, name_string, fake_names): New global variables. (generate_names): New function. (vtv_string_hans): New function. (main): Add call to generate_names. Update middle for-loop to initialize new parameters for __VLTRegisterPair... calls; move calls to __VLTRegisterPair... to middle for-loop. Add calls to __VLTRegisterSet... * testsuite/register_pair_mt.cc: Ditto; renamed to register_set_pair_mt.cc * testsuite/libvtv.cc/vtv.exp: New file. * testsuite/libvtv.mempool.cc/mempool.exp: New file. * testsuite/libvtv.mt.cc/mt.exp: New file. * testsuite/lib/libvtv.exp: New file. * testsuite/lib/libvtv-dg.exp: New file. * testsuite/config/default.exp: New file. * testsuite/Makefile.am: New file. (Old file was moved to other-tests subdirectory.) * testsuite/Makefile.in: New file (generated). * testsuite/mempool_negative.c: Change to C++ file; move to libvtv.mempool.cc; include vtv-change-permission.h. (main): Add call to __VLTChangePermission. * testsuite/mempool_positive.c: Change to C++ file; move to libvtv.mempool.cc; include vtv-change-permission.h. (main): Add call to __VLTChangePermission. * testsuite/temp_deriv3.cc: Move to other-tests subdirectory. * testsuite/environment-fail-64.s: Ditto. * testsutite/dlopen.cc: Ditto. * testsuite/so.cc: Ditto. * testsuite/temp_deriv2.cc: Ditto. * testsuite/field-test.cc: Ditto. * testsuite/dlopen_mt.cc: Ditto. * testsuite/environment-fail-32.s: Ditto. * testsuite/temp_deriv.cc: Ditto. * testsuite/replace-fail.cc: Ditto. * testsuite/other-tests/Makefile.am: New file. Copied from the Makefile.am
[PATCH] Fix PR58300, issue with -fvtable-verify=preinit
This fixes a bug where using -fvtable-verify=preinit sometimes causes an ICE. In particular, when the preinit flag was used, the vtable verification constructor initialization function was being written to the assembly file before it was being checked with cgraph_process_new_functions. This sometimes caused an assertion failure in decide_is_symbol_needed. This patch fixes the problem by reordering events so that the function is written to the assembly file after the call to cgraph_process_new_functions. I have verified that this fixes the test case attached to the bug, and it has passed all my regular vtable verification tests. Is this patch ok to commit? -- Caroline Tice cmt...@google.com 2013-09-04 Caroline Tice cmt...@google.com PR c++/58300 * vtable-class-hierarchy.c (vtv_generate_init_routine): In preinit case, move call to assemble_vtv_preinit_initializer to after call to cgraph_process_new_functions. bug-58300.patch Description: Binary data
Re: [PATCH, vtv update] Change fixed size array to a vector; fix diagnostic messages.
PING! Could somebody please, pretty please review this patch? It's not very big or complex, and it's been out there for three weeks nowIs there something else I need to do for this? -- Caroline Tice cmt...@google.com On Wed, Aug 21, 2013 at 1:30 PM, Caroline Tice cmt...@google.com wrote: Ping? Ping? On Wed, Aug 14, 2013 at 12:14 PM, Caroline Tice cmt...@google.com wrote: Ping? On Thu, Aug 8, 2013 at 3:16 PM, Caroline Tice cmt...@google.com wrote: This patch replaces the fixed sized array that was holding vtable pointers for a particular class hierarchy with a vector, allowing for dynamic resizing. It also fixes issues with the warning diagnostics. I am in the process of running regression tests with this patch; assuming they all pass, is this patch OK to commit? -- Caroline Tice cmt...@google.com 2013-08-08 Caroline Tice cmt...@google.com * vtable-class-hierarchy.c: Remove unnecessary include statements. (MAX_SET_SIZE): Remove unnecessary constant. (register_construction_vtables): Make vtable_ptr_array parameter into a vector; remove num_args parameter. Change array accesses to vector accesses. (register_other_binfo_vtables): Ditto. (insert_call_to_register_set): Ditto. (insert_call_to_register_pair): Ditto. (output_set_info): Ditto. Also change warning calls to warning_at calls, and fix format of warning messages. (register_all_pairs): Change vtbl_ptr_array from an array into a vector. Remove num_vtable_args (replace with calls to vector length). Change array stores accesses to vector functions. Change calls to register_construction_vtables, register_other_binfo_vtables, insert_call_to_register_set, insert_call_to_register_pair and output_set_info to match their new signatures. Change warning to warning_at and fix the format of the warning message.
Re: [PATCH, vtv update] Change fixed size array to a vector; fix diagnostic messages.
On Wed, Aug 28, 2013 at 10:30 AM, Diego Novillo dnovi...@google.com wrote:
On 2013-08-28 12:59 , Caroline Tice wrote:
static void
-output_set_info (tree record_type, tree *vtbl_ptr_array, int array_size)
+output_set_info (tree record_type, vectree *vtbl_ptr_array)
Okay, will do.
Since this function does not modify vtbl_ptr_array, you could pass it by
value.
@@ -1023,23 +1010,23 @@ register_all_pairs (tree body)
if (flag_vtv_debug)
output_set_info (current-class_info-class_type,
- vtbl_ptr_array, num_vtable_args);
+ vtbl_ptr_array);
- if (num_vtable_args 1)
+ if (vtbl_ptr_array-length() 1)
{
- insert_call_to_register_set (current-class_name,
num_vtable_args,
+ insert_call_to_register_set (current-class_name,
vtbl_ptr_array, body, arg1, arg2,
size_hint_arg);
registered_at_least_one = true;
}
- else if (num_vtable_args = 0)
+ else
num_vtable_args was an int that we manually incremented/decremented,
so it might have been negative. I am
assuming that a vectree.length() can never be negative. So before
the else-clause was explicitly checking that the value was 0 or 1
(since the if-condition takes all values greater than 1. Now I am
implicitly assuming that when we get to the else-clause the value must
be 0 or 1, because the if-condition took care of all values greater
than 1, and values less that zero cannot occur. Is that assumption
incorrect?
Do you need to review this again after I fix your suggestion, or can I
go ahead and commit it after?
-- Caroline
cmt...@google.com
You've changed the meaning of this else now. Intended? I can't tell from
context.
The patch looks fine, otherwise.
Diego.
Re: [PATCH, vtv update] Add documentation for --enable-vtable-verify configure option...
Discussion on this topic seems to have petered out. I have removed references to ENABLE_VTABLE_VERIFY from libvtv, as requested. I cannot remove them from libgcc, as they really need to be there. I have updated the documentation in install.texi, as requested. Please review the attached patch, which does all of this, and let me know if it is ok to commit. -- Caroline Tice cmt...@google.com gcc ChangeLog: 2013-08-28 Caroline Tice cmt...@google.com * doc/install.texi: Add documentation for the --enable-vtable-verify and the --disable-libvtv configure options. libvtv ChangeLog: 2013-08-28 Caroline Tice cmt...@google.com * Makefile.am: Remove #if ENABLE_VTABLE_VERIFY checks around definitions of SUBDIRS, libvtv_la_SOURCES and libvtv_include_HEADERS. * Makefile.in: Regenerate. * configure.ac: Remove checks and tests for --enable-vtable-verify. * configure: Regenerate. On Thu, Aug 8, 2013 at 3:11 PM, Joseph S. Myers jos...@codesourcery.com wrote: On Thu, 8 Aug 2013, Caroline Tice wrote: The attached patch adds documentation for the --enable-vtable-verify configure option to install.texi. Is this ok to commit? Could you please answer the questions I raised in the first four paragraphs of http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00427.html at greater length? This patch appears to document it just as a libstdc++ configure option, but as noted there it appears to affect more than libstdc++ (even though it would be more sensible if it *only* affected libstdc++, and even better if it just enabled extra multilibs). -- Joseph S. Myers jos...@codesourcery.com vtv-update-doc.patch Description: Binary data
Re: [PATCH, vtv update] Fix /tmp directory issues in libvtv
Thank you for pointing out my error. I will commit the following patch. -- Caroline Tice cmt...@google.com 2013-08-26 Caroline Tice cmt...@google.com * MAINTAINERS: Correct earliers update: Move myself from libvtv Various Reviewers to libvtv Various Maintainers. Index: MAINTAINERS === --- MAINTAINERS (revision 201986) +++ MAINTAINERS (working copy) @@ -178,6 +178,7 @@ libobjc Nicola Pero nicola.pero@meta- libobjc Andrew Pinski pins...@gmail.com libquadmath Tobias Burnus bur...@net-b.de libquadmath Jakub Jelinek ja...@redhat.com +libvtv Caroline Tice cmt...@google.com loop discovery Michael Hayes m.ha...@elec.canterbury.ac.nz soft-fp Joseph Myers jos...@codesourcery.com scheduler (+ haifa) Jim Wilson wil...@tuliptree.org @@ -288,7 +289,6 @@ libsanitizer, asan.c Jakub Jelinek jaku libsanitizer, asan.c Dodji Seketeli do...@redhat.com libsanitizer, asan.c Kostya Serebryany k...@google.com libsanitizer, asan.c Dmitry Vyukov dvyu...@google.com -libvtv Caroline Tice cmt...@google.com loop optimizer Zdenek Dvorak o...@ucw.cz loop optimizer Daniel Berlin dber...@dberlin.org LTO Diego Novillo dnovi...@google.com On Mon, Aug 26, 2013 at 2:10 PM, Florian Weimer fwei...@redhat.com wrote: On 08/25/2013 09:33 PM, Gerald Pfeifer wrote: On Tue, 20 Aug 2013, Florian Weimer wrote: As the libvtv reviewer, you don't need permission to commit your changes. :-) Actually, reviewers do need someone else's approval for their own changes (unlike maintainers and of course not for trivial changes). Ah, but I'm not a global reviewer, so I couldn't approve the change anyway. :-) Caroline, I think your patch in http://gcc.gnu.org/ml/gcc-patches/2013-08/msg00619.html doesn't match the ChangeLog update and what was approved by Jeff. It should have gone into the Various Maintainers section. -- Florian Weimer / Red Hat Product Security Team
Re: [PATCH/Merge Request] Vtable Verification feature.
I was not aware that the links required Javascript. Can anybody tell me how to fix them so that they don't? -- Caroline Tice cmt...@google.com On Thu, Aug 22, 2013 at 9:58 AM, Joseph S. Myers jos...@codesourcery.com wrote: On Mon, 12 Aug 2013, Caroline Tice wrote: I have finished my first pass at the porting documentation. It is now available on the vtable verification feature web site: http://gcc.gnu.org/wiki/vtv Please provide the links there in a form that does not require JavaScript to read them (in particular, does not require running non-free JavaScript, as per GNU Project principles). That's certainly possible for Google Docs documents; you can read https://docs.google.com/document/pub?id=1ZfyfkB62EFaR4_g4JKm4--guz3vxm9pciOBziMHTnK4pli=1 (GCC Architectural Goals) without JavaScript, for example. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH/Merge Request] Vtable Verification feature.
I believe I have figured this out and fixed the problem. If you are still seeing the issue, please let me know. -- Caroline Tice cmt...@google.com On Thu, Aug 22, 2013 at 10:07 AM, Caroline Tice cmt...@google.com wrote: I was not aware that the links required Javascript. Can anybody tell me how to fix them so that they don't? -- Caroline Tice cmt...@google.com On Thu, Aug 22, 2013 at 9:58 AM, Joseph S. Myers jos...@codesourcery.com wrote: On Mon, 12 Aug 2013, Caroline Tice wrote: I have finished my first pass at the porting documentation. It is now available on the vtable verification feature web site: http://gcc.gnu.org/wiki/vtv Please provide the links there in a form that does not require JavaScript to read them (in particular, does not require running non-free JavaScript, as per GNU Project principles). That's certainly possible for Google Docs documents; you can read https://docs.google.com/document/pub?id=1ZfyfkB62EFaR4_g4JKm4--guz3vxm9pciOBziMHTnK4pli=1 (GCC Architectural Goals) without JavaScript, for example. -- Joseph S. Myers jos...@codesourcery.com
Re: [PATCH, vtv update] Change fixed size array to a vector; fix diagnostic messages.
Ping? Ping? On Wed, Aug 14, 2013 at 12:14 PM, Caroline Tice cmt...@google.com wrote: Ping? On Thu, Aug 8, 2013 at 3:16 PM, Caroline Tice cmt...@google.com wrote: This patch replaces the fixed sized array that was holding vtable pointers for a particular class hierarchy with a vector, allowing for dynamic resizing. It also fixes issues with the warning diagnostics. I am in the process of running regression tests with this patch; assuming they all pass, is this patch OK to commit? -- Caroline Tice cmt...@google.com 2013-08-08 Caroline Tice cmt...@google.com * vtable-class-hierarchy.c: Remove unnecessary include statements. (MAX_SET_SIZE): Remove unnecessary constant. (register_construction_vtables): Make vtable_ptr_array parameter into a vector; remove num_args parameter. Change array accesses to vector accesses. (register_other_binfo_vtables): Ditto. (insert_call_to_register_set): Ditto. (insert_call_to_register_pair): Ditto. (output_set_info): Ditto. Also change warning calls to warning_at calls, and fix format of warning messages. (register_all_pairs): Change vtbl_ptr_array from an array into a vector. Remove num_vtable_args (replace with calls to vector length). Change array stores accesses to vector functions. Change calls to register_construction_vtables, register_other_binfo_vtables, insert_call_to_register_set, insert_call_to_register_pair and output_set_info to match their new signatures. Change warning to warning_at and fix the format of the warning message.
Re: [PATCH, vtv update] Fix /tmp directory issues in libvtv
That fixed it, thanks! Attached is the latest patch (Florian, I will send you the regenerated Makefile.in and configure separately). Please review and let me know if this is OK to commit! -- Caroline cmt...@google.com 2013-08-20 Caroline Tice cmt...@google.com * Makefile.am (DEFS): Add @DEFS@, to inherit defintions. * Makefile.in: Regenerate. * configure.ac: Add check for __secure_getenv and secure_getenv. * configure: Regenerate. * vtv_utils.cc : Include stdlib.h (HAVE_SECURE_GETENV): Add checks and definitions for secure_getenv. (log_dirs): Remove file static constant. (__vtv_open_log): Increase size of log file name. Add the user and process ids to the file name. Do not put the log files in /tmp. Instead try to get the directory name from an environment variable; if that fails try to use stderr. Add O_NOFOLLOW to the flags for 'open'. Update function comment. * vtv_rts.cc (log_memory_protection_data): Remove %d from file name. On Tue, Aug 20, 2013 at 11:35 AM, Florian Weimer fwei...@redhat.com wrote: On 08/20/2013 12:24 AM, Caroline Tice wrote: Hi All, I could really use some help here from someone who has a better understanding of how the config/Makefile system works than I do. In my libvtv/configure.ac file, I have: AC_GNU_SOURCE AC_CHECK_FUNCS([__secure_getenv]) AC_GNU_SOURCE AC_CHECK_FUNCS([secure_getenv]) There's nothing in Makefile.am that makes use of the substitution variables, so configure doesn't actually emit code to define the matching preprocessor macros. I tried to change that, by putting DEFS = @DEFS@ into libvtv/Makefile.am (libvtv/Makefile.in is derived from that). The diff below is slightly garbled, but you get the idea. diff --git a/libvtv/Makefile.am b/libvtv/Makefile.am index 73acfb4..567bd81 100644 --- a/libvtv/Makefile.am +++ b/libvtv/Makefile.am @@ -30,7 +30,7 @@ ACLOCAL_AMFLAGS = -I .. -I ../config # May be used by toolexeclibdir. gcc_version := $(shell cat $(top_srcdir)/../gcc/BASE-VER) -DEFS = +DEFS = @DEFS@ AM_CPPFLAGS = -I$(top_srcdir)/../include AM_CFLAGS = $(XCFLAGS) AM_CCASFLAGS = $(XCFLAGS) But I ended up with: libtool: compile: /home/fw/src/gnu/gcc/build/./gcc/xgcc -B/home/fw/src/gnu/gcc/build/./gcc/ -DPACKAGE_NAME=\GNU Vtable Verification Runtime Library\ -DPACKAGE_TARNAME=\libvtv\ -DPACKAGE_VERSION=\1.0\ -DPACKAGE_STRING=\GNU Vtable Verification Runtime Library 1.0\ -DPACKAGE_BUGREPORT=\\ -DPACKAGE_URL=\http://www.gnu.org/software/libvtv/\; -DPACKAGE=\libvtv\ -DVERSION=\1.0\ -DSTDC_HEADERS=1 -DHAVE_SYS_TYPES_H=1 -DHAVE_SYS_STAT_H=1 -DHAVE_STDLIB_H=1 -DHAVE_STRING_H=1 -DHAVE_MEMORY_H=1 -DHAVE_STRINGS_H=1 -DHAVE_INTTYPES_H=1 -DHAVE_STDINT_H=1 -DHAVE_UNISTD_H=1 -DHAVE_DLFCN_H=1 -DLT_OBJDIR=\.libs/\ -D__EXTENSIONS__=1 -D_ALL_SOURCE=1 -D_GNU_SOURCE=1 -D_POSIX_PTHREAD_SEMANTICS=1 -D_TANDEM_SOURCE=1 -DHAVE___SECURE_GETENV=1 -I. -I../../../git/libvtv -I../../../git/libvtv/../include -D_GNU_SOURCE -Wall -Wextra -fno-exceptions -Wl,-u_vtable_map_vars_start,-u_vtable_map_vars_end -g -O2 -D_GNU_SOURCE -MT vtv_rts.lo -MD -MP -MF .deps/vtv_rts.Tpo -c ../../../git/libvtv/vtv_rts.cc -fPIC -DPIC -o .libs/vtv_rts.o ../../../git/libvtv/vtv_rts.cc:134:28: fatal error: bits/c++config.h: No such file or directory #include bits/c++config.h ^ (The caret is at the wrong position, IMHO.) But I suspect something went wrong with regenerating Makefile.in. Have you got automake 1.11? Then you could make the change, regenerate Makefile.in, and try again. -- Florian Weimer / Red Hat Product Security Team vtv-update-tmpdir.patch Description: Binary data
Re: [PATCH, vtv update] Fix /tmp directory issues in libvtv
Ok, committed (with the space fix). -- Caroline Tice cmt...@google.com On Tue, Aug 20, 2013 at 12:43 PM, Florian Weimer fwei...@redhat.com wrote: On 08/20/2013 09:15 PM, Caroline Tice wrote: That fixed it, thanks! Attached is the latest patch (Florian, I will send you the regenerated Makefile.in and configure separately). Please review and let me know if this is OK to commit! As the libvtv reviewer, you don't need permission to commit your changes. :-) But I bootstrapped on x86_64-debian-linux-gnu, and can confirm that this now works as intended; __secure_getenv is picked up. One minor nit: Index: libvtv/vtv_utils.cc === --- libvtv/vtv_utils.cc (revision 201802) +++ libvtv/vtv_utils.cc (working copy) + decriptor. There's a stray space at the end of this line. -- Florian Weimer / Red Hat Product Security Team
Re: [PATCH, vtv update] Fix /tmp directory issues in libvtv
Hi All, I could really use some help here from someone who has a better understanding of how the config/Makefile system works than I do. In my libvtv/configure.ac file, I have: AC_GNU_SOURCE AC_CHECK_FUNCS([__secure_getenv]) AC_GNU_SOURCE AC_CHECK_FUNCS([secure_getenv]) This gets translated in my libvtv/configure file to: for ac_func in __secure_getenv do : ac_fn_c_check_func $LINENO __secure_getenv ac_cv_func___secure_getenv if test x$ac_cv_func___secure_getenv = xyes; then : cat confdefs.h _ACEOF #define HAVE___SECURE_GETENV 1 _ACEOF fi done for ac_func in secure_getenv do : ac_fn_c_check_func $LINENO secure_getenv ac_cv_func_secure_getenv if test x$ac_cv_func_secure_getenv = xyes; then : cat confdefs.h _ACEOF #define HAVE_SECURE_GETENV 1 _ACEOF fi done After running 'make all', I look in the libvtv/config.log, I see configure:4560: checking for __secure_getenv configure:4560: /usr/local/google2/cmtice/gcc-fsf.clean.obj/./gcc/xgcc -B/usr/l\ ocal/google2/cmtice/gcc-fsf.clean.obj/./gcc/ -B/usr/local/x86_64-unknown-linux-\ gnu/bin/ -B/usr/local/x86_64-unknown-linux-gnu/lib/ -isystem /usr/local/x86_64-\ unknown-linux-gnu/include -isystem /usr/local/x86_64-unknown-linux-gnu/sys-incl\ ude-o conftest -g -O2 conftest.c 5 configure:4560: $? = 0 configure:4560: result: yes configure:4575: checking for secure_getenv configure:4575: /usr/local/google2/cmtice/gcc-fsf.clean.obj/./gcc/xgcc -B/usr/l\ ocal/google2/cmtice/gcc-fsf.clean.obj/./gcc/ -B/usr/local/x86_64-unknown-linux-\ gnu/bin/ -B/usr/local/x86_64-unknown-linux-gnu/lib/ -isystem /usr/local/x86_64-\ unknown-linux-gnu/include -isystem /usr/local/x86_64-unknown-linux-gnu/sys-incl\ ude-o conftest -g -O2 conftest.c 5 /tmp/cc2jF2RF.o: In function `main': /usr/local/google2/cmtice/gcc-fsf.clean.obj/x86_64-unknown-linux-gnu/libvtv/con\ ftest.c:61: undefined reference to `secure_getenv' collect2: error: ld returned 1 exit status configure:4575: $? = 1 configure: failed program was: [snip] configure:4575: result: no So it looks to me like the check for __secure_getenv succeeded, so HAVE___SECURE_GETENV *should* have been defined in confdefs.h, and the test for it in my program *should* succeed. The source code in my program looks like this (at the moment): #define secure_getenv getenv #ifdef HAVE___SECURE_GETENV #define secure_getenv __secure_getenv #endif [snip] logs_prefix = secure_getenv (VTV_LOGS_DIR); BUT...when I check to see what version of the getenv symbol made it into libvtv.so, it is the wrong version: $ readelf -s libvtv.so | grep getenv 4: 0 FUNCGLOBAL DEFAULT UND getenv@GLIBC_2.2.5 (2) 76: 0 FUNCGLOBAL DEFAULT UND getenv@@GLIBC_2.2.5 If I alter the source program to by removing the #ifdef HAVE___SECURE_GETENV check, and just force it to try to use __secure_getenv, the program works properly, and the readelf -s libvtv.so | grep getenv shows __secure_getenv as the function. WHAT am I doing wrong? Help? -- Caroline Tice cmt...@google.com On Mon, Aug 19, 2013 at 9:37 AM, Florian Weimer fwei...@redhat.com wrote: On 08/17/2013 12:29 AM, Caroline Tice wrote: OK, I *think* I have done as you requested. I have to try the environment variable before falling back on stderr (there's a program we want to use this on that disables the ability to write to stderr). I have added the secure_getenv stuff as you requested. The fixed patch is attached. Please review the patch and let me know if this is OK to commit. Thanks! I found a packaged version of autoconf 2.64 and bootstrapped with --enable-vtable-verify. It's a bit confusing that libvtv is always built, but ends up being empty. It seems that HAVE_*SECURE_GETENV is not properly passed down to the compiler invocation: /bin/bash ./libtool --tag=CXX --mode=compile /home/fw/src/gnu/gcc/build/./gcc/xgcc -B/home/fw/src/gnu/gcc/build/./gcc/ -I. -I../../../git/libvtv -I../../../git/libvtv/../include -D_GNU_SOURCE -Wall -Wextra -fno-exceptions -I./../libstdc++-v3/include -I./../libstdc++-v3/include/x86_64-unknown-linux-gnu -I../../../git/libvtv/../libstdc++-v3/libsupc++ -Wl,-u_vtable_map_vars_start,-u_vtable_map_vars_end -g -O2 -D_GNU_SOURCE -MT vtv_utils.lo -MD -MP -MF .deps/vtv_utils.Tpo -c -o vtv_utils.lo ../../../git/libvtv/vtv_utils.cc libtool: compile: /home/fw/src/gnu/gcc/build/./gcc/xgcc -B/home/fw/src/gnu/gcc/build/./gcc/ -I. -I../../../git/libvtv -I../../../git/libvtv/../include -D_GNU_SOURCE -Wall -Wextra -fno-exceptions -I./../libstdc++-v3/include -I./../libstdc++-v3/include/x86_64-unknown-linux-gnu -I../../../git/libvtv/../libstdc++-v3/libsupc++ -Wl,-u_vtable_map_vars_start,-u_vtable_map_vars_end -g -O2 -D_GNU_SOURCE -MT vtv_utils.lo -MD -MP -MF .deps/vtv_utils.Tpo -c ../../../git/libvtv/vtv_utils.cc -fPIC -DPIC -o .libs/vtv_utils.o As a result, the DSO ends up referencing getenv, even though secure_getenv is available (and has been detected
Re: [PATCH, vtv update] Fix /tmp directory issues in libvtv
OK, I *think* I have done as you requested. I have to try the environment variable before falling back on stderr (there's a program we want to use this on that disables the ability to write to stderr). I have added the secure_getenv stuff as you requested. The fixed patch is attached. Please review the patch and let me know if this is OK to commit. Thanks! -- Caroline Tice cmt...@google.com 2013-08-16 Caroline Tice cmt...@google.com * configure.ac: Add check for __secure_getenv and secure_getenv. * configure: Regenerate. * vtv_utils.cc : Include stdlib.h (HAVE_SECURE_GETENV): Add checks and definitions for secure_getenv. (log_dirs): Remove file static constant. (__vtv_open_log): Increase size of log file name. Add the user and process ids to the file name. Do not put the log files in /tmp. Instead try to get the directory name from an environment variable; if that fails try to use stderr. Add O_NOFOLLOW to the flags for 'open'. Update function comment. * vtv_rts.cc (log_memory_protection_data): Remove %d from file name. On Wed, Aug 14, 2013 at 8:33 AM, Florian Weimer fwei...@redhat.com wrote: On 08/12/2013 07:07 PM, Caroline Tice wrote: The feature is supposed to be active in production code (like the stack protector). Okay, and it makes sense to enable this in programs that run with different privileges than the invoking user. If a trust transition occurred during the past execve, libvtv must not use the current directory to store files, or derive file paths from environment variables. Using shared directories such as /tmp is also tricky. (The current libvtv version in trunk has an arbitrary file creation vulnerability because of the hardcoded path in /tmp.) Realistically, I think libvtv can only log to standard error (or perhaps syslog/the journal) if it detects it runs with elevated privileges. One way to achieve that is to return dup(2) as the file descriptor if logs_dir is NULL (after changing getenv to secure_getenv), instead of setting logs_dir to .. This means that unless the environment variable is set, nothing would be logged to disk. I'm not sure if this what you want. But it follows the principle of least surprise, though. Creating log files in strange places because of a crash is unusual. If you insist on dumping stuff into the current directory by default, you'll have to use getauxval(AT_SECURE) (glibc 2.17 and later), __libc_enable_secure (some glibc systems, but not Fedora and the ELs), issetugid (Solaris and the BSDs) or an explicit comparison between getuid()/geteuid() and getgid()/getegid() (all the rest, slightly unsafe on Linux). I can write down this approach in more detail, it should probably go into the Fedora security guide anyway. I also noticed this in log_memory_protection_data: log_fd = __vtv_open_log (vtv_memory_protection_data_%d.log); The _%d should probably be dropped because the argument is not a format string. -- Florian Weimer / Red Hat Product Security Team vtv-update-tmpdir.patch Description: Binary data
Re: [PATCH, vtv update] Change fixed size array to a vector; fix diagnostic messages.
Ping? On Thu, Aug 8, 2013 at 3:16 PM, Caroline Tice cmt...@google.com wrote: This patch replaces the fixed sized array that was holding vtable pointers for a particular class hierarchy with a vector, allowing for dynamic resizing. It also fixes issues with the warning diagnostics. I am in the process of running regression tests with this patch; assuming they all pass, is this patch OK to commit? -- Caroline Tice cmt...@google.com 2013-08-08 Caroline Tice cmt...@google.com * vtable-class-hierarchy.c: Remove unnecessary include statements. (MAX_SET_SIZE): Remove unnecessary constant. (register_construction_vtables): Make vtable_ptr_array parameter into a vector; remove num_args parameter. Change array accesses to vector accesses. (register_other_binfo_vtables): Ditto. (insert_call_to_register_set): Ditto. (insert_call_to_register_pair): Ditto. (output_set_info): Ditto. Also change warning calls to warning_at calls, and fix format of warning messages. (register_all_pairs): Change vtbl_ptr_array from an array into a vector. Remove num_vtable_args (replace with calls to vector length). Change array stores accesses to vector functions. Change calls to register_construction_vtables, register_other_binfo_vtables, insert_call_to_register_set, insert_call_to_register_pair and output_set_info to match their new signatures. Change warning to warning_at and fix the format of the warning message.
Re: [PATCH, vtv update] Fix /tmp directory issues in libvtv
On Mon, Aug 12, 2013 at 10:07 AM, Caroline Tice cmt...@google.com wrote: On Mon, Aug 12, 2013 at 4:15 AM, Florian Weimer fwei...@redhat.com wrote: On 08/12/2013 12:39 AM, Caroline Tice wrote: On Sun, Aug 11, 2013 at 1:04 PM, Florian Weimer fwei...@redhat.com wrote: On 08/11/2013 01:08 AM, Caroline Tice wrote: OK, I have removed the attempt to use $HOME for the logs; they will now either go into the directory specified by the environment variable VTV_LOGS_DIR, or they will go into the current directory. I also added code to use secure_getenv, rather than getenv, if it is available. Is this patch ok to commit? + logs_prefix = secure_getenv (VTV_LOGS_DIR); + if (!logs_prefix || strlen (logs_prefix) == 0) +logs_prefix = (char *) .; Hmm. If you fall back to the current directory, using secure_getenv doesn't have the intended security effect. I wonder if we can simply label this functionality as unsafe for SUID/SGID programs, like we (hopefully) do for profiling. I am unclear how to do this? Could you elaborate please? I think it boils whether vtv is a debugging feature (like mudflap or profiling), or supposed to be active in production code (like the stack protector). If the latter, we have to go to greater lengths in restricting the logging feature when running SUID/SGID. The feature is supposed to be active in production code (like the stack protector). SO...I am completely unfamiliar with the greater lengths in restricting the logging feature when running SUID/SGID, so I have no idea what I am supposed to do here. Could you either please elaborate further, or point me to an example or some documentation? Thank you! -- Caroline Tice cmt...@google.com Looks like we lost the Cc: to gcc-patches. Not sure if this is intentional. Feel free to add it again. Done. -- Caroline Tice cmt...@google.com -- Florian Weimer / Red Hat Product Security Team
Vtable verification - configure enable flags
Starting a new thread, based on the discussion from [PATCH/Merge Request] Vtable Verification feature I would like to start a new thread to discuss some of the issues here (see snip from old thread below). On Thu, Aug 8, 2013 at 4:01 PM, Joseph S. Myers jos...@codesourcery.com wrote: On Thu, 8 Aug 2013, Caroline Tice wrote: Actually if you ever want to use the feature with your compiler you should build your compiler with --enable-vtable-verify. This will, as you noted, insert calls in libstdc++ to build the verification data structures and to verify the virtual calls in libstdc++ itself. However libstdc++ itself contains 'stub' (do-nothing) versions of the functions that build the data structures and perform the verification. So if you want to turn on verification with libstdc++, you link it with libvtv (which contains the real versions of those functions) and if you don't want verification with libstdc++ you just don't link in libvtv. There is no need to multiple versions of libstdc++. But presumably libstdc++ with the extra calls is less efficient than libstdc++ without them, even if the calls are just to stub functions? A GNU/Linux distributor would likely want to enable their users to use this functionality, meaning distributing a GCC build with libvtv included and a version of libstdc++ with the calls present for users who wish to build programs (linking with libstdc++ and libvtv) for which the calls in libstdc++ are checked. But they might not want to have even the stub calls executed for all installed C++ programs. So it would seem natural to provide both copies of libstdc++ - and desirable for this to be possible without needing separate GCC builds. I supposed the libgcc files could be built all the time (on systems that support libvtv). Would there be any down side to this? In my model, it's appropriate to build those independent of the libstdc++ configure option - and likewise to build everything in libvtv independent of the configure option. There should be no need for the configure option to affect anything other than, at most, libstdc++ - that is, the handling of the option in other configure scripts should be removed, with the case where the relevant support is built being enabled by default. -- Joseph S. Myers jos...@codesourcery.com Currently the there are two relevant flags, --enable-libvtv (--disable-libvtv) and --enable-vtable-verify (--disable-vtable-verify). --enable-libvtv is the default. This will cause libvtv.so to be built, as long as the underlying system is one that supports VTV (currently only linux systems). --enable-vtable-verify controls whether or not the vtv_* files get built, in libgcc, and also whether or not libstdc++ is built with calls to the vtv functions embedded or not. We can remove references to --enable-vtable-verify from libvtv, but it still needs to be there (or something similiar) in libgcc, because the vtv_* files in libgcc contain system-specific references (section names) that are not valid on all systems. And --enable-vtable-verify is necessary in libstdc++, because you definitely want to be able to control whether or not libstdc++ is built with its virtual calls verifiable. What do other people believe is the right thing to do here? -- Caroline Tice cmt...@google.com
Re: [PATCH, vtv update] Fix /tmp directory issues in libvtv
On Mon, Aug 12, 2013 at 4:15 AM, Florian Weimer fwei...@redhat.com wrote: On 08/12/2013 12:39 AM, Caroline Tice wrote: On Sun, Aug 11, 2013 at 1:04 PM, Florian Weimer fwei...@redhat.com wrote: On 08/11/2013 01:08 AM, Caroline Tice wrote: OK, I have removed the attempt to use $HOME for the logs; they will now either go into the directory specified by the environment variable VTV_LOGS_DIR, or they will go into the current directory. I also added code to use secure_getenv, rather than getenv, if it is available. Is this patch ok to commit? + logs_prefix = secure_getenv (VTV_LOGS_DIR); + if (!logs_prefix || strlen (logs_prefix) == 0) +logs_prefix = (char *) .; Hmm. If you fall back to the current directory, using secure_getenv doesn't have the intended security effect. I wonder if we can simply label this functionality as unsafe for SUID/SGID programs, like we (hopefully) do for profiling. I am unclear how to do this? Could you elaborate please? I think it boils whether vtv is a debugging feature (like mudflap or profiling), or supposed to be active in production code (like the stack protector). If the latter, we have to go to greater lengths in restricting the logging feature when running SUID/SGID. The feature is supposed to be active in production code (like the stack protector). Looks like we lost the Cc: to gcc-patches. Not sure if this is intentional. Feel free to add it again. Done. -- Caroline Tice cmt...@google.com -- Florian Weimer / Red Hat Product Security Team
Re: [PATCH] Update MAINTAINERS file
Ping? Is this one of those I can commit without waiting for approval? -- Caroline Tice cmt...@google.com On Sat, Aug 10, 2013 at 1:16 PM, Caroline Tice cmt...@google.com wrote: I would like to make the following changes to the MAINTAINERS file. May I commit this? -- Caroline Tice cmt...@google.com 2013-08-10 Caroline Tice cmt...@google.com * MAINTAINERS: Add myself as libvtv maintainer. Correct my email address in the Write After Approval section.
Re: [PATCH/Merge Request] Vtable Verification feature.
On Thu, Aug 8, 2013 at 9:34 AM, Caroline Tice cmt...@google.com wrote: On Thu, Aug 8, 2013 at 3:27 AM, Rainer Orth r...@cebitec.uni-bielefeld.de wrote: Caroline, As an aside, I had a very quick look at libvtv to determine what's required for a port to a non-Linux platform. It would be good if the requirements (currently, ELF, dl_iterate_phdr, __fortify_fail, certainly several more) could be documented to ease porters' task. I will be working on some documentation for this. Thanks. Rainer I have finished my first pass at the porting documentation. It is now available on the vtable verification feature web site: http://gcc.gnu.org/wiki/vtv I've never attempted to write porting documentation before, so I'm hoping that what I wrote is what was needed. If I missed anything important, please let me know. -- Caroline Tice cmt...@google.com
[PATCH] Update MAINTAINERS file
I would like to make the following changes to the MAINTAINERS file. May I commit this? -- Caroline Tice cmt...@google.com 2013-08-10 Caroline Tice cmt...@google.com * MAINTAINERS: Add myself as libvtv maintainer. Correct my email address in the Write After Approval section. maintainers.patch Description: Binary data
Re: [PATCH, vtv update] Fix /tmp directory issues in libvtv
OK, I have removed the attempt to use $HOME for the logs; they will now either go into the directory specified by the environment variable VTV_LOGS_DIR, or they will go into the current directory. I also added code to use secure_getenv, rather than getenv, if it is available. Is this patch ok to commit? -- Caroline Tice cmt...@google.com 2013-08-10 Caroline Tice cmt...@google.com * configure.ac: Add check for __secure_getenv and secure_getenv. * configure: Regenerate. * vtv_utils.cc : Include stdlib.h (HAVE_SECURE_GETENV): Add checks and definitions for secure_getenv. (log_dirs): Remove file static constant. (__vtv_open_log): Increase size of log file name. Add the user and process ids to the file name. Do not put the log files in /tmp. Instead try to get the directory name from an environment variable; if that fails use the current directory. Add O_NOFOLLOW to the flags for 'open'. Update function comment. On Fri, Aug 9, 2013 at 12:06 AM, Florian Weimer fwei...@redhat.com wrote: On 08/09/2013 12:09 AM, Caroline Tice wrote: + logs_dir = getenv (VTV_LOGS_DIR); This needs to use __secure_getenv or secure_getenv, depending on the glibc version, so that it doesn't wreak havoc in SUID/SGID binaries (or after other kinds of privilege transitions). Relevant autoconf checks are described here: http://sourceware.org/glibc/wiki/Tips_and_Tricks/secure_getenv -- Florian Weimer / Red Hat Product Security Team vtv-update-tmpdir.patch Description: Binary data
Re: [PATCH/Merge Request] Vtable Verification feature.
On Thu, Aug 8, 2013 at 5:55 AM, Ramana Radhakrishnan ramra...@arm.com wrote:
On 08/06/13 22:39, Benjamin De Kosnik wrote:
+# Filter out unsupported systems.
+case ${target} in
+ x86_64-*-linux* | i?86-*-linux*)
+ VTV_SUPPORTED=yes
+ ;;
+ powerpc*-*-linux*)
+ ;;
+ sparc*-*-linux*)
+ ;;
+ arm*-*-linux*)
+ ;;
What about powerpc, sparc and arm? Why are they mentioned here if no
actual decision is made about support?
This is more a practical consideration: it's the middle of summer
break. Let's error on the side of caution for the moment, and get
this in causing minimal disruption on a convenient platform that I can
verify myself easily.
Once this is in trunk, let a million flowers bloom! There is no
reason specific platform/target maintainers can't enable it at their
leisure on a per-setup manner and when they can verify testresults
easily.
arm*-*-linux* is broken currently for what looks like the same reasons as
the powerpc port.
Have you tried Benjamin Kosnik's patch? Does it fix the problem?
I spent some time this morning looking into what it would take to enable
this for arm*-*-linux* today and the first thing that I noticed is that the
testsuite is essentially driven by a shell script that caters to native
testing on the only port that this was developed for. It also expects -m32
and -m64 to be a standard option in all ports that want to turn this on.
Also because the tests are not using dejagnu it's going to be a right pain
to get this to work for cross-testing and unless that works reliably one
isn't going to be able to get this feature working easily.
Also whatever is target specific in the testsuite like environment-fail-32.s
and environment-fail-64.s needs to live in it's own target folders.
Basically this feature needs to follow conventions that exist already in
other parts of the source base or atleast have plans to get there surely ?
I haven't seen this in the reviews that I've seen so far, so apologies if
this has already been raised.
I know that the testsuite, as committed, is not in DejaGnu format and
that it will need to be converted to DejaGnu format. I also know that
it is currently specific to the platform on which the feature was
developed. The plan has always been to fix this. There was some
private discussion about whether it would be better to commit the
testsuite in its current state or not to commit a testsuite at all
with the initial project commit. I was told that it would be better
to show that there is some way of testing this feature, even if the
testsuite is not in its final form, than to not show any way of
testing it at all.
I will work on getting the testsuite into a better form and also on
writing up some documentation on the platform-specific pieces of the
vtable verification feature (for those who wish to port it to other
platforms).
Re: [PATCH/Merge Request] Vtable Verification feature.
On Thu, Aug 8, 2013 at 3:27 AM, Rainer Orth r...@cebitec.uni-bielefeld.de wrote: Caroline, your libgcc ChangeLog entries are all broken: they lack the initial * as can easily be seen in Emacs' Change Log Mode. Please fix. Wow, I'm really sorry about that! I don't know how that happened. I'll fix it immediately. As an aside, I had a very quick look at libvtv to determine what's required for a port to a non-Linux platform. It would be good if the requirements (currently, ELF, dl_iterate_phdr, __fortify_fail, certainly several more) could be documented to ease porters' task. I will be working on some documentation for this. Thanks. Rainer -- - Rainer Orth, Center for Biotechnology, Bielefeld University
[PATCH, vtv update] Add documentation for --enable-vtable-verify configure option...
The attached patch adds documentation for the --enable-vtable-verify configure option to install.texi. Is this ok to commit? -- Caroline TIce cmt...@google.com 2013-08-08 Caroline Tice cmt...@google.com * doc/install.texi: Add documentation for the --enable-vtable-verify configure option. vtv-update-doc.patch Description: Binary data
[PATCH, vtv update] Fix /tmp directory issues in libvtv
This patch changes where the logging file mechanism in libvtv tries to write its log files. Instead of trying to use /tmp, it now first looks for an environment variable VTV_LOGS_DIR. If it can't find that it looks for the environment variable HOME. If it can't find that either, it uses the current directory. This also adds O_NOFOLLOW to the open command. There was a request that we also use O_EXCL, but that would cause a problem because we occasionally want to be able to append to existing log files, and adding O_EXCL would cause that to fail. I also added the uid and pid to the log file names. Is this patch OK to commit? -- Caroline Tice cmt...@google.com 2013-08-08 Caroline Tice cmt...@google.com * vtv_utils.cc : Include stdlib.h (log_dirs): Remove file static constant. (__vtv_open_log): Increase size of log file name. Add the user and process ids to the file name. Do not put the log files in /tmp. Instead try to get the directory name from environment variables; if that fails use the current directory. Add O_NOFOLLOW to the flags for 'open'. Update function comment. vtv-update-tmpdir.patch Description: Binary data
[PATCH, vtv update] Change fixed size array to a vector; fix diagnostic messages.
This patch replaces the fixed sized array that was holding vtable pointers for a particular class hierarchy with a vector, allowing for dynamic resizing. It also fixes issues with the warning diagnostics. I am in the process of running regression tests with this patch; assuming they all pass, is this patch OK to commit? -- Caroline Tice cmt...@google.com 2013-08-08 Caroline Tice cmt...@google.com * vtable-class-hierarchy.c: Remove unnecessary include statements. (MAX_SET_SIZE): Remove unnecessary constant. (register_construction_vtables): Make vtable_ptr_array parameter into a vector; remove num_args parameter. Change array accesses to vector accesses. (register_other_binfo_vtables): Ditto. (insert_call_to_register_set): Ditto. (insert_call_to_register_pair): Ditto. (output_set_info): Ditto. Also change warning calls to warning_at calls, and fix format of warning messages. (register_all_pairs): Change vtbl_ptr_array from an array into a vector. Remove num_vtable_args (replace with calls to vector length). Change array stores accesses to vector functions. Change calls to register_construction_vtables, register_other_binfo_vtables, insert_call_to_register_set, insert_call_to_register_pair and output_set_info to match their new signatures. Change warning to warning_at and fix the format of the warning message. vtv-update.patch Description: Binary data vtv-update.patch Description: Binary data
Re: [PATCH/Merge Request] Vtable Verification feature.
On Wed, Aug 7, 2013 at 6:03 PM, Joseph S. Myers jos...@codesourcery.com wrote: Looking at the patch as committed, there seems to be some confusion about the nature of the --enable-vtable-verify configure option. Yes, there is a bit. It's documented only for libstdc++. Now, I still consider the existence of separate documentation for libstdc++ configure options to be unfortunate - I think all configure options for GCC should be documented in one place - but that's a separate matter. Although only documented for libstdc++, it actually appears to be used in the libgcc and libvtv configure scripts. The original intent of this flag was that if --enable-vtable-verify was NOT used, then NOTHING having to do with vtable verification would be built anywhere in the compiler, i.e. the binaries, libraries, etc. would not contain anything that wasn't there before the vtable verification patch was committed. The idea (and implementation) has evolved a bit, and I am not sure what the right way to handle this option is at the moment. Given that it has effects on more than just libstdc++, it needs documenting in gcc/doc/install.texi, the main documentation of configure options for GCC. I recently submitted a patch that adds the documentation for my current understanding of the way this option works. Then there's the question of what the semantics should be. My presumption is that the feature should be available for all GCC builds, at least by default on platforms where it can work, and the only thing needing a configure option should be whether the checks are enabled for libstdc++ itself (and ideally that would work by multilibbing libstdc++ rather than needing separate GCC builds to get libstdc++ with and without the checks). Actually if you ever want to use the feature with your compiler you should build your compiler with --enable-vtable-verify. This will, as you noted, insert calls in libstdc++ to build the verification data structures and to verify the virtual calls in libstdc++ itself. However libstdc++ itself contains 'stub' (do-nothing) versions of the functions that build the data structures and perform the verification. So if you want to turn on verification with libstdc++, you link it with libvtv (which contains the real versions of those functions) and if you don't want verification with libstdc++ you just don't link in libvtv. There is no need to multiple versions of libstdc++. Thus if the platform supports the feature, all relevant libgcc files should be built, and anything for libstdc++ needed for user code to use the feature should be built - the only thing not enabled by default would be checks for libstdc++'s classes' own vtables. (And unless there are difficulties in building the libgcc files on some systems, they could be built unconditionally, whether or not any other support needed for libvtv is present. I supposed the libgcc files could be built all the time (on systems that support libvtv). Would there be any down side to this? Actually, it looks like they may depend on an ELF target, but not on anything more.) Could you confirm that the libstdc++ ABI is not affected by the configure option - that the same symbols, at the same symbol versions, with the same semantics, are exported from libstdc++.so for builds with and without the feature enabled? The libstdc++ ABI has been enhanced to export the vtable verification functions for which it contains stub versions (otherwise they could never be overwritten by the versions in libvtv). Other than that the libstdc++ ABI exports the same symbols at the same symbol versions with the same semantics. I believe this export is unconditional. The file cp/vtable-class-hierarchy.c includes tm.h. Includes of tm.h from front ends are discouraged, and should have comments on them listing what target macros are used by the file in question (and so need to be converted to hooks before the include can be removed). Could you add such a comment to the #include (or if it's redundant, remove all redundant #includes from that file)? You have a +#define MAX_SET_SIZE 5000 which superficially looks like an arbitrary limit. Could you add a comment explaining why no input files, not matter how extreme, could ever exceed the limit of 5000? You have a couple of gcc_asserts regarding this limit, and an on-stack array for which it's at least not immediately obvious that all accesses are checked to ensure that a buffer overrun is impossible. If you have an arbitrary limit, and some input *can* exceed it (so triggering the gcc_asserts or buffer overrun), the right fix is of course to remove it, probably using vec.h to produce a dynamically growing array instead of hardcoding a size at all. But failing that, exceeding the limit must result in a sorry () call, not an assertion failure or buffer overrun, neither of which is acceptable behavior for any compiler input whatever. Other people have
Re: [PATCH, vtv update] Fix /tmp directory issues in libvtv
On Thu, Aug 8, 2013 at 3:26 PM, Mike Stump mikest...@comcast.net wrote: On Aug 8, 2013, at 3:09 PM, Caroline Tice cmt...@google.com wrote: This patch changes where the logging file mechanism in libvtv tries to write its log files. Instead of trying to use /tmp, it now first looks for an environment variable VTV_LOGS_DIR. If it can't find that it looks for the environment variable HOME. Hum… I kinda don't think HOME should be used. Why not?
Re: [PATCH/Merge Request] Vtable Verification feature.
Which version gets used depends on their ordering in the link line. When -fvtable-verify=std or -fvtable-verify=preinit is used, the gcc driver inserts -lvtv very early into the link line (earlier than -lstdc++), so this happens automatically. -- Caroline cmt...@google.com On Thu, Aug 8, 2013 at 4:33 PM, Jason Merrill ja...@redhat.com wrote: On 08/08/2013 06:34 PM, Caroline Tice wrote: Actually if you ever want to use the feature with your compiler you should build your compiler with --enable-vtable-verify. This will, as you noted, insert calls in libstdc++ to build the verification data structures and to verify the virtual calls in libstdc++ itself. However libstdc++ itself contains 'stub' (do-nothing) versions of the functions that build the data structures and perform the verification. How do you ensure that the libvtv versions get used rather than the stubs in libstdc++? Jason
Re: [PATCH/Merge Request] Vtable Verification feature.
libvtv was supposed to be automatically disabled for darwin; we are in the process of trying to figure out why this did not work as it was supposed to. In the meantime, if you add --disable-libvtv explicitly to your configure command, that should turn off the attempts to configure/build it. I'll get another patch out to fix the problem as soon as I can. -- Caroline Tice cmt...@google.com On Wed, Aug 7, 2013 at 5:29 AM, Dominique Dhumieres domi...@lps.ens.fr wrote: Revision 201555 breaks boostrap on x86_64-apple-darwin10: ... Checking multilib configuration for libvtv... make all-recursive Making all in testsuite /bin/sh: line 0: cd: testsuite: No such file or directory make[4]: *** [all-recursive] Error 1 make[3]: *** [all] Error 2 make[2]: *** [all-stage1-target-libvtv] Error 2 make[1]: *** [stage1-bubble] Error 2 make: *** [all] Error 2 According to * configure.ac: Add target-libvtv to target_libraries; disable libvtv on non-linux systems; add target-libvtv to noconfigdirs; add libsupc++/.libs to C++ library search paths. libvtv should not be built on darwin, but in config.log I see ... configure:3222: checking for libvtv support configure:3232: result: yes ... TIA Dominique
[PATCH] Add vtable verification feature announcement to news on main page...
As requested, here is the patch to announce the vtable verification feature on the main gcc.gnu.org web page. Is this ok to commit? -- Caroline Tice cmt...@google.com wwwdocs.patch Description: Binary data
Re: [PATCH/Merge Request] Vtable Verification feature.
Hi Iain, Thanks for the pointer (I had noticed this but I appreciate the help!). I do not know of anyone working on a Darwin port for this at this time. -- Caroline Tice cmt...@google.com On Wed, Aug 7, 2013 at 10:31 AM, Iain Sandoe i...@codesourcery.com wrote: hi Caroline, A (very) quick look suggests that only *) results in a return of UNSUPPORTED from libvtv/configure.tgt Do you know if anyone is working on a Darwin port for this lib? thanks, Iain On 7 Aug 2013, at 17:57, Caroline Tice wrote: libvtv was supposed to be automatically disabled for darwin; we are in the process of trying to figure out why this did not work as it was supposed to. In the meantime, if you add --disable-libvtv explicitly to your configure command, that should turn off the attempts to configure/build it. I'll get another patch out to fix the problem as soon as I can. -- Caroline Tice cmt...@google.com On Wed, Aug 7, 2013 at 5:29 AM, Dominique Dhumieres domi...@lps.ens.fr wrote: Revision 201555 breaks boostrap on x86_64-apple-darwin10: ... Checking multilib configuration for libvtv... make all-recursive Making all in testsuite /bin/sh: line 0: cd: testsuite: No such file or directory make[4]: *** [all-recursive] Error 1 make[3]: *** [all] Error 2 make[2]: *** [all-stage1-target-libvtv] Error 2 make[1]: *** [stage1-bubble] Error 2 make: *** [all] Error 2 According to * configure.ac: Add target-libvtv to target_libraries; disable libvtv on non-linux systems; add target-libvtv to noconfigdirs; add libsupc++/.libs to C++ library search paths. libvtv should not be built on darwin, but in config.log I see ... configure:3222: checking for libvtv support configure:3232: result: yes ... TIA Dominique
Re: [PATCH/Merge Request] Vtable Verification feature.
+ gcc_patches list On Wed, Aug 7, 2013 at 2:56 PM, Caroline Tice cmt...@google.com wrote: No, it was not intended. How do I undo that? -- Caroline cmt...@google.com On Wed, Aug 7, 2013 at 2:54 PM, David Edelsohn dje@gmail.com wrote: Caroline, When libvtv was checked in, libvtv/autom4te.cache subdirectory was committed as well. I don't think that this was intended. - David
Re: [PATCH/Merge Request] Vtable Verification feature.
Thank you! -- Caroline On Wed, Aug 7, 2013 at 3:09 PM, Paolo Carlini paolo.carl...@oracle.com wrote: .. I removed it. Thanks, Paolo.
Re: [PATCH/Merge Request] Vtable Verification feature.
I was talking with Diego, and he suggested the possibility of putting the log files in the same directory that the gcc dump files go, i.e. the one specified by dump_base_name. Do you think that would be acceptable? -- Caroline Tice cmt...@google.com On Tue, Aug 6, 2013 at 12:12 PM, Ian Lance Taylor i...@google.com wrote: On Tue, Aug 6, 2013 at 11:43 AM, Caroline Tice cmt...@google.com wrote: On Tue, Aug 6, 2013 at 11:31 AM, Ian Lance Taylor i...@google.com wrote: The output to the file doesn't have any indication of what file is being compiled, so it will be ambiguous when run in parallel. You are mistaken. It outputs one line to the log file for each compilation unit. The output line begins with the name of the file that was being compiled. In my use case, I have used this to build a very large system, which resulted in something like at 8000 line log file of counts, which I then used my sum script on to see how the verifications were going. I was mistaken in detail but I'm not sure I was mistaken in principle. What happens if you are building the large system twice in different directories on the same machine? Or, for that matter, if two different people are doing so? Or if one person did it a while ago, and now you want to do it, but you can't open the file because it is owned by the other person? Maybe you should simply change -fvtv-counts to take a file name, then we don't have to worry about any of this. It's not quite that simple: the -fvtv-counts flag actually causes two files to be created; also there's another flag, -fvtv-debug that generates a third file (i wanted a lot of information when I was working on and debugging this feature). Also if users are arbitrarily allowed to name the counts file anything, the summing script program I wrote won't be able to find them. That doesn't seem like a compelling argument to me, since one could pass the file names to the summing script as well. As far as I can see, on a multi-user system, there is no reasonable alternative to permitting the user to specify the file names to use, or at least a directory where the files should be placed. And if permit that, why not simply require it? Ian
Re: [PATCH/Merge Request] Vtable Verification feature.
Actually, I think that was dump_dir_name. -- Caroline On Tue, Aug 6, 2013 at 1:12 PM, Caroline Tice cmt...@google.com wrote: I was talking with Diego, and he suggested the possibility of putting the log files in the same directory that the gcc dump files go, i.e. the one specified by dump_base_name. Do you think that would be acceptable? -- Caroline Tice cmt...@google.com On Tue, Aug 6, 2013 at 12:12 PM, Ian Lance Taylor i...@google.com wrote: On Tue, Aug 6, 2013 at 11:43 AM, Caroline Tice cmt...@google.com wrote: On Tue, Aug 6, 2013 at 11:31 AM, Ian Lance Taylor i...@google.com wrote: The output to the file doesn't have any indication of what file is being compiled, so it will be ambiguous when run in parallel. You are mistaken. It outputs one line to the log file for each compilation unit. The output line begins with the name of the file that was being compiled. In my use case, I have used this to build a very large system, which resulted in something like at 8000 line log file of counts, which I then used my sum script on to see how the verifications were going. I was mistaken in detail but I'm not sure I was mistaken in principle. What happens if you are building the large system twice in different directories on the same machine? Or, for that matter, if two different people are doing so? Or if one person did it a while ago, and now you want to do it, but you can't open the file because it is owned by the other person? Maybe you should simply change -fvtv-counts to take a file name, then we don't have to worry about any of this. It's not quite that simple: the -fvtv-counts flag actually causes two files to be created; also there's another flag, -fvtv-debug that generates a third file (i wanted a lot of information when I was working on and debugging this feature). Also if users are arbitrarily allowed to name the counts file anything, the summing script program I wrote won't be able to find them. That doesn't seem like a compelling argument to me, since one could pass the file names to the summing script as well. As far as I can see, on a multi-user system, there is no reasonable alternative to permitting the user to specify the file names to use, or at least a directory where the files should be placed. And if permit that, why not simply require it? Ian
Re: [PATCH/Merge Request] Vtable Verification feature.
I have made all the requested changes. I am in the process of
bootstrapping and running the regressions one last time. Assuming the
bootstrapping regression tests pass, is this ok to commit?
-- Caroline
cmt...@google.com
On Tue, Aug 6, 2013 at 2:45 PM, Diego Novillo dnovi...@google.com wrote:
On Tue, Aug 6, 2013 at 2:39 PM, Benjamin De Kosnik b...@redhat.com wrote:
+# Filter out unsupported systems.
+case ${target} in
+ x86_64-*-linux* | i?86-*-linux*)
+ VTV_SUPPORTED=yes
+ ;;
+ powerpc*-*-linux*)
+ ;;
+ sparc*-*-linux*)
+ ;;
+ arm*-*-linux*)
+ ;;
What about powerpc, sparc and arm? Why are they mentioned here if no
actual decision is made about support?
This is more a practical consideration: it's the middle of summer
break. Let's error on the side of caution for the moment, and get
this in causing minimal disruption on a convenient platform that I can
verify myself easily.
On a practical note, the libsanitizer acceptance
criteria was/is as above, seems sensible to do the same thing with
libvtv.
Once this is in trunk, let a million flowers bloom! There is no
reason specific platform/target maintainers can't enable it at their
leisure on a per-setup manner and when they can verify testresults
easily.
Agreed. The comments I had have been already addressed by Caroline,
AFAICT. Once she has that, the patch can go in.
Diego.
[PATCH/Merge Request] Vtable Verification feature.
Quick Reminder; The vtable verification feature (controlled by a flag) is designed to detect, at run time, if/when the vtable pointer in a C++ object has been corrupted, before allowing virtual calls through that pointer. If pointer corruption is detected, execution of the program is halted. I have created (with some help) a git branch on gcc.gnu.org to contain the vtable verification feature work. This work is now well integrated with GCC trunk, and the sources are in a good state for future work. I believe all previous review comments have been addressed. The feature is disabled by default, and disabled on non-linux OSes. Regression testing on linux is showing great results (no regressions). There is a wiki page at http://gcc.gnu.org/wiki/vtv that contains information about how to use the feature, as well as links to the Feature Proposal, User's Guide, and the presentation on this feature at last year's GNU Tools Cauldron. This work has been ongoing since GCC 4.7 and is now ready to merge into trunk for the GCC 4.9.0 release. I would like permission to merge this in (and or information on the best way to proceed from here). Thanks. -- Caroline Tice cmt...@google.com
Re: [PATCH, updated] Vtable pointer verification, C++ front end changes (patch 1 of 3)
On Tue, Jun 4, 2013 at 10:20 AM, Jason Merrill ja...@redhat.com wrote: On 05/24/2013 12:15 PM, Caroline Tice wrote: Changes to g++spec.c only affect the g++ driver, not the gcc driver. Are you sure this is what you want? Can't you handle this stuff directly in the specs like address sanitizer does? I haven't seen a response to this comment. It seems correct to only update the g++ driver, since this option only does things for C++ programs. But people don't always compile C++ programs with the g++ driver; the gcc driver works fine for programs that don't require libstdc++. I'm not sure what address sanitizer does directly in the specs...Which specs would these be? SANITIZER_EARLY_SPEC and SANITIZER_SPEC in gcc.c. Ok, I will make the spec changes directly in gcc.cc Originally I was not doing anything with the specs, and so I didn't want to have to add a special link command, to link in a special library whenever the fvtable-verify option is used. Now that I've started changing the specs anyway, I suppose I could go ahead and put this in its own separate library and add a link option. Is that really the way I should go with this? I think so, yes. Will do. These changes should not be necessary. Just set DECL_ONE_ONLY on the vtable map variables. I am sorry to disagree with you, but you are incorrect. The second change can be eliminated (the one that adds SECTION_LINKONCE to flags), but the first change above is necessary. What the first bit of code does is to ensure that each vtable map variable gets its own unique comdat name. When I tried removing this code, it put all the vtable map variables into the same section with a single comdat name. Ah, I see. Then that's a bug that ought to be fixed: resolve_unique_section needs to deal better with decls with both DECL_SECTION_NAME and DECL_ONE_ONLY set. But I guess for now let's leave your code as it is with a FIXME note. Ok, will do. + /* We need to check value and find the bit where we + have something with 2 arguments, the first + argument of which is an ADDR_EXPR and the second + argument of which is an INTEGER_CST. */ + + while (value TREE_OPERAND_LENGTH (value) == 1 + TREE_CODE (TREE_OPERAND (value, 0)) == ADDR_EXPR) +value = TREE_OPERAND (value, 0); The comment doesn't match the code; you're testing for something with one operand. And it seems strange to test for anything with one operand. I will update the comment. Are you sure that's the right fix? What is the purpose of this code? The comment sounds like you want to strip away an offset relative to a vtable pointer and get the actual vtable. Testing for length 1 won't accomplish that for you. The test for length 1 was just making sure the tree operand existed before trying to access it; but I will rewrite this so it is less confusing/misleading. + (vtable_decl = get_vtbl_decl_for_binfo (base_binfo)) + !(DECL_VTABLE_OR_VTT_P (vtable_decl) +DECL_CONSTRUCTION_VTABLE_P (vtable_decl))) get_vtbl_decl_for_binfo will never return a construction vtable. When I tried removing that condition, my thunk test failed. When I add it back, the test passes again. Failed how? I'm puzzled. All that check could do would be to avoid registering a construction vtable, which should be harmless anyway. I looked at this again; it was causing a failure because of operator error on my part when I tried removing it; I only removed part of the clause, and this test was always failing, causing the overall negated clause to always pass; when I removed that clause then the overall negated clause sometimes failed, which resulted in a vtable pointer not being added to my set. Removing the entire negated clause removes the unnecessary check and does not cause any new test failures. Bottom line: I can remove the test that you were complaining about and still get things to work properly. +create_undef_reference_to___vtv_init (tree init_routine_body) Why do we need this, given that we'll already have references to the various registration functions? This inserts a reference to a symbol that will only be resolved if the proper libraries are linked in. It is a way to make sure that, if some library did not get linked in properly, we get a link time failure rather than a run time failure. Won't the references to the registration functions also cause link time failures? No, because there were no other link time references (the library was always being linked in with --whole-archive for just that reason). But I have found a different mechanism
Fwd: [PATCH, updated] Vtable pointer verification, C++ front end changes (patch 1 of 3)
Trying to send again; gcc_patches list bounced original message.
-- Forwarded message --
From: Caroline Tice cmt...@google.com
Date: Fri, May 24, 2013 at 9:15 AM
Subject: Re: [PATCH, updated] Vtable pointer verification, C++ front
end changes (patch 1 of 3)
To: Jason Merrill ja...@redhat.com
Cc: Diego Novillo dnovi...@google.com, Luis Lozano
lloz...@google.com, Bhaskar Janakiraman bjanakira...@google.com,
GCC Patches gcc-patches@gcc.gnu.org
I'm afraid I have been busy trying to finish up other things recently,
but now I am ready to focus full time on this again.
On Mon, Apr 8, 2013 at 6:21 PM, Jason Merrill ja...@redhat.com wrote:
Hi, sorry it has taken me so long to get back to this. Hopefully we can wrap
it up quickly now that we're back in stage 1.
On 02/25/2013 02:24 PM, Caroline Tice wrote:
-CXX_FOR_TARGET='$$r/$(HOST_SUBDIR)/gcc/xg++ -B$$r/$(HOST_SUBDIR)/gcc/
-nostdinc++ `if test -f $$r/$(TARGET_SUBDIR)
/libstdc++-v3/scripts/testsuite_flags; then $(SHELL)
$$r/$(TARGET_SUBDIR)/libstdc++-v3/scripts/testsuite_flags --build-
includes; else echo -funconfigured-libstdc++-v3 ; fi`
-L$$r/$(TARGET_SUBDIR)/libstdc++-v3/src -L$$r/$(TARGET_SUBDIR)/li
bstdc++-v3/src/.libs'
+CXX_FOR_TARGET='$$r/$(HOST_SUBDIR)/gcc/xg++ -B$$r/$(HOST_SUBDIR)/gcc/
-nostdinc++ `if test -f $$r/$(TARGET_SUBDIR)
/libstdc++-v3/scripts/testsuite_flags; then $(SHELL)
$$r/$(TARGET_SUBDIR)/libstdc++-v3/scripts/testsuite_flags --build-
includes; else echo -funconfigured-libstdc++-v3 ; fi`
-L$$r/$(TARGET_SUBDIR)/libstdc++-v3/src -L$$r/$(TARGET_SUBDIR)/li
bstdc++-v3/src/.libs -L$$r/$(TARGET_SUBDIR)/libstdc++-v3/libsupc++/.libs'
You shouldn't need this, since libstdc++ includes libsupc++. And if you did
need to do it, it would need to be in configure.ac or it will be discarded by
the next autoconf.
Ok, I will remove this.
+ information aboui which vtable will actually be emitted. */
about
Done.
+vtv_finish_verification_constructor_init_function (tree function_body)
+{
+ tree fn;
+
+ finish_compound_stmt (function_body);
+ fn = finish_function (0);
+ DECL_STATIC_CONSTRUCTOR (fn) = 1;
+ decl_init_priority_insert (fn, MAX_RESERVED_INIT_PRIORITY - 1);
Why did you stop using finish_objects? If it was to be able to return the
function, you can get that from current_function_decl before calling
finish_objects.
I stopped using finish_objects because it always called
expand_or_defer_fn, and I did not want that to happen here.
Index: gcc/cp/g++spec.c
Changes to g++spec.c only affect the g++ driver, not the gcc driver. Are you
sure this is what you want? Can't you handle this stuff directly in the
specs like address sanitizer does?
I haven't seen a response to this comment.
It seems correct to only update the g++ driver, since this option only
does things for C++ programs. I'm not sure what address sanitizer
does directly in the specs...Which specs would these be?
+ vtv_rts.cc \
+ vtv_malloc.cc \
+ vtv_utils.cc
It seems to me that this code belongs in a separate library like
libsanitizer, not in libstdc++.
Or this one.
Originally I was not doing anything with the specs, and so I didn't
want to have to add a special link command, to link in a special
library whenever the fvtable-verify option is used. Now that I've
started changing the specs anyway, I suppose I could go ahead and put
this in its own separate library and add a link option. Is that
really the way I should go with this?
- switch_to_section (sect);
+ if (sect-named.name
+ (strcmp (sect-named.name, .vtable_map_vars) == 0))
+ {
+#if defined (OBJECT_FORMAT_ELF)
+ targetm.asm_out.named_section (sect-named.name,
+sect-named.common.flags
+| SECTION_LINKONCE,
+DECL_NAME (decl));
+ in_section = sect;
+#else
+ switch_to_section (sect);
+#endif
+}
+ else
+switch_to_section (sect);
+ if (strcmp (name, .vtable_map_vars) == 0)
+ flags |= SECTION_LINKONCE;
These changes should not be necessary. Just set DECL_ONE_ONLY on the vtable
map variables.
I am sorry to disagree with you, but you are incorrect. The second
change can be eliminated (the one that adds SECTION_LINKONCE to
flags), but the first change above is necessary. What the first bit
of code does is to ensure that each vtable map variable gets its own
unique comdat name. When I tried removing this code, it put all the
vtable map variables into the same section with a single comdat name.
This will not work for us because the linker may need to pick and
choose individual variables out of each object file, not accept or
reject all the variables in an object file as a single group.
I believe this change was necessary so that each vtable map variable
would have
Re: [PATCH, updated] Vtable pointer verification, main gcc changes (patch 2 of 3)
On Wed, Mar 13, 2013 at 10:48 AM, Diego Novillo dnovi...@google.com wrote:
On 2013-02-25 14:27 , Caroline Tice wrote:
Index: libgcc/Makefile.in
===
--- libgcc/Makefile.in(revision 196266)
+++ libgcc/Makefile.in(working copy)
@@ -22,6 +22,7 @@
libgcc_topdir = @libgcc_topdir@
host_subdir = @host_subdir@
+gcc_srcdir = $(libgcc_topdir)/gcc
gcc_objdir = $(MULTIBUILDTOP)../../$(host_subdir)/gcc
srcdir = @srcdir@
@@ -969,6 +970,16 @@ crtendS$(objext): $(srcdir)/crtstuff.c
# This is a version of crtbegin for -static links.
crtbeginT$(objext): $(srcdir)/crtstuff.c
$(crt_compile) $(CRTSTUFF_T_CFLAGS) -c $ -DCRT_BEGIN -DCRTSTUFFT_O
+
+# These are used in vtable verification; see comments in source files for
+# more details.
+vtv_start$(objext): $(gcc_srcdir)/vtv_start.c
+$(crt_compile) $(CRTSTUFF_T_CFLAGS_S) \
+ -c $(gcc_srcdir)/vtv_start.c
+
+vtv_end$(objext): $(gcc_srcdir)/vtv_end.c
+$(crt_compile) $(CRTSTUFF_T_CFLAGS_S) \
+ -c $(gcc_srcdir)/vtv_end.c
endif
Why not have these two files in libgcc? I don't think we want to depend on
source files in gcc/ from libgcc/
I will put them there.
/* IPA Passes */
extern struct simple_ipa_opt_pass pass_ipa_lower_emutls;
Index: gcc/tree-vtable-verify.c
===
--- gcc/tree-vtable-verify.c(revision 0)
+++ gcc/tree-vtable-verify.c(revision 0)
I would like to get rid of this naming convention where we prefix file names
with 'tree-'. Just vtable-verify.c is fine.
@@ -0,0 +1,724 @@
+/* Copyright (C) 2005, 2006, 2007, 2008, 2009, 2010, 2011
Copyright (C) 2013
+/* Virtual Table Pointer Security Pass - Detect corruption of vtable
pointers
+ before using them for virtual method dispatches. */
Do you have a URL to a paper/presentation for it?
No, I don't at this time.
+
+ To find and reference the set of valid vtable pointers for any given
+ virtual class, we create a special global varible for each virtual
s/varible/variable/
+
+ Some implementation details for this pass:
+
+ To find the all of the virtual calls, we iterate through all the
s/the all/all/
+ struct vtable_registration key;
+ struct vtable_registration **slot;
+
+ gcc_assert (node node-registered);
+
+ key.vtable_decl = vtable_decl;
+ slot = (struct vtable_registration **) htab_find_slot (node-registered,
+ key, NO_INSERT);
Unless you need to use this in an old branch, I strongly suggest using the
new hash table facilities (hash-table.h).
Ok, will do.
+
+
+/* Here are the three two structures into which we insert vtable map nodes.
'three two'?
+ /* Verify that there aren't any qualifiers on the type. */
+ type_quals = TYPE_QUALS (TREE_TYPE (class_type_decl));
+ gcc_assert (type_quals == TYPE_UNQUALIFIED);
+
+ /* Get the mangled name for the unqualified type. */
+ class_name = DECL_ASSEMBLER_NAME (class_type_decl);
DECL_ASSEMBLER_NAME has side-effects (it generates one if there isn't one
already). Just to avoid this unwanted side effect, protect it with if
(HAS_DECL_ASSEMBLER_NAME_P). In fact, I think you should abort if the
class_type_decl does *not* have one. So, just adding gcc_assert
(HAS_DECL_ASSEMBLER_NAME_P(class_type_decl)) should be sufficient.
Will do.
+
+/* This function attempts to recover the declared class of an object
+ that is used in making a virtual call. We try to get the type from
+ the type cast in the gimple assignment statement that extracts the
+ vtable pointer from the object (DEF_STMT). The gimple statment
s/statment/statement/
+ usually looks something like this:
+
+ D.2201_4 = MEM[(struct Event *)this_1(D)]._vptr.Event*/
+
+static tree
+/* extract_object_class_type (gimple def_stmt) */
Remove this?
Yes (I didn't realize I had left the commented out code in the patch;
I'm sorry about that).
+extract_object_class_type (tree rhs)
+{
+ /* tree rhs = NULL_TREE; */
+
+ /* Try to find and extract the type cast from that stmt. */
+
+ /* rhs = gimple_assign_rhs1 (def_stmt); */
+ /*
+ if (TREE_CODE (rhs) == COMPONENT_REF)
+{
+ while (TREE_CODE (rhs) == COMPONENT_REF
+ (TREE_CODE (TREE_OPERAND (rhs, 0)) == COMPONENT_REF))
+rhs = TREE_OPERAND (rhs, 0);
+
+ if (TREE_CODE (rhs) == COMPONENT_REF
+ TREE_CODE (TREE_OPERAND (rhs, 0)) == MEM_REF
+ TREE_CODE (TREE_TYPE (TREE_OPERAND (rhs, 0)))== RECORD_TYPE)
+rhs = TREE_TYPE (TREE_OPERAND (rhs, 0));
+ else
+rhs = NULL_TREE;
+}
+ else
+rhs = NULL_TREE;
+ */
What's this? Is it needed? There's some other commented code here that
seems out of place.
I will remove it. I didn't mean for it to be in the patch.
+
+ tree result = NULL_TREE;
+
+
+ if (TREE_CODE (rhs) == COMPONENT_REF
Re: [PATCH, updated] Vtable pointer verification, C++ front end changes (patch 1 of 3)
Ping? -- Caroline Tice cmt...@google.com On Mon, Feb 25, 2013 at 11:24 AM, Caroline Tice cmt...@google.com wrote: Here are the latest changes to the vtable pointer verification patches (again there are 3 patches: c++ front end, main gcc, and c++ runtime library). I think these address all the review comments I have received so far. This patch is the for C++ front end. Please review these changes and let me know if they will be ok to commit once stage 1 opens. -- Caroline Tice cmt...@google.com 2013-02-25 Caroline Tice cmt...@google.com * class.c (finish_struct_1): Add call to vtv_save_class_info if vtable verification is turned on. * config-lang.in: Add vtable-class-hierarchy.c to the list of files that use GCC's garbage collector. * g++spec.c (VTABLE_LOAD_MODULE_UNIT): New macro for link option needed with vtable verification. (lang_specific_driver): Added variable to indicate if vtable verification option is used, and which flavor. Process -fvtable-verify option, update num_args if option is present, and add appropriate driver options if fvtable-verify is present. * vtable-class-hierarchy.c: New file, containing the bulk of vtable verification's front-end work. * mangle.c (get_mangled_vtable_map_var_name): New function. * decl2.c (start_objects): Update function comment. (cp_write_global_declarations): If vtable verification is turned on, call vtv_recover_class_info and vtv_compute_class_hierarchy_transitive_closure before calling finalize_compilation_unit. Call vtv_generate_init_routine after. (vtv_start_verification_constructor_init_function): New externally visible wrapper function to call start_objects for vtable verification. (vtv_finish_verification_constructor_init_function): New function. * init.c (build_vtbl_address): Make function not static (externally visible). * Make-lang.in: (CXX_AND_OBJCXX_OBJS): Add vtable-class-hierarchy.o to list of object files. (vtable-class-hierarchy.o): Add rule for building object file. * pt.c (mark_class_instantiated): Add call to vtv_save_class_info if vtable verification is turned on. * cp-tree.h: Add extern function declarations for vtv_start_verification_constructor_init_function, vtv_finish_verification_constructor_init_function, build_vtbl_address, vtv_compute_class_hierarchy_transitive_closure, vtv_generate_init_routine, vtv_save_class_info, vtv_recover_class_info and get_mangled_vtable_map_var_name.
Re: [PATCH, updated] Vtable pointer verification, main gcc changes (patch 2 of 3)
Ping? -- Caroline cmt...@google.com On Mon, Feb 25, 2013 at 11:27 AM, Caroline Tice cmt...@google.com wrote: Here are the latest changes to the vtable pointer verification patches (again there are 3 patches: c++ front end, main gcc, and c++ runtime library). I think these address all the review comments I have received so far. This patch is the for main gcc. Please review these changes and let me know if they will be ok to commit once stage 1 opens. -- Caroline Tice cmt...@google.com 2013-02-25 Caroline Tice cmt...@google.com * configure (CXX_FOR_TARGET): Add libstdc++-v3/libsupc++/.libs to the library search path. * ligbcc/config.host (extra_parts): Add vtv_start.o and vtv_end.o to the list. * libgcc/Makefile.in: Add definitin for gcc_srcdir; add rules for building vtv_start.o and vtv_end.o. * passes.c (init_optimization_pass): Add pass_vtable_verify. * vtv_start.c: New file. * tree-pass.h (pass_vtable_verify): Declare new pass. * tree-vtable-verify.c: New file, contains vtable verification tree pass. * tree-vtable-verify.h: New file. * common.opt: (fvtable-verify=): New option. Also define vtv_priority values for the option. * timevar.def (TV_VTABLE_VERIFICATION): Declare new time var. * config/gnu-user.h: Add vtv_start.o to STARTFILE_SPEC if fvtable-verify is present; Add vtv_end.o to ENDFILE_SPEC if fvtable-verify is present. * tree.h: Add extern function declaration for save_vtable_map_decl. * vtv_end.c: New file. * flag-types.h (vtv_priority): New enum, for values for new '-fvtable-verify=' option. * Makefile.in (OBJS): Add tree-vtable-verify.o to list of object files. (tree-vtable-verify.o): Add rule for building object file. (GTFILES): Add tree-vtable-verify.c to list of files that use GCC's garbage collector. * varasm.c (assemble_variable): Add code for handling variables that go into the .vtable_map_vars section. (assemble_vtv_perinit_initializer): New function. (default_section_type_flags): Add SECTION_LINKONCE to .vtable_map_vars section items. * output.h (assemble_vtv_preinit_initializer): External function decl.
[PATCH, updated] Vtable pointer verification, runtime library changes (patch 3 of 3)
Here are the latest changes to the vtable pointer verification patches (again there are 3 patches: c++ front end, main gcc, and c++ runtime library). I think these address all the review comments I have received so far. This patch is the for C++ runtime library changes. Please review these changes and let me know if they will be ok to commit once stage 1 opens. -- Caroline Tice cmt...@google.com 2013-02-25 Caroline Tice cmt...@google.com * config/abi/pre/gnu.ver: Add vtable verification runtime functions to the list of globally visible symbols. * acinclude.m4: Add GLIBCXX_ENABLE_VTABLE_VERIFY option and comments. * configure (ENABLE_VTABLE_VERIFY_FALSE, ENABLE_VTABLE_VERIFY_TRUE, enable_vtable_verify): New definitions. (predep_objects): Add vtv_start.o. if --enable-vtable-verify=yes was used. (postdep_objects): Add vtv_end.o. if --enable-vtable-verify=yes was used. Add test for --enable-vtabkle-verify=yes. Add definitions for ENABLE_VTABLE_VERIFY_FALSE and ENABLE_VTABLE_VERIFY_TRUE. * src/Makefile.am (libvtv__la_LIBADD): Add definition, conditioned on ENABLE_VTABLE_VERIFY. (LIBVTV_FLAGS): Add definition, conditioned on ENABLE_VTABLE_VERIFY. (libstdc___la_LDFLAGS): Add 'Wl,-u_vtable_map_var_start, -u_vtable_map_var_end' if ENABLE_VTABLE_VERIFY is true. (CXXLINK): Add LIBVTV_FLAGS. * src/Makefile.in: Regenerate from Makefile.am. * configure.ac (GLIBCXX_ENABLE_VTABLE_VERIFY): New definition. * libsupc++/vtv_utils.cc: New file. * libsupc++/vtv_rts.h: New file. * libsupc++/vtv_map.h: New file. * libsupc++/vtv_set.h: New file. * libsupc++/vtv_utils.h: New file. * libsupc++/vtv_init.cc: New file. * libsupc++/vtv_malloc.h: New file. * libsupc++/Makefile.am (sources): Add vtv_rts.cc, vtv_malloc.cc and vtv_utils.cc to the list. (vtv_init_sources, vtv_stubs_soruces, libvtv_init_la_sources, libvtv_stubs_la_sources): New definitions. (toolexeclib_LTLIBRARIES): Add libvtv_init.la and libvtv_stubs.la. * libsupc++/Makefile.in: Regenerate from Makefile.am. * libsupc++/vtv_stubs.cc: New file. * libsupc++/vtv_malloc.cc: New file. * libsupc++/vtv_rts.cc: New file. * libsupc++/vtv_fail.h: New file.
