| Message ID | 1508763498-38147-1-git-send-email-dmalcolm@redhat.com |
|---|---|
| State | New |
| Headers | show |
| Series | Include <memory> from system.h (PR bootstrap/82610) | expand |
On Mon, Oct 23, 2017 at 2:58 PM, David Malcolm <dmalcolm@redhat.com> wrote: > On Sun, 2017-10-22 at 09:28 +0200, Gerald Pfeifer wrote: >> On Thu, 19 Oct 2017, David Malcolm wrote: >> > > In file included from /scratch/tmp/gerald/gcc-HEAD/gcc/unique- >> > > ptr-tests.cc:23: >> > > In file included from /scratch/tmp/gerald/gcc- >> > > HEAD/gcc/../include/unique-ptr.h:77: >> > > In file included from /usr/include/c++/v1/memory:629: >> > > /usr/include/c++/v1/typeinfo:199:2: error: no member named >> > > 'fancy_abort' in namespace 'std::__1'; did you mean simply >> > > 'fancy_abort'? >> > > _VSTD::abort(); >> > > ^~~~~~~ >> > > /usr/include/c++/v1/__config:390:15: note: expanded from macro >> > > '_VSTD' >> > > #define _VSTD std::_LIBCPP_NAMESPACE >> > >> > There seem to have been similar problems on OS X: >> > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82610 >> >> Yes, I believe it's the same actually (unearthed by clang as system >> compiler). >> >> > The proposed fix there is to include <memory> in system.h, which >> > presumably would fix this also. >> >> That appears to work around the bootstrap failure on my tester as >> well. >> >> How can we go about fixing this in the tree? >> >> Gerald > > Here's the patch by fxcoudert from the PR (plus a ChangeLog entry) > > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu; > reported by fxcoudert as fixing the issue on darwin and by > Gerald as fixing the issue on "newer versions of FreeBSD that use > clang 4.0 as system compiler". > > OK for trunk? Not entirely happy as unique-ptr.h doesn't use <memory> but well. Ok to unbreak bootstrap. Thanks, Richard. > Sorry again about the breakage. > > gcc/ChangeLog: > PR bootstrap/82610 > * system.h [__cplusplus]: Include <memory>. > --- > gcc/system.h | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/gcc/system.h b/gcc/system.h > index f0664e9..d6e1637 100644 > --- a/gcc/system.h > +++ b/gcc/system.h > @@ -233,6 +233,7 @@ extern int errno; > # include <vector> > #endif > # include <cstring> > +# include <memory> > # include <new> > # include <utility> > #endif > -- > 1.8.5.3 >
On 10/23/2017 02:51 PM, Richard Biener wrote: > On Mon, Oct 23, 2017 at 2:58 PM, David Malcolm <dmalcolm@redhat.com> wrote: >> OK for trunk? > > Not entirely happy as unique-ptr.h doesn't use <memory> but well. > Actually it does. It's needed in C++11 mode, because that's where std::unique_ptr is defined: #if __cplusplus >= 201103 /* In C++11 mode, all we need is import the standard std::unique_ptr. */ template<typename T> using unique_ptr = std::unique_ptr<T>; > Ok to unbreak bootstrap. Thanks, Pedro Alves
On Mon, 2017-10-23 at 15:51 +0200, Richard Biener wrote: > On Mon, Oct 23, 2017 at 2:58 PM, David Malcolm <dmalcolm@redhat.com> > wrote: > > On Sun, 2017-10-22 at 09:28 +0200, Gerald Pfeifer wrote: > > > On Thu, 19 Oct 2017, David Malcolm wrote: > > > > > In file included from /scratch/tmp/gerald/gcc- > > > > > HEAD/gcc/unique- > > > > > ptr-tests.cc:23: > > > > > In file included from /scratch/tmp/gerald/gcc- > > > > > HEAD/gcc/../include/unique-ptr.h:77: > > > > > In file included from /usr/include/c++/v1/memory:629: > > > > > /usr/include/c++/v1/typeinfo:199:2: error: no member named > > > > > 'fancy_abort' in namespace 'std::__1'; did you mean simply > > > > > 'fancy_abort'? > > > > > _VSTD::abort(); > > > > > ^~~~~~~ > > > > > /usr/include/c++/v1/__config:390:15: note: expanded from > > > > > macro > > > > > '_VSTD' > > > > > #define _VSTD std::_LIBCPP_NAMESPACE > > > > > > > > There seem to have been similar problems on OS X: > > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82610 > > > > > > Yes, I believe it's the same actually (unearthed by clang as > > > system > > > compiler). > > > > > > > The proposed fix there is to include <memory> in system.h, > > > > which > > > > presumably would fix this also. > > > > > > That appears to work around the bootstrap failure on my tester as > > > well. > > > > > > How can we go about fixing this in the tree? > > > > > > Gerald > > > > Here's the patch by fxcoudert from the PR (plus a ChangeLog entry) > > > > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu; > > reported by fxcoudert as fixing the issue on darwin and by > > Gerald as fixing the issue on "newer versions of FreeBSD that use > > clang 4.0 as system compiler". > > > > OK for trunk? > > Not entirely happy as unique-ptr.h doesn't use <memory> but well. I'm not sure I understand you here. include/unique-ptr.h has: #if __cplusplus >= 201103 /* In C++11 mode, all we need is import the standard std::unique_ptr. */ template<typename T> using unique_ptr = std::unique_ptr<T>; /* Pull in move as well. */ using std::move; #else /* C++11 */ ...etc..., most of the file, the pre-C++11 implementation So in C++11 and later it's using std::unique_ptr, for which, as I understand it <memory> is the standard include, e.g.: http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf says in (20.6.2 Header <memory> synopsis [memory.syn]): "The header <memory> defines several types and function templates that describe properties of pointers and pointer-like types, manage memory for containers and other template types, [...] The header also defines the templates unique_ptr, shared_ptr, weak_ptr, and various template functions that operate on objects of these types (20.7)." Would you prefer the includes of <memory> in gcc/system.h and include/unique-ptr.h to be guarded by #if __cplusplus >= 201103 ? (not sure if it works yet, but I can try it) Thanks Dave > Ok to unbreak bootstrap. > > Thanks, > Richard. > > > Sorry again about the breakage. > > > > gcc/ChangeLog: > > PR bootstrap/82610 > > * system.h [__cplusplus]: Include <memory>. > > --- > > gcc/system.h | 1 + > > 1 file changed, 1 insertion(+) > > > > diff --git a/gcc/system.h b/gcc/system.h > > index f0664e9..d6e1637 100644 > > --- a/gcc/system.h > > +++ b/gcc/system.h > > @@ -233,6 +233,7 @@ extern int errno; > > # include <vector> > > #endif > > # include <cstring> > > +# include <memory> > > # include <new> > > # include <utility> > > #endif > > -- > > 1.8.5.3 > >
On October 23, 2017 4:15:17 PM GMT+02:00, David Malcolm <dmalcolm@redhat.com> wrote: >On Mon, 2017-10-23 at 15:51 +0200, Richard Biener wrote: >> On Mon, Oct 23, 2017 at 2:58 PM, David Malcolm <dmalcolm@redhat.com> >> wrote: >> > On Sun, 2017-10-22 at 09:28 +0200, Gerald Pfeifer wrote: >> > > On Thu, 19 Oct 2017, David Malcolm wrote: >> > > > > In file included from /scratch/tmp/gerald/gcc- >> > > > > HEAD/gcc/unique- >> > > > > ptr-tests.cc:23: >> > > > > In file included from /scratch/tmp/gerald/gcc- >> > > > > HEAD/gcc/../include/unique-ptr.h:77: >> > > > > In file included from /usr/include/c++/v1/memory:629: >> > > > > /usr/include/c++/v1/typeinfo:199:2: error: no member named >> > > > > 'fancy_abort' in namespace 'std::__1'; did you mean simply >> > > > > 'fancy_abort'? >> > > > > _VSTD::abort(); >> > > > > ^~~~~~~ >> > > > > /usr/include/c++/v1/__config:390:15: note: expanded from >> > > > > macro >> > > > > '_VSTD' >> > > > > #define _VSTD std::_LIBCPP_NAMESPACE >> > > > >> > > > There seem to have been similar problems on OS X: >> > > > https://gcc.gnu.org/bugzilla/show_bug.cgi?id=82610 >> > > >> > > Yes, I believe it's the same actually (unearthed by clang as >> > > system >> > > compiler). >> > > >> > > > The proposed fix there is to include <memory> in system.h, >> > > > which >> > > > presumably would fix this also. >> > > >> > > That appears to work around the bootstrap failure on my tester as >> > > well. >> > > >> > > How can we go about fixing this in the tree? >> > > >> > > Gerald >> > >> > Here's the patch by fxcoudert from the PR (plus a ChangeLog entry) >> > >> > Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu; >> > reported by fxcoudert as fixing the issue on darwin and by >> > Gerald as fixing the issue on "newer versions of FreeBSD that use >> > clang 4.0 as system compiler". >> > >> > OK for trunk? >> >> Not entirely happy as unique-ptr.h doesn't use <memory> but well. > >I'm not sure I understand you here. > >include/unique-ptr.h has: > > #if __cplusplus >= 201103 > > /* In C++11 mode, all we need is import the standard > std::unique_ptr. */ > template<typename T> using unique_ptr = std::unique_ptr<T>; > > /* Pull in move as well. */ > using std::move; > > #else /* C++11 */ > > ...etc..., most of the file, the pre-C++11 implementation > >So in C++11 and later it's using std::unique_ptr, for which, as I >understand it <memory> is the standard include, e.g.: >http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2012/n3337.pdf >says in (20.6.2 Header <memory> synopsis [memory.syn]): > >"The header <memory> defines several types and function templates that >describe properties of pointers and pointer-like types, manage memory >for containers and other template types, [...] The header also defines >the templates unique_ptr, shared_ptr, weak_ptr, and various template >functions that operate on objects of these types (20.7)." > >Would you prefer the includes of <memory> in gcc/system.h and >include/unique-ptr.h to be guarded by #if __cplusplus >= 201103 ? (not >sure if it works yet, but I can try it) I guess so. But we have to make gdb happy as well. It really depends how much each TU grows with the extra (unneeded) include grows in C++11 and C++04 mode. Richard. >Thanks >Dave > > >> Ok to unbreak bootstrap. >> >> Thanks, >> Richard. >> >> > Sorry again about the breakage. >> > >> > gcc/ChangeLog: >> > PR bootstrap/82610 >> > * system.h [__cplusplus]: Include <memory>. >> > --- >> > gcc/system.h | 1 + >> > 1 file changed, 1 insertion(+) >> > >> > diff --git a/gcc/system.h b/gcc/system.h >> > index f0664e9..d6e1637 100644 >> > --- a/gcc/system.h >> > +++ b/gcc/system.h >> > @@ -233,6 +233,7 @@ extern int errno; >> > # include <vector> >> > #endif >> > # include <cstring> >> > +# include <memory> >> > # include <new> >> > # include <utility> >> > #endif >> > -- >> > 1.8.5.3 >> >
Hi, On Mon, 23 Oct 2017, Richard Biener wrote: > I guess so. But we have to make gdb happy as well. It really depends how > much each TU grows with the extra (unneeded) include grows in C++11 and > C++04 mode. The c++ headers unconditionally included from system.h, with: % echo '#include <$name>' | g++-7 -E -x c++ - | wc -l new: 3564 cstring: 533 utility: 3623 memory: 28066 compile time: % echo -e '#include <$name>\nint i;' | time g++-7 -c -x c++ - new: 0:00.06elapsed, 17060maxresident, 0major+3709minor cstring: 0:00.03elapsed, 13524maxresident, 0major+3075minor utility: 0:00.05elapsed, 16952maxresident, 0major+3776minor memory: 0:00.25elapsed, 40356maxresident, 0major+9764minor Hence, <memory> is not cheap at all, including it unconditionally from system.h when it isn't actually used by many things doesn't seem a good idea. Ciao, Michael.
On 23/10/17 17:07 +0200, Michael Matz wrote: >Hi, > >On Mon, 23 Oct 2017, Richard Biener wrote: > >> I guess so. But we have to make gdb happy as well. It really depends how >> much each TU grows with the extra (unneeded) include grows in C++11 and >> C++04 mode. > >The c++ headers unconditionally included from system.h, with: > >% echo '#include <$name>' | g++-7 -E -x c++ - | wc -l >new: 3564 >cstring: 533 >utility: 3623 >memory: 28066 That's using the -std=gnu++4 default for g++-7, and for that mode the header *is* needed, to get the definition of std::unique_ptr. For C++98 (when it isn't needed) that header is much smaller: tmp$ echo '#include <memory>' | g++ -E -x c++ - | wc -l 28101 tmp$ echo '#include <memory>' | g++ -E -x c++ - -std=gnu++98 | wc -l 4267 (Because it doesn't contain std::unique_ptr and std::shared_ptr before C++11). >compile time: >% echo -e '#include <$name>\nint i;' | time g++-7 -c -x c++ - >new: 0:00.06elapsed, 17060maxresident, 0major+3709minor >cstring: 0:00.03elapsed, 13524maxresident, 0major+3075minor >utility: 0:00.05elapsed, 16952maxresident, 0major+3776minor >memory: 0:00.25elapsed, 40356maxresident, 0major+9764minor > >Hence, <memory> is not cheap at all, including it unconditionally from >system.h when it isn't actually used by many things doesn't seem a good >idea. > > >Ciao, >Michael.
On 10/23/2017 04:17 PM, Jonathan Wakely wrote: > On 23/10/17 17:07 +0200, Michael Matz wrote: >> Hi, >> >> On Mon, 23 Oct 2017, Richard Biener wrote: >> >>> I guess so. But we have to make gdb happy as well. It really depends how >>> much each TU grows with the extra (unneeded) include grows in C++11 and >>> C++04 mode. >> >> The c++ headers unconditionally included from system.h, with: >> >> % echo '#include <$name>' | g++-7 -E -x c++ - | wc -l >> new: 3564 >> cstring: 533 >> utility: 3623 >> memory: 28066 > > That's using the -std=gnu++4 default for g++-7, and for that mode > the header *is* needed, to get the definition of std::unique_ptr. > > For C++98 (when it isn't needed) that header is much smaller: > > tmp$ echo '#include <memory>' | g++ -E -x c++ - | wc -l > 28101 > tmp$ echo '#include <memory>' | g++ -E -x c++ - -std=gnu++98 | wc -l > 4267 > > (Because it doesn't contain std::unique_ptr and std::shared_ptr before > C++11). > >> compile time: >> % echo -e '#include <$name>\nint i;' | time g++-7 -c -x c++ - >> new: 0:00.06elapsed, 17060maxresident, 0major+3709minor >> cstring: 0:00.03elapsed, 13524maxresident, 0major+3075minor >> utility: 0:00.05elapsed, 16952maxresident, 0major+3776minor >> memory: 0:00.25elapsed, 40356maxresident, 0major+9764minor >> >> Hence, <memory> is not cheap at all, including it unconditionally from >> system.h when it isn't actually used by many things doesn't seem a good >> idea. >> I think the real question is whether it makes a difference in a full build. There won't be many translation units that don't include some other headers. (though of course I won't be surprised if it does make a difference.) If it's a real issue, you could fix this like how the other similar cases were handled by system.h, by adding this in system.h: #ifdef __cplusplus #ifdef INCLUDE_UNIQUE_PTR # include "unique-ptr.h" #endif #endif instead of unconditionally including <memory> there, and then translation units that want unique-ptr.h would do "#define INCLUDE_UNIQUE_PTR" instead of #include "unique-ptr.h", like done for a few other C++ headers. (I maintain that IMO this is kind of self-inflicted GCC pain due to the fact that "#pragma poison" poisons too much. If #pragma poison's behavior were adjusted (or a new variant/mode created) to ignore references to the poisoned symbol names in system headers (or something like that), then you wouldn't need this manual management of header dependencies in gcc/system.h and the corresponding '#define INCLUDE_FOO' contortions. There's nothing that you can reasonably do with a reference to a poisoned symbol in a system header, other than avoid having the system header have the '#pragma poison' in effect when its included, which leads to contortions like system.h's. Note that the poisoned names are _still used anyway_. So can we come up with a GCC change that would avoid having to worry about manually doing this? It'd likely help other projects too.) Thanks, Pedro Alves
On Mon, 2017-10-23 at 16:40 +0100, Pedro Alves wrote: > On 10/23/2017 04:17 PM, Jonathan Wakely wrote: > > On 23/10/17 17:07 +0200, Michael Matz wrote: > > > Hi, > > > > > > On Mon, 23 Oct 2017, Richard Biener wrote: > > > > > > > I guess so. But we have to make gdb happy as well. It really > > > > depends how > > > > much each TU grows with the extra (unneeded) include grows in > > > > C++11 and > > > > C++04 mode. > > > > > > The c++ headers unconditionally included from system.h, with: > > > > > > % echo '#include <$name>' | g++-7 -E -x c++ - | wc -l > > > new: 3564 > > > cstring: 533 > > > utility: 3623 > > > memory: 28066 > > > > That's using the -std=gnu++4 default for g++-7, and for that mode > > the header *is* needed, to get the definition of std::unique_ptr. > > > > For C++98 (when it isn't needed) that header is much smaller: > > > > tmp$ echo '#include <memory>' | g++ -E -x c++ - | wc -l > > 28101 > > tmp$ echo '#include <memory>' | g++ -E -x c++ - -std=gnu++98 | wc > > -l > > 4267 > > > > (Because it doesn't contain std::unique_ptr and std::shared_ptr > > before > > C++11). > > > > > compile time: > > > % echo -e '#include <$name>\nint i;' | time g++-7 -c -x c++ - > > > new: 0:00.06elapsed, 17060maxresident, 0major+3709minor > > > cstring: 0:00.03elapsed, 13524maxresident, 0major+3075minor > > > utility: 0:00.05elapsed, 16952maxresident, 0major+3776minor > > > memory: 0:00.25elapsed, 40356maxresident, 0major+9764minor > > > > > > Hence, <memory> is not cheap at all, including it unconditionally > > > from > > > system.h when it isn't actually used by many things doesn't seem > > > a good > > > idea. > > > > > I think the real question is whether it makes a difference in > a full build. There won't be many translation units that > don't include some other headers. (though of course I won't > be surprised if it does make a difference.) > > If it's a real issue, you could fix this like how the > other similar cases were handled by system.h, by adding this > in system.h: > > #ifdef __cplusplus > #ifdef INCLUDE_UNIQUE_PTR > # include "unique-ptr.h" > #endif > #endif > > instead of unconditionally including <memory> there, > and then translation units that want unique-ptr.h would > do "#define INCLUDE_UNIQUE_PTR" instead of #include "unique-ptr.h", > like done for a few other C++ headers. > > (I maintain that IMO this is kind of self-inflicted GCC pain due > to the fact that "#pragma poison" poisons too much. If #pragma > poison's behavior were adjusted (or a new variant/mode created) to > ignore references to the poisoned symbol names in system headers (or > something like that), then you wouldn't need this manual management > of header dependencies in gcc/system.h and the corresponding > '#define INCLUDE_FOO' contortions. There's nothing that you can > reasonably > do with a reference to a poisoned symbol in a system header, other > than > avoid having the system header have the '#pragma poison' in effect > when > its included, which leads to contortions like system.h's. Note that > the poisoned names are _still used anyway_. So can we come up with > a GCC change that would avoid having to worry about manually doing > this? It'd likely help other projects too.) > > Thanks, > Pedro Alves FWIW, this one isn't from #pragma poison, it's from: #define abort() fancy_abort (__FILE__, __LINE__, __FUNCTION__) (I messed up the --in-reply-to when posting the patch, but Gerald noted the issue was due to: /usr/include/c++/v1/typeinfo:199:2: error: no member named 'fancy_abort' in namespace 'std::__1'; did you mean simply 'fancy_abort'? _VSTD::abort(); ^~~~~~~ /usr/include/c++/v1/__config:390:15: note: expanded from macro '_VSTD' #define _VSTD std::_LIBCPP_NAMESPACE ^ /scratch/tmp/gerald/gcc-HEAD/gcc/system.h:725:13: note: 'fancy_abort' declared here extern void fancy_abort (const char *, int, const char *) ^ https://gcc.gnu.org/ml/gcc-patches/2017-10/msg01289.html )
Hi, On Mon, 23 Oct 2017, David Malcolm wrote: > FWIW, this one isn't from #pragma poison, it's from: > #define abort() fancy_abort (__FILE__, __LINE__, __FUNCTION__) > > (I messed up the --in-reply-to when posting the patch, but Gerald noted > the issue was due to: > /usr/include/c++/v1/typeinfo:199:2: error: no member named > 'fancy_abort' in namespace 'std::__1'; did you mean simply > 'fancy_abort'? > _VSTD::abort(); > ^~~~~~~ So if we really really have to add an unconditional include in system.h it's probably enough to include <typeinfo>, not <memory>. Ciao, Michael.
On 10/23/2017 04:50 PM, David Malcolm wrote: > FWIW, this one isn't from #pragma poison, it's from: > #define abort() fancy_abort (__FILE__, __LINE__, __FUNCTION__) > > (I messed up the --in-reply-to when posting the patch, but Gerald noted > the issue was due to: > /usr/include/c++/v1/typeinfo:199:2: error: no member named > 'fancy_abort' in namespace 'std::__1'; did you mean simply > 'fancy_abort'? > _VSTD::abort(); > ^~~~~~~ > /usr/include/c++/v1/__config:390:15: note: expanded from macro '_VSTD' > #define _VSTD std::_LIBCPP_NAMESPACE > ^ > /scratch/tmp/gerald/gcc-HEAD/gcc/system.h:725:13: note: 'fancy_abort' > declared here > extern void fancy_abort (const char *, int, const char *) > ^ > IMO the best fix would be to rename that "#define abort" to "#define gcc_abort" and then call gcc_abort instead in the few places that currently call abort. IME, the introduction of a new naked call to abort() isn't something that easily passes review. abort calls always stand out and give reviewers pause (or they should!). FWIW, GDB also doesn't want such naked abort() calls, I don't recall people-sneaking-in-abort-()-calls ever being a problem over there. Thanks, Pedro Alves
On Mon, Oct 23, 2017 at 6:24 PM, Pedro Alves <palves@redhat.com> wrote: > On 10/23/2017 04:50 PM, David Malcolm wrote: > >> FWIW, this one isn't from #pragma poison, it's from: >> #define abort() fancy_abort (__FILE__, __LINE__, __FUNCTION__) >> >> (I messed up the --in-reply-to when posting the patch, but Gerald noted >> the issue was due to: >> /usr/include/c++/v1/typeinfo:199:2: error: no member named >> 'fancy_abort' in namespace 'std::__1'; did you mean simply >> 'fancy_abort'? >> _VSTD::abort(); >> ^~~~~~~ >> /usr/include/c++/v1/__config:390:15: note: expanded from macro '_VSTD' >> #define _VSTD std::_LIBCPP_NAMESPACE >> ^ >> /scratch/tmp/gerald/gcc-HEAD/gcc/system.h:725:13: note: 'fancy_abort' >> declared here >> extern void fancy_abort (const char *, int, const char *) >> ^ >> > > IMO the best fix would be to rename that "#define abort" to > "#define gcc_abort" and then call gcc_abort instead in the few > places that currently call abort. > > IME, the introduction of a new naked call to abort() isn't something > that easily passes review. abort calls always stand out and give > reviewers pause (or they should!). FWIW, GDB also doesn't want > such naked abort() calls, I don't recall people-sneaking-in-abort-()-calls > ever being a problem over there. It's probably for our build vs. target tools stuff sharing source. I agree the best solution is trying to get rid of that define (and instead poision raw abort). Richard. > Thanks, > Pedro Alves >
diff --git a/gcc/system.h b/gcc/system.h index f0664e9..d6e1637 100644 --- a/gcc/system.h +++ b/gcc/system.h @@ -233,6 +233,7 @@ extern int errno; # include <vector> #endif # include <cstring> +# include <memory> # include <new> # include <utility> #endif