> > On 11-Jul-2021, at 11:19 PM, David Malcolm <dmalc...@redhat.com> wrote: > > On Sat, 2021-07-10 at 21:27 +0530, Ankur Saini wrote: >> AIM for today : >> >> - update the call_string to store a vector of pair of supernode* >> instead of pointer to it >> - create a typdef to give a meaning full name to these " pair of >> supernode* “ >> - send the patch list to gcc-patches mailing list >> - add the example program I created to the GCC tests >> >> — >> >> PROGRESS : >> >> - I successfully changed the entire call string representation again to >> keep track of "auto_vec<element_t> m_elements;” from "auto_vec<const >> std::pair<const supernode *,const supernode *>*> m_supernodes;” >> >> - while doing so, one hurdle I found was to change "hashval_t hash () >> const;”, function of which I quite didn’t understood properly, but >> looking at source, it looked like it just needed some value ( integer >> or pointer ) to add to ‘hstate’ and ' m_elements’ was neither a pointer >> nor an integer so I instead added pointer to callee supernode ( >> “second” of the m_elements ) to the ‘hstate’ for now. >> >> - 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’t 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 arse...@sourceware.org >> <mailto:arse...@sourceware.org> <mailto: >> arse...@sourceware.org <mailto:arse...@sourceware.org>> > > Thanks. > > I see the patch email in the list archives here: > https://gcc.gnu.org/pipermail/gcc-patches/2021-July/574888.html > <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. > > Given that neither you nor I seem to have received the patch I wonder > if anyone else received it?
Then I think it’s better to attach patch file with the updates here instead. > > Given that, I'm going to reply to that patch email inline here (by > copying and pasting it from the archive): > >> [PATCH 1/2] analyzer: refactor callstring to work with pairs of supernodes >> [GSoC] >> >> 2021-07-3 Ankur Saini <arse...@sourceware.org >> <mailto:arse...@sourceware.org>> > > There are some formatting rules that we follow with ChangeLog entries. > We have a script: > > ./contrib/gcc-changelog/git_check_commit.py --help Ok, I will keep in mind to double check with it from now on. > > that you can run to check the formatting. > >> * gcc/analyzer/call-string.cc <http://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 <http://program-point.cc/>: refactor >> program point to work with new call-string format > > The "gcc/analyzer" directory has its own ChangeLog file, and filenames > should be expressed relative to it, so these entries should read > something like: > > gcc/analyzer/ChangeLog: > * call-string.cc <http://call-string.cc/>: ...etc > * call-string.h: ...etc > * program-point.cc <http://program-point.cc/>: ...etc > > The entries should be sentences (i.e. initial capital letter and > trailing full-stop). > > They should be line-wrapped (I do it at 74 columns), giving this: > > gcc/analyzer/ChangeLog: > * call-string.cc <http://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 <http://program-point.cc/>: Refactor program point > to work with new > call-string format. > > 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. > > [...snip...] > >> @@ -152,22 +152,40 @@ call_string::push_call (const supergraph &sg, >> gcc_assert (call_sedge); >> const return_superedge *return_sedge = call_sedge->get_edge_for_return >> (sg); >> gcc_assert (return_sedge); >> - m_return_edges.safe_push (return_sedge); >> + const std::pair<const supernode *,const supernode *> *e = new >> (std::pair<const supernode *,const supernode *>) > > We don't want lines wider than 80 columns unless it can't be helped. > Does your text editor have a feature to warn you about overlong lines? > > Changing from: > std::pair<const supernode *,const supernode *> > to: > element_t > should make it much easier to avoid overlong lines. > > [...snip...] > >> 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 > > [...snip...] > >> + >> + void push_call (const supernode *src, >> + const supernode *dest); > > 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? > > 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. 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. > > [...snip...] > >> private: >> - auto_vec<const return_superedge *> m_return_edges; >> + //auto_vec<const return_superedge *> m_return_edges; >> + auto_vec<const std::pair<const supernode *,const supernode *>*> >> m_supernodes; >> }; > > 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. > > [...] > >>> From 95572742f1aaad1975aa35a663e8b26e671d4323 Mon Sep 17 00:00:00 2001 >> From: Ankur Saini <arse...@sourceware.org <mailto:arse...@sourceware.org>> >> Date: Sat, 10 Jul 2021 19:28:49 +0530 >> Subject: [PATCH 2/2] analyzer: make callstring's pairs of supernodes >> statically allocated [GSoC] >> >> 2021-07-10 Ankur Saini <arse...@sourceware.org >> <mailto:arse...@sourceware.org>> >> >> gcc/analyzer/ >> * call-string.cc <http://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<const >> std::pair<const supernode *,const supernode *>*> m_supernodes;" to enhance >> readibility > > ...and to avoid having really long lines! > >> * program-point.cc <http://program-point.cc/>: refactor program >> point to work with new call-string format > > 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 —soft <commit> ) and commit in order to squash or reword my commits before, but never knew we could change history locally like this, amazing : D > >> --- >> gcc/analyzer/call-string.cc <http://call-string.cc/> | 99 >> ++++++++++++++++++++--------------- >> gcc/analyzer/call-string.h | 21 +++++--- >> gcc/analyzer/program-point.cc <http://program-point.cc/> | 8 +-- >> 3 files changed, 73 insertions(+), 55 deletions(-) >> > [...] > >> 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; >> >> + typedef std::pair<const supernode *,const supernode *> element_t; > > 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": > > class call_string > { > public: > struct element_t > { > element_t (const supernode *caller, const supernode *callee) > : m_caller (caller), m_callee (callee) > { > } > > function *get_caller_function () const {/*etc*/} > function *get_callee_function () const {/*etc*/} > > const supernode *m_caller; > const supernode *m_callee; > }; > > }; > > [...snip...] > > which might clarify the code further. Instead of putting that struct inside the class, I declared it globally and overloaded some basic operators ( “==“ and “!=“ ) 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 ). > >> >> - 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”. > > 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’t quite fix this bug. > >> >> STATUS AT THE END OF THE DAY :- >> >> - 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* “ ( done ) >> - send the patch list to gcc-patches mailing list ( done ? ) >> - add the example program I created to the GCC tests ( done ) >> > > Hope this is constructive > Dave — ( new fixed patch should be attached with this mail ) Thanks - Ankur
