Re: [PATCH 1/3] improve detection of attribute conflicts (PR 81544)

[Martin Sebor <msebor@gmail.com>]

On 08/09/2017 01:55 PM, Joseph Myers wrote:
> On Wed, 9 Aug 2017, Martin Sebor wrote:
>
>> the same function with the other of this pair attributes.  I'd
>> also be okay with not diagnosing this combination if I could
>> convince myself that it's safe (or can be made safe) and treated
>> consistently.
>
> I'd expect it to be safe; it might simply happen that some calls are
> optimized based on incomplete information (and even that is doubtful, as
> all optimizations except front-end folding should be taking place after
> all the declarations have been seen).

The following is the example I have in mind.  It compiles silently
without the patch but aborts at runtime.  By warning. the patch
makes it clear that the pure attribute is ignored.  (If/when GCC
starts warning about attribute const violations it will also point
out the global read, but that will help only if the definition sees
the const declaration.)

The problem isn't that the declarations aren't merged at the call
site but rather that the middle-end gives const precedence over
pure so when both attributes are provided the former wins.

   int x;

   int __attribute__ ((const, noclone, noinline)) f (int);

   int main (void)
   {
       int i = f (0);
       x = 7;
       i += f (0);
       if (i != 7) __builtin_abort ();
   }

   int __attribute__ ((pure)) f (int i) { return x + i; }

>
>> The problem whose subset the diagnostic detects is declaring
>> the function const and calling it before the pure declaration
>> that corresponds to its definition is seen.  I.e., the inverse
>> of what the ssa-ccp-2.c test does.  I think a mismatch between
>> the attributes is as suspect as a mismatch in the function's
>> signature.
>
> In the ssa-ccp-2.c case, it's not clear to me the test intends to use the
> same function at all; maybe one of the foo9 functions should be renamed.

I think you're right.  It's likely that the intent is to verify
that a call to either a pure or a const function doesn't defeat
constant propagation.  Let me fix the test.  (This also seems
to be an example where the warning has already proved to be
useful -- it has pointed out a weakness in the test.)

> I think it's actually like having a non-prototype declaration and a
> prototype one, where a composite type is formed from two compatible types.
> (We have a warning in the very specific case of a non-prototype
> *definition* followed by a prototype declaration.)
>
>> In any event, it sounds to me that conceptually, it might be
>> useful to be able to specify which of a pair of mutually
>> exclusive (or redundant) attributes to retain and/or accept:
>> the first or the second, and not just whether to accept or
>> drop the most recent one.  That will make it possible to make
>> the most appropriate decision about each specific pair of
>> attributes without impacting any of the others.
>
> If they conflict, I'm not sure there's any basis for making a choice
> specific to a particular pair of attributes.

I'm not sure either because I haven't done a complete survey.
What I do know is that some conflicts are currently ignored
with both attributes accepted (sometimes leading to confusing
symptoms later), others cause hard errors, and others cause
warnings.  This varies depending on whether the conflict
is on the same declaration or on two distinct ones.  So I'm
thinking that generalizing the machinery to make it possible
to express some or most of these scenarios will make it easy
to apply the patch and use it as is most appropriate in each
situation.

>
> In cases like const and pure where one is a subset of the other, it makes
> sense to choose based on the pair of attributes - and not necessarily to
> warn under the same conditions as the warnings for conflicting attributes.

One way to deal with this case might be to warn only if pure
is seen on an already used const function.

Let me think about it some more, work up a new patch, and post
it for consideration.

Martin