On Fri, Jan 2, 2015 at 6:18 PM, Aaron Ballman <[email protected]> wrote: > On Fri, Jan 2, 2015 at 6:14 PM, Richard Smith <[email protected]> wrote: >> On Fri, Jan 2, 2015 at 2:56 PM, Aaron Ballman <[email protected]> >> wrote: >>> >>> On Tue, Dec 23, 2014 at 11:01 AM, Joerg Sonnenberger >>> <[email protected]> wrote: >>> > On Wed, Dec 17, 2014 at 09:57:17PM -0000, Aaron Ballman wrote: >>> >> Author: aaronballman >>> >> Date: Wed Dec 17 15:57:17 2014 >>> >> New Revision: 224465 >>> >> >>> >> URL: http://llvm.org/viewvc/llvm-project?rev=224465&view=rev >>> >> Log: >>> >> Adding a -Wunused-value warning for expressions with side effects used >>> >> in an unevaluated expression context, such as sizeof(), or decltype(). >>> > >>> > I think in the case of sizeof, it is too aggressive. It triggered in >>> > NetBSD's mount on logic like the following: >>> > >>> > char ** volatile argv; >>> > >>> > argv = calloc(count, sizeof(*argv)); >>> > >>> > because the volatile marker supposed makes the *argv have side effects. >>> > It is present in this case, because the function later on uses vfork and >>> > GCC complains about trashing local variables for a function that returns >>> > twice. setjmp would be slightly less obscure. >>> >>> The original patch had volatile reads as not being side-effecting, but >>> Richard desired the current behavior. The specifications are pretty >>> clear in that reads of a volatile value *are* side-effecting, but I >>> originally believed as you did, the above code is pretty idiomatic. >>> >>> > I think it should *not* trigger in this case for two important reasons: >>> > >>> > (1) The sizeof use is completely idiomatic. >>> >>> Agreed. However, the dereference of a volatile value is still >>> side-effecting, and having a side-effecting operation appear in a >>> context where side effects are not evaluated is what this patch was >>> all about. >>> >>> > (2) The only workaround for the warning introduces possible maintainance >>> > costs, as it would require duplicating the type of argv. >>> >>> That strikes me as the bigger reason why the warning should be >>> silenced in this particular case -- it *is* idiomatic code, and the >>> way to silence the warning isn't particularly ideal. >>> >>> > A C programmer should know and expect the memory access to not happen. >>> > I would say this is different from the case Aaron gave on IRC about >>> > sizeof(i++). That's a side effect most would expect to still happen. >>> > To keep the number of exceptions small, I propose the relax the warning >>> > to not trigger on dereference of volatile pointers. >>> >>> I'm not opposed to this, but would like Richard to weigh in. >> >> >> Well, my position on this was "let's try warning on it and see what happens" >> =) >> >> But I don't buy the argument here: a C programmer should know and expect >> side-effects inside sizeof to not happen, whether they're due to 'volatile' >> or an increment (this warning is *supposed* to have false-positives if the >> expression always has a side-effect that the programmer doesn't expect to >> actually happen...). >> >> It seems like the issue is that 'volatile' is sometimes used for reasons >> other than to ensure a side-effect (the example code above isn't a great >> case for this, where it's apparently been used as an attempt to provide >> atomicity/thread safety across a vfork, but there are other reasonable cases >> where it's used as a compiler optimization barrier), so we shouldn't assume >> a volatile load or store is *always* a side-effect. > > I think we should always assume a volatile *store* is a side-effect. ;-) > >> Also, in the general >> case of an access through a volatile-qualified type, we don't actually know >> whether the object itself is volatile-qualified, which affects whether there >> is actually a side-effect. On that basis, I think we should unconditionally >> treat volatile access as the maybe-side-effect case rather than the >> always-side-effect case. > > Okay, I will make that change shortly*.
Change has been applied in r225116. Thanks! ~Aaron > > Thanks! > > ~Aaron > > * Shortly may mean Monday. _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
