diff mbox

Fix uninitialized src_range within c_expr (Re: libcpp/C FE source range patch committed (r230331))

Message ID 1447773221.19594.84.camel@surprise
State New
Headers show

Commit Message

David Malcolm Nov. 17, 2015, 3:13 p.m. UTC 
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&regrtest 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&regrest; 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

Comments

Bernd Schmidt Nov. 17, 2015, 3:24 p.m. UTC | #1 
On 11/17/2015 04:13 PM, David Malcolm wrote:
> On Mon, 2015-11-16 at 22:34 +0100, Bernd Schmidt wrote:
>>
>> 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?).

The UNKNOWN_LOCATIONS pair would have been my approach, yes.

> 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.

Hmm, odd, I tried placing just the location_t start_loc line into the 
switch and that appeared to compile fine. But I guess this is not a huge 
problem.
>
> Is the combination of the 3 patches OK for trunk? (assuming
> bootstrap&regrest; it's only the braced-init tweak that hasn't been).

Yes.


Bernd
David Malcolm Nov. 17, 2015, 8:12 p.m. UTC | #2 
On Tue, 2015-11-17 at 16:24 +0100, Bernd Schmidt wrote:
> On 11/17/2015 04:13 PM, David Malcolm wrote:
> > On Mon, 2015-11-16 at 22:34 +0100, Bernd Schmidt wrote:
> >>
> >> 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?).
> 
> The UNKNOWN_LOCATIONS pair would have been my approach, yes.
> 
> > 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.
> 
> Hmm, odd, I tried placing just the location_t start_loc line into the 
> switch and that appeared to compile fine. But I guess this is not a huge 
> problem.
> >
> > Is the combination of the 3 patches OK for trunk? (assuming
> > bootstrap&regrest; it's only the braced-init tweak that hasn't been).
> 
> Yes.

Thanks.  I've committed the 3 patches to trunk as r230497, which should
fix the worst of the regressions caused by r230331 seen on AIX.  I'll
continue to investigate as per the discussion above.
David Edelsohn Nov. 21, 2015, 6:54 p.m. UTC | #3 
On Tue, Nov 17, 2015 at 3:12 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Tue, 2015-11-17 at 16:24 +0100, Bernd Schmidt wrote:
>> On 11/17/2015 04:13 PM, David Malcolm wrote:
>> > On Mon, 2015-11-16 at 22:34 +0100, Bernd Schmidt wrote:
>> >>
>> >> 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?).
>>
>> The UNKNOWN_LOCATIONS pair would have been my approach, yes.
>>
>> > 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.
>>
>> Hmm, odd, I tried placing just the location_t start_loc line into the
>> switch and that appeared to compile fine. But I guess this is not a huge
>> problem.
>> >
>> > Is the combination of the 3 patches OK for trunk? (assuming
>> > bootstrap&regrest; it's only the braced-init tweak that hasn't been).
>>
>> Yes.
>
> Thanks.  I've committed the 3 patches to trunk as r230497, which should
> fix the worst of the regressions caused by r230331 seen on AIX.  I'll
> continue to investigate as per the discussion above.

Hi, David

The new stret-1.m Objective C failure on AIX shows the same symptoms.
Is there another fix needed for Objective C?

#1  0x10016794 in _Z14linemap_lookupP9line_mapsj (set=0x70000000, line=991)
    at /nasfarm/edelsohn/src/src/libcpp/line-map.c:991
991       linemap_assert (line >= LINEMAPS_MACRO_LOWEST_LOCATION (set));


