[PATCH] D75768: Add a warning for builtin_return_address/frame_address with > 0 argument

[Erich Keane via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG843a9778fcd5: Add a warning for 
builtin_return_address/frame_address with  0 argument (authored by 
jstenglein, committed by erichkeane).

Changed prior to commit:
  https://reviews.llvm.org/D75768?vs=248809=249161#toc

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75768/new/

https://reviews.llvm.org/D75768

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Misc/warning-wall.c


Index: clang/test/Misc/warning-wall.c
===
--- clang/test/Misc/warning-wall.c
+++ clang/test/Misc/warning-wall.c
@@ -16,6 +16,7 @@
 CHECK-NEXT:  -Wformat-y2k
 CHECK-NEXT:  -Wformat-invalid-specifier
 CHECK-NEXT:-Wfor-loop-analysis
+CHECK-NEXT:-Wframe-address
 CHECK-NEXT:-Wimplicit
 CHECK-NEXT:  -Wimplicit-function-declaration
 CHECK-NEXT:  -Wimplicit-int
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -1853,6 +1853,17 @@
   case Builtin::BI__builtin_return_address:
 if (SemaBuiltinConstantArgRange(TheCall, 0, 0, 0x))
   return ExprError();
+
+// -Wframe-address warning if non-zero passed to builtin
+// return/frame address.
+Expr::EvalResult Result;
+if (TheCall->getArg(0)->EvaluateAsInt(Result, getASTContext()) &&
+Result.Val.getInt() != 0)
+  Diag(TheCall->getBeginLoc(), diag::warn_frame_address)
+  << ((BuiltinID == Builtin::BI__builtin_return_address)
+  ? "__builtin_return_address"
+  : "__builtin_frame_address")
+  << TheCall->getSourceRange();
 break;
   }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1780,6 +1780,11 @@
 def err_illegal_union_or_anon_struct_member : Error<
   "%select{anonymous struct|union}0 member %1 has a non-trivial "
   "%sub{select_special_member_kind}2">;
+
+def warn_frame_address : Warning<
+  "calling '%0' with a nonzero argument is unsafe">,
+  InGroup, DefaultIgnore;
+
 def warn_cxx98_compat_nontrivial_union_or_anon_struct_member : Warning<
   "%select{anonymous struct|union}0 member %1 with a non-trivial "
   "%sub{select_special_member_kind}2 is incompatible with C++98">,
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -99,6 +99,7 @@
   DiagGroup<"float-conversion", [FloatOverflowConversion,
  FloatZeroConversion]>;
 
+def FrameAddress : DiagGroup<"frame-address">;
 def DoublePromotion : DiagGroup<"double-promotion">;
 def EnumTooLarge : DiagGroup<"enum-too-large">;
 def UnsupportedNan : DiagGroup<"unsupported-nan">;
@@ -872,6 +873,7 @@
 DeleteNonVirtualDtor,
 Format,
 ForLoopAnalysis,
+FrameAddress,
 Implicit,
 InfiniteRecursion,
 IntInBoolContext,


Index: clang/test/Misc/warning-wall.c
===
--- clang/test/Misc/warning-wall.c
+++ clang/test/Misc/warning-wall.c
@@ -16,6 +16,7 @@
 CHECK-NEXT:  -Wformat-y2k
 CHECK-NEXT:  -Wformat-invalid-specifier
 CHECK-NEXT:-Wfor-loop-analysis
+CHECK-NEXT:-Wframe-address
 CHECK-NEXT:-Wimplicit
 CHECK-NEXT:  -Wimplicit-function-declaration
 CHECK-NEXT:  -Wimplicit-int
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -1853,6 +1853,17 @@
   case Builtin::BI__builtin_return_address:
 if (SemaBuiltinConstantArgRange(TheCall, 0, 0, 0x))
   return ExprError();
+
+// -Wframe-address warning if non-zero passed to builtin
+// return/frame address.
+Expr::EvalResult Result;
+if (TheCall->getArg(0)->EvaluateAsInt(Result, getASTContext()) &&
+Result.Val.getInt() != 0)
+  Diag(TheCall->getBeginLoc(), diag::warn_frame_address)
+  << ((BuiltinID == Builtin::BI__builtin_return_address)
+  ? "__builtin_return_address"
+  : "__builtin_frame_address")
+  << TheCall->getSourceRange();
 break;
   }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1780,6 +1780,11 @@
 def err_illegal_union_or_anon_struct_member : Error<
  

[PATCH] D75768: Add a warning for builtin_return_address/frame_address with > 0 argument

[Jeremy Stenglein via Phabricator via cfe-commits]

jstenglein added a comment.

In D75768#1912519 , @erichkeane wrote:

> I did a bit of research and I don't think i know of anything better than 
> 'most' here, and noone else had an opinion at the end of last week so I'll 
> take that silence as consent.  Approved!


