Hey! I think you are right. This will not regress any of the cases, because previous behaviour either checked str.size() >= 4 OR didn’t check it at all, but then called .substr(2, str.size() - 4), which would crash. I think, though, that explicitly checking str.size() >= 4 makes sense. See comments below. I will have a patch to fix it up. See comments below.
Adrian > On 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=I7JuuiH4eEYBv1meYlgoiyDdl9yvM1r3kGhI0NE1f2g%3D%0A&s=4eae7d09ce0f4330138bcb73d7c5b39ae009250829081e6ef41033dc71e8bfc4 >>> 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=I7JuuiH4eEYBv1meYlgoiyDdl9yvM1r3kGhI0NE1f2g%3D%0A&s=d7e208b0f19fa5d62b635aab96f22ed209d3ffb85762228e4cd2f87ff1cf1983 >>> >>> ============================================================================== >>> --- 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("__")) { I will make it: + if (AttrName.size() >= 4 && 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 > 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) { Checks size, so is fine. >>> - 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); - 4, so it would crash if shorter than 4 chars >> >>> + 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); >>> - 4, so it would crash if shorter than 4 chars >>> 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=I7JuuiH4eEYBv1meYlgoiyDdl9yvM1r3kGhI0NE1f2g%3D%0A&s=05439cdca48d2211d77529dfccfb6dc62c357b71314bb08784cd50fbdbedabe1 >>> >>> ============================================================================== >>> --- 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=I7JuuiH4eEYBv1meYlgoiyDdl9yvM1r3kGhI0NE1f2g%3D%0A&s=22aeed2be3ecdaf531240f222bc8fca0e439bbea1d5ad3d8602d1ada60780451 >>> >>> ============================================================================== >>> --- 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=I7JuuiH4eEYBv1meYlgoiyDdl9yvM1r3kGhI0NE1f2g%3D%0A&s=1e420d6424a37b153567dc5a4f5c2ffaa2d7e1ecfabc1ceea65e4c8b79a41218 >>> >>> ============================================================================== >>> --- 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=I7JuuiH4eEYBv1meYlgoiyDdl9yvM1r3kGhI0NE1f2g%3D%0A&s=a5ca94f7c36c4d43f7d7b0dcb2908f1d43a794f6c3fb6ddebe040b4f1d1c1273 >> >> _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
