rjmccall added a comment.

Thanks, patch basically looks good.  Minor requests and an observation for 
possible follow-up work.



================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2247
       // Otherwise, evaluate and record it.
-      if (const Expr *size = vat->getSizeExpr()) {
+      if (const Expr *SizeExpr = vat->getSizeExpr()) {
         // It's possible that we might have emitted this already,
----------------
Please continue to use lowercase names for local variables for consistency with 
the surrounding code.  The rename to `sizeExpr` is good; you can then rename 
`Size` to `size` or `sizeValue`.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2259
           //   greater than zero.
-          if (SanOpts.has(SanitizerKind::VLABound) &&
-              size->getType()->isSignedIntegerType()) {
+          if (SanOpts.has(SanitizerKind::VLABound) && SEType->isIntegerType()) 
{
             SanitizerScope SanScope(this);
----------------
Sema requires the bound expression to have integral type, so you don't need to 
do this.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2275
           // undefined behavior to have a negative bound.
-          entry = Builder.CreateIntCast(Size, SizeTy, /*signed*/ false);
+          MapEntry = Builder.CreateIntCast(Size, SizeTy, /*signed*/ false);
         }
----------------
This would be a different bug, but should UBSan also be doing a bounds check if 
the type is larger than `size_t`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116048

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

Reply via email to