Right, I didn't intend it to be something that would happen automatically and/or block a build if there was a warning, just something that you could optionally do through scons if you wanted, like running all the unit tests for instance. The documentation says IWYU will always return an error code so that make (or whatever the build system is) will know it didn't generate an object file since it gets substituted in for the compiler, and so I don't think we can really use it as a simple gate in that sense.
Having thought about this a bit more, we might at least want to start doing things like they suggest on their webpage for make, but just with the scons equivalent. We would set CC and CXX to the IWYU "compilers", and then also set --keep-going so that scons wouldn't stop when it gets an error (I assume equivalent to make's -k). I'm assuming that will produce a big spew of errors which we'd then want to fix, and would need to process with some sort of script after the fact if we wanted to make it a pre-submit check. We could at least periodically go through and scrub fixable, real issues flagged by IWYU periodically if that works. I can try this soon since it should be really easy to try. Gabe On Wed, Jan 13, 2021 at 8:12 AM Giacomo Travaglini via gem5-dev < [email protected]> wrote: > We could add it to scons as an extra flag which is defaulting to False, > and properly set in our regression scripts > (I am also in favour of adding IWYU) > > Kind Regards > > Giacomo > > > -----Original Message----- > > From: Nikos Nikoleris via gem5-dev <[email protected]> > > Sent: 13 January 2021 15:57 > > To: [email protected] > > Cc: Nikos Nikoleris <[email protected]> > > Subject: [gem5-dev] Re: IWYU tool and include checking from scons > > > > I am also in favor of a tool like iwyu and I have used it in the past. > > However, iirc iwyu suggestions were not perfect. For example, it > suggested to > > remove of "debug/xxx.hh" flags and in many cases required manual > > intervention. So before dealing with the scons mechanics we might have to > > deal with these cases. > > > > I am also a little worried about adding more dependencies for every > user. We > > often compile/run gem5 in clusters where lib/tool versions might not be > the > > latest. It would be great if this was part of our regressions and before > > committing we would check if something is missing. > > > > Nikos > > > > On 13/01/2021 15:39, Daniel Carvalho via gem5-dev wrote: > > > I am in favor of running a tool like IWYU to fix the codebase > > > (although I have never used it), but I am not sure if adding this to > > > the build system is a good idea: our current contribution frequency > > > (~1k > > > commits/year?) likely does not generate enough extra/missing includes > > > to require the overhead added to every build. There is also the issue > > > of adding another dependency VS making it optional and creating an > > > inconvenience for others: users that do not use the tool will generate > > > compilation warnings/errors for users that do. > > > > > > IMO, unless the overhead is unnoticeable, it would be enough to run > > > IWYU once a semester/year to try to find and fix any the period's > mistakes. > > > If doing this periodic approach, we should probably add an util script > > > that installs IWYU, builds/fixes the includes, and uninstalls IWYU, in > > > order to automate the process. > > > > > > Best, > > > Daniel > > > Em terça-feira, 12 de janeiro de 2021 22:41:58 BRT, Gabe Black via > > > gem5-dev <[email protected]> escreveu: > > > > > > > > > Hi folks. Daniel has submitted a big change which fixes up a bunch of > > > missing includes in files which were coincidentally getting the > > > definitions they needed indirectly from some other file. Way down the > > > line in c++20 I think the "modules" mechanism would be a great tool > > > avoiding these sorts of problems from the get go, but in the meantime > > > it would be helpful if we had a way to scan for these sorts of issues, > > > instead of finding them when they cause problems. These sorts of > > > overly broad, leaky includes also likely slow down builds by making > > > the compiler process more text than it really needs to, and also > > > introduces more dependencies at the scons level than are necessary. > > > > > > I'm not really familiar with it, but after a little bit of Googling I > > > found a tool called iwyu (include what you use) which looks at > > > includes and finds places where includes are missing (transitive > > > includes), and also includes which are not being used. It looks like > > > the way this tool is intended to be used is to substitute it for the > > > compiler, and then run it through, for instance, make. > > > > > > Rather than use it that way, I'm thinking we might be able to set up > > > additional scons rules which would build iwyu error reports based on > > > Source declarations in scons, alongside the normal build outputs. > > > There may be false positives in there somewhere, particularly from > > > code we don't control, and so we'd likely want to add in ways to flag > > exceptions. > > > > > > If this sort of scons integration works out, it would be nice to make > > > a pass of this tool part of the presubmit checks, assuming it doesn't > > > add tons of time to the build. > > > > > > What do folks think? I don't necessarily have a lot of time to work on > > > this myself, although I might give it a shot if I find some time. It > > > could also be something for someone wanting to cut their teeth on > > > scons to help with, and I could help give pointers and help with the > > > high level design if someone wanted to try it. If you do want to give > > > it a try, at the very least keep me in the loop so we don't have to > > > redo things in a major way when it comes time to check something in. > > > > > > Gabe > > > _______________________________________________ > > > gem5-dev mailing list -- [email protected] <mailto:[email protected]> > > > To unsubscribe send an email to [email protected] > > > <mailto:[email protected]> > > > %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s > > > > > > _______________________________________________ > > > gem5-dev mailing list -- [email protected] To unsubscribe send an > > > email to [email protected] > > > %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s > > > > > IMPORTANT NOTICE: The contents of this email and any attachments are > > confidential and may also be privileged. If you are not the intended > recipient, > > please notify the sender immediately and do not disclose the contents to > any > > other person, use it for any purpose, or store or copy the information > in any > > medium. Thank you. > > _______________________________________________ > > gem5-dev mailing list -- [email protected] To unsubscribe send an email > to > > gem5-dev- > > [email protected] %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s > IMPORTANT NOTICE: The contents of this email and any attachments are > confidential and may also be privileged. If you are not the intended > recipient, please notify the sender immediately and do not disclose the > contents to any other person, use it for any purpose, or store or copy the > information in any medium. Thank you. > _______________________________________________ > gem5-dev mailing list -- [email protected] > To unsubscribe send an email to [email protected] > %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
_______________________________________________ gem5-dev mailing list -- [email protected] To unsubscribe send an email to [email protected] %(web_page_url)slistinfo%(cgiext)s/%(_internal_name)s
