Hi Andreas, I had two initial motivations (beyond just getting my code to compile):
1. It seems stupid that we would fail to build just because some unforeseen combination of swig, compiler, and gem5 versions generates a warning. It's strange to me that things used to compile on my machine, and now they don't, even with the same swig and gcc versions; clearly as new versions of swig and gcc come out it's only going to get monotonically worse. I don't even see why we care whether the swig-generated code causes any compiler warnings at all, since there's not much we can do about it; trying to stay ahead of the game by adding appropriate -Wno-* flags to swig_env seems like a bit of a waste of time IMO. Using -Werror compounds the problem by putting that task on the critical path of getting your code to build. 2. I thought we could simplify things by getting rid of swig_env entirely and just using the existing Werror mechanism. So I initially thought we could both make life easier for users and simplify the code, which would be a win-win. It turns out I was totally wrong about #2, so now we have to trade off making life easier vs. making the code more complicated, so it's less obvious to me what to do. Certainly just adding -Wno-uninitialized is the obvious minimal stopgap that addresses my current problem, but I still think #1 is a longer-term problem. I'm fine with adding -Wno-uninitialized to suppress the current warning and getting rid of -Werror in swig_env to at least get this problem off the critical path in the future. Doing the former only seems like too much of a band-aid to me, as it doesn't do anything to keep us from having this same problem over and over again in the future. I agree with you that if we want to have multiple levels of warnings (my #4 below) then it makes sense to add at least a third one to cope with generated code that we do have some control over. Steve On Thu, Mar 21, 2013 at 1:16 AM, Andreas Hansson <[email protected]>wrote: > Hi Steve, > > Concerning the different proposals: > > 1. I'm fine with this, but would rather go for one of the other solutions > to not create too much clutter. > > 2. clang does support the -w flag > > 3. See 2. > > 4. It is a sensible idea, but it also opens up for questions concerning > our own auto-generated code. For example, I tried adding a warning for > self-assignment, and it turns out we do this a lot in the generated > decoders (in fact the behaviour seems to rely on it and breaks if they are > removed). If we go through with this "tiering" approach then I think we > should have user generated, machine generated (that we control) and other > machine generated that we believe to be correct. > > 5. I would even say why not simply add the no-uninitialized and be done > with it? > > Andreas > > > On 20/03/2013 23:16, "Steve Reinhardt" <[email protected]> wrote: > > >Yea, Ubuntu 10.04 LTS is what I have too. > > > >So unsurprisingly this ended up being more complicated than I expected. > > Getting rid of swig_env entirely and just building swig files with > >'Werror=False' works, but spews out a zillion warnings, because a lot of > >what we do with swig_env is add '-Wno-foo' for all the '-Wfoo' warnings we > >normally want. > > > >Unfortunately all the warnings are baked in to env up front, so even > >though > >the logical thing would be not to add them in the first place, that's a > >non-trivial change. > > > >I did have success with adding '-w' in swig_env, which is a gcc option > >that > >suppresses all warnings. I don't know whether clang supports that or not > >though. > > > >So I see several paths here: > > > >1. Just stop adding -Werror to swig_env (like Mitch has done). Doesn't > >really simplify anything, but at least the few warnings we're not managing > >to avoid now don't kill your build. > > > >2. Stop trying to undo specific warnings that we know the generated code > >will violate, and use '-w' instead just to suppress all warnings. This is > >only a reasonable solution if clang (and other compilers we might want to > >use) also support -w. > > > >3. Find a way to go through the compiler flags for swig_env and delete all > >the ones that match '-W*'. This is like #2, but has the advantage of not > >relying on '-w'. I'm also not 100% sure how to do this in scons, but I > >assume it is possible. > > > >4. Instead of accumulating compiler warning flags directly into the main > >build environment, accumulate them off to the side somewhere, then have > >the > >default environment be the main environment plus all the warnings. This > >allows us to create swig_env by cloning the main environment *before* the > >warning flags are added. > > > > > >Given these options, #1 is the easiest but still leaves some ugly code in > >place; #2 is nice because we can get rid of all the '-Wno-foo' stuff, but > >concerns me because I'm not sure how universal '-w' is; #4 is basically > >the > >way I wish we'd done it in the first place, but is by far the most effort > >at this point; and #3's main attraction is that it is sort of like #4 but > >is probably less work. > > > >Any thoughts? Any alternatives I missed? > > > >Steve > > > > > > > >On Wed, Mar 20, 2013 at 1:11 PM, Mitch Hayenga > ><[email protected] > >> wrote: > > > >> I have same error/warning on the ECE cluster here at Wisconsin (Ubuntu > >> 10.04 LTS which happens to be gcc 4.4 and swig 1.3.40). I just disabled > >> Werror for swig. > >> > >> On Wed, Mar 20, 2013 at 1:45 PM, nathan binkert <[email protected]> > >>wrote: > >> > >> > It's probably all my fault, and my guess is that the swig_env predated > >> > the Werror flag. I'm fine with your proposal. > >> > > >> > Nate > >> > > >> > On Wed, Mar 20, 2013 at 11:33 AM, Steve Reinhardt <[email protected]> > >> > wrote: > >> > > Why do we care about compiler warnings in swig-generated code? > >> > > > >> > > I just started running some tests on my Ubuntu 10.04 machine (swig > >> > 1.3.40, > >> > > gcc 4.4.3), and I ran into this on a few different _wrap.cc files: > >> > > > >> > > [ CXX] ALPHA/python/swig/event_wrap.cc -> .fo > >> > > cc1plus: warnings being treated as errors > >> > > build/ALPHA/python/swig/event_wrap.cc: In function 'PyObject* > >> > > _wrap_new_Cycles(PyObject*, PyObject*)': > >> > > build/ALPHA/python/swig/event_wrap.cc:3380: error: 'argv[0]' may be > >> used > >> > > uninitialized in this function > >> > > build/ALPHA/python/swig/event_wrap.cc: In function 'PyObject* > >> > > _wrap_simulate(PyObject*, PyObject*)': > >> > > build/ALPHA/python/swig/event_wrap.cc:4590: error: 'argv[0]' may be > >> used > >> > > uninitialized in this function > >> > > scons: *** [build/ALPHA/python/swig/event_wrap.fo] Error 1 > >> > > scons: building terminated because of errors. > >> > > > >> > > I went poking around in src/SConscript and I see that we have a > >>mildly > >> > > elaborate setup to (1) use a different build environment for swig > >> files, > >> > > swig_env, so we can have finer-grain control over warnings, and (2) > >> > figure > >> > > out based on the compiler type and version which warnings to > >>suppress. > >> I > >> > > saw that the -Wno-maybe-uninitialized flag was added to swig_env > >>only > >> for > >> > > gcc versions >= 4.7, so I changed that to make it unconditional, > >>only > >> to > >> > > find that that flag isn't even valid for gcc 4.4; I had to use > >> > > -Wno-uninitialized instead. > >> > > > >> > > So I could solve my immediate problem by adding -Wno-uninitialized > >>for > >> > gcc > >> > > 4.4, though I'm not sure if this is really a gcc 4.4 issue or a swig > >> > 1.3.40 > >> > > issue (or maybe it's specific to that particular combination). But > >>in > >> > the > >> > > bigger picture I wonder: why do we really care? We're not going to > >>go > >> > fix > >> > > the swig output to get rid of warnings, and as long as the code > >> compiles > >> > > and runs, we probably don't care about them. Wouldn't it be > >>simpler to > >> > get > >> > > rid of the whole swig_env thing and all the code that sets it up, > >>and > >> > just > >> > > add 'Werror=False' when we compile swig sources? > >> > > > >> > > Steve > >> > > _______________________________________________ > >> > > gem5-dev mailing list > >> > > [email protected] > >> > > http://m5sim.org/mailman/listinfo/gem5-dev > >> > _______________________________________________ > >> > gem5-dev mailing list > >> > [email protected] > >> > http://m5sim.org/mailman/listinfo/gem5-dev > >> > > >> _______________________________________________ > >> gem5-dev mailing list > >> [email protected] > >> http://m5sim.org/mailman/listinfo/gem5-dev > >> > >_______________________________________________ > >gem5-dev mailing list > >[email protected] > >http://m5sim.org/mailman/listinfo/gem5-dev > > > > > -- 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] > http://m5sim.org/mailman/listinfo/gem5-dev > _______________________________________________ gem5-dev mailing list [email protected] http://m5sim.org/mailman/listinfo/gem5-dev
