On Mon, 2015-11-16 at 22:34 +0100, Bernd Schmidt wrote:
> On 11/16/2015 09:50 PM, David Malcolm wrote:
> > The root cause is uninitialized data. Specifically, the C parser's
> > struct c_expr gained a "src_range" field, and it turns out there are a
> > few places where I wasn't initializing this when returning c_expr
> > instances on the stack, and in some cases the values could get used.
>
> > I'm working on a followup to fix the remaining places I identified via
> > review of the source.
>
> The patch is mostly OK IMO and should be installed to fix the problems,
I'm attaching two followup patches.
The patch as is introduces some ICEs due to accessing EXPR_LOCATION ()
of a c_expr's "value" field, for some cases where value is NULL. The
first attached patch bulletproofs both implementations of
set_c_expr_source_range for this case.
I've successfully bootstrapped and regression-tested the combination of
this plus the previous patch on x86_64-pc-linux-gnu; I've also got a
bootstrap®rtest ongoing on powerpc-ibm-aix7.1.3.0.
> but I think there are a few more things to consider.
>
> Should c_expr perhaps acquire a constructor so that this problem is
> avoided in the future? The whole thing seems somewhat error-prone.
I agree that it's error prone, and the ctor approach is what I've been
trying for the C++ FE [1] but I suspect that touching that in the C FE
would be a much more invasive patch (unless we simply give it a default
ctor that makes the src_range be a pair of UNKNOWN_LOCATIONS?). I'll
give it a go, but it feels like a separate followup.
> > @@ -4278,9 +4278,11 @@ c_parser_braced_init (c_parser *parser, tree type,
> > bool nested_p)
> > obstack_free (&braced_init_obstack, NULL);
> > return ret;
> > }
> > + location_t close_loc = c_parser_peek_token (parser)->location;
>
> It looks like we're peeking the token twice here (via a
> c_parser_token_is_not call above the quoted code). Probably not too
> expensive but maybe we can avoid it.
Thanks; I'm also attaching a patch that does so (not yet bootstrapped,
but will do so).
> > case RID_VA_ARG:
> > - c_parser_consume_token (parser);
> > + {
> > + location_t start_loc = loc;
>
> Does this really have to be indented in an extra braced block? Please
> fix if not.
This case gains a pair of locals: start_loc and end_loc (so that we can
track the spelling range whilst retaining the "loc" used for the caret),
and I preferred to confine their scope to within the case, hence the
extra braced block. Omitting the braced block leads to:
../../src/gcc/c/c-parser.c:7494:7: error: jump to case label [-fpermissive]
case RID_OFFSETOF:
^
../../src/gcc/c/c-parser.c:7472:17: error: crosses initialization of
‘location_t end_loc’
location_t end_loc = c_parser_peek_token (parser)->get_finish ();
^
etc. I could fix that by moving the locals to the top of the function,
but that seems messy, so it seemed best to add the braces (and hence
indent).
Hope that sounds like the right trade-off.
Is the combination of the 3 patches OK for trunk? (assuming
bootstrap®rest; it's only the braced-init tweak that hasn't been).
Thanks
Dave
[1] in "[PATCH/RFC] C++ FE: expression ranges (v2)":
https://gcc.gnu.org/ml/gcc-patches/2015-11/msg01859.html
>From a19859a1ecdf850684b8d191b0ff57c8e50cc121 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalc...@redhat.com>
Date: Tue, 17 Nov 2015 06:09:52 -0500
Subject: [PATCH 2/3] Bulletproof set_c_expr_source_range against NULL
expr->value
gcc/c/ChangeLog:
* c-parser.c (set_c_expr_source_range): Bulletproof both
overloaded implementations against NULL expr->value.
---
gcc/c/c-parser.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index bcad80c..9ab7ceb 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -65,7 +65,8 @@ set_c_expr_source_range (c_expr *expr,
{
expr->src_range.m_start = start;
expr->src_range.m_finish = finish;
- set_source_range (expr->value, start, finish);
+ if (expr->value)
+ set_source_range (expr->value, start, finish);
}
void
@@ -73,7 +74,8 @@ set_c_expr_source_range (c_expr *expr,
source_range src_range)
{
expr->src_range = src_range;
- set_source_range (expr->value, src_range);
+ if (expr->value)
+ set_source_range (expr->value, src_range);
}
--
1.8.5.3
>From 205da878acc752adb275da3ca61a342a9a124f93 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalc...@redhat.com>
Date: Tue, 17 Nov 2015 10:18:39 -0500
Subject: [PATCH 3/3] Lookup next token once at end of c_parser_braced_init,
not twice
---
gcc/c/c-parser.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
index 9ab7ceb..eedcaa4 100644
--- a/gcc/c/c-parser.c
+++ b/gcc/c/c-parser.c
@@ -4270,7 +4270,8 @@ c_parser_braced_init (c_parser *parser, tree type, bool nested_p)
break;
}
}
- if (c_parser_next_token_is_not (parser, CPP_CLOSE_BRACE))
+ c_token *next_tok = c_parser_peek_token (parser);
+ if (next_tok->type != CPP_CLOSE_BRACE)
{
ret.value = error_mark_node;
ret.original_code = ERROR_MARK;
@@ -4280,7 +4281,7 @@ c_parser_braced_init (c_parser *parser, tree type, bool nested_p)
obstack_free (&braced_init_obstack, NULL);
return ret;
}
- location_t close_loc = c_parser_peek_token (parser)->location;
+ location_t close_loc = next_tok->location;
c_parser_consume_token (parser);
ret = pop_init_level (brace_loc, 0, &braced_init_obstack);
obstack_free (&braced_init_obstack, NULL);
--
1.8.5.3
