On Sun, Aug 24, 2014 at 05:47:23PM -0400, Trevor Saunders wrote:
> On Sun, Aug 24, 2014 at 01:58:52PM -0700, Andrew Pinski wrote:
> > On Sun, Aug 24, 2014 at 1:42 PM, Josh Triplett <j...@joshtriplett.org>
> > wrote:
> > > handle_section_attribute contains many levels of nested conditionals and
> > > branching code flow paths, with the error cases sometimes in the else
> > > case and sometimes in the if case. Simplify the code flow into a series
> > > of potential failure cases ending with the successful path, with no
> > > nesting and no else clauses.
> > > ---
> > >
> > > I started to work on an extension to the section attribute, and found
> > > myself cleaning up the logic in handle_section_attribute; I wanted to
> > > send this cleanup patch separately.
> > >
> > > I'm not a GCC committer, so I'm looking both for review and for someone
> > > to commit this patch.
> > >
> > > gcc/ChangeLog | 5 +++
> > > gcc/c-family/c-common.c | 91
> > > +++++++++++++++++++++++++------------------------
> > > 2 files changed, 51 insertions(+), 45 deletions(-)
> > >
> > > diff --git a/gcc/ChangeLog b/gcc/ChangeLog
> > > index 703a489..0286bfb 100644
> > > --- a/gcc/ChangeLog
> > > +++ b/gcc/ChangeLog
> > > @@ -1,3 +1,8 @@
> > > +2014-08-24 Josh Triplett <j...@joshtriplett.org>
> > > +
> > > + * c-family/c-common.c (handle_section_attribute): Refactor to
> > > reduce
> > > + nesting and distinguish between error cases.
> > > +
> > > 2014-08-24 Oleg Endo <olege...@gcc.gnu.org>
> > >
> > > PR target/61996
> > > diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c
> > > index 58b9763..a63eedf 100644
> > > --- a/gcc/c-family/c-common.c
> > > +++ b/gcc/c-family/c-common.c
> > > @@ -7387,58 +7387,59 @@ handle_section_attribute (tree *node, tree
> > > ARG_UNUSED (name), tree args,
> > > {
> > > tree decl = *node;
> > >
> > > - if (targetm_common.have_named_sections)
> > > + if (!targetm_common.have_named_sections)
> > > {
> > > - user_defined_section_attribute = true;
> > > + error_at (DECL_SOURCE_LOCATION (*node),
> > > + "section attributes are not supported for this target");
> > > + goto fail;
> >
> > Why not move this to a different function and then do:
> >
> > if (function(...))
> > set_decl_section_name (decl, TREE_STRING_POINTER (TREE_VALUE (args)));
> > else
> > *no_add_attrs = true;
> >
> > return NULL_TREE;
> >
> > This is a way to get away from using gotos.
>
> or you could just set no_add_attrs to true at the start of the function
> and only set it to false when you know the attr is valid, which saves
> the weirdness of a second function.
Though that would then duplicate the "return NULL_TREE" in many places,
which seems suboptimal (and not much different than "goto fail").
Andrew, any preference between those two approaches?
- Josh Triplett
> Trev
>
> >
> > Thanks,
> > Andrew
> >
> >
> > > + }
> > >
> > > - if ((TREE_CODE (decl) == FUNCTION_DECL
> > > - || TREE_CODE (decl) == VAR_DECL)
> > > - && TREE_CODE (TREE_VALUE (args)) == STRING_CST)
> > > - {
> > > - if (TREE_CODE (decl) == VAR_DECL
> > > - && current_function_decl != NULL_TREE
> > > - && !TREE_STATIC (decl))
> > > - {
> > > - error_at (DECL_SOURCE_LOCATION (decl),
> > > - "section attribute cannot be specified for "
> > > - "local variables");
> > > - *no_add_attrs = true;
> > > - }
> > > + user_defined_section_attribute = true;
> > >
> > > - /* The decl may have already been given a section attribute
> > > - from a previous declaration. Ensure they match. */
> > > - else if (DECL_SECTION_NAME (decl) != NULL
> > > - && strcmp (DECL_SECTION_NAME (decl),
> > > - TREE_STRING_POINTER (TREE_VALUE (args))) !=
> > > 0)
> > > - {
> > > - error ("section of %q+D conflicts with previous
> > > declaration",
> > > - *node);
> > > - *no_add_attrs = true;
> > > - }
> > > - else if (TREE_CODE (decl) == VAR_DECL
> > > - && !targetm.have_tls && targetm.emutls.tmpl_section
> > > - && DECL_THREAD_LOCAL_P (decl))
> > > - {
> > > - error ("section of %q+D cannot be overridden", *node);
> > > - *no_add_attrs = true;
> > > - }
> > > - else
> > > - set_decl_section_name (decl,
> > > - TREE_STRING_POINTER (TREE_VALUE
> > > (args)));
> > > - }
> > > - else
> > > - {
> > > - error ("section attribute not allowed for %q+D", *node);
> > > - *no_add_attrs = true;
> > > - }
> > > + if (TREE_CODE (decl) != FUNCTION_DECL && TREE_CODE (decl) != VAR_DECL)
> > > + {
> > > + error ("section attribute not allowed for %q+D", *node);
> > > + goto fail;
> > > }
> > > - else
> > > +
> > > + if (TREE_CODE (TREE_VALUE (args)) != STRING_CST)
> > > {
> > > - error_at (DECL_SOURCE_LOCATION (*node),
> > > - "section attributes are not supported for this target");
> > > - *no_add_attrs = true;
> > > + error ("section attribute argument not a string constant");
> > > + goto fail;
> > > + }
> > > +
> > > + if (TREE_CODE (decl) == VAR_DECL
> > > + && current_function_decl != NULL_TREE
> > > + && !TREE_STATIC (decl))
> > > + {
> > > + error_at (DECL_SOURCE_LOCATION (decl),
> > > + "section attribute cannot be specified for local
> > > variables");
> > > + goto fail;
> > > }
> > >
> > > + /* The decl may have already been given a section attribute
> > > + from a previous declaration. Ensure they match. */
> > > + if (DECL_SECTION_NAME (decl) != NULL
> > > + && strcmp (DECL_SECTION_NAME (decl),
> > > + TREE_STRING_POINTER (TREE_VALUE (args))) != 0)
> > > + {
> > > + error ("section of %q+D conflicts with previous declaration",
> > > *node);
> > > + goto fail;
> > > + }
> > > +
> > > + if (TREE_CODE (decl) == VAR_DECL
> > > + && !targetm.have_tls && targetm.emutls.tmpl_section
> > > + && DECL_THREAD_LOCAL_P (decl))
> > > + {
> > > + error ("section of %q+D cannot be overridden", *node);
> > > + goto fail;
> > > + }
> > > +
> > > + set_decl_section_name (decl, TREE_STRING_POINTER (TREE_VALUE (args)));
> > > + return NULL_TREE;
> > > +
> > > +fail:
> > > + *no_add_attrs = true;
> > > return NULL_TREE;
> > > }
> > >
> > > --
> > > 2.1.0
> > >
