| Message ID | CAFULd4baLHKGQ2KKmt7uA8zAdqN4gWV-GHbh_-Quj3kp__v6NA@mail.gmail.com |
|---|---|
| State | New |
| Headers | show |
On Tue, Nov 11, 2014 at 10:41 AM, Uros Bizjak <ubizjak@gmail.com> wrote: > On Tue, Nov 11, 2014 at 9:09 AM, Uros Bizjak <ubizjak@gmail.com> wrote: >> On Mon, Nov 10, 2014 at 10:51 PM, Marc Glisse <marc.glisse@inria.fr> wrote: >>> On Mon, 10 Nov 2014, Richard Biener wrote: >>> >>>> No extra includes required? >>> >>> >>> <utility> is already included in wide-int.h and rtl.h, should probably move >>> those. >> >> Bah, we hit a problem. std::swap has been moved from <algorithm> to >> <utility> in C++11, and the patch breaks build on CentOS 5.11 >> (gcc-4.1.2). >> >> Short of reverting the i386.c patch, is there a quick solution by >> including some additional headers? > > Attached patch that implements both suggestions from Richi and Marc > fixes the bootstrap. > > 2014-11-11 Uros Bizjak <ubizjak@gmail.com> > > * system.h: Include algorithm and utility. > * rtl.h: Do not include utility here. > * wide-int.h: Ditto. > * tree-vect-data-refs.c (swap): Remove template. > (vect_prune_runtime_alias_test_list): Use std::swap instead of swap. > > Bootstrapped on x86_64-linux-gnu (CentOS 5.11). > > OK for mainline? > > BTW: There are lots of places where std::swap can be used, a nice > search-and-replace task for someone to start with gcc development. ;) Agreed ;) Note that we have to be careful to avoid pulling all of libstdc++ into all files via system.h (system.h is so a bad thing... :/). Ok. Thanks, Richard. > Uros.
On Tue, Nov 11, 2014 at 11:46:55AM +0100, Richard Biener wrote: > > BTW: There are lots of places where std::swap can be used, a nice > > search-and-replace task for someone to start with gcc development. ;) > > Agreed ;) Note that we have to be careful to avoid pulling all of libstdc++ > into all files via system.h (system.h is so a bad thing... :/). Apparently the nvptx port is another thing that fails to build because of the header ordering issues, this time not just when starting with clang, but just when building cross-compiler starting from gcc 4.9.1 on x86_64. There is: #include "config.h" #include "system.h" ... #include "hashtab.h" #include <sstream> and I get: g++ -c -g -DIN_GCC -DCROSS_DIRECTORY_STRUCTURE -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wmissing-format-attribute -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -fno-common -DHAVE_CONFIG_H -I. -I. -I../../gcc -I../../gcc/. -I../../gcc/../include -I../../gcc/../libcpp/include -I../../gcc/../libdecnumber -I../../gcc/../libdecnumber/dpd -I../libdecnumber -I../../gcc/../libbacktrace -o nvptx.o -MT nvptx.o -MMD -MP -MF ./.deps/nvptx.TPo ../../gcc/config/nvptx/nvptx.c In file included from /usr/include/c++/4.9.1/bits/basic_ios.h:37:0, from /usr/include/c++/4.9.1/ios:44, from /usr/include/c++/4.9.1/istream:38, from /usr/include/c++/4.9.1/sstream:38, from ../../gcc/config/nvptx/nvptx.c:54: /usr/include/c++/4.9.1/bits/locale_facets.h:240:53: error: macro "toupper" passed 2 arguments, but takes just 1 toupper(char_type *__lo, const char_type* __hi) const ^ and many more. As you said that we don't want to pull all of libstdc++ headers via system.h, looking at system.h, I see just the /* Define this so that inttypes.h defines the PRI?64 macros even when compiling with a C++ compiler. Define it here so in the event inttypes.h gets pulled in by another header it is already defined. */ #define __STDC_FORMAT_MACROS macro being critical to be included before any system headers, can't we move that into config.in/auto-host.h and just suggest header ordering of #include "config.h" #include <sstream> // or any other extra STL headers not provided by system.h you need #include "system.h" all other includes ? There are also some comments about stdarg.h and stdio.h ordering, dunno what it comes from and if it is still relevant when we require C++ compiler. Jakub
On Fri, Nov 14, 2014 at 8:18 AM, Jakub Jelinek <jakub@redhat.com> wrote: > On Tue, Nov 11, 2014 at 11:46:55AM +0100, Richard Biener wrote: >> > BTW: There are lots of places where std::swap can be used, a nice >> > search-and-replace task for someone to start with gcc development. ;) >> >> Agreed ;) Note that we have to be careful to avoid pulling all of libstdc++ >> into all files via system.h (system.h is so a bad thing... :/). > > Apparently the nvptx port is another thing that fails to build because of > the header ordering issues, this time not just when starting with clang, but > just when building cross-compiler starting from gcc 4.9.1 on x86_64. > > There is: > > #include "config.h" > #include "system.h" > ... > #include "hashtab.h" > #include <sstream> > > and I get: > g++ -c -g -DIN_GCC -DCROSS_DIRECTORY_STRUCTURE -fno-exceptions -fno-rtti -fasynchronous-unwind-tables -W -Wall -Wno-narrowing -Wwrite-strings -Wcast-qual -Wmissing-format-attribute -Woverloaded-virtual -pedantic -Wno-long-long -Wno-variadic-macros -Wno-overlength-strings -fno-common -DHAVE_CONFIG_H -I. -I. -I../../gcc -I../../gcc/. -I../../gcc/../include -I../../gcc/../libcpp/include -I../../gcc/../libdecnumber -I../../gcc/../libdecnumber/dpd -I../libdecnumber -I../../gcc/../libbacktrace -o nvptx.o -MT nvptx.o -MMD -MP -MF ./.deps/nvptx.TPo ../../gcc/config/nvptx/nvptx.c > In file included from /usr/include/c++/4.9.1/bits/basic_ios.h:37:0, > from /usr/include/c++/4.9.1/ios:44, > from /usr/include/c++/4.9.1/istream:38, > from /usr/include/c++/4.9.1/sstream:38, > from ../../gcc/config/nvptx/nvptx.c:54: > /usr/include/c++/4.9.1/bits/locale_facets.h:240:53: error: macro "toupper" passed 2 arguments, but takes just 1 > toupper(char_type *__lo, const char_type* __hi) const > ^ > and many more. > As you said that we don't want to pull all of libstdc++ > headers via system.h, looking at system.h, I see just the > > /* Define this so that inttypes.h defines the PRI?64 macros even > when compiling with a C++ compiler. Define it here so in the > event inttypes.h gets pulled in by another header it is already > defined. */ > #define __STDC_FORMAT_MACROS > > macro being critical to be included before any system headers, > can't we move that into config.in/auto-host.h and just suggest > header ordering of > > #include "config.h" > #include <sstream> // or any other extra STL headers not provided by system.h you need > #include "system.h" > all other includes Ick. > ? There are also some comments about stdarg.h and stdio.h ordering, > dunno what it comes from and if it is still relevant when we require > C++ compiler. I think we should simply discourage people from using sstream for example. But I don't see how we can live without system.h with all the weird host systems still around - thus your solution above will very likely not work. Eventually we can split system.h into a c-system.h and cxx-system.h so we can distinguish between uses in files compiled with a C and a C++ compiler? Richard. > Jakub
On Fri, Nov 14, 2014 at 11:57:57AM +0100, Richard Biener wrote: > > ? There are also some comments about stdarg.h and stdio.h ordering, > > dunno what it comes from and if it is still relevant when we require > > C++ compiler. > > I think we should simply discourage people from using sstream for > example. That would be my preference too of course, unfortunately Bernd chose to use it everywhere (grep '<<' nvptx.c'). > But I don't see how we can live without system.h with all the weird > host systems still around - thus your solution above will very likely > not work. Well, I wasn't suggesting without system.h, I was suggesting to include config.h first (that is required anyway), then C++ STL headers, then system.h and then other GCC headers. > Eventually we can split system.h into a c-system.h and cxx-system.h > so we can distinguish between uses in files compiled with a C and > a C++ compiler? That wouldn't help here. Jakub
On Fri, Nov 14, 2014 at 12:03 PM, Jakub Jelinek <jakub@redhat.com> wrote: > On Fri, Nov 14, 2014 at 11:57:57AM +0100, Richard Biener wrote: >> > ? There are also some comments about stdarg.h and stdio.h ordering, >> > dunno what it comes from and if it is still relevant when we require >> > C++ compiler. >> >> I think we should simply discourage people from using sstream for >> example. > > That would be my preference too of course, unfortunately Bernd chose to use > it everywhere (grep '<<' nvptx.c'). > >> But I don't see how we can live without system.h with all the weird >> host systems still around - thus your solution above will very likely >> not work. > > Well, I wasn't suggesting without system.h, I was suggesting to include > config.h first (that is required anyway), then C++ STL headers, then > system.h and then other GCC headers. I'm quite sure that we'll find a system where that won't work? But sure - maybe it's worth a try... Richard. >> Eventually we can split system.h into a c-system.h and cxx-system.h >> so we can distinguish between uses in files compiled with a C and >> a C++ compiler? > > That wouldn't help here. > > Jakub
On 11/14/2014 12:03 PM, Jakub Jelinek wrote: > On Fri, Nov 14, 2014 at 11:57:57AM +0100, Richard Biener wrote: >>> ? There are also some comments about stdarg.h and stdio.h ordering, >>> dunno what it comes from and if it is still relevant when we require >>> C++ compiler. >> >> I think we should simply discourage people from using sstream for >> example. > > That would be my preference too of course, unfortunately Bernd chose to use > it everywhere (grep '<<' nvptx.c'). I needed a way to output something but defer it to the end of the assembly file. sstream seemed like the tool for the job; if we go to the trouble of converting the compiler to C++ we might as well use the language as intended. FWIW Cesar also ran into this problem and moving the sstream include first seems to solve it, so I'll be committing that fix. Bernd
On Fri, Nov 14, 2014 at 12:27:52PM +0100, Bernd Schmidt wrote: > On 11/14/2014 12:03 PM, Jakub Jelinek wrote: > >On Fri, Nov 14, 2014 at 11:57:57AM +0100, Richard Biener wrote: > >>>? There are also some comments about stdarg.h and stdio.h ordering, > >>>dunno what it comes from and if it is still relevant when we require > >>>C++ compiler. > >> > >>I think we should simply discourage people from using sstream for > >>example. > > > >That would be my preference too of course, unfortunately Bernd chose to use > >it everywhere (grep '<<' nvptx.c'). > > I needed a way to output something but defer it to the end of the assembly > file. sstream seemed like the tool for the job; if we go to the trouble of > converting the compiler to C++ we might as well use the language as > intended. > > FWIW Cesar also ran into this problem and moving the sstream include first > seems to solve it, so I'll be committing that fix. Please include it after config.h though. Jakub
On 11/14/14 03:57, Richard Biener wrote: > > I think we should simply discourage people from using sstream for > example. I can live with that -- I've seen sstream stuff fly by in a few places, almost asked the submitters to remove it (I find the style of coding most folks use with sstream horrid to read), but ultimately decided not to object. If we're going to discourage sstream, better to do so now before too much more gets in. jeff
Index: tree-vect-data-refs.c =================================================================== --- tree-vect-data-refs.c (revision 217340) +++ tree-vect-data-refs.c (working copy) @@ -2718,14 +2718,6 @@ return 0; } -template <class T> static void -swap (T& a, T& b) -{ - T c (a); - a = b; - b = c; -} - /* Function vect_vfa_segment_size. Create an expression that computes the size of segment @@ -2858,7 +2850,7 @@ vect_prune_runtime_alias_test_list (loop_vec_info dr_with_seg_len (dr_b, segment_length_b)); if (compare_tree (DR_BASE_ADDRESS (dr_a), DR_BASE_ADDRESS (dr_b)) > 0) - swap (dr_with_seg_len_pair.first, dr_with_seg_len_pair.second); + std::swap (dr_with_seg_len_pair.first, dr_with_seg_len_pair.second); comp_alias_ddrs.safe_push (dr_with_seg_len_pair); } @@ -2908,8 +2900,8 @@ vect_prune_runtime_alias_test_list (loop_vec_info and DR_A1 and DR_A2 are two consecutive memrefs. */ if (*dr_a1 == *dr_a2) { - swap (dr_a1, dr_b1); - swap (dr_a2, dr_b2); + std::swap (dr_a1, dr_b1); + std::swap (dr_a2, dr_b2); } if (!operand_equal_p (DR_BASE_ADDRESS (dr_a1->dr), Index: wide-int.h =================================================================== --- wide-int.h (revision 217340) +++ wide-int.h (working copy) @@ -216,8 +216,6 @@ the same result as X + X; the precision of the shift amount Y can be arbitrarily different from X. */ - -#include <utility> #include "system.h" #include "hwint.h" #include "signop.h" Index: rtl.h =================================================================== --- rtl.h (revision 217340) +++ rtl.h (working copy) @@ -20,7 +20,6 @@ #ifndef GCC_RTL_H #define GCC_RTL_H -#include <utility> #include "statistics.h" #include "machmode.h" #include "input.h" Index: system.h =================================================================== --- system.h (revision 217340) +++ system.h (working copy) @@ -208,7 +208,9 @@ #endif #ifdef __cplusplus +# include <algorithm> # include <cstring> +# include <utility> #endif /* Some of glibc's string inlines cause warnings. Plus we'd rather