[OBVIOUS] Fix ifunc detection.

Message ID d7c7a480-a58d-a54b-cfc4-d1333b8e77e8@suse.cz
State New
Headers show
Series
  • [OBVIOUS] Fix ifunc detection.
Related show

Commit Message

Martin Liška Jan. 26, 2018, 3:24 p.m.
Hi.

This fixes detection of ifunc target capability.
I'm going to install the patch.

Martin

gcc/testsuite/ChangeLog:

2018-01-26  Martin Liska  <mliska@suse.cz>

	* lib/target-supports.exp: Return a value, otherwise -Wreturn-type
	warning is seen.
---
 gcc/testsuite/lib/target-supports.exp | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Thomas Schwinge March 2, 2018, 4:38 p.m. | #1
Hi!

On Fri, 26 Jan 2018 16:24:48 +0100, Martin Liška <mliska@suse.cz> wrote:
> This fixes detection of ifunc target capability.

> I'm going to install the patch.


You could also just have approved the patch I had sent two months before:
<http://mid.mail-archive.com/87fu9aiemr.fsf@euler.schwinge.homeip.net>.
;-)

One remark:

> --- a/gcc/testsuite/lib/target-supports.exp

> +++ b/gcc/testsuite/lib/target-supports.exp

> @@ -449,7 +449,7 @@ proc check_ifunc_available { } {

>  	extern "C" {

>  	#endif

>  	typedef void F (void);

> -	F* g (void) {}

> +	F* g (void) { return 0; }

>  	void f () __attribute__ ((ifunc ("g")));

>  	#ifdef __cplusplus

>  	}


Is it OK to "return 0" from this ifunc handler, or might some analysis in
GCC trip over that (at some later point)?  In my patch, I returned the
address of an "extern" function.


Grüße
 Thomas
Jeff Law March 2, 2018, 4:55 p.m. | #2
On 03/02/2018 09:38 AM, Thomas Schwinge wrote:
> Hi!

> 

> On Fri, 26 Jan 2018 16:24:48 +0100, Martin Liška <mliska@suse.cz> wrote:

>> This fixes detection of ifunc target capability.

>> I'm going to install the patch.

> 

> You could also just have approved the patch I had sent two months before:

> <http://mid.mail-archive.com/87fu9aiemr.fsf@euler.schwinge.homeip.net>.

> ;-)

> 

> One remark:

> 

>> --- a/gcc/testsuite/lib/target-supports.exp

>> +++ b/gcc/testsuite/lib/target-supports.exp

>> @@ -449,7 +449,7 @@ proc check_ifunc_available { } {

>>  	extern "C" {

>>  	#endif

>>  	typedef void F (void);

>> -	F* g (void) {}

>> +	F* g (void) { return 0; }

>>  	void f () __attribute__ ((ifunc ("g")));

>>  	#ifdef __cplusplus

>>  	}

> 

> Is it OK to "return 0" from this ifunc handler, or might some analysis in

> GCC trip over that (at some later point)?  In my patch, I returned the

> address of an "extern" function.

ISTM the question is whether or not ifuncs are ever allowed to return
NULL.  Maybe ping the glibc folks since that's where the extension started?

jeff
H.J. Lu March 2, 2018, 4:58 p.m. | #3
On Fri, Mar 2, 2018 at 8:55 AM, Jeff Law <law@redhat.com> wrote:
> On 03/02/2018 09:38 AM, Thomas Schwinge wrote:

>> Hi!

>>

>> On Fri, 26 Jan 2018 16:24:48 +0100, Martin Liška <mliska@suse.cz> wrote:

>>> This fixes detection of ifunc target capability.

>>> I'm going to install the patch.

>>

>> You could also just have approved the patch I had sent two months before:

>> <http://mid.mail-archive.com/87fu9aiemr.fsf@euler.schwinge.homeip.net>.

>> ;-)

>>

>> One remark:

>>

>>> --- a/gcc/testsuite/lib/target-supports.exp

>>> +++ b/gcc/testsuite/lib/target-supports.exp

