On Fri, 26 Aug 2016, Prasad Ghangal wrote:
> >> Thanks for your feedback. I had missed removing some unwanted code
> >> while code cleanup. I have updated the patch.
> >> I am not sure if we should move all gimple parsing related functions
> >> to the new file (?)
> >
> > I think it might be good to make the parts of the C parser you use more
> > obvious (you'd need to export functions like c_parser_next_token_is).
> >
> > The easiest way to "force" that is to put all of the gimple parsing into
> > a separate file.
> >
> > Note I am not so much concerned about this at the moment, the parts to
> > improve would be avoiding more of the C-isms like convert_lvalue_to_rvalue,
> > handling of SIZEOF_EXPR and other stuff that looks redundant (you've
> > probably copied this from the C parsing routines and refactored it).
> > Also the GIMPLE parser shouldn't do any warnings (just spotted
> > a call to warn_for_memset).
> >
> PFA updated patch (successfully bootstrapped and tested on
> x86_64-pc-linux-gnu). I have removed unnecessary code. On side I am
> also trying to move gimple parser related functions to new file. But
> for it we also have to move structs like c_token, c_parser. Won't it
> disturb the c-parser code structure ?
I think the GIMPLE parsing should go in a separate file (meaning exporting
relevant types and function declarations in a new c-parser.h).
> diff --git a/gcc/c-family/c.opt b/gcc/c-family/c.opt
> index a5358ed..3c4d2cc 100644
> --- a/gcc/c-family/c.opt
> +++ b/gcc/c-family/c.opt
> @@ -200,6 +200,10 @@ F
> Driver C ObjC C++ ObjC++ Joined Separate MissingArgError(missing path after
> %qs)
> -F <dir> Add <dir> to the end of the main framework include path.
>
> +fgimple
> +C Var(flag_gimple) Init(0)
> +Enable parsing GIMPLE
You should get a test failure here from the missing "." at the end of the
help text.
Of course the option also needs documenting in invoke.texi.
> @@ -1659,6 +1695,9 @@ c_parser_declaration_or_fndef (c_parser *parser, bool
> fndef_ok,
> tree all_prefix_attrs;
> bool diagnosed_no_specs = false;
> location_t here = c_parser_peek_token (parser)->location;
> + bool gimple_body_p = false;
> + opt_pass *pass = NULL;
> + bool startwith_p = false;
The comment above the function needs updating to document the new syntax.
> +static void
> +c_parser_gimple_expression (c_parser *parser, gimple_seq *seq)
> +{
> + struct c_expr lhs, rhs;
> + gimple *assign = NULL;
> + enum tree_code subcode = NOP_EXPR;
> + location_t loc;
> + tree arg = NULL_TREE;
> + auto_vec<tree> vargs;
> +
> + lhs = c_parser_gimple_unary_expression (parser);
> + rhs.value = error_mark_node;
> +
> + if (c_parser_next_token_is (parser, CPP_EQ))
> + {
> + c_parser_consume_token (parser);
> + }
Redundant braces around a single statement. Also, this looks wrong, in
that it seems like you'd accept a random '=' token at this point
regardless of what follows and whether '=' makes sense in this context.
You need to have proper cases: if '=' parse what makes sense after '=',
otherwise parse what makes sense without '=', so that invalid syntax is
not accepted.
> + if (c_parser_next_token_is (parser, CPP_AND) ||
> + c_parser_next_token_is (parser, CPP_MULT) ||
> + c_parser_next_token_is (parser, CPP_PLUS) ||
> + c_parser_next_token_is (parser, CPP_MINUS) ||
> + c_parser_next_token_is (parser, CPP_COMPL) ||
> + c_parser_next_token_is (parser, CPP_NOT))
Operators go at the start of a continuation line, not at the end of the
line.
> + if (!c_parser_require (parser, CPP_OPEN_PAREN, "expected %<(%>"))
> + {
> + return;
> + }
Please generally review the patch for redundant braces and remove them.
> + /* ssa token string. */
> + const char *ssa_token = IDENTIFIER_POINTER (c_parser_peek_token
> (parser)->value);
> + token = new char [strlen (ssa_token)];
> + strcpy (token, ssa_token);
That looks like a buffer overrun. To copy a string ssa_token, you need
strlen (ssa_token) + 1 bytes of space.
> + /* seperate var name and version. */
Uppercase letters at start of comments, throughout the patch (and it
should be "Separate", with 'a' not 'e').
--
Joseph S. Myers
[email protected]