Thanks... could you please commit the patch as I don't have commit permission 
yet?  Also I saw a build-bot failure but it looks to be in clang-format on the 
test file which doesn't make sense.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75768/new/

https://reviews.llvm.org/D75768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75768: Add a warning for builtin_return_address/frame_address with > 0 argument

[Erich Keane via Phabricator via cfe-commits]

erichkeane accepted this revision.
erichkeane added a comment.
This revision is now accepted and ready to land.

I did a bit of research and I don't think i know of anything better than 'most' 
here, and noone else had an opinion at the end of last week so I'll take that 
silence as consent.  Approved!


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75768/new/

https://reviews.llvm.org/D75768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75768: Add a warning for builtin_return_address/frame_address with > 0 argument

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri resigned from this revision.
lebedev.ri added a reviewer: aaron.ballman.
lebedev.ri added a comment.

This seems reasonable to me but i'll defer to other reviewers.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75768/new/

https://reviews.llvm.org/D75768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75768: Add a warning for builtin_return_address/frame_address with > 0 argument

[Jeremy Stenglein via Phabricator via cfe-commits]

jstenglein added a comment.

In D75768#1910339 , @erichkeane wrote:

> In D75768#1910324 , @jstenglein 
> wrote:
>
> > In D75768#1910303 , @erichkeane 
> > wrote:
> >
> > > Any reason to not put this in -Wall like GCC? I'm not terribly 
> > > knowledgeable about the intended guidance for adding to Wmost, so if you 
> > > can clarify this decision I'd be grateful.  Otherwise I think this patch 
> > > looks fine.
> >
> >
> > Thanks for the comments.  It is already part of -Wall since I put it in 
> > -Wmost and -Wall includes -Wmost:
> >  def All : DiagGroup<"all", [Most, Parentheses, Switch, SwitchBool, 
> > MisleadingIndentation]>;
>
>
> Right, I know.  I'm wondering about what made you choose most?  I dont know 
> our rules for warning groups, so hopefully someone else can comment.


I chose Most to get the warning included in -Wall - if there is a better 
placement let me know.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75768/new/

https://reviews.llvm.org/D75768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75768: Add a warning for builtin_return_address/frame_address with > 0 argument

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

In D75768#1910324 , @jstenglein wrote:

> In D75768#1910303 , @erichkeane 
> wrote:
>
> > Any reason to not put this in -Wall like GCC? I'm not terribly 
> > knowledgeable about the intended guidance for adding to Wmost, so if you 
> > can clarify this decision I'd be grateful.  Otherwise I think this patch 
> > looks fine.
>
>
> Thanks for the comments.  It is already part of -Wall since I put it in 
> -Wmost and -Wall includes -Wmost:
>  def All : DiagGroup<"all", [Most, Parentheses, Switch, SwitchBool, 
> MisleadingIndentation]>;


Right, I know.  I'm wondering about what made you choose most?  I dont know our 
rules for warning groups, so hopefully someone else can comment.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75768/new/

https://reviews.llvm.org/D75768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75768: Add a warning for builtin_return_address/frame_address with > 0 argument

[Jeremy Stenglein via Phabricator via cfe-commits]

jstenglein added a comment.

In D75768#1910303 , @erichkeane wrote:

> Any reason to not put this in -Wall like GCC? I'm not terribly knowledgeable 
> about the intended guidance for adding to Wmost, so if you can clarify this 
> decision I'd be grateful.  Otherwise I think this patch looks fine.


Thanks for the comments.  It is already part of -Wall since I put it in -Wmost 
and -Wall includes -Wmost:
def All : DiagGroup<"all", [Most, Parentheses, Switch, SwitchBool, 
MisleadingIndentation]>;


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75768/new/

https://reviews.llvm.org/D75768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75768: Add a warning for builtin_return_address/frame_address with > 0 argument

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Any reason to not put this in -Wall like GCC? I'm not terribly knowledgeable 
about the intended guidance for adding to Wmost, so if you can clarify this 
decision I'd be grateful.  Otherwise I think this patch looks fine.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75768/new/

https://reviews.llvm.org/D75768



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D75768: Add a warning for builtin_return_address/frame_address with > 0 argument

[Jeremy Stenglein via Phabricator via cfe-commits]

jstenglein created this revision.
jstenglein added reviewers: efriedma, lebedev.ri, rsmith, erichkeane.
jstenglein added a project: clang.
Herald added a subscriber: cfe-commits.

Clang is missing a warning for __builtin_return_address/__builtin_frame_address 
called with > 0 argument.  Gcc provides a warning for this via -Wframe-address:

https://gcc.gnu.org/onlinedocs/gcc/Return-Address.html

As calling these functions with argument > 0 has caused several crashes for us, 
we would like to have the same warning as gcc here.  This diff adds the warning 
and makes it part of -Wmost.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D75768

