Re: [PATCH] Add INCLUDE_UNIQUE_PTR and use it (PR bootstrap/82610)

[Richard Biener <richard dot guenther at gmail dot com>]

On October 23, 2017 8:15:20 PM GMT+02:00, David Malcolm <dmalcolm@redhat.com> wrote:
>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
>
>Here's a different patch, which instead moves the include of our
>"unique-ptr.h" to system.h (conditionalized on INCLUDE_UNIQUE_PTR),
>after the decl of "free" and before the redefinition of "abort".
>
>It also makes the include of <memory> in unique-ptr.h be conditional
>on C++11 or later.
>
>Hence it makes the new stuff only be included for the places where
>we're actually using unique_ptr.
>
>Successfully bootstrapped & regrtested on x86_64-pc-linux-gnu,
>using gcc 4.8 for the initial bootstrap (hence testing both gnu+03
>and then gnu++14 in the selftests, for stage 1 and stages 2 and 3
>respectively).
>
>I don't know if it actually fixes the bootstrap issue seen with
>clang on Darwin and OpenBSD, though - but I expect it to.
>
>OK for trunk?

OK. 

Richard. 

>gcc/ChangeLog:
>	PR bootstrap/82610
>	* system.h: Conditionally include "unique-ptr.h" if
>	INCLUDE_UNIQUE_PTR is defined.
>	* unique-ptr-tests.cc: Remove include of "unique-ptr.h" in favor
>	of defining INCLUDE_UNIQUE_PTR before including "system.h".
>
>include/ChangeLog:
>	* unique-ptr.h: Make include of <memory> conditional on C++11 or
>	later.
>---
> gcc/system.h            | 10 ++++++++++
> gcc/unique-ptr-tests.cc |  2 +-
> include/unique-ptr.h    |  4 +++-
> 3 files changed, 14 insertions(+), 2 deletions(-)
>
>diff --git a/gcc/system.h b/gcc/system.h
>index f0664e9..1714af4 100644
>--- a/gcc/system.h
>+++ b/gcc/system.h
>@@ -720,6 +720,16 @@ extern int vsnprintf (char *, size_t, const char
>*, va_list);
> #define __builtin_expect(a, b) (a)
> #endif
> 
>+/* Some of the headers included by <memory> can use "abort" within a
>+   namespace, e.g. "_VSTD::abort();", which fails after we use the
>+   preprocessor to redefine "abort" as "fancy_abort" below.
>+   Given that unique-ptr.h can use "free", we need to do this after
>"free"
>+   is declared but before "abort" is overridden.  */
>+
>+#ifdef INCLUDE_UNIQUE_PTR
>+# include "unique-ptr.h"
>+#endif
>+
> /* Redefine abort to report an internal error w/o coredump, and
>    reporting the location of the error in the source file.  */
> extern void fancy_abort (const char *, int, const char *)
>diff --git a/gcc/unique-ptr-tests.cc b/gcc/unique-ptr-tests.cc
>index f5b72a8..d275694 100644
>--- a/gcc/unique-ptr-tests.cc
>+++ b/gcc/unique-ptr-tests.cc
>@@ -18,9 +18,9 @@ along with GCC; see the file COPYING3.  If not see
> <http://www.gnu.org/licenses/>.  */
> 
> #include "config.h"
>+#define INCLUDE_UNIQUE_PTR
> #include "system.h"
> #include "coretypes.h"
>-#include "unique-ptr.h"
> #include "selftest.h"
> 
> #if CHECKING_P
>diff --git a/include/unique-ptr.h b/include/unique-ptr.h
>index c5ca031..0b6f11a 100644
>--- a/include/unique-ptr.h
>+++ b/include/unique-ptr.h
>@@ -74,7 +74,9 @@
> #ifndef GNU_UNIQUE_PTR_H
> #define GNU_UNIQUE_PTR_H 1
> 
>-#include <memory>
>+#if __cplusplus >= 201103
>+# include <memory>
>+#endif
> 
> namespace gnu
> {