[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&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®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

-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0002-Bulletproof-set_c_expr_source_range-against-NULL-exp.patch
Type: text/x-patch
Size: 1096 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20151117/2f198693/attachment.bin>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0003-Lookup-next-token-once-at-end-of-c_parser_braced_ini.patch
Type: text/x-patch
Size: 1230 bytes
Desc: not available
URL: <http://gcc.gnu.org/pipermail/gcc-patches/attachments/20151117/2f198693/attachment-0001.bin>