diff mbox

[RFC] Implementing ifunc target hook

Message ID CACysShiTsza6Tpd-05jiHBdkcqybNg1wEij_cKzpQ90S5Ey0NQ@mail.gmail.com
State New
Headers show

Commit Message

Alexander Ivchenko March 26, 2013, 3:14 p.m. UTC 
Hi,

Since almost three months have passed I feel that I need to recheck the patch
before commiting it. I fixed what Maxim mentioned and also I fixed:

the support of IFUNC
and the build of compiler could be broken. Now we define
HAVE_GNU_INDIRECT_FUNCTION as 0 in
those cases.

ok for trunk?

thanks,
Alexander

2013/1/15 Maxim Kuvyrkov <maxim.kuvyrkov@gmail.com>:
> On 15/01/2013, at 4:55 AM, Alexander Ivchenko wrote:
>
>> thank you very much for your review!
>>
>> I fixed the arm build and all other issues that you raised.
>>
>> the patch is attached. Bootstrap and tested on x86-64 linux
>
>
> The patch is OK with the cleanups mentioned below (no need to resubmit for review).  Unfortunately, you will have to wait for Stage 1 to commit your patch.
>
>
>> --- a/gcc/config.gcc
>> +++ b/gcc/config.gcc
>> @@ -636,6 +636,11 @@ case ${target} in
>>        native_system_header_dir=/include
>>        ;;
>>    esac
>> +  case $target in
>> +    *linux*)
>> +      tm_p_file="${tm_p_file} linux-protos.h"
>> +      ;;
>> +  esac
>>    # glibc / uclibc / bionic switch.
>>    # uclibc and bionic aren't usable for GNU/Hurd and neither for GNU/k*BSD.
>>    case $target in
>
> Can we merge this above hunk into subsequent "case $target" statement ...
>
>> @@ -661,8 +666,10 @@ case ${target} in
>>    # Add Android userspace support to Linux targets.
>>    case $target in
>>      *linux*)
>> +      tmake_file="${tmake_file} t-linux-android"
>>        tm_file="$tm_file linux-android.h"
>>        extra_options="$extra_options linux-android.opt"
>> +      extra_objs="$extra_objs linux-android.o"
>>        ;;
>>    esac
>
> ... here?
>
>>    # Enable compilation for Android by default for *android* targets.
>> @@ -863,7 +870,9 @@ arm*-*-netbsdelf*)
>>       tmake_file="${tmake_file} arm/t-arm"
>>       ;;
>>  arm*-*-linux-*)                      # ARM GNU/Linux with ELF
>> +     tmake_file="${tmake_file} t-linux-android"
>
> Merge this with tmake_file= setting a couple of lines down.  Put t-linux-android last on the line.
>
>>       tm_file="dbxelf.h elfos.h gnu-user.h linux.h linux-android.h glibc-stdint.h arm/elf.h arm/linux-gas.h arm/linux-elf.h"
>> +     extra_objs="$extra_objs linux-android.o"
>
> Please push extra_objs= setting a couple of lines down so that addition of t-linux-android and linux-android.o are side-by-side.
>
>>       case $target in
>>       arm*b-*-linux*)
>>           tm_defines="${tm_defines} TARGET_BIG_ENDIAN_DEFAULT=1"
>> diff --git a/gcc/config/linux-android.c b/gcc/config/linux-android.c
>> new file mode 100644
>> index 0000000..f3d82a5
>> --- /dev/null
>> +++ b/gcc/config/linux-android.c
>> @@ -0,0 +1,34 @@
>> +/* Functions for Linux Android as target machine for GNU C compiler.
>> +   Copyright (C) 2001, 2002, 2003, 2004, 2005, 2007, 2008, 2010, 2011,
>> +   2012, 2013.
>
> Should be "Copyright (C) 2013."  The copyright dates start with the year in which  a file was added.
>
> Also, for any file that your changes touch please add 2013 to the list of copyright years.  This is an annoying chore that committers have to do at the beginning of each year.
>
>> diff --git a/gcc/config/linux-protos.h b/gcc/config/linux-protos.h
>> new file mode 100644
>> index 0000000..aae1d28
>> --- /dev/null
>> +++ b/gcc/config/linux-protos.h
>> @@ -0,0 +1,22 @@
>> +/* Prototypes.
>> +   Copyright (C) 2001, 2002, 2003, 2004, 2005, 2007, 2008, 2010, 2011,
>> +   2012, 2013
>
> "Copyright (C) 2013."
>
>> --- /dev/null
>> +++ b/gcc/config/t-linux-android
>> @@ -0,0 +1,23 @@
>> +# Copyright (C) 2002, 2003, 2004, 2005, 2006, 2007, 2008, 2009, 2010, 2011, 2013
>
> "Copyright (C) 2013."
>
> Thanks!
>
> --
> Maxim Kuvyrkov
>

Comments

Maxim Kuvyrkov March 26, 2013, 8:59 p.m. UTC | #1 
On 27/03/2013, at 4:14 AM, Alexander Ivchenko wrote:

> Hi,
> 
> Since almost three months have passed I feel that I need to recheck the patch
> before commiting it. I fixed what Maxim mentioned and also I fixed:

The patch is OK with 2 changes:

1. s/default_have_ifunc_p/default_has_ifunc_p/
The new target hook is called "has_ifunc_p", so "has" in the name of its default implementation is more appropriate.

> 
> diff --git a/gcc/configure b/gcc/configure
> old mode 100755
> new mode 100644
> index eac96cd..928693a
> --- a/gcc/configure
> +++ b/gcc/configure
> @@ -22055,11 +22055,14 @@ else
>   enable_gnu_indirect_function="$default_gnu_indirect_function"
> fi
> 
> -if test x$enable_gnu_indirect_function = xyes; then
> 
> -$as_echo "#define HAVE_GNU_INDIRECT_FUNCTION 1" >>confdefs.h
> +gif=`if test $enable_gnu_indirect_function == yes; then echo 1; else
> echo 0; fi`

2. gif=`if test x$enable_gnu_indirect_function = xyes; then echo 1; else
echo 0; fi`

Note that canonical equality operator of 'test' is "=", not "==".  The 'x' before the variable is a good practice to handle empty definitions of shell variables (`if test = yes;` will produce an error).

Oh, and in the changelog you have a typo "linux-androids.h" -> "linux-android.h".

Otherwise OK.

Thanks,

--
Maxim Kuvyrkov
KugelWorks
Kirill Yukhin March 27, 2013, 9:56 a.m. UTC | #2 
>
> Otherwise OK.
>
> Thanks,

Hi,  chacked into trunk: http://gcc.gnu.org/ml/gcc-cvs/2013-03/msg00785.html


Thanks, K
Jakub Jelinek April 2, 2013, 1:24 p.m. UTC | #3 
On Wed, Mar 27, 2013 at 01:56:48PM +0400, Kirill Yukhin wrote:
> >
> > Otherwise OK.
> >
> > Thanks,
> 
> Hi,  chacked into trunk: http://gcc.gnu.org/ml/gcc-cvs/2013-03/msg00785.html

This leads to:
../../gcc/config/t-linux-android:22: warning: overriding recipe for target `linux-android.o'
../../gcc/config/t-linux-android:22: warning: ignoring old recipe for target `linux-android.o'
for arm*-linux* target (cross in my case).  t-linux-android is listed twice.

	Jakub
diff mbox

Patch

diff --git a/gcc/configure b/gcc/configure
old mode 100755
new mode 100644
index eac96cd..928693a
--- a/gcc/configure
+++ b/gcc/configure
@@ -22055,11 +22055,14 @@  else
   enable_gnu_indirect_function="$default_gnu_indirect_function"
 fi

-if test x$enable_gnu_indirect_function = xyes; then

-$as_echo "#define HAVE_GNU_INDIRECT_FUNCTION 1" >>confdefs.h
+gif=`if test $enable_gnu_indirect_function == yes; then echo 1; else
echo 0; fi`
+
+cat >>confdefs.h <<_ACEOF
+#define HAVE_GNU_INDIRECT_FUNCTION $gif
+_ACEOF
+

-fi

 if test $in_tree_ld != yes ; then
   ld_ver=`$gcc_cv_ld --version 2>/dev/null | sed 1q`
diff --git a/gcc/configure.ac b/gcc/configure.ac
index 40a1af7..51d334c 100644
--- a/gcc/configure.ac
+++ b/gcc/configure.ac
@@ -2299,10 +2299,11 @@  AC_ARG_ENABLE(gnu-indirect-function,
 Valid choices are 'yes' and 'no'.]) ;;
   esac],
  [enable_gnu_indirect_function="$default_gnu_indirect_function"])
-if test x$enable_gnu_indirect_function = xyes; then
-  AC_DEFINE(HAVE_GNU_INDIRECT_FUNCTION, 1,
-   [Define if your system supports gnu indirect functions.])
-fi
+
+gif=`if test $enable_gnu_indirect_function == yes; then echo 1; else
echo 0; fi`
+AC_DEFINE_UNQUOTED(HAVE_GNU_INDIRECT_FUNCTION, $gif,
+[Define if your system supports gnu indirect functions.])
+

HAVE_GNU_INDIRECT_FUNCTION was not defined on targets that don't have