erik.pilkington added inline comments.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:319
+ // If the parameter has a pass_object_size attribute, then we should use
+ // it's (potentially) more strict checking mode. Otherwise, conservatively
+ // assume type 0.
----------------
aaron.ballman wrote:
> it's -> its
The opposite mistake as above, lol.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:321
+ // assume type 0.
+ int BOSType = 0;
+ if (auto *POS = FD->getParamDecl(ObjIdx)->getAttr<PassObjectSizeAttr>())
----------------
george.burgess.iv wrote:
> Should this also try to consider `fortify_stdlib`?
I reverted `fortify_stdlib` in r356103 (although you couldn't possible know
this, since that was after you made this comment ;)). We're going to use your
`pass_object_size` attribute instead.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:367
+ DiagID = diag::warn_memcpy_chk_overflow;
+ if (!evaluateObjectSize(TheCall->getNumArgs()-1) ||
+ !evaluateSizeArg(TheCall->getNumArgs()-2))
----------------
aaron.ballman wrote:
> george.burgess.iv wrote:
> > nit: All of these cases (and the two lambdas above) look super similar.
> > Might it be clearer to just set `SizeIndex` and `ObjectIndex` variables
> > from here, and actually `evaluate` them before `UsedSize.ule(ComputedSize)`?
> >
> > If not, I'm OK with this as-is.
> Formatting looks off here -- run the patch through clang-format?
Sure, I guess it's a bit cleaner to do that.
================
Comment at: clang/lib/Sema/SemaChecking.cpp:423
+ StringRef FunctionName = getASTContext().BuiltinInfo.getName(BuiltinID);
+ if (DiagID == diag::warn_memcpy_chk_overflow) {
+ // __builtin___memcpy_chk -> memcpy
----------------
aaron.ballman wrote:
> Why don't we want to do this for the new fortify diagnostics?
No reason, just was trying to avoid changing the output of the `_chk`
diagnostic. We might as as well though, it cleans up the diagnostic.
================
Comment at: clang/lib/Sema/SemaExpr.cpp:5929
+ checkFortifiedBuiltinMemoryFunction(FDecl, TheCall);
+
----------------
george.burgess.iv wrote:
> Why isn't this in CheckBuiltinFunctionCall?
For the same reason I added the `bool` parameter to `getBuiltinCallee`, we
don't usually consider calls to `__attribute__((overloadable))` be builtins, so
we never reach `CheckBuiltinFunctionCall` for them. We're planning on using
that attribute though:
```
int sprintf(__attribute__((pass_object_size(_FORTIFY_LEVEL))) char *buffer,
const char *format, ...)
__attribute__((overloadable))
__asm__("___sprintf_pass_object_size_chk");
```
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D58797/new/
https://reviews.llvm.org/D58797
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
