from:"Andrews, Elizabeth via cfe\-commits"

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

2019-12-12 Thread Andrews, Elizabeth via cfe-commits

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 
Sent: Wednesday, December 11, 2019 2:35 PM
To: Andrews, Elizabeth ; Elizabeth Andrews 

Cc: cfe-commits 
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
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 
mailto:cfe-commits@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
@@ -103,6 +103,8 @@ struct S {
   static constexpr T t = 0x8000;
   std::string s;
   void f(char c) { s += c | static_cast(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(t)); }
 };

 template S;

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 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(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(DC);
+if (RD && RD->isDependentContext() && RD->isCurrentInstantiation(DC))
+  E->setTypeDependent(T->isDependentType());
+  }
+
   if (HasQualOrFound) {
 // FIXME: Wrong. We should be looking at the

RE: r325081 - Implement function attribute artificial

2018-02-14 Thread Andrews, Elizabeth via cfe-commits

Thanks Richard. I’ll update the documentation and submit a new patch for review.

Elizabeth

From: Keane, Erich
Sent: Wednesday, February 14, 2018 11:33 AM
To: Richard Smith ; Andrews, Elizabeth 

Cc: cfe-commits 
Subject: RE: r325081 - Implement function attribute artificial

Thanks for your comments Richard, I’ve added the author to this email so that 
she can take a look.

From: Richard Smith [mailto:rich...@metafoo.co.uk]
Sent: Tuesday, February 13, 2018 5:39 PM
To: Keane, Erich >
Cc: cfe-commits >
Subject: Re: r325081 - Implement function attribute artificial

On 13 February 2018 at 16:14, Erich Keane via cfe-commits 
> wrote:
Author: erichkeane
Date: Tue Feb 13 16:14:07 2018
New Revision: 325081

URL: http://llvm.org/viewvc/llvm-project?rev=325081=rev
Log:
Implement function attribute artificial

Added support in clang for GCC function attribute 'artificial'. This attribute
is used to control stepping behavior of debugger with respect to inline
functions.

Patch By: Elizabeth Andrews (eandrews)

Differential Revision: https://reviews.llvm.org/D43259


Added:
cfe/trunk/test/CodeGen/artificial.c
cfe/trunk/test/Sema/artificial.c
Modified:
cfe/trunk/include/clang/Basic/Attr.td
cfe/trunk/include/clang/Basic/AttrDocs.td
cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
cfe/trunk/lib/Sema/SemaDeclAttr.cpp

Modified: cfe/trunk/include/clang/Basic/Attr.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/Attr.td?rev=325081=325080=325081=diff
==
--- cfe/trunk/include/clang/Basic/Attr.td (original)
+++ cfe/trunk/include/clang/Basic/Attr.td Tue Feb 13 16:14:07 2018
@@ -111,6 +111,9 @@ def SharedVar : SubsetSubjecthasGlobalStorage()}], "global variables">;

+def InlineFunction : SubsetSubjectisInlineSpecified()}], "inline functions">;
+
 // FIXME: this hack is needed because DeclNodes.td defines the base Decl node
 // type to be a class, not a definition. This makes it impossible to create an
 // attribute subject which accepts a Decl. Normally, this is not a problem,
@@ -588,6 +591,12 @@ def AlwaysInline : InheritableAttr {
   let Documentation = [Undocumented];
 }

+def Artificial : InheritableAttr {
+  let Spellings = [GCC<"artificial">];
+  let Subjects = SubjectList<[InlineFunction], WarnDiag>;
+  let Documentation = [ArtificialDocs];
+}
+
 def XRayInstrument : InheritableAttr {
   let Spellings = [Clang<"xray_always_instrument">,
Clang<"xray_never_instrument">];

Modified: cfe/trunk/include/clang/Basic/AttrDocs.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/AttrDocs.td?rev=325081=325080=325081=diff
==
--- cfe/trunk/include/clang/Basic/AttrDocs.td (original)
+++ cfe/trunk/include/clang/Basic/AttrDocs.td Tue Feb 13 16:14:07 2018
@@ -3273,3 +3273,13 @@ For more information see
 or `msvc documentation `_.
 }];
 }
+
+def ArtificialDocs : Documentation {
+  let Category = DocCatFunction;
+  let Content = [{
+The ``artificial`` attribute is used with inline functions to treat the inline
+function as a unit while debugging. For more information see GCC_ 
documentation.

I find this to be hard to understand. What does "treat the inline function as a 
unit" actually mean? How about something like:

The ``artificial`` attribute can be applied to an inline function. If such a 
function is inlined, the attribute indicates that debuggers should associate 
the resulting instructions with the call site, rather than with the 
corresponding line within the inlined callee.

+
+.. _GCC: https://gcc.gnu.org/onlinedocs/gcc-4.6.4/gcc/Function-Attributes.html

If you still want to reference the GCC documentation, could you pick a newer 
version? :)

+  }];
+}

