I just gave this a try, and unfortunately because the compiler is used to check if, for example, including Python.h works, we can't just put the iwyu tool in there in its place. I think we're going to have to get scons involved for this to give us meaningful results.
Gabe On Wed, Jan 13, 2021 at 7:15 PM Gabe Black <[email protected]> wrote: > 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
