r224465 - Adding a -Wunused-value warning for expressions with side effects used in an unevaluated expression context, such as sizeof(), or decltype(). Also adds a similar warning when the expression passed to typeid() *is* evaluated, since it is equally likely that the user would expect the expression operand to be unevaluated in that case.

[Aaron Ballman]

On Fri, Jan 2, 2015 at 6:18 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
> On Fri, Jan 2, 2015 at 6:14 PM, Richard Smith <richard at metafoo.co.uk> wrote:
>> On Fri, Jan 2, 2015 at 2:56 PM, Aaron Ballman <aaron at aaronballman.com>
>> wrote:
>>>
>>> On Tue, Dec 23, 2014 at 11:01 AM, Joerg Sonnenberger
>>> <joerg at britannica.bec.de> 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.