[clang] 878a24e - Reapply "Fix crash on switch conditions of non-integer types in templates"

Alexander Kornienko via cfe-commits cfe-commits at lists.llvm.org
Mon Dec 30 06:07:48 PST 2019


Any news here? The problem is still there.

On Fri, Dec 13, 2019 at 12:33 AM Andrews, Elizabeth <
elizabeth.andrews at intel.com> wrote:

> Hi Alexander,
>
>
>
> I am debugging this now but I assume the checks for the if condition were
> skipped before this commit (therefore no crash) because ‘c’ was set as type
> dependent. The checks are probably run now since c’s type dependency
> changed with this patch. The checks might need to be guarded better, but I
> am not sure.  I will take a look and get back to you ASAP.
>
>
>
> Elizabeth
>
>
>
> *From:* Alexander Kornienko <alexfh at google.com>
> *Sent:* Wednesday, December 11, 2019 2:35 PM
> *To:* Andrews, Elizabeth <elizabeth.andrews at intel.com>; Elizabeth Andrews
> <llvmlistbot at llvm.org>
> *Cc:* cfe-commits <cfe-commits at lists.llvm.org>
> *Subject:* Re: [clang] 878a24e - Reapply "Fix crash on switch conditions
> of non-integer types in templates"
>
>
>
> After this commit clang started generating assertion failures on valid
> code. There are tons of instances in our codebase. Here's a reduced test
> case:
>
> template<int b>
> class a {
>   int c : b;
>   void f() {
>     if (c)
>       ;
>   }
> };
>
>
>
> Please take a look.
>
>
>
> On Wed, Dec 4, 2019 at 12:39 AM Elizabeth Andrews via cfe-commits <
> cfe-commits at lists.llvm.org> wrote:
>
>
> Author: Elizabeth Andrews
> Date: 2019-12-03T15:27:19-08:00
> New Revision: 878a24ee244a24c39d1c57e9af2e88c621f7cce9
>
> URL:
> https://github.com/llvm/llvm-project/commit/878a24ee244a24c39d1c57e9af2e88c621f7cce9
> DIFF:
> https://github.com/llvm/llvm-project/commit/878a24ee244a24c39d1c57e9af2e88c621f7cce9.diff
>
> LOG: Reapply "Fix crash on switch conditions of non-integer types in
> templates"
>
> This patch reapplies commit 759948467ea. Patch was reverted due to a
> clang-tidy test fail on Windows. The test has been modified. There
> are no additional code changes.
>
> Patch was tested with ninja check-all on Windows and Linux.
>
> Summary of code changes:
>
> Clang currently crashes for switch statements inside a template when the
> condition is a non-integer field member because contextual implicit
> conversion is skipped when parsing the condition. This conversion is
> however later checked in an assert when the case statement is handled.
> The conversion is skipped when parsing the condition because
> the field member is set as type-dependent based on its containing class.
> This patch sets the type dependency based on the field's type instead.
>
> This patch fixes Bug 40982.
>
> Added:
>     clang/test/SemaTemplate/non-integral-switch-cond.cpp
>
> Modified:
>
> clang-tools-extra/test/clang-tidy/checkers/bugprone-string-integer-assignment.cpp
>     clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
>     clang/lib/AST/Expr.cpp
>     clang/lib/Sema/SemaChecking.cpp
>     clang/test/SemaCXX/constant-expression-cxx2a.cpp
>     clang/test/SemaTemplate/dependent-names.cpp
>     clang/test/SemaTemplate/enum-argument.cpp
>     clang/test/SemaTemplate/member-access-expr.cpp
>
> Removed:
>
>
>
>
> ################################################################################
> diff  --git
> a/clang-tools-extra/test/clang-tidy/checkers/bugprone-string-integer-assignment.cpp
> b/clang-tools-extra/test/clang-tidy/checkers/bugprone-string-integer-assignment.cpp
> index 18fe5ef4e5c2..2c288e0bbddf 100644
> ---
> a/clang-tools-extra/test/clang-tidy/checkers/bugprone-string-integer-assignment.cpp
> +++
> b/clang-tools-extra/test/clang-tidy/checkers/bugprone-string-integer-assignment.cpp
> @@ -1,4 +1,4 @@
> -// RUN: %check_clang_tidy %s bugprone-string-integer-assignment %t
> +// RUN: %check_clang_tidy %s bugprone-string-integer-assignment %t -- --
> -fno-delayed-template-parsing
>
>  namespace std {
>  template<typename T>
> @@ -103,6 +103,8 @@ struct S {
>    static constexpr T t = 0x8000;
>    std::string s;
>    void f(char c) { s += c | static_cast<int>(t); }
> +  // CHECK-MESSAGES: :[[@LINE-1]]:25: warning: an integer is interpreted
> as a chara
> +  // CHECK-FIXES: {{^}}  void f(char c) { s += std::to_string(c |
> static_cast<int>(t)); }
>  };
>
>  template S<int>;
>
> diff  --git
> a/clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
> b/clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
> index 119eff67318e..8e546b44ab74 100644
> --- a/clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
> +++ b/clang-tools-extra/test/clang-tidy/checkers/misc-unused-parameters.cpp
> @@ -233,7 +233,7 @@ struct a {
>  template <class>
>  class d {
>    a e;
> -  void f() { e.b(); }
> +  void f() { e.b(0); }
>  };
>  }  // namespace
>  }  // namespace PR38055
>
> diff  --git a/clang/lib/AST/Expr.cpp b/clang/lib/AST/Expr.cpp
> index 322b3a7fa740..a73531ad5fad 100644
> --- a/clang/lib/AST/Expr.cpp
> +++ b/clang/lib/AST/Expr.cpp
> @@ -1678,6 +1678,15 @@ MemberExpr *MemberExpr::Create(
>    MemberExpr *E = new (Mem) MemberExpr(Base, IsArrow, OperatorLoc,
> MemberDecl,
>                                         NameInfo, T, VK, OK, NOUR);
>
> +  if (FieldDecl *Field = dyn_cast<FieldDecl>(MemberDecl)) {
> +    DeclContext *DC = MemberDecl->getDeclContext();
> +    // dyn_cast_or_null is used to handle objC variables which do not
> +    // have a declaration context.
> +    CXXRecordDecl *RD = dyn_cast_or_null<CXXRecordDecl>(DC);
> +    if (RD && RD->isDependentContext() && RD->isCurrentInstantiation(DC))
> +      E->setTypeDependent(T->isDependentType());
> +  }
> +
>    if (HasQualOrFound) {
>      // FIXME: Wrong. We should be looking at the member declaration we
> found.
>      if (QualifierLoc &&
> QualifierLoc.getNestedNameSpecifier()->isDependent()) {
>
> diff  --git a/clang/lib/Sema/SemaChecking.cpp
> b/clang/lib/Sema/SemaChecking.cpp
> index ed42833531d4..825e0faa3037 100644
> --- a/clang/lib/Sema/SemaChecking.cpp
> +++ b/clang/lib/Sema/SemaChecking.cpp
> @@ -14706,6 +14706,8 @@ void Sema::RefersToMemberWithReducedAlignment(
>    bool AnyIsPacked = false;
>    do {
>      QualType BaseType = ME->getBase()->getType();
> +    if (BaseType->isDependentType())
> +      return;
>      if (ME->isArrow())
>        BaseType = BaseType->getPointeeType();
>      RecordDecl *RD = BaseType->castAs<RecordType>()->getDecl();
>
> diff  --git a/clang/test/SemaCXX/constant-expression-cxx2a.cpp
> b/clang/test/SemaCXX/constant-expression-cxx2a.cpp
> index 8db705dcdc67..c2e443b9bec1 100644
> --- a/clang/test/SemaCXX/constant-expression-cxx2a.cpp
> +++ b/clang/test/SemaCXX/constant-expression-cxx2a.cpp
> @@ -18,6 +18,7 @@ namespace std {
>  [[nodiscard]] void *operator new(std::size_t, std::align_val_t, const
> std::nothrow_t&) noexcept;
>  [[nodiscard]] void *operator new[](std::size_t, const std::nothrow_t&)
> noexcept;
>  [[nodiscard]] void *operator new[](std::size_t, std::align_val_t, const
> std::nothrow_t&) noexcept;
> +[[nodiscard]] void *operator new[](std::size_t, std::align_val_t);
>  void operator delete(void*, const std::nothrow_t&) noexcept;
>  void operator delete(void*, std::align_val_t, const std::nothrow_t&)
> noexcept;
>  void operator delete[](void*, const std::nothrow_t&) noexcept;
> @@ -1050,7 +1051,7 @@ namespace dynamic_alloc {
>      // Ensure that we don't try to evaluate these for overflow and crash.
> These
>      // are all value-dependent expressions.
>      p = new char[n];
> -    p = new (n) char[n];
> +    p = new ((std::align_val_t)n) char[n];
>      p = new char(n);
>    }
>  }
>
> diff  --git a/clang/test/SemaTemplate/dependent-names.cpp
> b/clang/test/SemaTemplate/dependent-names.cpp
> index 67ef238083f0..a8de159a1d46 100644
> --- a/clang/test/SemaTemplate/dependent-names.cpp
> +++ b/clang/test/SemaTemplate/dependent-names.cpp
> @@ -273,9 +273,6 @@ namespace PR10187 {
>        }
>        int e[10];
>      };
> -    void g() {
> -      S<int>().f(); // expected-note {{here}}
> -    }
>    }
>
>    namespace A2 {
>
> diff  --git a/clang/test/SemaTemplate/enum-argument.cpp
> b/clang/test/SemaTemplate/enum-argument.cpp
> index 7ff419613990..a79ed8403e9f 100644
> --- a/clang/test/SemaTemplate/enum-argument.cpp
> +++ b/clang/test/SemaTemplate/enum-argument.cpp
> @@ -1,5 +1,4 @@
>  // RUN: %clang_cc1 -fsyntax-only -verify %s
> -// expected-no-diagnostics
>
>  enum Enum { val = 1 };
>  template <Enum v> struct C {
> @@ -31,7 +30,7 @@ namespace rdar8020920 {
>      unsigned long long bitfield : e0;
>
>      void f(int j) {
> -      bitfield + j;
> +      bitfield + j; // expected-warning {{expression result unused}}
>      }
>    };
>  }
>
> diff  --git a/clang/test/SemaTemplate/member-access-expr.cpp
> b/clang/test/SemaTemplate/member-access-expr.cpp
> index 8dba2e68d656..ef10d72a0ef8 100644
> --- a/clang/test/SemaTemplate/member-access-expr.cpp
> +++ b/clang/test/SemaTemplate/member-access-expr.cpp
> @@ -156,7 +156,7 @@ namespace test6 {
>      void get(B **ptr) {
>        // It's okay if at some point we figure out how to diagnose this
>        // at instantiation time.
> -      *ptr = field;
> +      *ptr = field; // expected-error {{assigning to 'test6::B *' from
> incompatible type 'test6::A *}}
>      }
>    };
>  }
>
> diff  --git a/clang/test/SemaTemplate/non-integral-switch-cond.cpp
> b/clang/test/SemaTemplate/non-integral-switch-cond.cpp
> new file mode 100644
> index 000000000000..23c8e0ef8d4e
> --- /dev/null
> +++ b/clang/test/SemaTemplate/non-integral-switch-cond.cpp
> @@ -0,0 +1,14 @@
> +// RUN: %clang_cc1 -fsyntax-only -verify %s
> +
> +struct NOT_AN_INTEGRAL_TYPE {};
> +
> +template <typename T>
> +struct foo {
> +  NOT_AN_INTEGRAL_TYPE Bad;
> +  void run() {
> +    switch (Bad) { // expected-error {{statement requires expression of
> integer type ('NOT_AN_INTEGRAL_TYPE' invalid)}}
> +    case 0:
> +      break;
> +    }
> +  }
> +};
>
>
>
> _______________________________________________
> cfe-commits mailing list
> cfe-commits at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20191230/18415648/attachment-0001.html>


More information about the cfe-commits mailing list