Re: [PATCH v1] libstdc++-v3: Update VTV vars for libtool link commands [PR99172]

Re: [PATCH v1] libstdc++-v3: Update VTV vars for libtool link commands [PR99172]

[Caroline Tice]

Adding the libstdc++ mailing list to the patch.

-- Caroline
cmtice@google.com

On Wed, Mar 10, 2021 at 8:50 PM Caroline Tice <cmtice@google.com> 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
> cmtice@google.com
>
> libstdc++-v3/ChangeLog
>
> 2021-03-10  Caroline Tice  <cmtice@google.com>
>
>         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]

[Jonathan Wakely]

On 11/03/21 07:31 -0800, Caroline Tice via Libstdc++ wrote:
>Adding the libstdc++ mailing list to the patch.

Thanks.

>-- Caroline
>cmtice@google.com
>
>On Wed, Mar 10, 2021 at 8:50 PM Caroline Tice <cmtice@google.com> 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?

Should the same change be made to CXXLINK in src/*/Makefile.am, for
consistency if nothing else?

The patch is OK for gcc-11 now, but for stage 1 I'm wondering about
simplifying it.

Why do we ever add -Wl,-u options to CXXFLAGS when those are linker
options?

Could we move the -Wl,-u options to VTV_CXXLINKFLAGS instead, so
they're only used when actually linking?

Then VTV_CXXFLAGS would just be -fvtable-verify=std (and so
VTV_PCH_CXXFLAGS would be redundant), and instead of having
AM_CXXFLAGS and AM_CXXFLAGS_LT we could just filter out the
-fvtable-verify=std option from the CXXLINK options:

CXXLINK = \
	$(LIBTOOL) --tag CXX \
	$(AM_LIBTOOLFLAGS) $(LIBTOOLFLAGS) \
	--mode=link $(CXX) \
	$(VTV_CXXLINKFLAGS) \
	$(OPT_LDFLAGS) $(SECTION_LDFLAGS) \
	$(filter-out,-fvtable-verify=std,$(AM_CXXFLAGS)) \
	$(LTLDFLAGS) -o $@

Would that make sense?

Or just add  -fvtable-verify=none to the CXXLINK command to cancel out
the  -fvtable-verify=std option from AM_CXXFLAGS:

CXXLINK = \
	$(LIBTOOL) --tag CXX \
	$(AM_LIBTOOLFLAGS) $(LIBTOOLFLAGS) \
	--mode=link $(CXX) \
	$(VTV_CXXLINKFLAGS) \
	$(OPT_LDFLAGS) $(SECTION_LDFLAGS) \
	$(AM_CXXFLAGS) -fvtable-verify=none \
	$(LTLDFLAGS) -o $@

That seems cleaner to me, rather than adding another variable with
minor differences from the existing AM_CXXFLAGS.

Re: [PATCH v1] libstdc++-v3: Update VTV vars for libtool link commands [PR99172]

[Jonathan Wakely]

On 11/03/21 16:27 +0000, 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.

Re: [PATCH v1] libstdc++-v3: Update VTV vars for libtool link commands [PR99172]

[Jakub Jelinek]

On Thu, Mar 11, 2021 at 04:31:51PM +0000, Jonathan Wakely via Gcc-patches wrote:
> On 11/03/21 16:27 +0000, 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.

	Jakub

Re: [PATCH v1] libstdc++-v3: Update VTV vars for libtool link commands [PR99172]

[Jonathan Wakely]

On 11/03/21 17:46 +0100, Jakub Jelinek via Libstdc++ wrote:
>On Thu, Mar 11, 2021 at 04:31:51PM +0000, Jonathan Wakely via Gcc-patches wrote:
>> On 11/03/21 16:27 +0000, 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.

Re: [PATCH v1] libstdc++-v3: Update VTV vars for libtool link commands [PR99172]

[Caroline Tice]

[-- Attachment #1: Type: text/plain, Size: 1908 bytes --]

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
cmtice@google.com

libstdc++-v3/ChangeLog

2021-03-12  Caroline Tice  <cmtice@google.com>

        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
cmtice@google.com


On Thu, Mar 11, 2021 at 9:10 AM Jonathan Wakely <jwakely@redhat.com> wrote:
>
> On 11/03/21 17:46 +0100, Jakub Jelinek via Libstdc++ wrote:
> >On Thu, Mar 11, 2021 at 04:31:51PM +0000, Jonathan Wakely via Gcc-patches wrote:
> >> On 11/03/21 16:27 +0000, 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.
>

[-- Attachment #2: v2-0001-libstdc-v3-Update-VTV-vars-for-libtool-link-comma.patch --]
[-- Type: application/octet-stream, Size: 2369 bytes --]

From dc3eb3ba4bfaa77d3164b495697777737e5e564c Mon Sep 17 00:00:00 2001
From: Caroline Tice <cmtice@google.com>
Date: Fri, 12 Mar 2021 07:34:36 -0800
Subject: [PATCH v2] libstdc++-v3: Update VTV vars for libtool link commands
 [PR99172]

This fixes 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.
---
 libstdc++-v3/src/Makefile.am | 4 +++-
 libstdc++-v3/src/Makefile.in | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/libstdc++-v3/src/Makefile.am b/libstdc++-v3/src/Makefile.am
index 78e788cccb3..16f4cc6eff4 100644
--- a/libstdc++-v3/src/Makefile.am
+++ b/libstdc++-v3/src/Makefile.am
@@ -214,12 +214,14 @@ compatibility-condvar.o: compatibility-condvar.cc
 # set this option because CONFIG_CXXFLAGS has to be after
 # OPTIMIZE_CXXFLAGS on the compile line so that -O2 can be overridden
 # as the occasion calls for it.
-AM_CXXFLAGS = \
+AM_CXXFLAGS_PRE = \
 	-std=gnu++98 \
 	$(glibcxx_compiler_pic_flag) \
 	$(XTEMPLATE_FLAGS) $(VTV_CXXFLAGS) \
 	$(WARN_CXXFLAGS) $(OPTIMIZE_CXXFLAGS) $(CONFIG_CXXFLAGS)
 
+AM_CXXFLAGS = $(filter-out -fvtable-verify=std,$(AM_CXXFLAGS_PRE))
+
 # Libtool notes
 
 # 1) In general, libtool expects an argument such as `--tag=CXX' when
diff --git a/libstdc++-v3/src/Makefile.in b/libstdc++-v3/src/Makefile.in
index 684b7aee16b..4df5c829a7f 100644
--- a/libstdc++-v3/src/Makefile.in
+++ b/libstdc++-v3/src/Makefile.in
@@ -578,12 +578,13 @@ libstdc___la_LINK = $(CXXLINK) $(libstdc___la_LDFLAGS) $(lt_host_flags)
 # set this option because CONFIG_CXXFLAGS has to be after
 # OPTIMIZE_CXXFLAGS on the compile line so that -O2 can be overridden
 # as the occasion calls for it.
-AM_CXXFLAGS = \
+AM_CXXFLAGS_PRE = \
 	-std=gnu++98 \
 	$(glibcxx_compiler_pic_flag) \
 	$(XTEMPLATE_FLAGS) $(VTV_CXXFLAGS) \
 	$(WARN_CXXFLAGS) $(OPTIMIZE_CXXFLAGS) $(CONFIG_CXXFLAGS)
 
+AM_CXXFLAGS = $(filter-out -fvtable-verify=std,$(AM_CXXFLAGS_PRE))
 
 # Libtool notes
 
-- 
2.31.0.rc2.261.g7f71774620-goog

Re: [PATCH v1] libstdc++-v3: Update VTV vars for libtool link commands [PR99172]

[Jonathan Wakely]

On 12/03/21 15:33 -0800, Caroline Tice wrote:
>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.

This means we also don't pass it to LTCXXCOMPILE, instead of just
removing it from CXXLINK as in the original patch.  Since I have never
understood what libtool is for, I don't really know if that's OK, but
if it works then it works.

>Is this OK to commit now?

OK, thanks.

>-- Caroline Tice
>cmtice@google.com
>
>libstdc++-v3/ChangeLog
>
>2021-03-12  Caroline Tice  <cmtice@google.com>
>
>        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
>cmtice@google.com
>
>
>On Thu, Mar 11, 2021 at 9:10 AM Jonathan Wakely <jwakely@redhat.com> wrote:
>>
>> On 11/03/21 17:46 +0100, Jakub Jelinek via Libstdc++ wrote:
>> >On Thu, Mar 11, 2021 at 04:31:51PM +0000, Jonathan Wakely via Gcc-patches wrote:
>> >> On 11/03/21 16:27 +0000, 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.
>>