From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 85191 invoked by alias); 26 Jul 2017 14:13:39 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Received: (qmail 85154 invoked by uid 89); 26 Jul 2017 14:13:37 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.9 required=5.0 tests=BAYES_00,RP_MATCHES_RCVD,SPF_PASS autolearn=ham version=3.3.2 spammy=Hx-languages-length:4247 X-HELO: mx1.suse.de Received: from mx2.suse.de (HELO mx1.suse.de) (195.135.220.15) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 26 Jul 2017 14:13:35 +0000 Received: from relay1.suse.de (charybdis-ext.suse.de [195.135.220.254]) by mx1.suse.de (Postfix) with ESMTP id 42389ABC3; Wed, 26 Jul 2017 14:13:33 +0000 (UTC) Date: Wed, 26 Jul 2017 14:13:00 -0000 From: Richard Biener To: Jakub Jelinek cc: Jan Hubicka , gcc-patches@gcc.gnu.org Subject: Re: Patch ping In-Reply-To: <20170726134715.GW2123@tucnak> Message-ID: References: <20170725094050.GR2123@tucnak> <20170726134715.GW2123@tucnak> User-Agent: Alpine 2.20 (LSU 67 2015-01-07) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII X-SW-Source: 2017-07/txt/msg01668.txt.bz2 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 SUSE LINUX GmbH, GF: Felix Imendoerffer, Jane Smithard, Graham Norton, HRB 21284 (AG Nuernberg)