Patchwork rs6000_handle_option global state avoidance, part 1

login
register
mail settings
Submitter Joseph S. Myers
Date May 5, 2011, 7:34 p.m.
Message ID <Pine.LNX.4.64.1105051932362.20285@digraph.polyomino.org.uk>
Download mbox | patch
Permalink /patch/94302/
State New
Headers show

Comments

Joseph S. Myers - May 5, 2011, 7:34 p.m.
On Thu, 5 May 2011, Michael Eager wrote:

> David Edelsohn wrote:
> > On Wed, May 4, 2011 at 7:54 AM, Joseph S. Myers <joseph@codesourcery.com>
> > wrote:
> > 
> > > Two options, -mcmodel= and -mfpu=, had cases that fell through to the
> > > next case without comments to indicate if this was intended.  I added
> > > comments to make the semantics explicit.  Given the documentation, it
> > > may well be intentional for -mcmodel= but is more doubtful for -mfpu=.
> > 
> > I doubt that either of the fall through cases was intended.
> > 
> > Alan, is mcmodel suppose to set m64?
> > 
> > Michael, is mfpu suppose to set mrecip?
> 
> No.  There was a break statement at the end of case OPT_mfpu which
> disappeared when OPT_mrecip was added.

Thanks.  I'll apply this patch which removes the fall through, and adds 
explicit Var and Init to the mfpu= entry in rs6000.opt to avoid problems 
(when building as C++, as shown by a regression tester) with 
0-initialization of the field that gets automatically generated by the 
.opt machinery for any Target option not using Var.

2011-05-05  Joseph Myers  <joseph@codesourcery.com>

	* config/rs6000/rs6000.c (rs6000_handle_option): Don't fall
	through from -mfpu= handling.
	* config/rs6000/rs6000.opt (mfpu=): Use Var and Init.
Michael Eager - May 5, 2011, 8:56 p.m.
Joseph S. Myers wrote:
> On Thu, 5 May 2011, Michael Eager wrote:
> 
>> David Edelsohn wrote:
>>> On Wed, May 4, 2011 at 7:54 AM, Joseph S. Myers <joseph@codesourcery.com>
>>> wrote:
>>>
>>>> Two options, -mcmodel= and -mfpu=, had cases that fell through to the
>>>> next case without comments to indicate if this was intended.  I added
>>>> comments to make the semantics explicit.  Given the documentation, it
>>>> may well be intentional for -mcmodel= but is more doubtful for -mfpu=.
>>> I doubt that either of the fall through cases was intended.
>>>
>>> Alan, is mcmodel suppose to set m64?
>>>
>>> Michael, is mfpu suppose to set mrecip?
>> No.  There was a break statement at the end of case OPT_mfpu which
>> disappeared when OPT_mrecip was added.
> 
> Thanks.  I'll apply this patch which removes the fall through, and adds 
> explicit Var and Init to the mfpu= entry in rs6000.opt to avoid problems 
> (when building as C++, as shown by a regression tester) with 
> 0-initialization of the field that gets automatically generated by the 
> .opt machinery for any Target option not using Var.

Looks good.
David Edelsohn - May 5, 2011, 11:29 p.m.
On Thu, May 5, 2011 at 3:34 PM, Joseph S. Myers <joseph@codesourcery.com> wrote:
> On Thu, 5 May 2011, Michael Eager wrote:
>
>> David Edelsohn wrote:
>> > On Wed, May 4, 2011 at 7:54 AM, Joseph S. Myers <joseph@codesourcery.com>
>> > wrote:
>> >
>> > > Two options, -mcmodel= and -mfpu=, had cases that fell through to the
>> > > next case without comments to indicate if this was intended.  I added
>> > > comments to make the semantics explicit.  Given the documentation, it
>> > > may well be intentional for -mcmodel= but is more doubtful for -mfpu=.
>> >
>> > I doubt that either of the fall through cases was intended.
>> >
>> > Alan, is mcmodel suppose to set m64?
>> >
>> > Michael, is mfpu suppose to set mrecip?
>>
>> No.  There was a break statement at the end of case OPT_mfpu which
>> disappeared when OPT_mrecip was added.
>
> Thanks.  I'll apply this patch which removes the fall through, and adds
> explicit Var and Init to the mfpu= entry in rs6000.opt to avoid problems
> (when building as C++, as shown by a regression tester) with
> 0-initialization of the field that gets automatically generated by the
> .opt machinery for any Target option not using Var.
>
> 2011-05-05  Joseph Myers  <joseph@codesourcery.com>
>
>        * config/rs6000/rs6000.c (rs6000_handle_option): Don't fall
>        through from -mfpu= handling.
>        * config/rs6000/rs6000.opt (mfpu=): Use Var and Init.

Okay.  The entire patch looks good with the two fall through cases corrected.

Thanks, David

Patch

Index: gcc/config/rs6000/rs6000.opt
===================================================================
--- gcc/config/rs6000/rs6000.opt	(revision 173434)
+++ gcc/config/rs6000/rs6000.opt	(working copy)
@@ -492,7 +492,7 @@  Target RejectNegative Var(rs6000_simple_
 Floating point unit does not support divide & sqrt
 
 mfpu=
-Target RejectNegative Joined Enum(fpu_type_t)
+Target RejectNegative Joined Enum(fpu_type_t) Var(rs6000_fpu_type) Init(FPU_NONE)
 -mfpu=	Specify FP (sp, dp, sp-lite, dp-lite) (implies -mxilinx-fpu)
 
 Enum
Index: gcc/config/rs6000/rs6000.c
===================================================================
--- gcc/config/rs6000/rs6000.c	(revision 173434)
+++ gcc/config/rs6000/rs6000.c	(working copy)
@@ -4480,7 +4480,7 @@  rs6000_handle_option (struct gcc_options
 	  opts_set->x_target_flags |= MASK_SOFT_FLOAT;
 	  opts->x_rs6000_single_float = opts->x_rs6000_double_float = 0;
 	}
-      /* Fall through.  */
+      break;
 
     case OPT_mrecip:
       opts->x_rs6000_recip_name = (value) ? "default" : "none";