[MIPS] Fix warning in sysdeps/mips/dl-trampoline.c

Message ID 872cdd55-2333-4304-9335-3ca5574f30c5@BAMAIL02.ba.imgtec.org
State Superseded
Delegated to: Carlos O'Donell
Headers

Commit Message

Steve Ellcey Dec. 11, 2014, 12:19 a.m. UTC
  Here is another warning/error fix.  The mips dl-trampline.c is doing a
switch on a comparision expression and this gives the error:

../sysdeps/mips/dl-trampoline.c: In function '__dl_runtime_resolve':
../sysdeps/mips/dl-trampoline.c:142:15: error: switch condition has boolean value [-Werror=switch-bool]
       switch (l->l_info[VERSYMIDX (DT_VERSYM)] != NULL)
               ^
cc1: all warnings being treated as errors

I looked at changing the switch to an if statement because there are only
two cases (default and 0) but it turned out to be a bit tricky because
in some cases the default case does a break and in other cases it falls
through to the '0' case.  So instead I just cast the comparision to an
int before switching on it.

Ok to checkin?

Steve Ellcey
sellcey@imgtec.com


2014-12-10  Steve Ellcey  <sellcey@imgtec.com>

	* sysdeps/mips/dl-trampoline.c: Cast switch expression.
  

Comments

Carlos O'Donell Dec. 11, 2014, 1:01 a.m. UTC | #1
On 12/10/2014 07:19 PM, Steve Ellcey  wrote:
> Here is another warning/error fix.  The mips dl-trampline.c is doing a
> switch on a comparision expression and this gives the error:
> 
> ../sysdeps/mips/dl-trampoline.c: In function '__dl_runtime_resolve':
> ../sysdeps/mips/dl-trampoline.c:142:15: error: switch condition has boolean value [-Werror=switch-bool]
>        switch (l->l_info[VERSYMIDX (DT_VERSYM)] != NULL)
>                ^
> cc1: all warnings being treated as errors
> 
> I looked at changing the switch to an if statement because there are only
> two cases (default and 0) but it turned out to be a bit tricky because
> in some cases the default case does a break and in other cases it falls
> through to the '0' case.  So instead I just cast the comparision to an
> int before switching on it.
> 
> Ok to checkin?

Why not "switch (l->l_info[VERSYMIDX (DT_VERSYM)] != NULL ? 0 : 1)" to
avoid the (int) case potentially hiding future type errors?

> Steve Ellcey
> sellcey@imgtec.com
> 
> 
> 2014-12-10  Steve Ellcey  <sellcey@imgtec.com>
> 
> 	* sysdeps/mips/dl-trampoline.c: Cast switch expression.
> 
> diff --git a/sysdeps/mips/dl-trampoline.c b/sysdeps/mips/dl-trampoline.c
> index f565654..3aa147b 100644
> --- a/sysdeps/mips/dl-trampoline.c
> +++ b/sysdeps/mips/dl-trampoline.c
> @@ -139,7 +139,7 @@ __dl_runtime_resolve (ElfW(Word) sym_index,
>    /* FIXME: The symbol versioning stuff is not tested yet.  */
>    if (__builtin_expect (ELFW(ST_VISIBILITY) (sym->st_other), 0) == 0)
>      {
> -      switch (l->l_info[VERSYMIDX (DT_VERSYM)] != NULL)
> +      switch ((int) (l->l_info[VERSYMIDX (DT_VERSYM)] != NULL))
>  	{
>  	default:
>  	  {
>
  
Steve Ellcey Dec. 11, 2014, 4:21 p.m. UTC | #2
On Wed, 2014-12-10 at 20:01 -0500, Carlos O'Donell wrote:

> Why not "switch (l->l_info[VERSYMIDX (DT_VERSYM)] != NULL ? 0 : 1)" to
> avoid the (int) case potentially hiding future type errors?

That seems reasonable but shouldn't it be "switch (l->l_info[VERSYMIDX
(DT_VERSYM)] != NULL ? 1 : 0)"  I.e. return 1 if the boolean expression
is true and 0 if the boolean expression is false?

Steve
  
Carlos O'Donell Dec. 11, 2014, 4:31 p.m. UTC | #3
On 12/11/2014 11:21 AM, Steve Ellcey wrote:
> On Wed, 2014-12-10 at 20:01 -0500, Carlos O'Donell wrote:
> 
>> Why not "switch (l->l_info[VERSYMIDX (DT_VERSYM)] != NULL ? 0 : 1)" to
>> avoid the (int) case potentially hiding future type errors?
> 
> That seems reasonable but shouldn't it be "switch (l->l_info[VERSYMIDX
> (DT_VERSYM)] != NULL ? 1 : 0)"  I.e. return 1 if the boolean expression
> is true and 0 if the boolean expression is false?

Yes, you are correct, my mistake.

Cheers,
Carlos.
  

Patch

diff --git a/sysdeps/mips/dl-trampoline.c b/sysdeps/mips/dl-trampoline.c
index f565654..3aa147b 100644
--- a/sysdeps/mips/dl-trampoline.c
+++ b/sysdeps/mips/dl-trampoline.c
@@ -139,7 +139,7 @@  __dl_runtime_resolve (ElfW(Word) sym_index,
   /* FIXME: The symbol versioning stuff is not tested yet.  */
   if (__builtin_expect (ELFW(ST_VISIBILITY) (sym->st_other), 0) == 0)
     {
-      switch (l->l_info[VERSYMIDX (DT_VERSYM)] != NULL)
+      switch ((int) (l->l_info[VERSYMIDX (DT_VERSYM)] != NULL))
 	{
 	default:
 	  {