Add new warning -Walloca for use of builtin alloca function

[ziyig]

Summary

Add new warning -Walloca for use of builtin alloca function.

Also warns the use of __builtin_alloca_with_align. GCC has this warning, and we'd like to have this for compatibility.

[george.burgess.iv]

Thanks for this!

Mostly just nitpicking the warning's wording. :)

clang/include/clang/Basic/DiagnosticSemaKinds.td

2776

nit: I'd just say "use of function '%0'" here; "builtin" doesn't really add much.

I also wonder if we should be saying anything more than "we found a use of this function." Looks like GCC doesn't (https://godbolt.org/z/sYs_8G), but since this warning is sort of opinionated in itself, might it be better to expand this to "use of '%0' is discouraged"?

WDYT, Aaron?

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

2776

What is the purpose to this diagnostic, aside from GCC compatibility? What does it protect against?

If there's a reason users should not use alloc(), it would be better for the diagnostic to spell it out.

Btw, I'm okay with this being default-off because the GCC warning is as well. I'm mostly hoping we can do better with our diagnostic wording.

clang/lib/Sema/SemaChecking.cpp

1172

Do we want to warn on all uses of alloca(), or just the ones that get past the call to SemaBuiltinAllocaWithAlign()?

clang/test/Sema/warn-alloca.c

1

I'd appreciate a test demonstrating no warnings if -Wall is passed without an explicit -Walloca.

[george.burgess.iv]

clang/include/clang/Basic/DiagnosticSemaKinds.td

2776

I'm mostly hoping we can do better with our diagnostic wording

+1

What is the purpose to this diagnostic?

I think the intent boils down to that alloca is easily misused, and leads to e.g., https://www.qualys.com/2017/06/19/stack-clash/stack-clash.txt . Since its use often boils down to nothing but a tiny micro-optimization, some parties would like to discourage its use.

Both glibc and bionic recommend against the use of alloca in their documentation, though glibc's docs are less assertive than bionic's, and explicitly call out "[alloca's] use can improve efficiency compared to the use of malloc plus free."

Greping a codebase and investigating the first 15 results:

• all of them look like microoptimizations; many of them also sit close to other malloc/new ops, so allocating on these paths presumably isn't prohibitively expensive
• all but two of the uses of alloca have no logic to fall back to the heap malloc if the array they want to allocate passes a certain threshold. Some of the uses make it look *really* easy for the array to grow very large.
• one of the uses compares the result of alloca to NULL. Since alloca's behavior is undefined if it fails, ...

I'm having trouble putting this into a concise and actionable diagnostic message, though. The best I can come up with at the moment is something along the lines of "use of function %0 is subtle; consider using heap allocation instead."

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

2776

Okay, that's along the lines of what I was thinking.

Part of me thinks that this should not diagnose calls to alloca() that are given a constant value that we can test for sanity at compile time. e.g., calling alloca(10) is highly unlikely to be a problem, but calling alloca(1000000) certainly could be, while alloca(x) is a different class of problem without good static analysis.

That said, perhaps we could get away with use of function %0 is discouraged; there is no way to check for failure but failure may still occur, resulting in a possibly exploitable security vulnerability or something along those lines?

clang/lib/Sema/SemaChecking.cpp

1175

I think this should see how the user spelled the builtin call. It would be a bit strange for a user who wrote alloca() in their code to get a diagnostic about not calling __builtin_alloca().

[george.burgess.iv]

clang/include/clang/Basic/DiagnosticSemaKinds.td

2776

Yeah, GCC has a similar -Walloca-larger-than=N that does roughly what you described. The icky part is exactly what you said. GCC will warn about unknown values, but considers control flow when doing so: https://godbolt.org/z/J3pGiT

It looks like it's the same "we apply optimizations and then see what happens" behavior as similar diagnostics: https://godbolt.org/z/lLyteP

WRT the diag we emit here, maybe we could use notes to break it up and imply things? e.g.

warning: use of function %0 is discouraged, due to its security implications
note: 'malloc' or 'new' are suggested alternatives, since they have well-defined behavior on failure

...not sold on the idea, but it's a thought.

If we don't want to break it to pieces, I'm fine with your suggestion

[aaron.ballman]

clang/include/clang/Basic/DiagnosticSemaKinds.td

2776

I'm not certain the note adds value because it will always be printed on the same line as the warning. A note would make sense if we had a secondary location to annotate though.

[george.burgess.iv]

clang/include/clang/Basic/DiagnosticSemaKinds.td

2776

SGTM

[ziyig]

Updated the warning message and the test cases.

[ziyig]

clang/lib/Sema/SemaChecking.cpp

1172

I don't have strong opinion about this. Which one do you think is better?

[aaron.ballman]

clang/lib/Sema/SemaChecking.cpp

1172

I think the code is fine as-is. SemaBuiltinAllocaWithAlign() returns true when there's an error with the call, and so users will not get the warning only if the call is erroneous, which seems fine given that the code didn't compile. It turns out this matches GCC's behavior as well.

1175

You should pass in TheCall->getDirectCallee() and not try to get the name directly; the diagnostics engine will do the right thing automatically.

[aaron.ballman]

LGTM, thank you for the patch!

clang/include/clang/Basic/DiagnosticSemaKinds.td

def err_anonymous_property: Error<
   "anonymous property is not supported">;
 def err_property_is_variably_modified : Error<
   "property %0 has a variably modified type">;
 def err_no_accessor_for_property : Error<
   "no %select{getter|setter}0 defined for property %1">;
 def err_cannot_find_suitable_accessor : Error<
   "cannot find suitable %select{getter|setter}0 for property %1">;
 
+def warn_alloca : Warning<
+  "use of builtin function %0">,
+  InGroup<DiagGroup<"alloca">>, DefaultIgnore;
+
 def warn_alloca_align_alignof : Warning<
   "second argument to __builtin_alloca_with_align is supposed to be in bits">,
   InGroup<DiagGroup<"alloca-with-align-alignof">>;
 
 def err_alignment_too_small : Error<
   "requested alignment must be %0 or greater">;
 def err_alignment_too_big : Error<
   "requested alignment must be %0 or smaller">;

clang/lib/Sema/SemaChecking.cpp

case Builtin::BI__builtin_shufflevector:
     // SemaBuiltinShuffleVector guts it, but then doesn't release it.
   case Builtin::BI__builtin_prefetch:
     if (SemaBuiltinPrefetch(TheCall))
       return ExprError();
     break;
   case Builtin::BI__builtin_alloca_with_align:
     if (SemaBuiltinAllocaWithAlign(TheCall))
       return ExprError();
+    LLVM_FALLTHROUGH;
+  case Builtin::BI__builtin_alloca:
+    Diag(TheCall->getBeginLoc(), diag::warn_alloca)
+        << Context.BuiltinInfo.getName(BuiltinID);
     break;
   case Builtin::BI__assume:
   case Builtin::BI__builtin_assume:
     if (SemaBuiltinAssume(TheCall))
       return ExprError();
     break;
   case Builtin::BI__builtin_assume_aligned:
     if (SemaBuiltinAssumeAligned(TheCall))

clang/test/Sema/warn-alloca.c

+// RUN: %clang_cc1 -fsyntax-only -verify -Walloca %s
+
+void test1(int a) {
+  __builtin_alloca(a); // expected-warning {{use of builtin function __builtin_alloca}}
+}
+
+void test2(int a) {
+  __builtin_alloca_with_align(a, 32); // expected-warning {{use of builtin function __builtin_alloca_with_align}}
+}
+