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

[David Edelsohn]

On Tue, Nov 17, 2015 at 3:12 PM, David Malcolm  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 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=0x7000, line=991)
at /nasfarm/edelsohn/src/src/libcpp/line-map.c:991
991   linemap_assert (line >= LINEMAPS_MACRO_LOWEST_LOCATION (set));


Thanks, David

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

[David Malcolm]

On Sat, 2015-11-21 at 13:54 -0500, David Edelsohn wrote:
> On Tue, Nov 17, 2015 at 3:12 PM, David Malcolm  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 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=0x7000, 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)

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

[David Edelsohn]

On Sat, Nov 21, 2015 at 3:00 PM, David Malcolm  wrote:
> On Sat, 2015-11-21 at 13:54 -0500, David Edelsohn wrote:
>> On Tue, Nov 17, 2015 at 3:12 PM, David Malcolm  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 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=0x7000, 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

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

[David Malcolm]

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 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 (_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 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 
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 
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 

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

[Bernd Schmidt]

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 it's only the braced-init tweak that hasn't been).


Yes.


Bernd

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

[David Malcolm]

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

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

[Bernd Schmidt]

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



@@ -4278,9 +4278,11 @@ c_parser_braced_init (c_parser *parser, tree type, bool 
nested_p)
obstack_free (_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.



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.



Bernd