>>> @@ -449,7 +449,7 @@ proc check_ifunc_available { } {

>>>      extern "C" {

>>>      #endif

>>>      typedef void F (void);

>>> -    F* g (void) {}

>>> +    F* g (void) { return 0; }

>>>      void f () __attribute__ ((ifunc ("g")));

>>>      #ifdef __cplusplus

>>>      }

>>

>> Is it OK to "return 0" from this ifunc handler, or might some analysis in

>> GCC trip over that (at some later point)?  In my patch, I returned the

>> address of an "extern" function.

> ISTM the question is whether or not ifuncs are ever allowed to return

> NULL.  Maybe ping the glibc folks since that's where the extension started?


No, ifunc selector should never return NULL.

-- 
H.J.
Jakub Jelinek March 2, 2018, 5 p.m. | #4
On Fri, Mar 02, 2018 at 09:55:22AM -0700, Jeff Law wrote:
> On 03/02/2018 09:38 AM, Thomas Schwinge wrote:

> > Hi!

> > 

> > On Fri, 26 Jan 2018 16:24:48 +0100, Martin Liška <mliska@suse.cz> wrote:

> >> This fixes detection of ifunc target capability.

> >> I'm going to install the patch.

> > 

> > You could also just have approved the patch I had sent two months before:

> > <http://mid.mail-archive.com/87fu9aiemr.fsf@euler.schwinge.homeip.net>.

> > ;-)

> > 

> > One remark:

> > 

> >> --- a/gcc/testsuite/lib/target-supports.exp

> >> +++ b/gcc/testsuite/lib/target-supports.exp

> >> @@ -449,7 +449,7 @@ proc check_ifunc_available { } {

> >>  	extern "C" {

> >>  	#endif

> >>  	typedef void F (void);

> >> -	F* g (void) {}

> >> +	F* g (void) { return 0; }

> >>  	void f () __attribute__ ((ifunc ("g")));

> >>  	#ifdef __cplusplus

> >>  	}

> > 

> > Is it OK to "return 0" from this ifunc handler, or might some analysis in

> > GCC trip over that (at some later point)?  In my patch, I returned the

> > address of an "extern" function.

> ISTM the question is whether or not ifuncs are ever allowed to return

> NULL.  Maybe ping the glibc folks since that's where the extension started?


Returning NULL means immediate segfault if somebody tries to call that
function.

	Jakub
Jeff Law March 2, 2018, 5:08 p.m. | #5
On 03/02/2018 10:00 AM, Jakub Jelinek wrote:
> On Fri, Mar 02, 2018 at 09:55:22AM -0700, Jeff Law wrote:

>> On 03/02/2018 09:38 AM, Thomas Schwinge wrote:

>>> Hi!

>>>

>>> On Fri, 26 Jan 2018 16:24:48 +0100, Martin Liška <mliska@suse.cz> wrote:

>>>> This fixes detection of ifunc target capability.

>>>> I'm going to install the patch.

>>>

>>> You could also just have approved the patch I had sent two months before:

>>> <http://mid.mail-archive.com/87fu9aiemr.fsf@euler.schwinge.homeip.net>.

>>> ;-)

>>>

>>> One remark:

>>>

>>>> --- a/gcc/testsuite/lib/target-supports.exp

>>>> +++ b/gcc/testsuite/lib/target-supports.exp

