Re: [debug/profile-mode] broken c++config.h

Re: [debug/profile-mode] broken c++config.h

[Jonathan Wakely]

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

2012/3/16 Paweł Sikora:
> Hi,
>
> during gcc build process there's some sed magic (libstdc++-v3/include/Makefile.{am,in}) in action
> which modifies libstdc++/include/bits/c++config.h (replaces '#define _GLIBCXX_EXTERN_TEMPLATE'
> according to --enable-extern-template=yes/no settings). in fact, this sed rule also produces
> a wrong '#define _GLIBCXX_EXTERN_TEMPLATE 1 -1' in line 293:
>
> #if defined(_GLIBCXX_DEBUG) || defined(_GLIBCXX_PROFILE)
> # define _GLIBCXX_STD_C __cxx1998
> # define _GLIBCXX_BEGIN_NAMESPACE_CONTAINER \
>         namespace _GLIBCXX_STD_C { _GLIBCXX_BEGIN_NAMESPACE_VERSION
> # define _GLIBCXX_END_NAMESPACE_CONTAINER \
>         } _GLIBCXX_END_NAMESPACE_VERSION
> # undef _GLIBCXX_EXTERN_TEMPLATE
> # define _GLIBCXX_EXTERN_TEMPLATE 1 -1               <==== here
> #endif
>
> this broken macro causes runtime errors on mingw (see PR52540).
> attached small patch fixes all weird runtime errors for me.

Should the addition be \$$ to escape it for the shell as well as for make?
(I know it works, but that might not be true for all shells.)

The diff for Makefile.in is unnecessary, as it should be regenerated
from Makefile.am

Apart from that the patch looks good, I've CC'd gcc-patches, could you
provide a changelog entry?

Thanks.

