[PATCH] Fix ICE in Clang when dealing with attribute(__no_sanitize_*__)

[Aaron Ballman via cfe-commits]

On Thu, Oct 8, 2015 at 12:46 PM, Adrian Zgorzalek <adek at fb.com> wrote:
> Feedback applied, new patch in the attachment.

Thank you for working on this! A few comments:

> From 13f4df6def5f26768f9ea048e013f779bb4a7814 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Adrian=20Zgorza=C5=82ek?= <adek at fb.com>
> Date: Wed, 7 Oct 2015 15:55:53 -0700
> Subject: [PATCH] Fix ICE in Clang when dealing with
>  attribute(__no_sanitize_*__)
>
> Summary:
>
> Both syntaxes: __attribute__((no_sanitize_address)) and
> __attribute__((__no_sanitize__address__)) are valid, following
> documentation:
>
> > The attribute identifier (but not scope) can also be specified with a
> > preceding and following __ (double underscore) to avoid interference
> > from a macro with the same name. For instance, gnu::__const__ can be
> > used instead of gnu::const.
>
> This patch is fixing ICE when __const__ syntax is used.
>
> Test Plan:
>
> Added unittests in the patch which cover these cases. After applying
> this patch they don't crash anymore.
> ---
>  lib/Sema/SemaDeclAttr.cpp                 | 28 +++++++++++++++++-----------
>  test/SemaCXX/attr-no-sanitize-address.cpp |  2 ++
>  test/SemaCXX/attr-no-sanitize-memory.cpp  |  2 ++
>  test/SemaCXX/attr-no-sanitize-thread.cpp  |  2 ++
>  4 files changed, 23 insertions(+), 11 deletions(-)

FWIW, it's usually better to supply svn patches instead of git patches
(my vcs has troubles applying patches like this).

>
> diff --git a/lib/Sema/SemaDeclAttr.cpp b/lib/Sema/SemaDeclAttr.cpp
> index 3cf9567..b2f2cff 100644
> --- a/lib/Sema/SemaDeclAttr.cpp
> +++ b/lib/Sema/SemaDeclAttr.cpp
> @@ -1308,6 +1308,17 @@ void Sema::AddAssumeAlignedAttr(SourceRange AttrRange, Decl *D, Expr *E,
>              AssumeAlignedAttr(AttrRange, Context, E, OE, SpellingListIndex));
>  }
>
> +/// Normalize the attribute, __foo__ becomes foo.
> +/// Returns true if normalization was applied.
> +static bool normalizeAttrName(StringRef& AttrName) {

I would prefer this be called normalizeName since it doesn't just
normalize attribute names (for instance, this is used to normalize
attribute arguments as well).

> +  if (AttrName.startswith("__") && AttrName.endswith("__")) {
> +    assert(AttrName.size() > 4 && "Too short attribute name");

"Name too short" instead.

> +    AttrName = AttrName.substr(2, AttrName.size() - 4);

I prefer: drop_front() and drop_back() instead.

> +    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, Decl *D, const AttributeList &AL) {
>
>    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 (normalizeAttrName(ModuleName)) {
>      Module = &S.PP.getIdentifierTable().get(ModuleName);
>    }
>
> @@ -2648,9 +2656,7 @@ static void handleFormatAttr(Sema &S, Decl *D, const AttributeList &Attr) {
>    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 (normalizeAttrName(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 *D, const AttributeList &Attr) {
>    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);
> +  static_cast<void>(normalizeAttrName(Str));

No need for the static_cast to void here; we're okay ignoring this
return value implicitly.

>
>    unsigned DestWidth = 0;
>    bool IntegerMode = true;
> @@ -4533,8 +4537,10 @@ static void handleNoSanitizeAttr(Sema &S, Decl *D, const AttributeList &Attr) {
>
>  static void handleNoSanitizeSpecificAttr(Sema &S, Decl *D,
>                                           const AttributeList &Attr) {
> +  StringRef AttrName = Attr.getName()->getName();
> +  static_cast<void>(normalizeAttrName(AttrName));

No need for the static_cast here either.

~Aaron

>    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")
> diff --git a/test/SemaCXX/attr-no-sanitize-address.cpp b/test/SemaCXX/attr-no-sanitize-address.cpp
> index 9ca2863..9499742 100644
> --- a/test/SemaCXX/attr-no-sanitize-address.cpp
> +++ b/test/SemaCXX/attr-no-sanitize-address.cpp
> @@ -8,6 +8,8 @@
>
>  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}}
>
> diff --git a/test/SemaCXX/attr-no-sanitize-memory.cpp b/test/SemaCXX/attr-no-sanitize-memory.cpp
> index 9cbcb03..41809a0 100644
> --- a/test/SemaCXX/attr-no-sanitize-memory.cpp
> +++ b/test/SemaCXX/attr-no-sanitize-memory.cpp
> @@ -8,6 +8,8 @@
>
>  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}}
>
> diff --git a/test/SemaCXX/attr-no-sanitize-thread.cpp b/test/SemaCXX/attr-no-sanitize-thread.cpp
> index 6cb9c71..d97e050 100644
> --- a/test/SemaCXX/attr-no-sanitize-thread.cpp
> +++ b/test/SemaCXX/attr-no-sanitize-thread.cpp
> @@ -8,6 +8,8 @@
>
>  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}}
>
> --
> 1.8.0
>
>