| Message ID | 20150427010939.GA7634@x |
|---|---|
| State | New |
| Headers | show |
On 04/26/2015 07:09 PM, Josh Triplett 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 do not have commit access; if you approve, please commit. > > This cleanup will help enable a larger patch enhancing the section attribute. > > gcc/Changelog entry: > > 2014-08-24 Josh Triplett <josh@joshtriplett.org> > > * c-family/c-common.c (handle_section_attribute): Refactor to reduce > nesting and distinguish between error cases. Have you done a bootstrap and regression test with this patch? While I don't expect problems, it's part of the standard procedure for patch submissions. There may be tests which look for "section_attribute note allowed for ..." in the !STRING_CST case that will have a different error message after your change. If so, they will need to be adjusted. I'll approve and commit once you've verified the bootstrap & regression test was OK. jeff
On Tue, Apr 28, 2015 at 06:10:33PM -0600, Jeff Law wrote: > On 04/26/2015 07:09 PM, Josh Triplett 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 do not have commit access; if you approve, please commit. > > > >This cleanup will help enable a larger patch enhancing the section attribute. > > > >gcc/Changelog entry: > > > >2014-08-24 Josh Triplett <josh@joshtriplett.org> > > > > * c-family/c-common.c (handle_section_attribute): Refactor to reduce > > nesting and distinguish between error cases. > Have you done a bootstrap and regression test with this patch? While I > don't expect problems, it's part of the standard procedure for patch > submissions. > > There may be tests which look for "section_attribute note allowed for ..." > in the !STRING_CST case that will have a different error message after your > change. If so, they will need to be adjusted. > > I'll approve and commit once you've verified the bootstrap & regression test > was OK. Yes, I've done a full bootstrap and regression test without issue. There don't appear to be any tests that rely on the text of any of those error messages. - Josh Triplett
On 04/26/2015 07:09 PM, Josh Triplett 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 do not have commit access; if you approve, please commit. > > This cleanup will help enable a larger patch enhancing the section attribute. > > gcc/Changelog entry: > > 2014-08-24 Josh Triplett <josh@joshtriplett.org> > > * c-family/c-common.c (handle_section_attribute): Refactor to reduce > nesting and distinguish between error cases. Installed on the trunk. WRT the larger patch, if you don't have a copyright assignment on file with the FSF, you should get that process started. assign@gnu.org. jeff
diff --git a/gcc/c-family/c-common.c b/gcc/c-family/c-common.c index 9797e17..588953d 100644 --- a/gcc/c-family/c-common.c +++ b/gcc/c-family/c-common.c @@ -7598,58 +7598,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; + } - 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; }
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 do not have commit access; if you approve, please commit. This cleanup will help enable a larger patch enhancing the section attribute. gcc/Changelog entry: 2014-08-24 Josh Triplett <josh@joshtriplett.org> * c-family/c-common.c (handle_section_attribute): Refactor to reduce nesting and distinguish between error cases. gcc/c-family/c-common.c | 91 +++++++++++++++++++++++++------------------------ 1 file changed, 46 insertions(+), 45 deletions(-)