[PATCH] D131979: [clang][UBSan] Fix __builtin_assume_aligned crash

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

Thanks.  Just a few small requests to re-use more of the existing logic here.



================
Comment at: clang/lib/Sema/SemaChecking.cpp:7659
+           << 0 << 2 << TheCall->getNumArgs() << Callee->getSourceRange();
+  }
+
----------------
It looks like this should be `if (NumArgs < 2)`.

We actually have helper functions in this file for doing this kind of arg-count checking, but it looks like we don't have one that takes a range.  Could you just generalize `checkArgCount` so that it takes a min and max arg count?  You can make the existing function call your generalization.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:7671
+                             AllArgs, CallType))
+    return true;
+
----------------
You can just pull the argument expressions out of the `CallExpr`; you don't need to call `GatherArgumentsForCall`.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:7684
+  if (FirstArgResult.isInvalid())
+    return true;
+
----------------
There is in fact a `DefaultFunctionArrayLvalueConversion` method you can use to do all of this at once, and you don't need to explicitly check for a function or array type first.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:7690
+  // cast instruction which cast type from user-written-type to VoidPtr in
+  // CodeGen.
+  AllArgs[0] = FirstArgResult.get();
----------------
This comment doesn't mean anything in the context of the current implementation.  If you want to keep something like it (you really don't need to), you could have a comment at the top explaining that we use custom type-checking because it accepts an arbitrary pointer type as the first argument.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:7695
+    TheCall->setArg(i, AllArgs[i]);
+  TheCall->computeDependence();
+
----------------
This is really just `TheCall->setArg(i, FirstArgResult.get())`.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:7701
   // We can't check the value of a dependent argument.
-  if (!Arg->isTypeDependent() && !Arg->isValueDependent()) {
+  if (!SecondArg->isTypeDependent() && !SecondArg->isValueDependent()) {
     llvm::APSInt Result;
----------------
Type-dependence implies value-dependence, so you can just check for the latter.

You should call `convertArgumentToType` with `size_t` in this block, just in case the argument is something like an l-value reference to a `const` integer variable.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:7719
+        Context, Context.getSizeType(), false);
+    ThirdArg = PerformCopyInitialization(Entity, SourceLocation(), ThirdArg);
+    if (ThirdArg.isInvalid())
----------------
You can use `convertArgumentToType` here.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131979