nothrow and const Attribtues

[Aaron Ballman]

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