Re: [PATCH] Fix arrays in rtx.u + add minor rtx verification

[Marek Polacek <polacek at redhat dot com>]

On Fri, Jun 20, 2014 at 11:10:18PM +0200, Jakub Jelinek wrote:
> On Fri, Jun 20, 2014 at 01:55:41PM -0600, Jeff Law wrote:
> > >like spot.  Most RTLs are allocated through rtx_alloc and the size
> > >is determined from RTX_HDR_SIZE (i.e. offsetof) and/or RTX_CODE_SIZE,
> > >so your rtl.h change IMHO shouldn't affect anything but make the
> > >expmed.c init_expmed_rtl structure somewhat longer.
> > Right.  This comment was actually very helpful in that I wasn't aware of
> > precisely which cases Marek was trying to address.
> > 
> > Presumably the [1] sizing is what prevents any compile-time checking of
> > this?
> 
> First version of Marek's patch did that (never instrumented [], [0] and
> [1] arrays, no matter where they appeared, and instrumented everything
> else).
> Latest patch only never instruments [] (which, by definition can only
> appear at the end of structure), other arrays (no matter what size)
> aren't instrumented if they aren't followed by any fields, or
> if the base of the handled components is not INDIRECT_REF/MEM_REF
> (so, typically is a decl).
> u.fld[1] array is the last field, so we don't warn for that, but when
> rtx_def appears in another structure (in expmed.c) or if e.g. even
> some code had a rtx_def typed variable and accessed say u.fld[1] in there,
> it would be instrumented.

Yeah - init_expmed in expmed.c has
XEXP (&all.plus, 1) = &all.reg;
which is expanded to
(((&all.plus)->u.fld[1]).rt_rtx) = &all.reg;
but since the expression (&A)->B is the same as A.B (if &A is a valid
pointer expression), the above was turned into
all.plus.u.fld[1].rt_rtx) = &all.reg;
and that doesn't contain any INDIRECT_REF/MEM_REFs -> it's being
instrumented.  With this patch I don't see any -fsanitize=bounds
errors when doing bootstrap-ubsan.
 
> Whether we should have a strict array bounds mode where we would instrument
> even arrays at the end of structures (with the exception of []) is something
> to be discussed.

This should be basically just about adding a new option - e.g.
-fsanitize=bounds-strict.

	Marek