From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pg1-x52e.google.com (mail-pg1-x52e.google.com [IPv6:2607:f8b0:4864:20::52e]) by sourceware.org (Postfix) with ESMTPS id 7FF543857BAB for ; Thu, 11 Aug 2022 09:10:29 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 7FF543857BAB Received: by mail-pg1-x52e.google.com with SMTP id 24so172153pgr.7 for ; Thu, 11 Aug 2022 02:10:29 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=to:subject:message-id:date:from:in-reply-to:references:mime-version :x-gm-message-state:from:to:cc; bh=c3CzKlusRB5ZHIyHbviBM5Zl2G4W5kLJWDRctHX47bA=; b=jWU0GQJb9wVgSjpgAGe5xuHITONAUTlY0DKZDnYxJt4Iu7ZY0x1YYRSuuHyGaSOGS2 Mc4TFvL+QjnfcAybm9ZiiYdYBl9CEu8m3SJ5TvmqIs2GgwSPhR+fCP12tSLZpwmDOHwV aK2tBaOeeLWLmIVE/njCYXN1qNE4sGY8c3EaSxCeeOfMzRwojhu84qnNJe1k4UkwCz5U 890WOVOwxjvoXKbWrE1tGNrMdkSL6dFByp9W2rwQJrGC1wEh91Y/PEXotDZ4s7DRVDCz 0YQBKNeXMXoqS6IyLSaOMWPC4k/lvTCThs377YzurtMGfuGFEyGXbHWkWXhrvNSt/NP5 7IhQ== X-Gm-Message-State: ACgBeo0c4bSizymHojIMLnYdI4o5Ze7Sh8HUVCyGFZJi9J7tkGNQU9dG 2DmOwO4GFzVIOxjR4YPbgmPKoGs2xIT7QBfnuLM= X-Google-Smtp-Source: AA6agR4CmkiFRkOp1yVNAPNGXVF2wWBoGFsCAV5nY9N9hAC7Az7XdMmwQZDi4DcReHOZt3gRA3pfqrQ738aPl0du5Lg= X-Received: by 2002:a63:8bc3:0:b0:41d:4b74:b975 with SMTP id j186-20020a638bc3000000b0041d4b74b975mr17116925pge.309.1660209028432; Thu, 11 Aug 2022 02:10:28 -0700 (PDT) MIME-Version: 1.0 References: <2022febb8297564e4cd6f349f16bba6e8b91509b.camel@redhat.com> In-Reply-To: From: Mir Immad Date: Thu, 11 Aug 2022 14:40:15 +0530 Message-ID: Subject: Re: [PATCH] analyzer: fix ICE casued by dup2 in sm-fd.cc[PR106551] To: David Malcolm , gcc-patches@gcc.gnu.org X-Spam-Status: No, score=-8.0 required=5.0 tests=BAYES_00, DKIM_SIGNED, DKIM_VALID, DKIM_VALID_AU, DKIM_VALID_EF, FREEMAIL_ENVFROM_END_DIGIT, FREEMAIL_FROM, GIT_PATCH_0, HTML_MESSAGE, RCVD_IN_DNSWL_NONE, SPF_HELO_NONE, SPF_PASS, TXREP, T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org Content-Type: text/plain; charset="UTF-8" X-Content-Filtered-By: Mailman/MimeDel 2.1.29 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: Thu, 11 Aug 2022 09:10:34 -0000 With the fix for bogus warning in fd-uninit.c, the analyzer now does not warning for the following code for which it would previously emit -Wanalyzer-fd-use-without-check extern int m; test() { int fd = dup2(m, 1); close(fd); } So I had to remove such warnings from fd-dup-1.c test_20,21,22 (in the patch). Now these tests are only there to show fix for PR16551. Sending an updated patch (passes style and commit checker). Thanks. Immad. On Thu, Aug 11, 2022 at 12:14 AM David Malcolm wrote: > On Wed, 2022-08-10 at 22:51 +0530, Mir Immad wrote: > > > Can you please rebase and see if your patch > > > does fix it? > > > > No, the patch that I sent did not attempt to fix this. Now that I > > have made > > the correction, XFAIL in fd-uninit-1.c has changed to XPASS. > > Great - that means that, with your fix, we no longer bogusly emit that > false positive. > > > > > Should i remove the dg-bogus warning from fd-uninit-1.c test_1? > > Yes please. > > Thanks > Dave > > > > > Thanks. > > Immad. > > > > > > On Wed, Aug 10, 2022 at 10:26 PM David Malcolm > > wrote: > > > > > On Wed, 2022-08-10 at 20:34 +0530, Mir Immad wrote: > > > > > if you convert the "int m;" locals into an extern global, like > > > > in > > > > > comment #0 of bug 106551, does that still trigger the crash on > > > > > the > > > > > unpatched sm-fd.cc? > > > > > > > > Yes, it does, since m would be in "m_start" state. I'm sending an > > > > updated > > > > patch. > > > > > > Great! > > > > > > Note that I recently committed a fix for bug 106573, which has an > > > xfail > > > on a dg-bogus to mark a false positive which your patch hopefully > > > also > > > fixes (in fd-uninit-1.c). Can you please rebase and see if your > > > patch > > > does fix it? > > > > > > Thanks > > > Dave > > > > > > > > > > > > > > Thanks > > > > Immad. > > > > > > > > On Wed, Aug 10, 2022 at 1:32 AM David Malcolm < > > > > dmalcolm@redhat.com> > > > > wrote: > > > > > > > > > On Tue, 2022-08-09 at 21:42 +0530, Immad Mir wrote: > > > > > > This patch fixes the ICE caused by valid_to_unchecked_state, > > > > > > at analyzer/sm-fd.cc by handling the m_start state in > > > > > > check_for_dup. > > > > > > > > > > > > Tested lightly on x86_64. > > > > > > > > > > > > gcc/analyzer/ChangeLog: > > > > > > PR analyzer/106551 > > > > > > * sm-fd.cc (check_for_dup): handle the m_start > > > > > > state when transitioning the state of LHS > > > > > > of dup, dup2 and dup3 call. > > > > > > > > > > > > gcc/testsuite/ChangeLog: > > > > > > * gcc.dg/analyzer/fd-dup-1.c: New testcases. > > > > > > > > > > > > Signed-off-by: Immad Mir > > > > > > --- > > > > > > gcc/analyzer/sm-fd.cc | 4 ++-- > > > > > > gcc/testsuite/gcc.dg/analyzer/fd-dup-1.c | 28 > > > > > > +++++++++++++++++++++++- > > > > > > 2 files changed, 29 insertions(+), 3 deletions(-) > > > > > > > > > > > > diff --git a/gcc/analyzer/sm-fd.cc b/gcc/analyzer/sm-fd.cc > > > > > > index 8bb76d72b05..c8b9930a7b6 100644 > > > > > > --- a/gcc/analyzer/sm-fd.cc > > > > > > +++ b/gcc/analyzer/sm-fd.cc > > > > > > @@ -983,7 +983,7 @@ fd_state_machine::check_for_dup > > > > > > (sm_context > > > > > > *sm_ctxt, const supernode *node, > > > > > > case DUP_1: > > > > > > if (lhs) > > > > > > { > > > > > > - if (is_constant_fd_p (state_arg_1)) > > > > > > + if (is_constant_fd_p (state_arg_1) || state_arg_1 > > > > > > == > > > > > > m_start) > > > > > > sm_ctxt->set_next_state (stmt, lhs, > > > > > > m_unchecked_read_write); > > > > > > else > > > > > > sm_ctxt->set_next_state (stmt, lhs, > > > > > > @@ -1011,7 +1011,7 @@ fd_state_machine::check_for_dup > > > > > > (sm_context > > > > > > *sm_ctxt, const supernode *node, > > > > > > file descriptor i.e the first argument. */ > > > > > > if (lhs) > > > > > > { > > > > > > - if (is_constant_fd_p (state_arg_1)) > > > > > > + if (is_constant_fd_p (state_arg_1) || state_arg_1 > > > > > > == > > > > > > m_start) > > > > > > sm_ctxt->set_next_state (stmt, lhs, > > > > > > m_unchecked_read_write); > > > > > > else > > > > > > sm_ctxt->set_next_state (stmt, lhs, > > > > > > diff --git a/gcc/testsuite/gcc.dg/analyzer/fd-dup-1.c > > > > > > b/gcc/testsuite/gcc.dg/analyzer/fd-dup-1.c > > > > > > index eba2570568f..ed4d6de57db 100644 > > > > > > --- a/gcc/testsuite/gcc.dg/analyzer/fd-dup-1.c > > > > > > +++ b/gcc/testsuite/gcc.dg/analyzer/fd-dup-1.c > > > > > > @@ -220,4 +220,30 @@ test_19 (const char *path, void *buf) > > > > > > close (fd); > > > > > > } > > > > > > > > > > > > -} > > > > > > \ No newline at end of file > > > > > > +} > > > > > > + > > > > > > +void > > > > > > +test_20 () > > > > > > +{ > > > > > > + int m; > > > > > > + int fd = dup (m); /* { dg-warning "'dup' on possibly > > > > > > invalid > > > > > > file descriptor 'm'" } */ > > > > > > + close (fd); > > > > > > +} > > > > > > + > > > > > > +void > > > > > > +test_21 () > > > > > > +{ > > > > > > + int m; > > > > > > + int fd = dup2 (m, 1); /* { dg-warning "'dup2' on > > > > > > possibly > > > > > > invalid file descriptor 'm'" } */ > > > > > > + close (fd); > > > > > > +} > > > > > > + > > > > > > +void > > > > > > +test_22 (int flags) > > > > > > +{ > > > > > > + int m; > > > > > > + int fd = dup3 (m, 1, flags); /* { dg-warning "'dup3' on > > > > > > possibly > > > > > > invalid file descriptor 'm'" } */ > > > > > > + close (fd); > > > > > > +} > > > > > > > > > > Thanks for the updated patch. > > > > > > > > > > The test cases looked suspicious to me - I was wondering why > > > > > the > > > > > analyzer doesn't complain about the uninitialized values being > > > > > passed > > > > > to the various dup functions as parameters. So your test cases > > > > > seem to > > > > > have uncovered a hidden pre-existing bug in the analyzer's > > > > > uninitialized value detection, which I've filed for myself to > > > > > deal > > > > > with > > > > > as PR analyzer/106573. > > > > > > > > > > If you convert the "int m;" locals into an extern global, like > > > > > in > > > > > comment #0 of bug 106551, does that still trigger the crash on > > > > > the > > > > > unpatched sm-fd.cc? If so, then that's greatly preferable as a > > > > > regression test, since otherwise I'll have to modify that test > > > > > case > > > > > when I fix bug 106573. > > > > > > > > > > Dave > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >