Message ID | 1447773221.19594.84.camel@surprise |
---|---|
State | New |
Headers | show |
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®rest; it's only the braced-init tweak that hasn't been). Yes. Bernd
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®rest; 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.
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®rest; 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
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®rest; 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)
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®rest; 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
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