Re: [PATCH] Pure and const attributes for builtins

["Joseph S. Myers" <jsm28 at cam dot ac dot uk>]

On Sat, 24 Nov 2001, Roger Sayle wrote:

> > You should be using builtin-attrs.def here to specify default attributes
> > (and specify them for all ISO C99 functions for which these attributes
> > are appropriate, whether or not GCC handles them as built-in functions).
> 
> I'll leave it to the author of builtin-attrs.def to fix the problem :>

What problem?  I'm simply noting that this is a more appropriate way of
adding default attributes.  I haven't made noreturn attributes use it just
because I was trying to clean up where the compiler uses volatility of a
function declaration rather than its type to indicate noreturnness (so
that the attribute could be made strictly one applying to types) but
didn't manage to work out exactly where needed changing in what way.

> Assigning pure and const attributes for system library functions that are
> not builtins takes place in a completely different part of the code and
> is handled differently with respect to things like shadowing function
> prototypes (see my second patch).  For example, a operating system

Actually, I think that the present way of declaring built-in functions is
wrong and they should be handled more like how the default attributes are
now done: when a function is declared (explicitly or implictly), and not
before then, it should be treated as built in if the prototype is
consistent, but they should not be predeclared (except for the __builtin
versions).  (C++ has disabled the non-__builtin versions altogether,
leaving it to the library to use them; for C it is appropriate to have
them but it would be more appropriate to declare them in a lazy fashion
rather than predeclaring them.)

> providing a diagnostic version of a function may well choose not to mark
> it as pure as the implementation isn't pure.  For builtin functions, GCC
> (conceptually) provides the implementation and can therefore assert what
> the implementation's attributes should be.  The "-fno-builtin" can be
> used if GCC's understanding of the builtin is incorrect, but alas there
> is no equivalent for non-builtin library function attributes.

So add such an option!  (Whether global for all such attributes, by
attribute name, by function name, or all three.  Unlike
-fno-builtin-<function>, this would need to be supported for C++ as well
as C since the default attributes are still applied for C++.)

> Alas using -fno-builtin would not be testing whether the builtin-attr.def
> mechanism was working and not whether attributes were correctly being set
> and used on the builtin fdecls.  i.e. this would be testing a different
> area of the code than that updated by this patch.

The aim would be to test the *feature* - that is, that certain
optimisations can occur, based on the new information available from the
constancy and purity, that did not occur before.  Of course tests can be
made for the code as well if you have a suitable way of testing that.

> Reliably testing for constancy and purity of builtin functions is
> difficult.  For example, all of the *abs* versions that are marked
> const never result in a function being called.  Hence the assembly

That's why you'd need to disable the built-in function.  It is still
proper (except with -ffreestanding, when the existing default attributes
are disabled) to use the information about what a function called "abs"  
does when -fno-builtin is specified.

> would need to be checked to see if the implementation was inlined
> once or twice.  And an implementation such as the testcase you propose
> can fall fowl of inlining (where the non-purity is uncovered by the
> compiler, and the function is "correctly" called a second time).

Clearly, don't provide such an inline implementation visible in the
testcase.  The only slight complication might be where testing C99
functions not present on all implementations - but we could just let those
tests fail on systems without the function in libc.

We already have tests that the built-in functions work, so what we need
here is just tests that the information about constancy and purity is
visible.  It may well only be important for those functions which don't
always get optimised - does providing this information for abs actually
improve code at all (unless using -fno-builtin so calling an external abs
function)?

> I promise to look into both fixing builtin-attr and the feasibility
> of testing constancy and purity, but I'd like to hope that they're
> independent of whether these patches get accepted.

Investigating testability is a basic requirement for patch submission; see
contribute.html.  In a case such as this where testcases seem plausibly
possible within the existing test framework, justification for their
omission is definitely needed.

> p.s. you still haven't commented whether the you're happy that the updated
> patch, http://gcc.gnu.org/ml/gcc-patches/2001-11/msg01328.html, fixes your
> previous concerns http://gcc.gnu.org/ml/gcc-patches/2001-11/msg00954.html.
> I wouldn't want a solution to your high priority bug report PR c/3120 to
> be held up because a reviewer thinks you may still be unhappy with it.

I saw no problems with the updated patch and didn't see any need to
comment on it.

-- 
Joseph S. Myers
jsm28@cam.ac.uk