aaron.ballman accepted this revision.
aaron.ballman added a comment.
LGTM aside from some very minor nits.
================
Comment at: clang/include/clang/Basic/AttrDocs.td:987-989
+The first argument to the attribute is the type passed to
+`__builtin_object_size`, and the second is the flag that the fortified format
+functions accept.
----------------
erik.pilkington wrote:
> aaron.ballman wrote:
> > Maybe I'm being dense, but I still have no idea what I'd pass for either of
> > these arguments. When I hear "type", I immediately assume I should pass in
> > something like 'int', but that seems wrong given that this is an integer
> > argument. Is there some public documentation we could link to with a "for
> > more information, see <blah>" snippet? Similar for the fortified format
> > function flag.
> >
> > However, I'm also starting to wonder why this attribute is interesting to
> > users. Why not infer this attribute automatically, since there's only a
> > specified set of standard library functions that it can be applied to
> > anyway? Is it a bad idea to try to infer this for some reason?
> Yeah, I guess we could link to GCC's docs for `__builtin_object_size`, I'll
> update this. I think the flag argument has something to do with enabling
> checking `%n` output parameters, but I'm not totally sure and don't want to
> spread any misinformation in the clang docs. On our implementation, the value
> is just ignored.
>
> This attribute would never really be used by users that aren't C library
> implementers. The problem with doing this automatically is that library users
> need to be able to customize the checking mode by defining the
> `_FORTIFY_SOURCE` macro to a level in their `.c` files. We can't do this
> based on the presence of that macro for a couple reasons, firstly, I'm not
> sure we can assume that all `*_chk` variants are present just because
> `_FORTIFY_SOURCE` is defined (whether the `_chk` variants are present seems
> like a decision best left to the library). And secondly because that would
> mean that `clang -E t.c -o - | clang -xc -` would generate different code
> from `clang t.c`. Given that, it seems like an attribute is the best
> customization point.
Thank you for the explanation -- I agree that an attribute probably makes sense
then. I'd appreciate a note in the docs saying something along the lines of
"This attribute is intended for use within standard C library implementations
and should not generally be used for user applications." or some such. WDYT?
================
Comment at: clang/include/clang/Basic/AttrDocs.td:994-1012
+ - memcpy
+ - memmove
+ - memset
+ - stpcpy
+ - strcat
+ - strcpy
+ - strlcat
----------------
Rather than enumerate a long list, can you shorten it vertically by grouping
the functions? e.g., strpcpy, strcpy, et. al. on the same line
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1491
+ llvm::Value *FlagVal = llvm::ConstantInt::get(IntTy, Flag);
+ auto emitObjSize = [&]() -> llvm::Value * {
+ return evaluateOrEmitBuiltinObjectSize(CE->getArg(0), BOSType, SizeTy,
----------------
Is the explicit trailing return here needed? I would have assumed this could be
inferred.
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1548
+ assert(Err == ASTContext::GE_None && "Should not codegen an error");
+ llvm::FunctionType *LLVMVariantTy =
+ cast<llvm::FunctionType>(ConvertType(VariantTy));
----------------
`auto *`
================
Comment at: clang/lib/CodeGen/CGBuiltin.cpp:1574
+ if (auto *Attr = FD->getAttr<FortifyStdLibAttr>())
+ return emitFortifiedStdLibCall(*this, E, BuiltinID, Attr->getType(),
----------------
`const auto *` and some other identifier than `Attr` (so it won't conflict with
the type name).
================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6437
+ S.Diag(AL.getArgAsExpr(0)->getBeginLoc(),
+ diag::err_attribute_argument_outof_range)
+ << AL << 0 << 3;
----------------
lol, I'll fix that typo after you land your patch.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57918/new/
https://reviews.llvm.org/D57918
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
