nothrow and const Attribtues

[Aaron Ballman]

Ping

On Wed, Dec 11, 2013 at 12:34 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
> Ping
>
> On Mon, Dec 2, 2013 at 8:50 PM, Aaron Ballman <aaron at aaronballman.com> wrote:
>> 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).