>>>> @@ -449,7 +449,7 @@ proc check_ifunc_available { } {

>>>>  	extern "C" {

>>>>  	#endif

>>>>  	typedef void F (void);

>>>> -	F* g (void) {}

>>>> +	F* g (void) { return 0; }

>>>>  	void f () __attribute__ ((ifunc ("g")));

>>>>  	#ifdef __cplusplus

>>>>  	}

>>>

>>> Is it OK to "return 0" from this ifunc handler, or might some analysis in

>>> GCC trip over that (at some later point)?  In my patch, I returned the

>>> address of an "extern" function.

>> ISTM the question is whether or not ifuncs are ever allowed to return

>> NULL.  Maybe ping the glibc folks since that's where the extension started?

> 

> Returning NULL means immediate segfault if somebody tries to call that

> function.

I understand that.  I'm referring to the semantic definition of an
ifunc.  It's an extension and we ought to have a crisp definition of
whether or not it can return NULL.

THe fact that returning NULL will cause the vast majority of systems to
segfault isn't the issue.  The issue is how have we defined the extension.

jeff
Martin Liška March 8, 2018, 9:15 a.m. | #6
On 03/02/2018 05:38 PM, Thomas Schwinge wrote:
> Hi!

> 

> On Fri, 26 Jan 2018 16:24:48 +0100, Martin Liška <mliska@suse.cz> wrote:

>> This fixes detection of ifunc target capability.

>> I'm going to install the patch.

> 

> You could also just have approved the patch I had sent two months before:

> <http://mid.mail-archive.com/87fu9aiemr.fsf@euler.schwinge.homeip.net>.

> ;-)


Hello.

Sorry for overlooking your patch. It's better to not return 0. Thomas
please install the patch, it's obvious fix.

Thanks,
Martin

> 

> One remark:

> 

>> --- a/gcc/testsuite/lib/target-supports.exp

>> +++ b/gcc/testsuite/lib/target-supports.exp

>> @@ -449,7 +449,7 @@ proc check_ifunc_available { } {

>>  	extern "C" {

>>  	#endif

>>  	typedef void F (void);

>> -	F* g (void) {}

>> +	F* g (void) { return 0; }

>>  	void f () __attribute__ ((ifunc ("g")));

>>  	#ifdef __cplusplus

>>  	}

> 

> Is it OK to "return 0" from this ifunc handler, or might some analysis in

> GCC trip over that (at some later point)?  In my patch, I returned the

> address of an "extern" function.

> 

> 

> Grüße

>  Thomas

>
Martin Liška March 8, 2018, 9:26 a.m. | #7
On 03/08/2018 10:15 AM, Martin Liška wrote:
> On 03/02/2018 05:38 PM, Thomas Schwinge wrote:

>> Hi!

>>

>> On Fri, 26 Jan 2018 16:24:48 +0100, Martin Liška <mliska@suse.cz> wrote:

>>> This fixes detection of ifunc target capability.

>>> I'm going to install the patch.

>>

>> You could also just have approved the patch I had sent two months before:

>> <http://mid.mail-archive.com/87fu9aiemr.fsf@euler.schwinge.homeip.net>.

>> ;-)

> 

> Hello.

> 

> Sorry for overlooking your patch. It's better to not return 0. Thomas

> please install the patch, it's obvious fix.


Looks Thomas is out of office, thus I installed his patch as r258362.

Martin

> 

> Thanks,

> Martin

> 

>>

>> One remark:

>>

>>> --- a/gcc/testsuite/lib/target-supports.exp

>>> +++ b/gcc/testsuite/lib/target-supports.exp

>>> @@ -449,7 +449,7 @@ proc check_ifunc_available { } {

>>>  	extern "C" {

>>>  	#endif

>>>  	typedef void F (void);

>>> -	F* g (void) {}

>>> +	F* g (void) { return 0; }

>>>  	void f () __attribute__ ((ifunc ("g")));

>>>  	#ifdef __cplusplus

>>>  	}

>>

>> Is it OK to "return 0" from this ifunc handler, or might some analysis in

>> GCC trip over that (at some later point)?  In my patch, I returned the

>> address of an "extern" function.

>>

>>

>> Grüße

>>  Thomas

>>

>

Patch

diff --git a/gcc/testsuite/lib/target-supports.exp b/gcc/testsuite/lib/target-supports.exp
index 24514233cea..c2ec93b9c80 100644
--- a/gcc/testsuite/lib/target-supports.exp
+++ b/gcc/testsuite/lib/target-supports.exp
@@ -449,7 +449,7 @@  proc check_ifunc_available { } {
 	extern "C" {
 	#endif
 	typedef void F (void);
-	F* g (void) {}
+	F* g (void) { return 0; }
 	void f () __attribute__ ((ifunc ("g")));
 	#ifdef __cplusplus
 	}