nothrow and const Attribtues

[Aaron Ballman]

I'm not suggesting removing the dedupe for format -- that is handled
by mergeFormatAttr (which is called from handleFormatAttr to actually
add the attribute to the decl).

But it is an interesting enough question with regards to the other
attributes -- this strikes me as a general attribute problem. Some
attributes are acceptable to have duplicates specified (such as the
thread safety attributes), others are acceptable only if the duplicate
attribute is different (such as the work group attributes), and still
others it is unacceptable to duplicate at all. This makes me wonder if
it's something we could tablegen? Perhaps a bit that is on by default
which allows us to warn if an attribute has already been applied? If
the bit is off, then the semantic handler can do custom checking if
desired, or simply not diagnose it. The downside to this is something
I've run up against already: there's no way to map parsed attributes
to semantic attributes, so there's no easy way to see if an
AttributeList object is already applied to a Decl (esp since there's
not a one-to-one mapping of parsed to semantic attributes). So if this
is an good idea, it's not something that's trivial to implement.

FWIW, using the following patch (which is what I was proposing
originally), it appears as though builtins are still properly merged.
Testcase:

int abs(int) { return 0; }

yields:

TranslationUnitDecl 0x2a2100 <<invalid sloc>>
|-TypedefDecl 0x2a23d0 <<invalid sloc>> __builtin_va_list 'char *'
|-FunctionDecl 0x2a2490 <E:\Aaron Ballman\Desktop\test.c:1:5> abs 'int (int)' ex
tern
| |-ParmVarDecl 0x2a2500 <<invalid sloc>> 'int'
| |-NoThrowAttr 0x2a2540 <col:5>
| `-ConstAttr 0x2a2570 <col:5>
`-FunctionDecl 0x2a2590 prev 0x2a2490 <col:1, col:26> abs 'int (int)'
  |-ParmVarDecl 0x2a2410 <col:9, col:12> 'int'
  |-CompoundStmt 0x2a2688 <col:14, col:26>
  | `-ReturnStmt 0x2a2678 <col:16, col:23>1 error generated.

  |   `-IntegerLiteral 0x2a2658 <col:23> 'int' 0
  |-NoThrowAttr 0x2a2620 <col:5>
  `-ConstAttr 0x2a2640 <col:5>

~Aaron

On Mon, Dec 2, 2013 at 7:33 PM, Richard Smith <richard at metafoo.co.uk> wrote:
> On Mon, Dec 2, 2013 at 2:35 PM, Aaron Ballman <aaron at aaronballman.com>
> wrote:
>>
>> On Mon, Dec 2, 2013 at 5:25 PM, Richard Smith <richard at metafoo.co.uk>
>> wrote:
>> > On Mon, Dec 2, 2013 at 1:28 PM, Aaron Ballman <aaron at aaronballman.com>
>> > wrote:
>> >>
>> >> The nothrow and const attributes have a curious construct that does
>> >> not appear with any other attributes. Both of them check whether the
>> >> attribute has already been applied, and if it has, it silently fails
>> >> to reapply the attribute.  Eg)
>> >>
>> >>   if (NoThrowAttr *Existing = D->getAttr<NoThrowAttr>()) {
>> >>     if (Existing->getLocation().isInvalid())
>> >>       Existing->setRange(Attr.getRange());
>> >>   } else {
>> >>     D->addAttr(::new (S.Context)
>> >>                NoThrowAttr(Attr.getRange(), S.Context,
>> >>                            Attr.getAttributeSpellingListIndex()));
>> >>   }
>> >>
>> >> While this makes some sense, two things in particular are worrisome:
>> >>
>> >> 1) We lose syntactic information because we are losing the as-written
>> >> attribute information. This happens relatively frequently, but any
>> >> time we can cut that down would be good.
>> >> 2) This is not consistently applied across attributes.
>> >>
>> >> I am wondering whether it is reasonable to remove the check for the
>> >> existing attribute from the nothrow and const attributes, and simply
>> >> apply multiple times. Since neither of these attributes accept
>> >> arguments, it seems like this would be a harmless action. However, I
>> >> may not have the full picture; this is the original commit message:
>> >>
>> >> dgregor  6/15/2011 1:45:11 AM
>> >> Don't add redundant FormatAttr, ConstAttr, or NoThrowAttr attributes,
>> >> either imlicitly (for builtins) or explicitly (due to multiple
>> >> specification of the same attributes). Fixes <rdar://problem/9612060>.
>> >>
>> >> (Note, I am not suggesting any modifications for FormatAttr.)
>> >>
>> >> Thanks!
>> >
>> >
>> > Here's the testcase from that change:
>> >
>> > +int printf(const char * restrict, ...) __attribute__((__format__
>> > (__printf__, 1, 2)));
>> > +
>> > +void rdar9612060(void) {
>> > +  printf("%s", 2); // expected-warning{{conversion specifies type 'char
>> > *'
>> > but the argument has type 'int'}}
>> > +}
>> >
>> > It seems the problem that this change was trying to avoid was where an
>> > attribute is implicitly added because the function is a builtin, and
>> > then
>> > explicitly added. I suspect this is redundant when initially building
>> > the
>> > attribute -- mergeDeclAttributes will not propagate the attribute from
>> > the
>> > implicitly-declared builtin onto the explicitly-declared function.
>>
>> That makes sense - you would want to avoid duplicate diagnostics in
>> that case. I believe you are correct that it is redundant. Quick
>> testing by removing the code from const and nothrow handlers and
>> testing demonstrates no regressions. Do you have a better way for me
>> to test this?
>
>
> You could look at the output of -ast-dump and check that there are no
> duplicate attributes.
>
> I wonder what we should do about a case like this:
>
>   void f(const char*, ...) __attribute__((format(printf, 1, 2)))
> __attribute__((format(printf, 1, 2)));
>   void g() { f("%s"); }
>   void f(const char*, ...);
>   void h() { f("%s"); }
>
> I think removing the deduplication will result in g() producing two
> warnings, but h() only producing one (because mergeDeclAttributes will only
> propagate one of them from the earlier declaration to the later one).
-------------- next part --------------
A non-text attachment was scrubbed...
Name: ConstNoThrow.patch
Type: application/octet-stream
Size: 2349 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20131202/a08cf73f/attachment.obj>