[PATCH] D57918: Add an attribute that causes clang to emit fortified calls to C stdlib functions

Mon Feb 11 15:21:20 PST 2019

erik.pilkington marked 8 inline comments as done.
erik.pilkington added inline comments.

================
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.
----------------
aaron.ballman wrote:
> 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?
Sure, I added that in the commit.

================
Comment at: clang/lib/Sema/SemaDeclAttr.cpp:6437
+    S.Diag(AL.getArgAsExpr(0)->getBeginLoc(),
+           diag::err_attribute_argument_outof_range)
+        << AL << 0 << 3;
----------------
aaron.ballman wrote:
> lol, I'll fix that typo after you land your patch.
Thanks, guess you're less lazy then me :)

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

https://reviews.llvm.org/D57918