__builtin_assume: constexpr conversion to bool "has side effects"

[Quuxplusone]

Bugzilla Link	PR32424
Status	NEW
Importance	P normal
Reported by	Casey Carter (Casey@Carter.net)
Reported on	2017-03-25 14:14:49 -0700
Last modified on	2017-04-03 16:30:33 -0700
Version	trunk
Hardware	PC Linux
CC	dgregor@apple.com, hfinkel@anl.gov, llvm-bugs@lists.llvm.org, richard-llvm@metafoo.co.uk
Fixed by commit(s)
Attachments	file_32424.txt (164 bytes, text/plain)
Blocks
Blocked by
See also

Created attachment 18171
Minimal repro

This program:

  struct S {
    static constexpr bool value = false;
    constexpr operator bool () const noexcept { return value; }
  };

  int main() { __builtin_assume(S()); }

compiled with "-std=c++11 -Wassume" incorrectly diagnoses:

  prog.cc:6:31: warning: the argument to '__builtin_assume' has side effects that will be discarded [-Wassume]
  int main() { __builtin_assume(S()); }
                                ^~~

(See https://wandbox.org/permlink/yBxuUORdvKQYf5Ri)

[Quuxplusone]

Attached file_32424.txt (164 bytes, text/plain): Minimal repro

[Quuxplusone]

The fact that the conversion function is constexpr does not imply that it
cannot have side effects. We could in principle attempt to evaluate the
expression before emitting this warning, but we'd still emit a warning on a
case like:

  int main(int argc, const char**) { __builtin_assume(S{argc == 1}); }

... because in that case the argument is not a constant expression.

One thing we could do here would be to reword the diagnostic from "has side
effects" to "may have side effects" in the case where we've not proven a side-
effect exists. Another would be to not consider a function call to qualify as
"probably has side effects" -- but basing that determination on whether the
function is constexpr seems like an arbitrary choice, since constexpr and side-
effects are largely orthogonal.

[Quuxplusone]

(In reply to Richard Smith from comment #1)
> The fact that the conversion function is constexpr does not imply that it
> cannot have side effects. We could in principle attempt to evaluate the
> expression before emitting this warning, but we'd still emit a warning on a
> case like:
>
>   int main(int argc, const char**) { __builtin_assume(S{argc == 1}); }
>
> ... because in that case the argument is not a constant expression.

My intent here - which the bug report admittedly does not even begin to express
clearly - is that __builtin_assume should not diagnose that its argument "has
side effects that will be discarded" when the argument is a constant expression.

>
> One thing we could do here would be to reword the diagnostic from "has side
> effects" to "may have side effects" in the case where we've not proven a
> side-effect exists. Another would be to not consider a function call to
> qualify as "probably has side effects" -- but basing that determination on
> whether the function is constexpr seems like an arbitrary choice, since
> constexpr and side-effects are largely orthogonal.

My reaction to "has side effects" was to believe that I'd committed a horrible
mistake somewhere, and that I needed to track down this inadvertent side
effect. I was surprised to discover that even in the case where the argument is
a constant expression which cannot have side effects, it apparently did in fact
have side effects. "possibly has side effects" would provide a clear indication
that the compiler did not prove or disprove the existence of a side effect and
that the occurrence needs warning suppression / reformulation of the expression.

[Quuxplusone]

(In reply to Casey Carter from comment #2)
> (In reply to Richard Smith from comment #1)
> > The fact that the conversion function is constexpr does not imply that it
> > cannot have side effects. We could in principle attempt to evaluate the
> > expression before emitting this warning, but we'd still emit a warning on a
> > case like:
> >
> >   int main(int argc, const char**) { __builtin_assume(S{argc == 1}); }
> >
> > ... because in that case the argument is not a constant expression.
>
> My intent here - which the bug report admittedly does not even begin to
> express clearly - is that __builtin_assume should not diagnose that its
> argument "has side effects that will be discarded" when the argument is a
> constant expression.
>
> >
> > One thing we could do here would be to reword the diagnostic from "has side
> > effects" to "may have side effects" in the case where we've not proven a
> > side-effect exists. Another would be to not consider a function call to
> > qualify as "probably has side effects" -- but basing that determination on
> > whether the function is constexpr seems like an arbitrary choice, since
> > constexpr and side-effects are largely orthogonal.
>
> My reaction to "has side effects" was to believe that I'd committed a
> horrible mistake somewhere, and that I needed to track down this inadvertent
> side effect. I was surprised to discover that even in the case where the
> argument is a constant expression which cannot have side effects, it
> apparently did in fact have side effects. "possibly has side effects" would
> provide a clear indication that the compiler did not prove or disprove the
> existence of a side effect and that the occurrence needs warning suppression
> / reformulation of the expression.

I'll also note that whenever that warning is generated, the compiler will
ignore the assumption. However we word the message, that should remain clear.