Re: [Patch, avr] Propagate -mrelax gcc driver flag to assembler

[Senthil Kumar Selvaraj <senthil_kumar dot selvaraj at atmel dot com>]

On Mon, May 12, 2014 at 01:19:37PM +0200, Georg-Johann Lay wrote:
> Am 04/18/2014 11:52 AM, schrieb Senthil Kumar Selvaraj:
> >
> >On Sat, Apr 12, 2014 at 06:36:01PM +0200, Georg-Johann Lay wrote:
> >>Senthil Kumar Selvaraj schrieb:
> >>>This patch modifies AVR target's ASM spec to pass -mlink-relax to the
> >>>assembler if -mrelax is passed to the compiler driver. This was already
> >>>being passed on to the linker, this patch merely makes the assembler
> >>>also aware of it.
> >>>
> >>>The corresponding patch in binutils to handle the -mlink-relax patch is
> >>>already committed in the binutils repo. I'm not sure how to manage a
> >>>running a newer gcc with an older version of binutils though - how is this
> >>>generally handled?
> >>
> >>The right place is gcc/configure.ac and have a macro defined depending on
> >>whether gas supports -mlink-relax.
> >>
> >>
> >>Same should be done for -mrmw, IMO, for similar reasons, e.g. something like
> >>
> >>case "$target" in
> >>   ...
> >>   avr-*-*)
> >>   ...
> >>     gcc_GAS_CHECK_FEATURE([-mrmw option], gcc_cv_as_avr_mrmw,,
> >>       [-mrmw], [.text],,
> >>       [AC_DEFINE(HAVE_AS_AVR_MRMW_OPTION, 1,
> >>		[Define if your assembler supports -mrmw option.])])
> >>
> >>or
> >>
> >>     gcc_GAS_CHECK_FEATURE([-mrmw option], gcc_cv_as_avr_mrmw,,
> >>                           [-mrmw], [.text],,,)
> >>     if test x$gcc_cv_as_avr_mrmw = xyes; then
> >>       AC_DEFINE(HAVE_AS_AVR_MRMW_OPTION, 1,
> >>                 [Define if your assembler supports the -mrmw option.])
> >>
> >
> >Thanks Johann. The below patch adds the configury check for -mlink-relax,
> >along with the change to ASM_SPEC to propagate the -mrelax flag. I
> >modified the original patch a bit to make it easier to handle
> >conditional additions to SPECs (inspired by the sparc target).
> >
> >>
> >>However, the gcc-4_9-branch has already been created...
> >
> >Yes, but I figured it would be useful anyway - if this eventually gets
> >backported to 4_9, for example.
> >
> >If the below patch looks ok, could someone commit please? I don't have
> >commit access.
> >
> >Regards
> >Senthil
> >
> >gcc/ChangeLog
> >
> >2014-04-18  Senthil Kumar Selvaraj  <senthil_kumar.selvaraj@atmel.com>
> >
> >	* config/avr/avr.h: Pass on mlink-relax to assembler.
> >	* configure.ac: Test for mlink-relax support in assembler.
> >	* configure: Regenerate.
> >
> >diff --git gcc/config/avr/avr.h gcc/config/avr/avr.h
> >index 78434ec..b4e3eb1 100644
> >--- gcc/config/avr/avr.h
> >+++ gcc/config/avr/avr.h
> >@@ -512,7 +512,28 @@ extern const char *avr_device_to_sp8 (int argc, const char **argv);
> >      %{!fenforce-eh-specs:-fno-enforce-eh-specs} \
> >      %{!fexceptions:-fno-exceptions}"
> >
> >-#define ASM_SPEC "%:device_to_as(%{mmcu=*:%*}) "
> >+#ifdef HAVE_AS_RELAX_OPTION
> >+#define ASM_RELAX_SPEC "%{mrelax:-mlink-relax}"
> >+#else
> >+#define ASM_RELAX_SPEC ""
> >+#endif
> >+
> >+#define ASM_SPEC "%:device_to_as(%{mmcu=*:%*})\
> >+%(asm_relax)"
> >+
> >+/* This macro defines names of additional specifications to put in the specs
> >+   that can be used in various specifications like CC1_SPEC.  Its definition
> >+   is an initializer with a subgrouping for each command option.
> >+
> >+   Each subgrouping contains a string constant, that defines the
> >+   specification name, and a string constant that used by the GCC driver
> >+   program.
> >+
> >+   Do not define this macro if it does not need to do anything.  */
> >+
> >+#define EXTRA_SPECS \
> >+  { "asm_relax",	ASM_RELAX_SPEC }
> >+
> 
> Hi, wouldn't it be easier to add just a line to driver-avr.c:avr_device_to_as ?

