[cfe-dev] __has_attribute behavior with target-specific attributes

[Richard Smith]

[Adding back cfe-dev]

On Fri, Dec 6, 2013 at 10:42 AM, Aaron Ballman <aaron at aaronballman.com>wrote:

> So my plan for making __has_attribute target-aware is to basically get
> rid of TargetAttributeSema.cpp entirely. I'll update Attr.td to
> specify target information for the attributes, and SemaDeclAttr.cpp
> will dispatch target-specific attribute logic the same way it does
> target-non-specific attributes. Just that the common attribute handler
> will filter out target-specific attributes that don't apply to the
> current target. Then I can update HasAttribute's table-generated code
> to pay attention to the current target.
>

That sounds great to me.


> Also of note: HasAttribute currently does the wrong thing with
> underscores. It'll happily tell you __has_attribute(__dllexport__)
> exists when it shouldn't. I'll figure something out for that as well;
> probably by consolidating it with AttributeList::getKind in some way.
>

See bug 15853: in the shorter term, we should probably not be considering
spellings other than GNU spellings in __has_attribute anyway. As Alp points
out there, we will probably need a different approach there to support
other attribute syntaxes.

Long-term, I'd like to completely separate building of an *Attr object from
attaching it to things. When that's done, we could support a general
__is_valid_attribute check by parsing and building a *Attr object, and
seeing if a diagnostic is produced; that'd allow things like
__is_valid_attribute(__attribute__((printf(some_new_syntax, 1, 2)))) to
check for a specific flavour of an attribute.


> Thanks for the feedback!
>
> ~Aaron
>
> On Fri, Dec 6, 2013 at 1:28 PM, Richard Smith <richard at metafoo.co.uk>
> wrote:
> > On Fri, Dec 6, 2013 at 9:33 AM, Aaron Ballman <aaron at aaronballman.com>
> > wrote:
> >>
> >> Currently, the __has_attribute feature merely looks to see whether the
> >> given attribute name is known. It does not care whether the attribute
> >> is known for a particular target. So, for instance, the mips16
> >> attribute is only known to the Mips target, but
> >> __has_attribute(mips16) will return 1 for x86. There is at least one
> >> case in the wild (with chrome tsan) where the assumption was that
> >> __has_attribute was target-aware.
> >>
> >> After discussing this on IRC, the general consensus was that the
> >> current behavior is less than desirable because it requires the author
> >> to write #if target && __has_attribute which kind of defeats the
> >> purpose of __has_attribute in the first place.
> >>
> >> Work can be done to make __has_attribute target-aware, however, it
> >> would then make __has_attribute behave differently than __has_builtin
> >> (for example), so it may not be a desirable direction. It's also a
> >> functional change to a shipping feature. So I am proceeding with
> >> caution.
> >>
> >> As I see it, there are three options:
> >>
> >> 1) Change __has_attribute to be target-aware, update the documentation
> >> regarding the new functionality.
> >> 2) Leave the feature as-is, and update the documentation to be clear
> >> that __has_attribute is not target-aware.
> >> 3) Leave everything as it stands (including documentation) so we can
> >> change our minds in the future. I don't much care for this option.
> >>
> >> I am wondering what the proper direction to proceed is.
> >
> >
> > The general principle of __has_* is that they tell you about the current
> > state of the compiler (hence __has_feature and __has_extension will
> produce
> > different answers based on the current language mode). We do this even in
> > crazy cases like:
> >
> >   int a = __has_builtin(printf); // a == 1
> >   extern "C" void printf();
> >   int b = __has_builtin(printf); // b == 0
> >
> > I'm not sure what you mean about __has_builtin, by the way: that already
> > does the right thing for target-specific attributes.
> >
> >   int a = __has_builtin(__builtin_ia32_femms); // a == 1 on x86 and 0
> > elsewhere
> >
> > So I think the right thing is:
> >
> >   4) Fix __has_attribute to be target-aware, leave the documentation
> alone.
> >
> > Essentially, I think this is just a bug: __has_attribute was giving a
> false
> > negative for supported attributes in some cases.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-dev/attachments/20131206/318c7ddd/attachment.html>