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>
Thanks. 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. Given that neither you nor I seem to have received the patch I wonder if anyone else received it? 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> There are some formatting rules that we follow with ChangeLog entries. We have a script: ./contrib/gcc-changelog/git_check_commit.py --help that you can run to check the formatting. > * 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 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: ...etc * call-string.h: ...etc * 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: 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. Your text editor might have a mode for working with ChangeLog files. [...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. [...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. [...] > >From 95572742f1aaad1975aa35a663e8b26e671d4323 Mon Sep 17 00:00:00 2001 > From: Ankur Saini <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> > > 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<const > std::pair<const supernode *,const supernode *>*> m_supernodes;" to enhance > readibility ...and to avoid having really long lines! > * 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). > --- > 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(-) > [...] > 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. > > - 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. > > 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