Files:
  clang/include/clang/Basic/DiagnosticGroups.td
  clang/include/clang/Basic/DiagnosticSemaKinds.td
  clang/lib/Sema/SemaChecking.cpp
  clang/test/Misc/warning-wall.c
  clang/test/Sema/builtin-returnaddress.c


Index: clang/test/Sema/builtin-returnaddress.c
===
--- /dev/null
+++ clang/test/Sema/builtin-returnaddress.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -fsyntax-only -Wframe-address -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wmost -verify %s
+
+void* a(unsigned x) {
+return __builtin_return_address(0);
+}
+
+void* b(unsigned x) {
+return __builtin_return_address(1); // expected-warning{{calling 
'__builtin_return_address' with a nonzero argument is unsafe}}
+}
+
+void* c(unsigned x) {
+return __builtin_frame_address(0);
+}
+
+void* d(unsigned x) {
+return __builtin_frame_address(1); // expected-warning{{calling 
'__builtin_frame_address' with a nonzero argument is unsafe}}
+}
+
Index: clang/test/Misc/warning-wall.c
===
--- clang/test/Misc/warning-wall.c
+++ clang/test/Misc/warning-wall.c
@@ -16,6 +16,7 @@
 CHECK-NEXT:  -Wformat-y2k
 CHECK-NEXT:  -Wformat-invalid-specifier
 CHECK-NEXT:-Wfor-loop-analysis
+CHECK-NEXT:-Wframe-address
 CHECK-NEXT:-Wimplicit
 CHECK-NEXT:  -Wimplicit-function-declaration
 CHECK-NEXT:  -Wimplicit-int
Index: clang/lib/Sema/SemaChecking.cpp
===
--- clang/lib/Sema/SemaChecking.cpp
+++ clang/lib/Sema/SemaChecking.cpp
@@ -1851,6 +1851,17 @@
   case Builtin::BI__builtin_return_address:
 if (SemaBuiltinConstantArgRange(TheCall, 0, 0, 0x))
   return ExprError();
+
+// -Wframe-address warning if non-zero passed to builtin
+// return/frame address.
+Expr::EvalResult Result;
+if (TheCall->getArg(0)->EvaluateAsInt(Result, getASTContext()) &&
+Result.Val.getInt() != 0)
+  Diag(TheCall->getBeginLoc(), diag::warn_frame_address)
+  << ((BuiltinID == Builtin::BI__builtin_return_address)
+  ? "__builtin_return_address"
+  : "__builtin_frame_address")
+  << TheCall->getSourceRange();
 break;
   }
 
Index: clang/include/clang/Basic/DiagnosticSemaKinds.td
===
--- clang/include/clang/Basic/DiagnosticSemaKinds.td
+++ clang/include/clang/Basic/DiagnosticSemaKinds.td
@@ -1780,6 +1780,11 @@
 def err_illegal_union_or_anon_struct_member : Error<
   "%select{anonymous struct|union}0 member %1 has a non-trivial "
   "%sub{select_special_member_kind}2">;
+
+def warn_frame_address : Warning<
+  "calling '%0' with a nonzero argument is unsafe">,
+  InGroup, DefaultIgnore;
+
 def warn_cxx98_compat_nontrivial_union_or_anon_struct_member : Warning<
   "%select{anonymous struct|union}0 member %1 with a non-trivial "
   "%sub{select_special_member_kind}2 is incompatible with C++98">,
Index: clang/include/clang/Basic/DiagnosticGroups.td
===
--- clang/include/clang/Basic/DiagnosticGroups.td
+++ clang/include/clang/Basic/DiagnosticGroups.td
@@ -99,6 +99,7 @@
   DiagGroup<"float-conversion", [FloatOverflowConversion,
  FloatZeroConversion]>;
 
+def FrameAddress : DiagGroup<"frame-address">;
 def DoublePromotion : DiagGroup<"double-promotion">;
 def EnumTooLarge : DiagGroup<"enum-too-large">;
 def UnsupportedNan : DiagGroup<"unsupported-nan">;
@@ -868,6 +869,7 @@
 DeleteNonVirtualDtor,
 Format,
 ForLoopAnalysis,
+FrameAddress,
 Implicit,
 InfiniteRecursion,
 IntInBoolContext,


Index: clang/test/Sema/builtin-returnaddress.c
===
--- /dev/null
+++ clang/test/Sema/builtin-returnaddress.c
@@ -0,0 +1,19 @@
+// RUN: %clang_cc1 -fsyntax-only -Wframe-address -verify %s
+// RUN: %clang_cc1 -fsyntax-only -Wmost -verify %s
+
+void* a(unsigned x) {
+return __builtin_return_address(0);
+}
+
+void* b(unsigned x) {
+return __builtin_return_address(1); // expected-warning{{calling '__builtin_return_address' with a nonzero argument is unsafe}}
+}
+
+void* c(unsigned x) {
+return __builtin_frame_address(0);
+}
+
+void*