Re: Patch ping

[Jakub Jelinek <jakub at redhat dot com>]

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

>  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);

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

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

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

Attachment: U606m
Description: Text document