Modified: cfe/trunk/lib/CodeGen/CGDebugInfo.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/CodeGen/CGDebugInfo.cpp?rev=325081=325080=325081=diff
==
--- cfe/trunk/lib/CodeGen/CGDebugInfo.cpp (original)
+++ cfe/trunk/lib/CodeGen/CGDebugInfo.cpp Tue Feb 13 16:14:07 2018
@@ -3235,7 +3235,7 @@ void CGDebugInfo::EmitFunctionStart(Glob
   if (Name.startswith("\01"))
 Name = Name.substr(1);

-  if (!HasDecl || D->isImplicit()) {
+  if (!HasDecl || D->isImplicit() || D->hasAttr()) {
 Flags |= llvm::DINode::FlagArtificial;
 // Artificial functions should not silently reuse CurLoc.
 CurLoc = SourceLocation();

Modified:

RE: r314262 - Emit section information for extern variables.

2017-10-04 Thread Andrews, Elizabeth via cfe-commits

Alright. Will do. Thanks!

From: Friedman, Eli [mailto:efrie...@codeaurora.org]
Sent: Wednesday, October 4, 2017 4:51 PM
To: Keane, Erich ; Andrews, Elizabeth 
; Nico Weber 
Cc: cfe-commits 
Subject: Re: r314262 - Emit section information for extern variables.

On 10/4/2017 1:48 PM, Keane, Erich wrote:
Ah, cool!  I didn’t realize that, and that is actually exactly what Elizabeth 
is looking into now.

Elizabeth, if you send me a diff, I’ll commit it as review-after-commit if Eli 
is alright with this.  It should be something like changing:
+  if (VD->isThisDeclarationADefinition() != VarDecl::Definition) {
To
+  if (VD->isThisDeclarationADefinition() != VarDecl::Definition && 
VD->isThsDeclarationADefinition() != VarDecl::TentativeDefinition) {

Right?

I'd probably just write it "if (VD->isThisDeclarationADefinition() == 
VarDecl::DeclarationOnly)", but yes.  (And don't forget a testcase.)

-Eli

From: Friedman, Eli [mailto:efrie...@codeaurora.org]
Sent: Wednesday, October 4, 2017 1:46 PM
To: Andrews, Elizabeth 
; Nico Weber 

Cc: cfe-commits 
; Keane, Erich 

Subject: Re: r314262 - Emit section information for extern variables.

Oh, that's easy to explain; sorry, I didn't think of it when I was reviewing.

DefinitionKind has three possible values: DeclarationOnly, TentativeDefinition, 
and Definition.  (Tentative definitions are C weirdness that allows you to 
write "int x; int x = 10;".)

For the purpose of this warning, a TentativeDefinition should count as a 
definition.

-Eli

On 10/4/2017 1:38 PM, Andrews, Elizabeth wrote:
Hello,
I just spoke to Erich. The warning isn’t supposed to be emitted when the 
section attribute is specified on a definition. I’m not sure why struct r_debug 
_r_debug __attribute__((nocommon, section(".r_debug"))); failed the 
isThisDeclarationADefiniton check. I need to look into it.
Thanks,
Elizabeth

From: tha...@google.com [mailto:tha...@google.com] On 
Behalf Of Nico Weber
Sent: Wednesday, October 4, 2017 4:29 PM
To: Keane, Erich 
Cc: Andrews, Elizabeth 
; Friedman, 
Eli ; cfe-commits 

Subject: Re: r314262 - Emit section information for extern variables.

All I know about this code that it used to build (and still builds with gcc) 
and now it doesn't, sorry :-) What that code does seems somewhat questionable, 
but also somewhat useful, and it feels like the behavior change from this 
change here for that code might have been unintentional.

Your suggestion sounds reasonable to me.

On Wed, Oct 4, 2017 at 4:18 PM, Keane, Erich 
> wrote:
I saw that… I don’t see where it prevents the same change from being made in 
link.h.

As I mentioned, there is a solution to make this Warning less aggressive (in 
the lib/Sema/SemaDecl.cpp changes, add a condition that Old->isUsed() before 
the warning.  I’m wondering if that solves your issue.

It would result in
extern struct r_debug _r_debug;
struct r_debug _r_debug __attribute__((nocommon, section(".r_debug")));
Compiling, but :
extern struct r_debug _r_debug;
r_debug = something();
struct r_debug _r_debug __attribute__((nocommon, section(".r_debug")));
NOT compiling (which is almost definitely a bug).

From: tha...@google.com 
[mailto:tha...@google.com] On Behalf Of Nico Weber
Sent: Wednesday, October 4, 2017 1:14 PM
To: Keane, Erich >
Cc: Andrews, Elizabeth 
>; Friedman, 
Eli >; cfe-commits 
>

Subject: Re: r314262 - Emit section information for extern variables.

For why, here's the comment from the code I linked to:

/*
 * GDB looks for this symbol name when it cannot find PT_DYNAMIC->DT_DEBUG.
 * We don't have a PT_DYNAMIC, so it will find this.  Now all we have to do
 * is arrange for this space to be filled in with the dynamic linker's
 * _r_debug contents after they're initialized.  That way, attaching GDB to
 * this process or examining its core file will find the PIE we loaded, the
 * dynamic linker, and all the shared libraries, making debugging pleasant.
 */

On Wed, Oct 4, 2017 at 4:11 PM, Keane, Erich 
> wrote:
I’ve added Elizabeth, who is

RE: r314262 - Emit section information for extern variables.

2017-10-04 Thread Andrews, Elizabeth via cfe-commits

Hello,
I just spoke to Erich. The warning isn’t supposed to be emitted when the 
section attribute is specified on a definition. I’m not sure why struct r_debug 
_r_debug __attribute__((nocommon, section(".r_debug"))); failed the 
isThisDeclarationADefiniton check. I need to look into it.
Thanks,
Elizabeth

From: tha...@google.com [mailto:tha...@google.com] On Behalf Of Nico Weber
Sent: Wednesday, October 4, 2017 4:29 PM
To: Keane, Erich 
Cc: Andrews, Elizabeth ; Friedman, Eli 
; cfe-commits 
Subject: Re: r314262 - Emit section information for extern variables.

All I know about this code that it used to build (and still builds with gcc) 
and now it doesn't, sorry :-) What that code does seems somewhat questionable, 
but also somewhat useful, and it feels like the behavior change from this 
change here for that code might have been unintentional.

Your suggestion sounds reasonable to me.

On Wed, Oct 4, 2017 at 4:18 PM, Keane, Erich 
> wrote:
I saw that… I don’t see where it prevents the same change from being made in 
link.h.

As I mentioned, there is a solution to make this Warning less aggressive (in 
the lib/Sema/SemaDecl.cpp changes, add a condition that Old->isUsed() before 
the warning.  I’m wondering if that solves your issue.

It would result in
extern struct r_debug _r_debug;
struct r_debug _r_debug __attribute__((nocommon, section(".r_debug")));
Compiling, but :
extern struct r_debug _r_debug;
r_debug = something();
struct r_debug _r_debug __attribute__((nocommon, section(".r_debug")));
NOT compiling (which is almost definitely a bug).

From: tha...@google.com 
[mailto:tha...@google.com] On Behalf Of Nico Weber
Sent: Wednesday, October 4, 2017 1:14 PM
To: Keane, Erich >
Cc: Andrews, Elizabeth 
>; Friedman, 
Eli >; cfe-commits 
>

Subject: Re: r314262 - Emit section information for extern variables.

For why, here's the comment from the code I linked to:

/*
 * GDB looks for this symbol name when it cannot find PT_DYNAMIC->DT_DEBUG.
 * We don't have a PT_DYNAMIC, so it will find this.  Now all we have to do
 * is arrange for this space to be filled in with the dynamic linker's
 * _r_debug contents after they're initialized.  That way, attaching GDB to
 * this process or examining its core file will find the PIE we loaded, the
 * dynamic linker, and all the shared libraries, making debugging pleasant.
 */

On Wed, Oct 4, 2017 at 4:11 PM, Keane, Erich 
> wrote:
I’ve added Elizabeth, who is the original patch author.  Hopefully she can help 
out here.  Additionally, Eli did the original review, so hopefully he can chime 
in as well.

I believe the necessity for this warning came out of the discussion on fixing 
the section behavior.  The issue here is that redeclaring with a different 
‘section’ can cause some pretty nasty issues, since it isn’t terribly clear 
what happens if the variable is used in the meantime.

There is 1 change that I can think of that Elizabeth and I discussed, which was 
to only warn in the case where there was a USAGE between these two 
redeclarations.  I’m not sure that will allow na_cl to compile, however it is 
my belief that if there IS a usage between link.h:64 and nacl_bootstrap.c:434, 
that this is a bug in nacl that is simply being uncovered thanks to this new 
warning.

Is there a good/reasonable reason the source of nacl wants to redeclare this 
with a different ‘section’?

From: tha...@google.com 
[mailto:tha...@google.com] On Behalf Of Nico Weber
Sent: Wednesday, October 4, 2017 12:59 PM
To: Keane, Erich >
Cc: cfe-commits >
Subject: Re: r314262 - Emit section information for extern variables.

Hi Erich,

this breaks existing code. NaCl does this:

#include 
struct r_debug _r_debug __attribute__((nocommon, section(".r_debug")));

(There's a lengthy-ish comment for why in 
https://cs.chromium.org/chromium/src/native_client/src/trusted/service_runtime/linux/nacl_bootstrap.c?q=nacl_bootstrap.c=package:chromium=424)

link.h in e.g. the debian jessie sysroot says:
extern struct r_debug _r_debug;

After this change, clang complains:

../../native_client/src/trusted/service_runtime/linux/nacl_bootstrap.c:434:16: 
error: section attribute is specified on redeclared variable [-Werror,-Wsection]
struct r_debug _r_debug __attribute__((nocommon, section(".r_debug")));
   ^

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

RE: r325081 - Implement function attribute artificial

RE: r314262 - Emit section information for extern variables.

RE: r314262 - Emit section information for extern variables.

4 matches

Site Navigation

Mail list logo

Footer information