Well, I couldn't figure out how to do it without passing in the nested spec and
then do argument checking inside avr_device_to_as. Something like

#define ASM_SPEC "%:device_to_as(%{mmcu=*:%*} %{mrelax:-mlink-relax})"

and then handle argc==0, 1 and 2 cases by strcmp'ing against
-mlink-relax if HAVE_AVR_AS_LINK_RELAX_OPTION. 
Did I miss something?
> 
> >
> >  #define LINK_SPEC "\
> >  %{mrelax:--relax\
> >diff --git gcc/configure gcc/configure
> >index bfb1525..7815038 100755
> >--- gcc/configure
> >+++ gcc/configure
> >@@ -24142,6 +24142,39 @@ $as_echo "#define HAVE_AS_JSRDIRECT_RELOCS 1" >>confdefs.h
> >  fi
> >      ;;
> >
> >+  avr-*-*)
> >+    { $as_echo "$as_me:${as_lineno-$LINENO}: checking assembler for -mlink-relax option" >&5
> >+$as_echo_n "checking assembler for -mlink-relax option... " >&6; }
> >+if test "${gcc_cv_as_avr_relax+set}" = set; then :
> >+  $as_echo_n "(cached) " >&6
> >+else
> >+  gcc_cv_as_avr_relax=no
> >+  if test x$gcc_cv_as != x; then
> >+    $as_echo '.text' > conftest.s
> >+    if { ac_try='$gcc_cv_as $gcc_cv_as_flags -mlink-relax -o conftest.o conftest.s >&5'
> >+  { { eval echo "\"\$as_me\":${as_lineno-$LINENO}: \"$ac_try\""; } >&5
> >+  (eval $ac_try) 2>&5
> >+  ac_status=$?
> >+  $as_echo "$as_me:${as_lineno-$LINENO}: \$? = $ac_status" >&5
> >+  test $ac_status = 0; }; }
> >+    then
> >+	gcc_cv_as_avr_relax=yes
> >+    else
> >+      echo "configure: failed program was" >&5
> >+      cat conftest.s >&5
> >+    fi
> >+    rm -f conftest.o conftest.s
> >+  fi
> >+fi
> >+{ $as_echo "$as_me:${as_lineno-$LINENO}: result: $gcc_cv_as_avr_relax" >&5
> >+$as_echo "$gcc_cv_as_avr_relax" >&6; }
> >+if test $gcc_cv_as_avr_relax = yes; then
> >+
> >+$as_echo "#define HAVE_AS_RELAX_OPTION 1" >>confdefs.h
> >+
> >+fi
> >+  ;;
> >+
> >    cris-*-*)
> >      { $as_echo "$as_me:${as_lineno-$LINENO}: checking assembler for -no-mul-bug-abort option" >&5
> >  $as_echo_n "checking assembler for -no-mul-bug-abort option... " >&6; }
> >diff --git gcc/configure.ac gcc/configure.ac
> >index d7cae6c..cfa862d 100644
> >--- gcc/configure.ac
> >+++ gcc/configure.ac
> >@@ -3579,6 +3579,13 @@ case "$target" in
> >    [Define if your assembler supports the lituse_jsrdirect relocation.])])
> >      ;;
> >
> >+  avr-*-*)
> >+    gcc_GAS_CHECK_FEATURE([-mlink-relax option], gcc_cv_as_avr_relax,,
> >+      [-mlink-relax], [.text],,
> >+      [AC_DEFINE(HAVE_AS_RELAX_OPTION, 1,
> 
> IMO this is very confusing naming
> 
> - The gas option is not "relax" but "link-relax"
> - The option is avr specific, thus in order to avoid naming clashes
>   the variable(s) should be named something like
> 
> HAVE_AS_AVR_LINK_RELAX_OPTION

Ok, I'll resend the patch with the renamed define - I was following
whatever the sparc target was doing.

Regards
Senthil