On Fri, Oct 9, 2015 at 10:37 AM, Aaron Ballman <aa...@aaronballman.com> wrote:
> On Fri, Oct 9, 2015 at 8:53 AM, Aaron Ballman <aa...@aaronballman.com> wrote:
>> On Thu, Oct 8, 2015 at 6:10 PM, Richard Smith <rich...@metafoo.co.uk> wrote:
>>> On Thu, Oct 8, 2015 at 2:59 PM, Adrian Zgorzalek <a...@fb.com> wrote:
>>>>
>>>> Same story:
>>>> warning: 'ownership_takes' attribute only applies to functions
>>>> [-Wignored-attributes]
>>>> __attribute__((ownership_takes(__))) void f();
>>>> ^
>>>
>>>
>>> Oh, I see, you're building in C, and the diagnostic here is broken (we give
>>> the "only applies to functions" diagnostic when it's applied to a function
>>> without a prototype). =( Aaron, can I tempt you to fix that? ;)
>>
>> I'll look into that one. :-)
>
> I've corrected the issue, but am not overly enamored with the
> diagnostic wording. If you can think of something better, please let
> me know.
I've commit in r252055; if we think of better wording, we can modify it later.
~Aaron
>
> ~Aaron
>
>>
>> ~Aaron
>>
>>>
>>> Try this one:
>>>
>>> __attribute__((ownership_takes(__, 1))) void f(void*);
>>>>
>>>> On Oct 8, 2015, at 2:52 PM, Richard Smith <rich...@metafoo.co.uk> wrote:
>>>>
>>>> On Thu, Oct 8, 2015 at 2:45 PM, Adrian Zgorzalek <a...@fb.com> wrote:
>>>>>
>>>>>
>>>>> On Oct 8, 2015, at 2:17 PM, Richard Smith <rich...@metafoo.co.uk> wrote:
>>>>>
>>>>> On Thu, Oct 8, 2015 at 2:10 PM, Adrian Zgorzalek via cfe-commits
>>>>> <cfe-commits@lists.llvm.org> wrote:
>>>>>>
>>>>>> You are right, my bad, I thought this if covers all the cases, but <foo>
>>>>>> part could be empty.
>>>>>>
>>>>>> Here is the fix
>>>>>
>>>>>
>>>>> Please add a testcase ("__attribute__((ownership_takes(__))) void f();"
>>>>> maybe?).
>>>>>
>>>>> I tried different attributes but none of them triggers the assert though.
>>>>> They all spit out warning:
>>>>> warning: 'ownership_takes' attribute only applies to functions
>>>>> [-Wignored-attributes]
>>>>> __attribute__((ownership_takes(__))) f();
>>>>> ^
>>>>
>>>>
>>>> You missed the 'void'.
>>>>
>>>>>
>>>>> Do you have some other idea?
>>>>>
>>>>>
>>>>>
>>>>> The '&&' should go at the end of the previous line.
>>>>>
>>>>>> Adrian
>>>>>> > On Oct 8, 2015, at 1:52 PM, Aaron Ballman <aa...@aaronballman.com>
>>>>>> > wrote:
>>>>>> >
>>>>>> > On Thu, Oct 8, 2015 at 4:49 PM, Richard Smith <rich...@metafoo.co.uk>
>>>>>> > wrote:
>>>>>> >> On Thu, Oct 8, 2015 at 1:21 PM, Aaron Ballman
>>>>>> >> <aa...@aaronballman.com>
>>>>>> >> wrote:
>>>>>> >>>
>>>>>> >>> On Thu, Oct 8, 2015 at 4:16 PM, Richard Smith
>>>>>> >>> <rich...@metafoo.co.uk>
>>>>>> >>> wrote:
>>>>>> >>>> On Thu, Oct 8, 2015 at 12:24 PM, Aaron Ballman via cfe-commits
>>>>>> >>>> <cfe-commits@lists.llvm.org> wrote:
>>>>>> >>>>>
>>>>>> >>>>> Author: aaronballman
>>>>>> >>>>> Date: Thu Oct 8 14:24:08 2015
>>>>>> >>>>> New Revision: 249721
>>>>>> >>>>>
>>>>>> >>>>> URL:
>>>>>> >>>>> https://urldefense.proofpoint.com/v1/url?u=http://llvm.org/viewvc/llvm-project?rev%3D249721%26view%3Drev&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=ZpIyPFH5RmN0EBF%2B6Om2Hg%3D%3D%0A&m=kMsJztGqfQof%2FUicfGEXM78GJV6jd7CTOxlypvgU10A%3D%0A&s=8862f3210cdb7661e90a0d8588334d3e1cf64e92851d05bbcd2b159daf05c7dd
>>>>>> >>>>> Log:
>>>>>> >>>>> When mapping no_sanitize_* attributes to no_sanitize attributes,
>>>>>> >>>>> handle
>>>>>> >>>>> GNU-style formatting that involves prefix and suffix underscores.
>>>>>> >>>>> Cleans up
>>>>>> >>>>> other usages of similar functionality.
>>>>>> >>>>>
>>>>>> >>>>> Patch by Adrian Zgorzalek!
>>>>>> >>>>>
>>>>>> >>>>> Modified:
>>>>>> >>>>> cfe/trunk/lib/Sema/SemaDeclAttr.cpp
>>>>>> >>>>> cfe/trunk/test/SemaCXX/attr-no-sanitize-address.cpp
>>>>>> >>>>> cfe/trunk/test/SemaCXX/attr-no-sanitize-memory.cpp
>>>>>> >>>>> cfe/trunk/test/SemaCXX/attr-no-sanitize-thread.cpp
>>>>>> >>>>>
>>>>>> >>>>> Modified: cfe/trunk/lib/Sema/SemaDeclAttr.cpp
>>>>>> >>>>> URL:
>>>>>> >>>>>
>>>>>> >>>>>
>>>>>> >>>>> https://urldefense.proofpoint.com/v1/url?u=http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaDeclAttr.cpp?rev%3D249721%26r1%3D249720%26r2%3D249721%26view%3Ddiff&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=ZpIyPFH5RmN0EBF%2B6Om2Hg%3D%3D%0A&m=kMsJztGqfQof%2FUicfGEXM78GJV6jd7CTOxlypvgU10A%3D%0A&s=7dae8eb5cffae4493f0a6c26033810564fd7b688297fc93106a3b980f76cdd9f
>>>>>> >>>>>
>>>>>> >>>>>
>>>>>> >>>>>
>>>>>> >>>>> ==============================================================================
>>>>>> >>>>> --- cfe/trunk/lib/Sema/SemaDeclAttr.cpp (original)
>>>>>> >>>>> +++ cfe/trunk/lib/Sema/SemaDeclAttr.cpp Thu Oct 8 14:24:08 2015
>>>>>> >>>>> @@ -1308,6 +1308,17 @@ void Sema::AddAssumeAlignedAttr(SourceRa
>>>>>> >>>>> AssumeAlignedAttr(AttrRange, Context, E, OE,
>>>>>> >>>>> SpellingListIndex));
>>>>>> >>>>> }
>>>>>> >>>>>
>>>>>> >>>>> +/// Normalize the attribute, __foo__ becomes foo.
>>>>>> >>>>> +/// Returns true if normalization was applied.
>>>>>> >>>>> +static bool normalizeName(StringRef &AttrName) {
>>>>>> >>>>> + if (AttrName.startswith("__") && AttrName.endswith("__")) {
>>>>>> >>>>> + assert(AttrName.size() > 4 && "Name too short");
>>>>>> >>>>
>>>>>> >>>>
>>>>>> >>>> This assert will fire on the strings __, ___, and ____, which are
>>>>>> >>>> valid
>>>>>> >>>> in
>>>>>> >>>> some of the below cases.
>>>>>> >>>
>>>>>> >>> That assert won't fire on anything but ____ because it's &&, not ||.
>>>>>> >>
>>>>>> >>
>>>>>> >> I disagree. __ starts with __ and ends with __. The right thing to do
>>>>>> >> here
>>>>>> >> is remove the assert and put back the AttrName.size() > 4 check that
>>>>>> >> the
>>>>>> >> callers used to have.
>>>>>> >
>>>>>> > Hah, you are correct. I hadn't considered that point. I agree with
>>>>>> > you. :-)
>>>>>> >
>>>>>> > ~Aaron
>>>>>> >
>>>>>> >>
>>>>>> >>> I
>>>>>> >>> don't think these names were intended to be valid in their uses.
>>>>>> >>> However, you are correct that this will trigger assertions instead
>>>>>> >>> of
>>>>>> >>> diagnostics. Adrian, can you investigate?
>>>>>> >>>
>>>>>> >>>>
>>>>>> >>>>>
>>>>>> >>>>> + AttrName = AttrName.drop_front(2).drop_back(2);
>>>>>> >>>>> + return true;
>>>>>> >>>>> + }
>>>>>> >>>>> + return false;
>>>>>> >>>>> +}
>>>>>> >>>>> +
>>>>>> >>>>> static void handleOwnershipAttr(Sema &S, Decl *D, const
>>>>>> >>>>> AttributeList
>>>>>> >>>>> &AL) {
>>>>>> >>>>> // This attribute must be applied to a function declaration. The
>>>>>> >>>>> first
>>>>>> >>>>> // argument to the attribute must be an identifier, the name of
>>>>>> >>>>> the
>>>>>> >>>>> resource,
>>>>>> >>>>> @@ -1349,11 +1360,8 @@ static void handleOwnershipAttr(Sema &S,
>>>>>> >>>>>
>>>>>> >>>>> IdentifierInfo *Module = AL.getArgAsIdent(0)->Ident;
>>>>>> >>>>>
>>>>>> >>>>> - // Normalize the argument, __foo__ becomes foo.
>>>>>> >>>>> StringRef ModuleName = Module->getName();
>>>>>> >>>>> - if (ModuleName.startswith("__") && ModuleName.endswith("__") &&
>>>>>> >>>>> - ModuleName.size() > 4) {
>>>>>> >>>>> - ModuleName = ModuleName.drop_front(2).drop_back(2);
>>>>>> >>>>> + if (normalizeName(ModuleName)) {
>>>>>> >>>>> Module = &S.PP.getIdentifierTable().get(ModuleName);
>>>>>> >>>>> }
>>>>>> >>>>>
>>>>>> >>>>> @@ -2648,9 +2656,7 @@ static void handleFormatAttr(Sema &S, De
>>>>>> >>>>> IdentifierInfo *II = Attr.getArgAsIdent(0)->Ident;
>>>>>> >>>>> StringRef Format = II->getName();
>>>>>> >>>>>
>>>>>> >>>>> - // Normalize the argument, __foo__ becomes foo.
>>>>>> >>>>> - if (Format.startswith("__") && Format.endswith("__")) {
>>>>>> >>>>> - Format = Format.substr(2, Format.size() - 4);
>>>>>> >>>>> + if (normalizeName(Format)) {
>>>>>> >>>>> // If we've modified the string name, we need a new identifier
>>>>>> >>>>> for
>>>>>> >>>>> it.
>>>>>> >>>>> II = &S.Context.Idents.get(Format);
>>>>>> >>>>> }
>>>>>> >>>>> @@ -3131,9 +3137,7 @@ static void handleModeAttr(Sema &S, Decl
>>>>>> >>>>> IdentifierInfo *Name = Attr.getArgAsIdent(0)->Ident;
>>>>>> >>>>> StringRef Str = Name->getName();
>>>>>> >>>>>
>>>>>> >>>>> - // Normalize the attribute name, __foo__ becomes foo.
>>>>>> >>>>> - if (Str.startswith("__") && Str.endswith("__"))
>>>>>> >>>>> - Str = Str.substr(2, Str.size() - 4);
>>>>>> >>>>> + normalizeName(Str);
>>>>>> >>>>>
>>>>>> >>>>> unsigned DestWidth = 0;
>>>>>> >>>>> bool IntegerMode = true;
>>>>>> >>>>> @@ -4533,8 +4537,10 @@ static void handleNoSanitizeAttr(Sema &S
>>>>>> >>>>>
>>>>>> >>>>> static void handleNoSanitizeSpecificAttr(Sema &S, Decl *D,
>>>>>> >>>>> const AttributeList
>>>>>> >>>>> &Attr) {
>>>>>> >>>>> + StringRef AttrName = Attr.getName()->getName();
>>>>>> >>>>> + normalizeName(AttrName);
>>>>>> >>>>> std::string SanitizerName =
>>>>>> >>>>> - llvm::StringSwitch<std::string>(Attr.getName()->getName())
>>>>>> >>>>> + llvm::StringSwitch<std::string>(AttrName)
>>>>>> >>>>> .Case("no_address_safety_analysis", "address")
>>>>>> >>>>> .Case("no_sanitize_address", "address")
>>>>>> >>>>> .Case("no_sanitize_thread", "thread")
>>>>>> >>>>
>>>>>> >>>>
>>>>>> >>>> Is there any way we could use the spelling list index in this case
>>>>>> >>>> rather
>>>>>> >>>> than repeating the attribute names and __-stripping here?
>>>>>> >>>
>>>>>> >>> The spelling list index isn't exposed in a meaningful way (and I
>>>>>> >>> think
>>>>>> >>> that would be an abuse of it; I want to someday remove that
>>>>>> >>> implementation detail to something far more private).
>>>>>> >>>
>>>>>> >>> I was hoping there would be a way to use the semantic spelling, but
>>>>>> >>> the issue is that this particular attribute doesn't have semantic
>>>>>> >>> spellings. The NoSanitizeSpecificAttr would have one, but it has no
>>>>>> >>> AST node to hang those off of. Since we explicitly document that we
>>>>>> >>> do
>>>>>> >>> not want any additional names added to this list (see Attr.td line
>>>>>> >>> 1500), I think this is a reasonable solution as-is.
>>>>>> >>>
>>>>>> >>> ~Aaron
>>>>>> >>>
>>>>>> >>>>
>>>>>> >>>>> Modified: cfe/trunk/test/SemaCXX/attr-no-sanitize-address.cpp
>>>>>> >>>>> URL:
>>>>>> >>>>>
>>>>>> >>>>>
>>>>>> >>>>> https://urldefense.proofpoint.com/v1/url?u=http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-no-sanitize-address.cpp?rev%3D249721%26r1%3D249720%26r2%3D249721%26view%3Ddiff&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=ZpIyPFH5RmN0EBF%2B6Om2Hg%3D%3D%0A&m=kMsJztGqfQof%2FUicfGEXM78GJV6jd7CTOxlypvgU10A%3D%0A&s=46b5171e8784900ffa959a20f8824d63377f6d80d0c92c69d5a271edb5d83930
>>>>>> >>>>>
>>>>>> >>>>>
>>>>>> >>>>>
>>>>>> >>>>> ==============================================================================
>>>>>> >>>>> --- cfe/trunk/test/SemaCXX/attr-no-sanitize-address.cpp (original)
>>>>>> >>>>> +++ cfe/trunk/test/SemaCXX/attr-no-sanitize-address.cpp Thu Oct 8
>>>>>> >>>>> 14:24:08 2015
>>>>>> >>>>> @@ -5,12 +5,14 @@
>>>>>> >>>>> #if !__has_attribute(no_sanitize_address)
>>>>>> >>>>> #error "Should support no_sanitize_address"
>>>>>> >>>>> #endif
>>>>>> >>>>> -
>>>>>> >>>>> -void noanal_fun() NO_SANITIZE_ADDRESS;
>>>>>> >>>>> -
>>>>>> >>>>> -void noanal_fun_args() __attribute__((no_sanitize_address(1)));
>>>>>> >>>>> // \
>>>>>> >>>>> - // expected-error {{'no_sanitize_address' attribute takes no
>>>>>> >>>>> arguments}}
>>>>>> >>>>> -
>>>>>> >>>>> +
>>>>>> >>>>> +void noanal_fun() NO_SANITIZE_ADDRESS;
>>>>>> >>>>> +
>>>>>> >>>>> +void noanal_fun_alt() __attribute__((__no_sanitize_address__));
>>>>>> >>>>> +
>>>>>> >>>>> +void noanal_fun_args() __attribute__((no_sanitize_address(1)));
>>>>>> >>>>> // \
>>>>>> >>>>> + // expected-error {{'no_sanitize_address' attribute takes no
>>>>>> >>>>> arguments}}
>>>>>> >>>>> +
>>>>>> >>>>> int noanal_testfn(int y) NO_SANITIZE_ADDRESS;
>>>>>> >>>>>
>>>>>> >>>>> int noanal_testfn(int y) {
>>>>>> >>>>>
>>>>>> >>>>> Modified: cfe/trunk/test/SemaCXX/attr-no-sanitize-memory.cpp
>>>>>> >>>>> URL:
>>>>>> >>>>>
>>>>>> >>>>>
>>>>>> >>>>> https://urldefense.proofpoint.com/v1/url?u=http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-no-sanitize-memory.cpp?rev%3D249721%26r1%3D249720%26r2%3D249721%26view%3Ddiff&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=ZpIyPFH5RmN0EBF%2B6Om2Hg%3D%3D%0A&m=kMsJztGqfQof%2FUicfGEXM78GJV6jd7CTOxlypvgU10A%3D%0A&s=676ca5d3b6bef5b3c04a9d6861c547a35afb54f85454fc23551da843ce369b1c
>>>>>> >>>>>
>>>>>> >>>>>
>>>>>> >>>>>
>>>>>> >>>>> ==============================================================================
>>>>>> >>>>> --- cfe/trunk/test/SemaCXX/attr-no-sanitize-memory.cpp (original)
>>>>>> >>>>> +++ cfe/trunk/test/SemaCXX/attr-no-sanitize-memory.cpp Thu Oct 8
>>>>>> >>>>> 14:24:08
>>>>>> >>>>> 2015
>>>>>> >>>>> @@ -5,12 +5,14 @@
>>>>>> >>>>> #if !__has_attribute(no_sanitize_memory)
>>>>>> >>>>> #error "Should support no_sanitize_memory"
>>>>>> >>>>> #endif
>>>>>> >>>>> -
>>>>>> >>>>> -void noanal_fun() NO_SANITIZE_MEMORY;
>>>>>> >>>>> -
>>>>>> >>>>> -void noanal_fun_args() __attribute__((no_sanitize_memory(1))); //
>>>>>> >>>>> \
>>>>>> >>>>> - // expected-error {{'no_sanitize_memory' attribute takes no
>>>>>> >>>>> arguments}}
>>>>>> >>>>> -
>>>>>> >>>>> +
>>>>>> >>>>> +void noanal_fun() NO_SANITIZE_MEMORY;
>>>>>> >>>>> +
>>>>>> >>>>> +void noanal_fun_alt() __attribute__((__no_sanitize_memory__));
>>>>>> >>>>> +
>>>>>> >>>>> +void noanal_fun_args() __attribute__((no_sanitize_memory(1))); //
>>>>>> >>>>> \
>>>>>> >>>>> + // expected-error {{'no_sanitize_memory' attribute takes no
>>>>>> >>>>> arguments}}
>>>>>> >>>>> +
>>>>>> >>>>> int noanal_testfn(int y) NO_SANITIZE_MEMORY;
>>>>>> >>>>>
>>>>>> >>>>> int noanal_testfn(int y) {
>>>>>> >>>>>
>>>>>> >>>>> Modified: cfe/trunk/test/SemaCXX/attr-no-sanitize-thread.cpp
>>>>>> >>>>> URL:
>>>>>> >>>>>
>>>>>> >>>>>
>>>>>> >>>>> https://urldefense.proofpoint.com/v1/url?u=http://llvm.org/viewvc/llvm-project/cfe/trunk/test/SemaCXX/attr-no-sanitize-thread.cpp?rev%3D249721%26r1%3D249720%26r2%3D249721%26view%3Ddiff&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=ZpIyPFH5RmN0EBF%2B6Om2Hg%3D%3D%0A&m=kMsJztGqfQof%2FUicfGEXM78GJV6jd7CTOxlypvgU10A%3D%0A&s=30f400a0be1c699374d1f30504fa1f190158506f984bbeac66fe6998be9fd7b7
>>>>>> >>>>>
>>>>>> >>>>>
>>>>>> >>>>>
>>>>>> >>>>> ==============================================================================
>>>>>> >>>>> --- cfe/trunk/test/SemaCXX/attr-no-sanitize-thread.cpp (original)
>>>>>> >>>>> +++ cfe/trunk/test/SemaCXX/attr-no-sanitize-thread.cpp Thu Oct 8
>>>>>> >>>>> 14:24:08
>>>>>> >>>>> 2015
>>>>>> >>>>> @@ -5,12 +5,14 @@
>>>>>> >>>>> #if !__has_attribute(no_sanitize_thread)
>>>>>> >>>>> #error "Should support no_sanitize_thread"
>>>>>> >>>>> #endif
>>>>>> >>>>> -
>>>>>> >>>>> -void noanal_fun() NO_SANITIZE_THREAD;
>>>>>> >>>>> -
>>>>>> >>>>> -void noanal_fun_args() __attribute__((no_sanitize_thread(1))); //
>>>>>> >>>>> \
>>>>>> >>>>> - // expected-error {{'no_sanitize_thread' attribute takes no
>>>>>> >>>>> arguments}}
>>>>>> >>>>> -
>>>>>> >>>>> +
>>>>>> >>>>> +void noanal_fun() NO_SANITIZE_THREAD;
>>>>>> >>>>> +
>>>>>> >>>>> +void noanal_fun_alt() __attribute__((__no_sanitize_thread__));
>>>>>> >>>>> +
>>>>>> >>>>> +void noanal_fun_args() __attribute__((no_sanitize_thread(1))); //
>>>>>> >>>>> \
>>>>>> >>>>> + // expected-error {{'no_sanitize_thread' attribute takes no
>>>>>> >>>>> arguments}}
>>>>>> >>>>> +
>>>>>> >>>>> int noanal_testfn(int y) NO_SANITIZE_THREAD;
>>>>>> >>>>>
>>>>>> >>>>> int noanal_testfn(int y) {
>>>>>> >>>>>
>>>>>> >>>>>
>>>>>> >>>>> _______________________________________________
>>>>>> >>>>> cfe-commits mailing list
>>>>>> >>>>> cfe-commits@lists.llvm.org
>>>>>> >>>>>
>>>>>> >>>>> https://urldefense.proofpoint.com/v1/url?u=http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits&k=ZVNjlDMF0FElm4dQtryO4A%3D%3D%0A&r=ZpIyPFH5RmN0EBF%2B6Om2Hg%3D%3D%0A&m=kMsJztGqfQof%2FUicfGEXM78GJV6jd7CTOxlypvgU10A%3D%0A&s=76daddf3012e1755f9df59be7e195c26e1d2179d81b412635ff70771ec3f1ed5
>>>>>> >>>>
>>>>>> >>>>
>>>>>> >>
>>>>>> >>
>>>>>>
>>>>>>
>>>>>> _______________________________________________
>>>>>> cfe-commits mailing list
>>>>>> cfe-commits@lists.llvm.org
>>>>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>>>>>
>>>>>
>>>>
>>>>
>>>
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
