Add a warning for builtin_return_address/frame_address with > 0 argument

[jstenglein]

Summary

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.

[erichkeane]

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.

[jstenglein]

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]>;

[erichkeane]

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.

[jstenglein]

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.

[lebedev.ri]

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

[erichkeane]

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!

[jstenglein]

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.

clang/include/clang/Basic/DiagnosticGroups.td

 def ImplicitFixedPointConversion : DiagGroup<"implicit-fixed-point-conversion">;
 
 def FloatOverflowConversion : DiagGroup<"float-overflow-conversion">;
 def FloatZeroConversion : DiagGroup<"float-zero-conversion">;
 def FloatConversion :
   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">;
 def UnsupportedAbs : DiagGroup<"unsupported-abs">;
 def UnsupportedCB : DiagGroup<"unsupported-cb">;
 def UnsupportedGPOpt : DiagGroup<"unsupported-gpopt">;
 def UnsupportedTargetOpt : DiagGroup<"unsupported-target-opt">;
 def NonLiteralNullConversion : DiagGroup<"non-literal-null-conversion">;
def Extra : DiagGroup<"extra", [
   ]>;
 
 def Most : DiagGroup<"most", [
     CharSubscript,
     Comment,
     DeleteNonVirtualDtor,
     Format,
     ForLoopAnalysis,
+    FrameAddress,
     Implicit,
     InfiniteRecursion,
     IntInBoolContext,
     MismatchedTags,
     MissingBraces,
     Move,
     MultiChar,
     RangeLoopConstruct,

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_missing_default_ctor : Error<
   "%select{base class|member}2 %3 %select{which|which|of %1}0 "
   "does not have a default constructor">;
 def note_due_to_dllexported_class : Note<
   "due to %0 being dllexported%select{|; try compiling in C++11 mode}1">;
 
 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<FrameAddress>, 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">,
   InGroup<CXX98Compat>, DefaultIgnore;
 
 def note_nontrivial_virtual_dtor : Note<
   "destructor for %0 is not trivial because it is virtual">;
 def note_nontrivial_has_virtual : Note<

clang/lib/Sema/SemaChecking.cpp

#include "clang/Basic/Builtins.def"
   case Builtin::BI__builtin_os_log_format_buffer_size:
     if (SemaBuiltinOSLogFormat(TheCall))
       return ExprError();
     break;
   case Builtin::BI__builtin_frame_address:
   case Builtin::BI__builtin_return_address:
     if (SemaBuiltinConstantArgRange(TheCall, 0, 0, 0xFFFF))
       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;
   }
 
   // Since the target specific builtins for each arch overlap, only check those
   // of the arch we are compiling for.
   if (Context.BuiltinInfo.isTSBuiltin(BuiltinID)) {
     switch (Context.getTargetInfo().getTriple().getArch()) {
       case llvm::Triple::arm:

clang/test/Misc/warning-wall.c

 CHECK-NEXT:    -Wformat
 CHECK-NEXT:      -Wformat-extra-args
 CHECK-NEXT:      -Wformat-zero-length
 CHECK-NEXT:      -Wnonnull
 CHECK-NEXT:      -Wformat-security
 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
 CHECK-NEXT:    -Winfinite-recursion
 CHECK-NEXT:    -Wint-in-bool-context
 CHECK-NEXT:    -Wmismatched-tags
 CHECK-NEXT:    -Wmissing-braces
 CHECK-NEXT:    -Wmove