Re: bug#36370: 27.0.50; XFIXNAT called on negative numbers

[Pip Cet]

On Sat, Jun 29, 2019 at 10:31 AM Bruno Haible <address@hidden> wrote:
> Pip Cet wrote:
> > I would like to state that the current assume does
> > behave very badly when combined with -fno-inline-small-functions
> > -fno-inline.
>
> Since we can't address this limitation through an acceptable change
> to the 'assume' macro, we need to address it through documentation.

I agree. There's a limitation bad enough for me to consider it a bug
in current GCC, and even if that's lifted tomorrow, it wouldn't be
backported so it would be quite a while until I'd ask you to
reconsider the matter.

> > marking it
> > as __attribute__((always_inline)) is problematic because it might
> > directly contradict what the programmer was trying to achieve by
> > passing -fno-inline.
>
> __attribute__((always_inline)) exists precisely to make a distinction
> between functions where inlining is usually desirable vs. functions
> where inlining is essential.

Indeed. In this case, it's usually desirable but not essential for the
correctness of the program, IMHO. I understand you disagree.

> We don't need to warn against the uses
> of __attribute__((always_inline)) -- confusing behaviour in the debugger
> etc. -- becauses these drawbacks are already well-known.

I disagree completely. There's no warning in the GCC documentation for
the attribute.

> As a user of the 'assume' macro, I want a definitive statement about what
> I need to provide so that the macro works in the sense of improved 
> performance.
> A statement about "often" is too vague.

I agree.


> How about this proposed patch?
>
> diff --git a/lib/verify.h b/lib/verify.h
> index f8e4eff..ed1ba19 100644
> --- a/lib/verify.h
> +++ b/lib/verify.h
> @@ -261,7 +261,10 @@ template <int w>
>
>  /* Assume that R always holds.  This lets the compiler optimize
>     accordingly.  R should not have side-effects; it may or may not be
> -   evaluated.  Behavior is undefined if R is false.  */
> +   evaluated.  The behavior is undefined if R is false.
> +   If you want the use of this macro to improve, not deteriorate,
> +   performance, R should not contain function calls except to functions
> +   that are declared 'inline __attribute__((__always_inline__))'.  */

My suggestion would be "Assume that R always holds.  This lets the
compiler optimize accordingly.  Behavior is undefined if R is false,
fails to evaluate, or has side effects.  Performance will suffer if R
contains function calls that are not inlined at compile time."

That would describe the API as I understand you and Paul think it
always has been; I think it describes a drastically stricter API than
what the old comment did. As a reminder, my starting point was that I
wanted to make the API more lenient. So, obviously, I disagree with
the API change but it is more important that there's consensus on what
the API actually is than it is to have a good API.