nothrow and const Attribtues

[Douglas Gregor]

On Dec 11, 2013, at 9:34 AM, 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.

I'm generally in favor of anything that makes more of the attribute system declarative, and I think this is a good candidate. It's reasonable to have the bit to handle the duplication checks for simple attributes (where the attribute argument matters), then have Sema cope with de-duplication for the more interesting attributes.


>> FWIW, using the following patch (which is what I was proposing
>> originally),

This patch looks fine as well; it's an improvement over the status quo.

	- Doug


>> 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).