From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 25845 invoked by alias); 11 Sep 2013 13:43:57 -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 25836 invoked by uid 89); 11 Sep 2013 13:43:57 -0000 Received: from mail-we0-f180.google.com (HELO mail-we0-f180.google.com) (74.125.82.180) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES128-SHA encrypted) ESMTPS; Wed, 11 Sep 2013 13:43:57 +0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.0 required=5.0 tests=AWL,BAYES_50,FREEMAIL_FROM,KHOP_THREADED,NO_RELAYS autolearn=ham version=3.3.2 X-HELO: mail-we0-f180.google.com Received: by mail-we0-f180.google.com with SMTP id u57so6901638wes.39 for ; Wed, 11 Sep 2013 06:43:53 -0700 (PDT) MIME-Version: 1.0 X-Received: by 10.194.11.67 with SMTP id o3mr1723192wjb.0.1378907033613; Wed, 11 Sep 2013 06:43:53 -0700 (PDT) Received: by 10.194.200.74 with HTTP; Wed, 11 Sep 2013 06:43:53 -0700 (PDT) In-Reply-To: References: <20130910193228.GE6732@virgil.suse> Date: Wed, 11 Sep 2013 14:47:00 -0000 Message-ID: Subject: Re: [PATCH, PR 57748] Check for out of bounds access From: Richard Biener To: Bernd Edlinger Cc: Martin Jambor , GCC Patches Content-Type: text/plain; charset=ISO-8859-1 X-IsSubscribed: yes X-SW-Source: 2013-09/txt/msg00845.txt.bz2 On Wed, Sep 11, 2013 at 3:41 PM, Bernd Edlinger wrote: > On Tue, 10 Sep 2013 21:32:29, Martin Jambor wrote: >> Hi, >> >> On Fri, Sep 06, 2013 at 11:19:18AM +0200, Richard Biener wrote: >>> On Fri, Sep 6, 2013 at 10:35 AM, Bernd Edlinger >>> wrote: >>>> Richard, >>>> >>>>> But the movmisalign path skips all this code and with the >>>>> current code thinks the actual access is in the mode of the >>>>> whole structure. (and also misses the address adjustment >>>>> as shown in the other testcase for the bug) >>>>> >>>>> The movmisalign handling in this path is just broken. And >>>>> it's _not_ just for optimization! If you have >>>>> >>>>> struct __attribute__((packed)) { >>>>> double x; >>>>> v2df y; >>>>> } *p; >>>>> >>>>> and do >>>>> >>>>> p = malloc (48); // gets you 8-byte aligned memory >>>>> p->y = (v2df) { 1., 2. }; >>>>> >>>>> then you cannot skip the movmisaling handling because >>>>> we'd generate an aligned move (based on the mode) then. >>>>> >>>> >>>> Ok, test examples are really helpful here. >>>> >>>> This time the structure is BLKmode, unaligned, >>>> movmisalign = false anyway. >>>> >>>> I tried to make a test case out of your example, >>>> and as I expected, the code is also correct: >>>> >>>> foo: >>>> .cfi_startproc >>>> movdqa .LC1(%rip), %xmm0 >>>> movq $-1, (%rdi) >>>> movl $0x4048f5c3, 8(%rdi) >>>> movdqu %xmm0, 12(%rdi) >>>> ret >>>> >>>> movdqu. >>>> >>>> The test executes without trap. >>>> And I did everything to make the object unaligned. >>> >>> Yeah, well - for the expand path with BLKmode it's working fine. >>> We're doing all >>> the dances because of correctness for non-BLKmode expand paths. >>> >>>> I am sure we could completely remove the >>>> movmisalign path, and nothing would happen. >>>> probably. except maybe for a performance regression. >>> >>> In this place probably yes. But we can also fix it to use the proper mode and >>> properly do the offset adjustment. But just adding a bounds check looks >>> broken to me. >>> >>> Note that there may have been a correctness reason for this code in the >>> light of the IPA-SRA code. Maybe Martin remembers. >>> >> >> The misalignp path was added by you during the 4.7 development to fix >> PR 50444 which was indeed about expansion of a SRA generated statement >> >> MEM[(struct Engine *)e_1(D) + 40B].m = SR.18_17; >> >> If I disable this path on the 4.7 branch, the testcase is compiled >> incorrectly and aborts when run, apparently at least the 4.7's >> combination of expand_normal and store_field cannot cope with it. >> >> The path no longer tests the testcase though, because the component >> ref is not present in trunk, the LHS is now just >> >> MEM[(struct Engine *)e_3(D) + 40B] >> >> and so it is now handled just fine by the misaligned mem-ref case at >> the beginning of expand_assignment. > > I tried to remove the misaligned mem-ref case at the beginning of the > expand_assignment, just to see how it fails. I did this on trunk. > > But still all testcases pass, including pr50444.c... > > How can this be? Not sure. Can you try reverting the fix itself and bisect when the testcase will start to pass? (maybe the testcase passed even without the fix?) Richard. >>> If removing the movmisalign handling inside the handled-component >>> case in expand_assignment works out (make sure to also test a >>> strict-alignment target) then that is probably fine. >>> >> >> I think we'd also better check that we do have a test where we expand >> a COMPONENT_REF encapsulating a misaligned MEM_REF and a misaligned >> MEM_REF that is mem_ref_refers_to_non_mem_p. >> >> I'm now going through all the new comments in bugzilla and the >> testcases to see if I can still be of any help. >> >> Martin