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 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, 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.
[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 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
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.
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
[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] 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] 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)
{
[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)
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, 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
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)
