From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-pf1-x434.google.com (mail-pf1-x434.google.com [IPv6:2607:f8b0:4864:20::434]) by sourceware.org (Postfix) with ESMTPS id 8EDC43890038 for ; Mon, 12 Jul 2021 16:37:23 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.1 sourceware.org 8EDC43890038 Received: by mail-pf1-x434.google.com with SMTP id 17so16937847pfz.4 for ; Mon, 12 Jul 2021 09:37:23 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:from:message-id:mime-version:subject:date :in-reply-to:cc:to:references; bh=MjaJCYRtTB2yvFGqC1+L53jucB4OtZ5yrWxKJkv1Ia8=; b=phfpMSZF14CGVEXrUV+FjdlnUMXEb2V+iK4aR6ifNgVMPRJtU9fcv8OfpZ71fBZ1eL 8WOF7lbhBHAMk0MMuclZzcikGomCABxyoLHSWUMVVRfy+kHyaQHdqaEUHo/ZWUp5oeoN ZiGb3rbgrK7Qs1JPSX1GoTNAQYTkZ/B6ncdrNgfHw3fILr2zJuu8cu4Pevehz4joko4U AnGc3Da2zpRaBX2DsKd0jQ5JDFF/fbGfg70zc+qpGueHtQsHjyhkrPnI89WEoQQjO4Ev tjrca79dDcyqskRKVMoGJFbq6zgQ+GyvzGdO+ZVJj8U/rJC8vEV3A6CmmZ7YKQ/76kZv m21g== X-Gm-Message-State: AOAM533CQ6917206MTgvUEJTIG3gnJN/7Ij5b2Th+2Gdn/SN2Y5X21N4 nT5YLEGxg03KH13PSQvvftw= X-Google-Smtp-Source: ABdhPJyBuT2sTrIP8ku78ageaZXrvwRktV15Yqk6AqKMtKrQB8OGxtmTubIpzX020QDN+2G7mGenoA== X-Received: by 2002:a63:fe51:: with SMTP id x17mr40067pgj.58.1626107842584; Mon, 12 Jul 2021 09:37:22 -0700 (PDT) Received: from [192.168.100.4] ([45.118.158.251]) by smtp.gmail.com with ESMTPSA id e21sm16739213pfc.172.2021.07.12.09.37.18 (version=TLS1_2 cipher=ECDHE-ECDSA-AES128-GCM-SHA256 bits=128/128); Mon, 12 Jul 2021 09:37:21 -0700 (PDT) From: Ankur Saini Message-Id: Mime-Version: 1.0 (Mac OS X Mail 14.0 \(3654.20.0.2.21\)) Subject: Re: daily report on extending static analyzer project [GSoC] Date: Mon, 12 Jul 2021 22:07:16 +0530 In-Reply-To: <3ff03555914a6ab2e42a288c420050679f615a7b.camel@redhat.com> Cc: gcc@gcc.gnu.org To: David Malcolm References: <35A0246A-D4F8-4B41-A009-4A98F78E0395@gmail.com> <06DBCE04-B3AC-4091-979D-430507352213@gmail.com> <425d5e711663bbf0c1ebcfe05097780ebb2126a0.camel@redhat.com> <9e97b67cc5eb55a3a526b1c263a9980915556ce1.camel@redhat.com> <6ea20fa9093b566db6884cc2af51ae1bb7deee95.camel@redhat.com> <0ba8ad1a23fd87ef123ec51f76ccaf29ea114c79.camel@redhat.com> <0558C377-5A39-4D71-A2DC-DE23E737C65D@gmail.com> <0324cb3d52c80b3a6ff70488aa28e2322de7b832.camel@redhat.com> <9CDE4E6A-D7DB-43DE-AB00-95D1B4667061@gmail.com> <00236f15381ae32ac62704d40d064a726a849f50.camel@redhat.com> <0F8999E7-C7E8-4499-9293-55BF4185EF49@gmail.com> <3ff03555914a6ab2e42a288c420050679f615a7b.camel@redhat.com> X-Mailer: Apple Mail (2.3654.20.0.2.21) 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, GIT_PATCH_0, HTML_MESSAGE, KAM_SHORT, RCVD_IN_ABUSEAT, RCVD_IN_BARRACUDACENTRAL, 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 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Content-Filtered-By: Mailman/MimeDel 2.1.29 X-BeenThere: gcc@gcc.gnu.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gcc mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 12 Jul 2021 16:37:27 -0000 >=20 > On 11-Jul-2021, at 11:19 PM, David Malcolm = wrote: >=20 > On Sat, 2021-07-10 at 21:27 +0530, Ankur Saini wrote: >> AIM for today :=20 >>=20 >> - update the call_string to store a vector of pair of supernode* >> instead of pointer to it=20 >> - create a typdef to give a meaning full name to these " pair of >> supernode* =E2=80=9C >> - send the patch list to gcc-patches mailing list >> - add the example program I created to the GCC tests >>=20 >> =E2=80=94 >>=20 >> PROGRESS : >>=20 >> - I successfully changed the entire call string representation again = to >> keep track of "auto_vec m_elements;=E2=80=9D from = "auto_vec> std::pair*> m_supernodes;=E2=80=9D= =20 >>=20 >> - while doing so, one hurdle I found was to change "hashval_t hash () >> const;=E2=80=9D, function of which I quite didn=E2=80=99t understood = properly, but >> looking at source, it looked like it just needed some value ( integer >> or pointer ) to add to =E2=80=98hstate=E2=80=99 and ' m_elements=E2=80=99= was neither a pointer >> nor an integer so I instead added pointer to callee supernode ( >> =E2=80=9Csecond=E2=80=9D of the m_elements ) to the =E2=80=98hstate=E2=80= =99 for now.=20 >>=20 >> - for the callstring patch, I created a patch file ( using git = format- >> patch ) and sent it to patches mailing list (via git send email ) and >> CCed my mentor. >> Although I received a positive reply from the command log (git send >> email) saying the mail was sent , I didn=E2=80=99t received that mail = ( being >> subscribed to the patches list ) regarding the same ( I sent that = just >> before sending this mail ). >> The mail should be sent from arsenic@sourceware.org = > arsenic@sourceware.org >=20 >=20 > Thanks. >=20 > I see the patch email in the list archives here: > https://gcc.gnu.org/pipermail/gcc-patches/2021-July/574888.html = > but for some reason it's not showing up in my inbox. I'm not sure = why; > I recently got migrated to a new email server and my filters are > currently a mess so it could be a problem at my end; sorry if that's > the case. >=20 > Given that neither you nor I seem to have received the patch I wonder > if anyone else received it? Then I think it=E2=80=99s better to attach patch file with the updates = here instead. >=20 > Given that, I'm going to reply to that patch email inline here (by > copying and pasting it from the archive): >=20 >> [PATCH 1/2] analyzer: refactor callstring to work with pairs of = supernodes [GSoC] >>=20 >> 2021-07-3 Ankur Saini > >=20 > There are some formatting rules that we follow with ChangeLog entries. > We have a script: >=20 > ./contrib/gcc-changelog/git_check_commit.py --help Ok, I will keep in mind to double check with it from now on. >=20 > that you can run to check the formatting. >=20 >> * gcc/analyzer/call-string.cc : = refactor callstring to work with pair of supernodes instead of super = superedges >> * gcc/analyzer/call-string.h: make callstring work with pairs = of supernodes >> * gcc/analyzer/program-point.cc : = refactor program point to work with new call-string format >=20 > The "gcc/analyzer" directory has its own ChangeLog file, and filenames > should be expressed relative to it, so these entries should read > something like: >=20 > gcc/analyzer/ChangeLog: > * call-string.cc : ...etc > * call-string.h: ...etc > * program-point.cc : ...etc >=20 > The entries should be sentences (i.e. initial capital letter and > trailing full-stop). >=20 > They should be line-wrapped (I do it at 74 columns), giving this: >=20 > gcc/analyzer/ChangeLog: > * call-string.cc : Refactor callstring = to work with pair of > supernodes instead of superedges. > * call-string.h: Make callstring work with pairs of supernodes. > * program-point.cc : Refactor program = point to work with new > call-string format. >=20 > Your text editor might have a mode for working with ChangeLog files. Yes, there is a way to wrap text after certain number of columns in my = text editor, I would be using that from now on when working with = changelogs. >=20 > [...snip...] >=20 >> @@ -152,22 +152,40 @@ call_string::push_call (const supergraph &sg, >> gcc_assert (call_sedge); >> const return_superedge *return_sedge =3D = call_sedge->get_edge_for_return (sg); >> gcc_assert (return_sedge); >> - m_return_edges.safe_push (return_sedge); >> + const std::pair *e =3D new = (std::pair) >=20 > We don't want lines wider than 80 columns unless it can't be helped.=20= > Does your text editor have a feature to warn you about overlong lines? >=20 > Changing from: > std::pair > to: > element_t > should make it much easier to avoid overlong lines. >=20 > [...snip...] >=20 >> diff --git a/gcc/analyzer/call-string.h b/gcc/analyzer/call-string.h >> index 7721571ed60..0134d185b90 100644 >> --- a/gcc/analyzer/call-string.h >> +++ b/gcc/analyzer/call-string.h >=20 > [...snip...] >=20 >> + >> + void push_call (const supernode *src,=20 >> + const supernode *dest); >=20 > There's some odd indentation here. Does your text editor have option > to > (a) show visible whitespace (distinguish between tabs vs spaces) > (b) enforce a coding standard? >=20 > If your editor supports it, it's easy to comply with a project's = coding > standards, otherwise it can be a pain. Oh, I see. This explains the weird indentation convention I was seeing = throughout the source. Actually my editor dynamically adjusts the width = of the tab depending on the style used in source file and due to some = reasons, it decided that it was 2 space wide here, this was leading to = some weird indentations throughout the source.=20 Well now it should be fixed, I manually adjusted it to be standard 8 = wide now and switched of converting tabs to spaces in my editor = settings. >=20 > [...snip...] >=20 >> private: >> - auto_vec m_return_edges; >> + //auto_vec m_return_edges; >> + auto_vec*> = m_supernodes; >> }; >=20 > Commenting out lines is OK during prototyping. Obviously as the patch > gets closer to be being ready we want to simply delete them instead. Sorry, I might have missed this one during my reviewing phase. >=20 > [...] >=20 >>> =46rom 95572742f1aaad1975aa35a663e8b26e671d4323 Mon Sep 17 00:00:00 = 2001 >> From: Ankur Saini > >> Date: Sat, 10 Jul 2021 19:28:49 +0530 >> Subject: [PATCH 2/2] analyzer: make callstring's pairs of supernodes >> statically allocated [GSoC] >>=20 >> 2021-07-10 Ankur Saini > >>=20 >> gcc/analyzer/ >> * call-string.cc : store a vector = of std::pair of supernode* instead of pointer to them >> * call-string.h: create a typedef for "auto_vec*> m_supernodes;" to = enhance readibility >=20 > ...and to avoid having really long lines! >=20 >> * program-point.cc : refactor = program point to work with new call-string format >=20 > I think it's going to be much easier for me if you squash these two > patches into a single patch so I can review the combined change. (If > you've not seen it yet, try out "git rebase -i" to see how to do = this). woah, this is magic ! I always use to perform a soft reset ( git reset =E2=80=94soft = ) and commit in order to squash or reword my commits before, but never = knew we could change history locally like this, amazing : D >=20 >> --- >> gcc/analyzer/call-string.cc | 99 = ++++++++++++++++++++--------------- >> gcc/analyzer/call-string.h | 21 +++++--- >> gcc/analyzer/program-point.cc | 8 +-- >> 3 files changed, 73 insertions(+), 55 deletions(-) >>=20 > [...] >=20 >> diff --git a/gcc/analyzer/call-string.h b/gcc/analyzer/call-string.h >> index 0134d185b90..450af6da21a 100644 >> --- a/gcc/analyzer/call-string.h >> +++ b/gcc/analyzer/call-string.h >> @@ -28,6 +28,8 @@ class supernode; >> class call_superedge; >> class return_superedge; >>=20 >> + typedef std::pair element_t; >=20 > Rather than a std::pair, I think a struct inside call_string like this > would be better: rather than "first" and "second" we can refer to > "m_caller" and "m_callee", which is closer to being self-documenting, > and it allows us to add member functions e.g. "get_caller_function": >=20 > class call_string > { > public: > struct element_t > { > element_t (const supernode *caller, const supernode *callee) > : m_caller (caller), m_callee (callee) > { > } >=20 > function *get_caller_function () const {/*etc*/} > function *get_callee_function () const {/*etc*/} >=20 > const supernode *m_caller; > const supernode *m_callee; > }; >=20 > }; >=20 > [...snip...] >=20 > which might clarify the code further. Instead of putting that struct inside the class, I declared it globally = and overloaded some basic operators ( =E2=80=9C=3D=3D=E2=80=9C and = =E2=80=9C!=3D=E2=80=9C ) to make it work without having to change a lot = how it is being handled in other areas of source ( program_point.cc and = engine.cc ). >=20 >>=20 >> - I also added the example I was using to test the analyzer to >> regression tests as = "gcc/testsuite/gcc.dg/analyzer/call-via-fnptr.c=E2=80=9D. >=20 > Great! Please add it to the patch. I am thinking to add it with later patches as this one only focuses on = changing the call_string only and doesn=E2=80=99t quite fix this bug. >=20 >>=20 >> STATUS AT THE END OF THE DAY :-=20 >>=20 >> - update the call_string to store a vector of pair of supernode* >> instead of pointer to it ( done ) >> - create a typdef to give a meaning full name to these " pair of >> supernode* =E2=80=9C ( done ) >> - send the patch list to gcc-patches mailing list ( done ? ) >> - add the example program I created to the GCC tests ( done ) >>=20 >=20 > Hope this is constructive > Dave =E2=80=94 ( new fixed patch should be attached with this mail ) Thanks=20 - Ankur=