From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-qt1-x833.google.com (mail-qt1-x833.google.com [IPv6:2607:f8b0:4864:20::833]) by sourceware.org (Postfix) with ESMTPS id DC9AD389002A for ; Wed, 28 Jul 2021 22:09:41 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org DC9AD389002A Received: by mail-qt1-x833.google.com with SMTP id h27so2502600qtu.9 for ; Wed, 28 Jul 2021 15:09:41 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:subject:to:cc:references:from:message-id:date :user-agent:mime-version:in-reply-to:content-language :content-transfer-encoding; bh=XkN5VzLYDAgwlcK50NsD5hyB6q0XciepXfgw/tnPmW8=; b=i0mkguYWQ/BJA4dMbkXdnCUDwlLAmudDNLX2OXretOqjFhBs1rKoBg/YcSuKfyY6WX DtbL3Avnol68BXwUk77goBZYDXRR6hoPRRuVKNtbeC9e4D0Ullc1rMdSXhoMD6XO1TvO qhkMuPXLNeekZGjYKsyqcDNfIT2fFNxqOVVuXDUJmGyxUw/8RMrj6L2lnThibTQX4jo5 GOB5pNaNCb3bwCVvNgRY3eua3MwBQmOetxkXaXsplqmH5D9KrJDi6Z0FUjMEfY/A2jD2 qOcu8aihQeSXajYNX/V9t5aRErD79UvnItkyLC6I/6s0Q0NkLHS3GfSschuHB2/rQwnF ibnA== X-Gm-Message-State: AOAM532wTmPIz9GHyrkUwLaNvzrYgnks8dBylefob7RYnTLTtqg6nz3T 6zrhN8N1CaQhCfT0CguAgODDlx+6Fzk= X-Google-Smtp-Source: ABdhPJy95PGk1R34rcHUCl6pSfFAC04E9ZfQAlKSEiouOGUYN+3Wq2Y2Hez2I8edGlr5MO4QY8Y4Hg== X-Received: by 2002:ac8:44c2:: with SMTP id b2mr1616210qto.216.1627510181343; Wed, 28 Jul 2021 15:09:41 -0700 (PDT) Received: from [192.168.0.41] (75-166-102-22.hlrn.qwest.net. [75.166.102.22]) by smtp.gmail.com with ESMTPSA id k125sm696964qkf.16.2021.07.28.15.09.40 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 28 Jul 2021 15:09:41 -0700 (PDT) Subject: Re: [PATCH] add access warning pass To: Richard Biener Cc: gcc-patches References: From: Martin Sebor Message-ID: <57c10e50-49f8-7a09-270a-c7895c9c3340@gmail.com> Date: Wed, 28 Jul 2021 16:09:39 -0600 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101 Thunderbird/68.2.2 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Language: en-US Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-4.3 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_FROM, NICE_REPLY_A, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP autolearn=ham autolearn_force=no version=3.4.4 X-Spam-Checker-Version: SpamAssassin 3.4.4 (2020-01-24) on server2.sourceware.org X-BeenThere: gcc-patches@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Wed, 28 Jul 2021 22:09:43 -0000 On 7/28/21 3:23 AM, Richard Biener wrote: > On Fri, Jul 16, 2021 at 12:42 AM Martin Sebor via Gcc-patches > wrote: >> >> A number of access warnings as well as their supporting >> infrastructure (compute_objsize et al.) are implemented in >> builtins.{c,h} where they (mostly) operate on trees and run >> just before RTL expansion. >> >> This setup may have made sense initially when the warnings were >> very simple and didn't perform any CFG analysis, but it's becoming >> a liability. The code has grown both in size and in complexity, >> might need to examine the CFG to improve detection, and in some >> cases might achieve a better S/R ratio if run earlier. Running >> the warning code on trees is also slower because it doesn't >> benefit from the SSA_NAME caching provided by the pointer_query >> class. Finally, having the code there is also an impediment to >> maintainability as warnings and builtin expansion are unrelated >> to each other and contributors to one area shouldn't need to wade >> through unrelated code (similar for patch reviewers). >> >> The attached change introduces a new warning pass and a couple of >> new source and headers and, as the first step, moves the warning >> code from builtins.{c,h} there. To keep the initial changes as >> simple as possible the pass only runs a subset of existing >> warnings: -Wfree-nonheap-object, -Wmismatched-dealloc, and >> -Wmismatched-new-delete. The others (-Wstringop-overflow and >> -Wstringop-overread) still run on the tree representation and >> are still invoked from builtins.c or elsewhere. >> >> The changes have no functional impact either on codegen or on >> warnings. I tested them on x86_64-linux. >> >> As the next step I plan to change the -Wstringop-overflow and >> -Wstringop-overread code to run on the GIMPLE IL in the new pass >> instead of on trees in builtins.c. > > That's the maybe_warn_rdwr_sizes thing? Among others, yes. It includes most buffer overflow and overread warnings. > > + gimple *stmt = gsi_stmt (si); > + if (!is_gimple_call (stmt)) > + continue; > + > + check (as_a (stmt)); > > > if (gcall *call = dyn_cast (gsi_stmt (si))) > check (call); > > might be more C++-ish. Sure, I can do that. > > The patch looks OK - I skimmed it as mostly moving things > around plus adding a new pass. Okay, thanks. I have retested the updated change and pushed it in r12-2581. As a reminder, the git show output for builtins.c looks considerably different from the patch I posted because of what I mentioned below. Martin > > Thanks, > Richard. > >> Martin >> >> PS The builtins.c diff produced by git diff was much bigger than >> the changes justify. It seems that the code removal somehow >> confused it. To make review easy I replaced it with a plain >> unified diff of builtins.c that doesn't suffer from the problem.