Thanks, David
David Malcolm Nov. 21, 2015, 8 p.m. UTC | #4 
On Sat, 2015-11-21 at 13:54 -0500, David Edelsohn wrote:
> On Tue, Nov 17, 2015 at 3:12 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> > On Tue, 2015-11-17 at 16:24 +0100, Bernd Schmidt wrote:
> >> On 11/17/2015 04:13 PM, David Malcolm wrote:
> >> > On Mon, 2015-11-16 at 22:34 +0100, Bernd Schmidt wrote:
> >> >>
> >> >> 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?).
> >>
> >> The UNKNOWN_LOCATIONS pair would have been my approach, yes.
> >>
> >> > 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.
> >>
> >> Hmm, odd, I tried placing just the location_t start_loc line into the
> >> switch and that appeared to compile fine. But I guess this is not a huge
> >> problem.
> >> >
> >> > Is the combination of the 3 patches OK for trunk? (assuming
> >> > bootstrap&regrest; it's only the braced-init tweak that hasn't been).
> >>
> >> Yes.
> >
> > Thanks.  I've committed the 3 patches to trunk as r230497, which should
> > fix the worst of the regressions caused by r230331 seen on AIX.  I'll
> > continue to investigate as per the discussion above.
> 
> Hi, David
> 
> The new stret-1.m Objective C failure on AIX shows the same symptoms.
> Is there another fix needed for Objective C?
> 
> #1  0x10016794 in _Z14linemap_lookupP9line_mapsj (set=0x70000000, line=991)
>     at /nasfarm/edelsohn/src/src/libcpp/line-map.c:991
> 991       linemap_assert (line >= LINEMAPS_MACRO_LOWEST_LOCATION (set));

I believe this one is fixed by the patch I posted here:
 "[PATCH] Fix PR objc/68438 (uninitialized source ranges)"
   https://gcc.gnu.org/ml/gcc-patches/2015-11/msg02536.html

(it runs cleanly under valgrind on x86_64 with that patch applied)
David Edelsohn Nov. 21, 2015, 9:50 p.m. UTC | #5 
On Sat, Nov 21, 2015 at 3:00 PM, David Malcolm <dmalcolm@redhat.com> wrote:
> On Sat, 2015-11-21 at 13:54 -0500, David Edelsohn wrote:
>> On Tue, Nov 17, 2015 at 3:12 PM, David Malcolm <dmalcolm@redhat.com> wrote:
>> > On Tue, 2015-11-17 at 16:24 +0100, Bernd Schmidt wrote:
>> >> On 11/17/2015 04:13 PM, David Malcolm wrote:
>> >> > On Mon, 2015-11-16 at 22:34 +0100, Bernd Schmidt wrote:
>> >> >>
>> >> >> 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?).
>> >>
>> >> The UNKNOWN_LOCATIONS pair would have been my approach, yes.
>> >>
>> >> > 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.
>> >>
>> >> Hmm, odd, I tried placing just the location_t start_loc line into the
>> >> switch and that appeared to compile fine. But I guess this is not a huge
>> >> problem.
>> >> >
>> >> > Is the combination of the 3 patches OK for trunk? (assuming
>> >> > bootstrap&regrest; it's only the braced-init tweak that hasn't been).
>> >>
>> >> Yes.
>> >
>> > Thanks.  I've committed the 3 patches to trunk as r230497, which should
>> > fix the worst of the regressions caused by r230331 seen on AIX.  I'll
>> > continue to investigate as per the discussion above.
>>
>> Hi, David
>>
>> The new stret-1.m Objective C failure on AIX shows the same symptoms.
>> Is there another fix needed for Objective C?
>>
>> #1  0x10016794 in _Z14linemap_lookupP9line_mapsj (set=0x70000000, line=991)
>>     at /nasfarm/edelsohn/src/src/libcpp/line-map.c:991
>> 991       linemap_assert (line >= LINEMAPS_MACRO_LOWEST_LOCATION (set));
>
> I believe this one is fixed by the patch I posted here:
>  "[PATCH] Fix PR objc/68438 (uninitialized source ranges)"
>    https://gcc.gnu.org/ml/gcc-patches/2015-11/msg02536.html
>
> (it runs cleanly under valgrind on x86_64 with that patch applied)

Yep, thanks.  I missed the follow-on patch.

Thanks, David
diff mbox

Patch

From 205da878acc752adb275da3ca61a342a9a124f93 Mon Sep 17 00:00:00 2001
From: David Malcolm <dmalcolm@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