From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (qmail 108285 invoked by alias); 2 May 2018 07:22:34 -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 108274 invoked by uid 89); 2 May 2018 07:22:34 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-2.4 required=5.0 tests=AWL,BAYES_00,FREEMAIL_FROM,RCVD_IN_DNSWL_NONE,SPF_PASS autolearn=ham version=3.3.2 spammy=What's, sth, latent X-HELO: mail-lf0-f47.google.com Received: from mail-lf0-f47.google.com (HELO mail-lf0-f47.google.com) (209.85.215.47) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with ESMTP; Wed, 02 May 2018 07:22:32 +0000 Received: by mail-lf0-f47.google.com with SMTP id h4-v6so19387211lfc.1 for ; Wed, 02 May 2018 00:22:31 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:in-reply-to:references:from:date :message-id:subject:to:cc; bh=eRta1DPPnJek0O+lh04N4/GkJpTv1atVRxXuz+UB7UQ=; b=W1Cococ4AuZ50XT+T/swAkY0iIBETTjsEUgJ21NbFwQNYtNSWUwtLmRUJY9pxAEkbC 9sVwn87dgswsYIZIPfazdXbYFJiPT2CIEYFcdpIIjZIzaY3EC88nR84xupRx3i5yeb8z u1BaHzThwgyHtSb9CXRIaO48irfcPrSj0cCO4rg/8k12UKU+oLt/ZLPzuhQGYIK/j/d5 uFLShgw0ih7Xh7vnsH7UZ9qmuJbS37C2ChSI0s2s0s3Myidtgr0eT2o2E6I0OMkDdBYw Ahq2siX25rvyYUqVnE5Hgz+5GuAWumuzPdvlS9NP1IVGNMh7MaExmR0kgSol26Gjj/dL 0oJw== X-Gm-Message-State: ALQs6tCLoNoJklSEoi+pi7wpXJ3BpCK7uo5Po36JW9R/Eb7jpeIMTeGT zb6a2xBMnhWjA5J55MU/gx4/yGRb4DckqwPzCwk= X-Google-Smtp-Source: AB8JxZpT8ywRN6QVSxHiqzYpc81Q3IAy7ASMyCNqn9qsfwDValMwZ0N7NYBrmA8oNB0ddPk8xWBV0xAcJKQ+AfDKbTw= X-Received: by 2002:a2e:9d41:: with SMTP id y1-v6mr421076ljj.112.1525245749816; Wed, 02 May 2018 00:22:29 -0700 (PDT) MIME-Version: 1.0 Received: by 10.46.2.155 with HTTP; Wed, 2 May 2018 00:22:29 -0700 (PDT) In-Reply-To: <097132cc-18d7-3133-b425-e3ee9da28e77@gmail.com> References: <097132cc-18d7-3133-b425-e3ee9da28e77@gmail.com> From: Richard Biener Date: Wed, 02 May 2018 07:22:00 -0000 Message-ID: Subject: Re: [PATCH] restore -Warray-bounds for string literals (PR 83776) To: Martin Sebor Cc: Gcc Patch List , Richard Biener Content-Type: text/plain; charset="UTF-8" X-IsSubscribed: yes X-SW-Source: 2018-05/txt/msg00063.txt.bz2 On Fri, Jan 26, 2018 at 3:16 AM, Martin Sebor wrote: > PR tree-optimization/83776 - [6/7/8 Regression] missing > -Warray-bounds indexing past the end of a string literal, > identified a not-so-recent improvement to constant propagation > as the reason for GCC no longer being able to detect out-of- > bounds accesses to string literals. The root cause is that > the change caused accesses to strings to be transformed into > MEM_REFs that the -Warray-bounds checker isn't prepared to > handle. A simple example is: > > int h (void) > { > const char *p = "1234"; > return p[16]; // missing -Warray-bounds > } > > To fix the regression the attached patch extends the array bounds > checker to handle the small subset of MEM_REF expressions that > refer to string literals but stops of short of doing more than > that. There are outstanding gaps in the detection that the patch > intentionally doesn't handle. They are either caused by other > regressions (PR 84047) or by other latent bugs/limitations, or > by limitations in the approach I took to try to keep the patch > simple. I hope to address some of those in a follow-up patch > for GCC 9. + gimple *def = SSA_NAME_DEF_STMT (arg); + if (!is_gimple_assign (def)) + { + if (tree var = SSA_NAME_VAR (arg)) + arg = var; this is never correct. Whether sth has an underlying VAR_DECL or not is irrelevant. It looks like you'll always take the + else + return; path then anyways. So why obfuscate the code this way? + offset_int ofr[] = { + wi::to_offset (fold_convert (ptrdiff_type_node, vr->min)), + wi::to_offset (fold_convert (ptrdiff_type_node, vr->max)) + }; huh. Do you maybe want to use widest_int for ofr[]? What's wrong with wi::to_offset (vr->min)? Building another intermediate tree node here looks just bogus. What are you trying to do in this loop anyways? I suppose look at p_1 = &obj[i_2]; // already bounds-checked, but with ignore_off_by_one ... = MEM[p_1 + CST]; ? But then + if (TREE_CODE (varoff) != SSA_NAME) + break; you should at least handle INTEGER_CSTs here? + if (!vr || vr->type == VR_UNDEFINED || !vr->min || !vr->max) + break; please use positive tests here, VR_RANGE || VR_ANTI_RANGE. As usual the anti-range handling looks odd. Please add comments so we can follow what you were thinking when writing range merging code. Even better if you can stick to use existing code rather than always re-inventing the wheel... I think I commented on earlier variants but this doesn't seem to resemble any of them. Richard. > Martin