[-- Attachment #2: gcc-extern-template-sed.patch --]
[-- Type: text/x-patch, Size: 1654 bytes --]

diff --git a/libstdc++-v3/include/Makefile.am b/libstdc++-v3/include/Makefile.am
index 1e9b144..b099580 100644
--- a/libstdc++-v3/include/Makefile.am
+++ b/libstdc++-v3/include/Makefile.am
@@ -1105,7 +1105,7 @@ ${host_builddir}/c++config.h: ${CONFIG_HEADER} \
 	sed -e "s,define __GLIBCXX__,define __GLIBCXX__ $$date," \
 	-e "s,define _GLIBCXX_INLINE_VERSION, define _GLIBCXX_INLINE_VERSION $$ns_version," \
 	-e "s,define _GLIBCXX_HAVE_ATTRIBUTE_VISIBILITY, define _GLIBCXX_HAVE_ATTRIBUTE_VISIBILITY $$visibility," \
-	-e "s,define _GLIBCXX_EXTERN_TEMPLATE, define _GLIBCXX_EXTERN_TEMPLATE $$externtemplate," \
+	-e "s,define _GLIBCXX_EXTERN_TEMPLATE$$, define _GLIBCXX_EXTERN_TEMPLATE $$externtemplate," \
 	-e "$$ldbl_compat" \
 	    < ${glibcxx_srcdir}/include/bits/c++config > $@ ;\
 	sed -e 's/HAVE_/_GLIBCXX_HAVE_/g' \
diff --git a/libstdc++-v3/include/Makefile.in b/libstdc++-v3/include/Makefile.in
index 19a7c0e..9344d46 100644
--- a/libstdc++-v3/include/Makefile.in
+++ b/libstdc++-v3/include/Makefile.in
@@ -1497,7 +1497,7 @@ ${host_builddir}/c++config.h: ${CONFIG_HEADER} \
 	sed -e "s,define __GLIBCXX__,define __GLIBCXX__ $$date," \
 	-e "s,define _GLIBCXX_INLINE_VERSION, define _GLIBCXX_INLINE_VERSION $$ns_version," \
 	-e "s,define _GLIBCXX_HAVE_ATTRIBUTE_VISIBILITY, define _GLIBCXX_HAVE_ATTRIBUTE_VISIBILITY $$visibility," \
-	-e "s,define _GLIBCXX_EXTERN_TEMPLATE, define _GLIBCXX_EXTERN_TEMPLATE $$externtemplate," \
+	-e "s,define _GLIBCXX_EXTERN_TEMPLATE$$, define _GLIBCXX_EXTERN_TEMPLATE $$externtemplate," \
 	-e "$$ldbl_compat" \
 	    < ${glibcxx_srcdir}/include/bits/c++config > $@ ;\
 	sed -e 's/HAVE_/_GLIBCXX_HAVE_/g' \

Re: [debug/profile-mode] broken c++config.h

[Paolo Carlini]

On 03/16/2012 03:29 PM, Jonathan Wakely wrote:
> Apart from that the patch looks good, I've CC'd gcc-patches, could you 
> provide a changelog entry?
If this kind of change to the makefile turns out to be applied I think 
should go essentially everywhere, at least 4.6 and 4.7(.1) besides mainline.

Paolo.

Re: [debug/profile-mode] broken c++config.h

[Paweł Sikora]

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

On Friday 16 of March 2012 14:29:21 Jonathan Wakely wrote:
> 2012/3/16 Paweł Sikora:
> > Hi,
> >
> > during gcc build process there's some sed magic (libstdc++-v3/include/Makefile.{am,in}) in action
> > which modifies libstdc++/include/bits/c++config.h (replaces '#define _GLIBCXX_EXTERN_TEMPLATE'
> > according to --enable-extern-template=yes/no settings). in fact, this sed rule also produces
> > a wrong '#define _GLIBCXX_EXTERN_TEMPLATE 1 -1' in line 293:
> >
> > #if defined(_GLIBCXX_DEBUG) || defined(_GLIBCXX_PROFILE)
> > # define _GLIBCXX_STD_C __cxx1998
> > # define _GLIBCXX_BEGIN_NAMESPACE_CONTAINER \
> >         namespace _GLIBCXX_STD_C { _GLIBCXX_BEGIN_NAMESPACE_VERSION
> > # define _GLIBCXX_END_NAMESPACE_CONTAINER \
> >         } _GLIBCXX_END_NAMESPACE_VERSION
> > # undef _GLIBCXX_EXTERN_TEMPLATE
> > # define _GLIBCXX_EXTERN_TEMPLATE 1 -1               <==== here
> > #endif
> >
> > this broken macro causes runtime errors on mingw (see PR52540).
> > attached small patch fixes all weird runtime errors for me.
> 
> Should the addition be \$$ to escape it for the shell as well as for make?
> (I know it works, but that might not be true for all shells.)

i don't think that $, could be expaneded by any shell.

> The diff for Makefile.in is unnecessary, as it should be regenerated
> from Makefile.am
> 
> Apart from that the patch looks good, I've CC'd gcc-patches, could you
> provide a changelog entry?

attached. i hope it's correct :)

[-- Attachment #2: ChangeLog --]
[-- Type: text/x-changelog, Size: 223 bytes --]

2012-03-16  Paweł Sikora  <pawel.sikora@agmk.net>

	PR libstdc++/52540
	* include/Makefile.am (c++config.h): Fix sed rule to not break
	the _GLIBCXX_EXTERN_TEMPLATE redefinition.
	* include/Makefile.in: Regenerated.

Re: [debug/profile-mode] broken c++config.h

[Jonathan Wakely]

2012/3/16 Paweł Sikora:
>> Should the addition be \$$ to escape it for the shell as well as for make?
>> (I know it works, but that might not be true for all shells.)
>
> i don't think that $, could be expaneded by any shell.

I'm not worried about it not expanding, but rather being rejected as
invalid syntax - but maybe it's fine.  It certainly works OK with bash
and ksh.

>> The diff for Makefile.in is unnecessary, as it should be regenerated
>> from Makefile.am
>>
>> Apart from that the patch looks good, I've CC'd gcc-patches, could you
>> provide a changelog entry?
>
> attached. i hope it's correct :)

Looks great, thanks.

Re: [debug/profile-mode] broken c++config.h

[Paolo Carlini]

On 03/16/2012 06:16 PM, Jonathan Wakely wrote:
> 2012/3/16 Paweł Sikora:
>>> Should the addition be \$$ to escape it for the shell as well as for make?
>>> (I know it works, but that might not be true for all shells.)
>> i don't think that $, could be expaneded by any shell.
> I'm not worried about it not expanding, but rather being rejected as
> invalid syntax - but maybe it's fine.  It certainly works OK with bash
> and ksh.
Thus, are we going to apply the patch? Maybe Paolo can double check the 
sed detail.

Thanks,
Paolo.

Re: [debug/profile-mode] broken c++config.h

[Paolo Bonzini]

Il 19/03/2012 13:32, Paolo Carlini ha scritto:
>>>> Should the addition be \$$ to escape it for the shell as well as for
>>>> make?
>>>> (I know it works, but that might not be true for all shells.)
>>> i don't think that $, could be expaneded by any shell.
>> I'm not worried about it not expanding, but rather being rejected as
>> invalid syntax - but maybe it's fine.  It certainly works OK with bash
>> and ksh.
> Thus, are we going to apply the patch? Maybe Paolo can double check the
> sed detail.

I think \$$ is better.  Besides that it looks good.

Paolo

Re: [debug/profile-mode] broken c++config.h

[Paolo Carlini]

On 03/20/2012 10:22 AM, Paolo Bonzini wrote:
> I think \$$ is better.  Besides that it looks good.
Thanks for the review. Yesterday, when Benjamin had a look, I decided to 
go ahead and just commit the patch as posted (after having double 
checked that indeed it worked for me), thus no escaping. But I noticed 
that nearby we have got quite a few unescaped $$, should we change all 
of them?

Thanks,
Paolo.

Re: [debug/profile-mode] broken c++config.h

[Paolo Bonzini]

Il 20/03/2012 11:33, Paolo Carlini ha scritto:
> On 03/20/2012 10:22 AM, Paolo Bonzini wrote:
>> I think \$$ is better.  Besides that it looks good.
> Thanks for the review. Yesterday, when Benjamin had a look, I decided to
> go ahead and just commit the patch as posted (after having double
> checked that indeed it worked for me), thus no escaping. But I noticed
> that nearby we have got quite a few unescaped $$, should we change all
> of them?

As you prefer... apparently they do not cause trouble, so you might as
well not care.

Paolo