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

[Wang Yihan via Phabricator via cfe-commits]

yihanaa added a comment.

Thanks for your review John, I'll try to fix these problem later which you point out.



================
Comment at: clang/lib/Sema/SemaChecking.cpp:7659
+           << 0 << 2 << TheCall->getNumArgs() << Callee->getSourceRange();
+  }
+
----------------
rjmccall wrote:
> 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.
It's my fault, I'll fix it, thanks!


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

This GatherArgumentsForCall  was used to do the common sema checking and emit warning, like './main.cpp:5:40: warning: passing 'volatile char *' to parameter of type 'const void *' discards qualifiers [-Wincompatible-pointer-types-discards-qualifiers]' hahaha, for this is a common case, I also think  GatherArgumentsForCall is not a good choice
, so I try to find a replacement, e.g. ImpCastExprToType or other ways, what do you think about?


================
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();
----------------
rjmccall wrote:
> 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.
+1


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


================
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;
----------------
rjmccall wrote:
> 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.
+1


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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131979