On Wed, 26 Jul 2017, Jakub Jelinek wrote:
> On Wed, Jul 26, 2017 at 12:34:10PM +0200, Richard Biener wrote:
> > On Tue, 25 Jul 2017, Jakub Jelinek wrote:
> >
> > > Hi!
> > >
> > > I'd like to ping 2 patches:
> > >
> > > - UBSAN -fsanitize=pointer-overflow support
> > > - http://gcc.gnu.org/ml/gcc-patches/2017-06/msg01365.html
> >
> > The probablility stuff might need updating?
>
> Yes, done in my copy.
>
> > Can you put the TYPE_PRECISION (sizetype) != POINTER_SIZE check
> > in option processing and inform people that pointer overflow sanitizing
> > is not done instead?
>
> That is problematic, because during the option processing sizetype
> is NULL, it is set up only much later. And that processing depends on
Ah, ok - fine then as-is.
> the options being finalized and backend initialized etc.
> I guess I could emit a warning in the sanopt pass once or something.
> Or do it very late in do_compile, after lang_dependent_init (we don't
> handle any other option there though).
> But it is still just a theoretical case, because libubsan is supported
> only on selected subset of targets and none of those have such weird
> sizetype vs. pointer size setup. And without libubsan, the only thing
> one could do would be -fsanitize=undefined -fsanitize-undefined-trap-on-error
> or -fsanitize=pointer-overflow -fsanitize-undefined-trap-on-error,
> otherwise one would run into missing libubsan.
>
> > Where you handle DECL_BIT_FIELD_REPRESENTATIVE in
> > maybe_instrument_pointer_overflow you could instead of building
> > a new COMPONENT_REF strip the bitfield ref and just remember
> > DECL_FIELD_OFFSET/BIT_OFFSET to be added to the get_inner_reference
> > result?
>
> That is not enough, the bitfield could be in variable length structure etc.
> Furthermore, I need that COMPONENT_REF with the representative later
> in the function, so that I can compute the ptr and base_addr.
Ok.
> > You don't seem to use 'size' anywhere.
>
> size I thought about but then decided not to do anything with it.
> There are two cases, one is where there is no ADDR_EXPR and it actually
> a memory reference.
> In that case in theory the size could be used, but it would need
> to be used only for the positive offsets, so like:
> if (off > 0) {
> if (ptr + off + size < ptr)
> runtime_fail;
> } else if (ptr + off > ptr)
> runtime_fail;
> but when it is actually a memory reference, I suppose it will fail
> at runtime anyway when performing such an access, so I think it is
> unnecessary. And for the ADDR_EXPR case, the size is irrelevant, we
> are just taking address of the start of the object.
>
> > You fail to allow other handled components -- for no good reason?
>
> I was trying to have a quick bail out. What other handled components might
> be relevant? I guess IMAGPART_EXPR. For say BIT_FIELD_REF I don't think
> I can
> tree ptr = build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (t)), t);
REALPART/IMAGPART_EXPR, yes. You can't address BIT_FIELD_REF
apart those on byte boundary (&vector[4] is eventually folded to
a BIT_FIELD_REF). Similar for VIEW_CONVERT_EXPR, but you are
only building the address on the base?
> > You fail to handle
> > &MEM[ptr + CST] a canonical gimple invariant way of ptr +p CST,
> > the early out bitpos == 0 will cause non-instrumentation here.
>
> Guess I could use:
> if ((offset == NULL_TREE
> && bitpos == 0
> && (TREE_CODE (inner) != MEM_REF
> || integer_zerop (TREE_OPERAND (inner, 1))))
> The rest of the code will handle it.
Yeah.
>
> > (I'd just round down in the case of bitpos % BITS_PER_UNIT != 0)
>
> But then the
> tree ptr = build1 (ADDR_EXPR, build_pointer_type (TREE_TYPE (t)), t);
> won't work again.
Hmm. So instead of building the address on the original tree you
could build the difference based on what get_inner_reference returns
in bitpos/offset?
> > > - noipa attribute addition
> > >
> > > http://gcc.gnu.org/ml/gcc-patches/2016-12/msg01501.html
> > >
> >
> > Ok.
>
> Thanks, will retest it now.
>
> Here is the -fsanitize=pointer-overflow patch untested, updated
> for the probability and other stuff mentioned above.
>
> Jakub
>
--
Richard Biener <rguent...@suse.de>
SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB
21284 (AG Nuernberg)
