This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


The bulk of this patch touches what I think is considered
the middle-end (attribs.c) so let me include its maintainers,
Ian, Jeff, and Richard:

  https://gcc.gnu.org/ml/gcc-patches/2017-08/msg01087.html

The high-level description and rationale for the change are
here:

  https://gcc.gnu.org/ml/gcc-patches/2017-08/msg00602.html

Thanks
Martin

On 08/17/2017 10:03 AM, Martin Sebor wrote:
Attached is an updated patch with just the minor editorial
tweaks for the issues pointed out by Marek (stray comments
and spaces), and with the C++ and libstdc++ bits removed
and posted separately.  I also added some text to the manual
to clarify the const/pure effects.

I thought quite a bit more about the const/pure attributes we
discussed and tried the approach of warning only on a const
declaration after one with attribute pure has been used, but
otherwise allowing and silently ignoring pure after const.
In the end I decided against it, for a few reasons (most of
which I already mentioned but just to summarize).

First, there is the risk that someone will write code based
on the pure declaration even if there's no intervening call
to the function between it and the const one.  Code tends to
be sloppy, and it's also not uncommon to declare the same
function more than once, for whatever reason.  (The ssa-ccp-2.c
test is an example of the latter.)

Second, there are cases of attribute conflicts that GCC already
points out that are much more benign in their effects (the ones
I know about are always_inline/noinline and cold/hot).  In light
of the risk above it seems only helpful to include const/pure in
the same set.

Third, I couldn't find another pair of attributes that GCC would
deliberately handle this way (silently accept both but prefer one
over the other), and introducing a special case just for these
two seemed like a wart.

Finally, compiling Binutils, GDB, Glkibc, and the Linux kernel
with the enhanced warning didn't turn up any code that does this
sort of thing, either intentionally or otherwise.

With that, I've left the handling unchanged.

I do still have the question whether diagnosing attribute
conflicts under -Wattributes is right.  The manual implies
that -Wattributes is for attributes in the wrong places or
on the wrong entities, and that -Wignored-attributes should
be expected instead when GCC decides to drop one for some
reason.

It is a little unfortunate that many -Wattributes warnings
print just "attribute ignored" (and have done so for years).
I think they should all be enhanced to also print why the
attribute is ignored (e.g., "'packed' attribute on function
declarations ignored/not valid/not supported" or something
like that).  Those that ignore attributes that would
otherwise be valid e.g., because they conflict with other
specifiers of the same attribute but with a different
operand might then be candidate for changing to
-Wignored-attributes.

Thanks
Martin



Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]