| Message ID | 4F744C01.8030909@st.com |
|---|---|
| State | New |
| Headers | show |
Ping? On 29.03.2012 13:48, Christophe Lyon wrote: > Hello, > > According to a comment in configure/configure.ac: > # We want to ensure that TARGET libraries (which we know are built with > # gcc) are built with "-O2 -g", so include those options when setting > # CFLAGS_FOR_TARGET and CXXFLAGS_FOR_TARGET. > > but the current code does not ensure this. > > I propose the patch below to fix this. > > 2012-03-29 Christophe Lyon<christophe.lyon@st.com> > > * configure.ac (CFLAGS_FOR_TARGET, CXXFLAGS_FOR_TARGET): Make sure > they contain -O2. > * configure: Regenerate. > > Index: configure.ac > =================================================================== > --- configure.ac (revision 2515) > +++ configure.ac (working copy) > @@ -2223,7 +2223,7 @@ if test "x$CFLAGS_FOR_TARGET" = x; then > esac > case " $CFLAGS " in > *" -g "* | *" -g3 "*) ;; > - *) CFLAGS_FOR_TARGET="-g $CFLAGS" ;; > + *) CFLAGS_FOR_TARGET="-g $CFLAGS_FOR_TARGET" ;; > esac > fi > AC_SUBST(CFLAGS_FOR_TARGET) > @@ -2236,7 +2236,7 @@ if test "x$CXXFLAGS_FOR_TARGET" = x; the > esac > case " $CXXFLAGS " in > *" -g "* | *" -g3 "*) ;; > - *) CXXFLAGS_FOR_TARGET="-g $CXXFLAGS" ;; > + *) CXXFLAGS_FOR_TARGET="-g $CXXFLAGS_FOR_TARGET" ;; > esac > fi > AC_SUBST(CXXFLAGS_FOR_TARGET) > Index: configure > =================================================================== > --- configure (revision 2515) > +++ configure (working copy) > @@ -6739,7 +6739,7 @@ if test "x$CFLAGS_FOR_TARGET" = x; then > esac > case " $CFLAGS " in > *" -g "* | *" -g3 "*) ;; > - *) CFLAGS_FOR_TARGET="-g $CFLAGS" ;; > + *) CFLAGS_FOR_TARGET="-g $CFLAGS_FOR_TARGET" ;; > esac > fi > > @@ -6752,7 +6751,7 @@ if test "x$CXXFLAGS_FOR_TARGET" = x; the > esac > case " $CXXFLAGS " in > *" -g "* | *" -g3 "*) ;; > - *) CXXFLAGS_FOR_TARGET="-g $CXXFLAGS" ;; > + *) CXXFLAGS_FOR_TARGET="-g $CXXFLAGS_FOR_TARGET" ;; > esac > fi > > > . >
Ping? This patch was tested successfully on cross GCC x86 -> arm-none-eabi, to make sure that target libs are built with "-O2 -g" as stated in a comment in configure. It fixes what looks like a cut & paste error. Without this patch, overriding CFLAGS not including -O2 leads to CFLAGS_FOR_TARGET lacking -O2, which results in target libs such as libstdc++ being compiled without -O2. OK? Christophe. On 16.04.2012 14:51, Christophe Lyon wrote: > Ping? > > On 29.03.2012 13:48, Christophe Lyon wrote: >> Hello, >> >> According to a comment in configure/configure.ac: >> # We want to ensure that TARGET libraries (which we know are built with >> # gcc) are built with "-O2 -g", so include those options when setting >> # CFLAGS_FOR_TARGET and CXXFLAGS_FOR_TARGET. >> >> but the current code does not ensure this. >> >> I propose the patch below to fix this. >> >> 2012-03-29 Christophe Lyon<christophe.lyon@st.com> >> >> * configure.ac (CFLAGS_FOR_TARGET, CXXFLAGS_FOR_TARGET): Make sure >> they contain -O2. >> * configure: Regenerate. >> >> Index: configure.ac >> =================================================================== >> --- configure.ac (revision 2515) >> +++ configure.ac (working copy) >> @@ -2223,7 +2223,7 @@ if test "x$CFLAGS_FOR_TARGET" = x; then >> esac >> case " $CFLAGS " in >> *" -g "* | *" -g3 "*) ;; >> - *) CFLAGS_FOR_TARGET="-g $CFLAGS" ;; >> + *) CFLAGS_FOR_TARGET="-g $CFLAGS_FOR_TARGET" ;; >> esac >> fi >> AC_SUBST(CFLAGS_FOR_TARGET) >> @@ -2236,7 +2236,7 @@ if test "x$CXXFLAGS_FOR_TARGET" = x; the >> esac >> case " $CXXFLAGS " in >> *" -g "* | *" -g3 "*) ;; >> - *) CXXFLAGS_FOR_TARGET="-g $CXXFLAGS" ;; >> + *) CXXFLAGS_FOR_TARGET="-g $CXXFLAGS_FOR_TARGET" ;; >> esac >> fi >> AC_SUBST(CXXFLAGS_FOR_TARGET) >> Index: configure >> =================================================================== >> --- configure (revision 2515) >> +++ configure (working copy) >> @@ -6739,7 +6739,7 @@ if test "x$CFLAGS_FOR_TARGET" = x; then >> esac >> case " $CFLAGS " in >> *" -g "* | *" -g3 "*) ;; >> - *) CFLAGS_FOR_TARGET="-g $CFLAGS" ;; >> + *) CFLAGS_FOR_TARGET="-g $CFLAGS_FOR_TARGET" ;; >> esac >> fi >> >> @@ -6752,7 +6751,7 @@ if test "x$CXXFLAGS_FOR_TARGET" = x; the >> esac >> case " $CXXFLAGS " in >> *" -g "* | *" -g3 "*) ;; >> - *) CXXFLAGS_FOR_TARGET="-g $CXXFLAGS" ;; >> + *) CXXFLAGS_FOR_TARGET="-g $CXXFLAGS_FOR_TARGET" ;; >> esac >> fi >> >> >> . >> > . >
On Mon, 25 Jun 2012, Christophe Lyon wrote:
> Ping?
I advise CCing appropriate maintainers (in this case, build system
maintainers) on pings.
On 25.06.2012 17:24, Joseph S. Myers wrote: > On Mon, 25 Jun 2012, Christophe Lyon wrote: > >> Ping? > I advise CCing appropriate maintainers (in this case, build system > maintainers) on pings. > Ping again, CCing build system maintainers as suggested by Joseph. (BTW I'm proposing to modify code which was last modified by Paolo Bonzini according to git blame) Christophe.
On Jun 26, 2012, Christophe Lyon <christophe.lyon@st.com> wrote: > On 25.06.2012 17:24, Joseph S. Myers wrote: >> On Mon, 25 Jun 2012, Christophe Lyon wrote: >> >>> Ping? >> I advise CCing appropriate maintainers (in this case, build system >> maintainers) on pings. >> > Ping again, CCing build system maintainers as suggested by Joseph. Is this a ping for the patch you quoted in your Jun 25 email? It's generally good practice to include a link to the message holding the patch in a ping. I looked at the patch in there, and I'm afraid I don't understand how it achieves the ChangeLog-suggested purpose of ensuring -O2 makes to C*FLAGS_FOR_TARGET, when all it appears to do is to prepend -g. Can you please clarify? Thanks,
On 26.06.2012 22:17, Alexandre Oliva wrote: > On Jun 26, 2012, Christophe Lyon <christophe.lyon@st.com> wrote: > >> On 25.06.2012 17:24, Joseph S. Myers wrote: >>> On Mon, 25 Jun 2012, Christophe Lyon wrote: >>> >>>> Ping? >>> I advise CCing appropriate maintainers (in this case, build system >>> maintainers) on pings. >>> >> Ping again, CCing build system maintainers as suggested by Joseph. > Is this a ping for the patch you quoted in your Jun 25 email? It's > generally good practice to include a link to the message holding the > patch in a ping. Yes it is, sorry. The original proposal was: http://gcc.gnu.org/ml/gcc-patches/2012-03/msg01855.html > I looked at the patch in there, and I'm afraid I don't understand how it > achieves the ChangeLog-suggested purpose of ensuring -O2 makes to > C*FLAGS_FOR_TARGET, when all it appears to do is to prepend -g. Can you > please clarify? > With more context, the current code fragment is: CFLAGS_FOR_TARGET=$CFLAGS case " $CFLAGS " in *" -O2 "*) ;; *) CFLAGS_FOR_TARGET="-O2 $CFLAGS" ;; esac case " $CFLAGS " in *" -g "* | *" -g3 "*) ;; *) CFLAGS_FOR_TARGET="-g $CFLAGS" ;; esac where pre-pending -g discards -O2 if it was pre-pended just above. That's why I replace CFLAGS by CFLAGS_FOR_TARGET when pre-pending -g. Ditto for CXXFLAGS. Christophe.
On Jun 27, 2012, Christophe Lyon <christophe.lyon@st.com> wrote: >> I looked at the patch in there, and I'm afraid I don't understand how it >> achieves the ChangeLog-suggested purpose of ensuring -O2 makes to >> C*FLAGS_FOR_TARGET, when all it appears to do is to prepend -g. Can you >> please clarify? > With more context, the current code fragment is: > CFLAGS_FOR_TARGET=$CFLAGS > case " $CFLAGS " in > *" -O2 "*) ;; > *) CFLAGS_FOR_TARGET="-O2 $CFLAGS" ;; > esac > case " $CFLAGS " in > *" -g "* | *" -g3 "*) ;; > *) CFLAGS_FOR_TARGET="-g $CFLAGS" ;; > esac > where pre-pending -g discards -O2 if it was pre-pended just above. I see, thanks for clarifying. I suggest changing both occurrences of $CFLAGS within the case statements, then; the more uniform logic is more appealing to me. Patch approved with these changes. Thanks,
Index: configure.ac =================================================================== --- configure.ac (revision 2515) +++ configure.ac (working copy) @@ -2223,7 +2223,7 @@ if test "x$CFLAGS_FOR_TARGET" = x; then esac case " $CFLAGS " in *" -g "* | *" -g3 "*) ;; - *) CFLAGS_FOR_TARGET="-g $CFLAGS" ;; + *) CFLAGS_FOR_TARGET="-g $CFLAGS_FOR_TARGET" ;; esac fi AC_SUBST(CFLAGS_FOR_TARGET) @@ -2236,7 +2236,7 @@ if test "x$CXXFLAGS_FOR_TARGET" = x; the esac case " $CXXFLAGS " in *" -g "* | *" -g3 "*) ;; - *) CXXFLAGS_FOR_TARGET="-g $CXXFLAGS" ;; + *) CXXFLAGS_FOR_TARGET="-g $CXXFLAGS_FOR_TARGET" ;; esac fi AC_SUBST(CXXFLAGS_FOR_TARGET) Index: configure =================================================================== --- configure (revision 2515) +++ configure (working copy) @@ -6739,7 +6739,7 @@ if test "x$CFLAGS_FOR_TARGET" = x; then esac case " $CFLAGS " in *" -g "* | *" -g3 "*) ;; - *) CFLAGS_FOR_TARGET="-g $CFLAGS" ;; + *) CFLAGS_FOR_TARGET="-g $CFLAGS_FOR_TARGET" ;; esac fi @@ -6752,7 +6751,7 @@ if test "x$CXXFLAGS_FOR_TARGET" = x; the esac case " $CXXFLAGS " in *" -g "* | *" -g3 "*) ;; - *) CXXFLAGS_FOR_TARGET="-g $CXXFLAGS" ;; + *) CXXFLAGS_FOR_TARGET="-g $CXXFLAGS_FOR_TARGET" ;; esac fi
Hello, According to a comment in configure/configure.ac: # We want to ensure that TARGET libraries (which we know are built with # gcc) are built with "-O2 -g", so include those options when setting # CFLAGS_FOR_TARGET and CXXFLAGS_FOR_TARGET. but the current code does not ensure this. I propose the patch below to fix this. 2012-03-29 Christophe Lyon <christophe.lyon@st.com> * configure.ac (CFLAGS_FOR_TARGET, CXXFLAGS_FOR_TARGET): Make sure they contain -O2. * configure: Regenerate.