george.burgess.iv added a comment.

Thanks for looking into this!

I dunno what implications this has elsewhere, but I'd like to note a few things 
about this general approach:

- AIUI, this'll only work for FORTIFY functions which are literally calling 
`__builtin___foo_chk`; an equivalent implementation without such a builtin, 
like below, should be blocked by other code in clang 
<https://github.com/llvm/llvm-project/blob/9c3f9b9c12b0f79b74d1d349bbac46cadaca7dbf/clang/lib/CodeGen/CodeGenModule.cpp#L2695-L2728>,
 since it results in infinite recursion <https://godbolt.org/z/mDZ-rs>. glibc 
implements quite a few (most?) FORTIFY'ed functions in this way.



  void *memcpy(void *a, void *b, size_t c) {
    if (__builtin_object_size(a, 0) == -1)
      return memcpy(a, b, c);
    return __memcpy_chk(a, b, c, __builtin_object_size(a, 0));
  }



- This change only gives us `-D_FORTIFY_SOURCE=1`-level checking here. If we 
want to offer better support for `-D_FORTIFY_SOURCE=2` (including potentially 
FORTIFY's compile-time diagnostics), which is AIUI what most of the world uses, 
we'll need to add clang-specific support into glibc's headers. At that point, 
this patch becomes a nop, since the FORTIFY'ed memcpy becomes an overload for 
the built-in one.

In other words, I think this patch will improve our support for the pieces of 
glibc's FORTIFY that're implemented with `__builtin___*_chk`, but I don't think 
there's much more room to incrementally improve beyond that before we need to 
start hacking on glibc directly. If we get to that point, the changes we need 
to make in glibc will likely obviate the need for this patch.

So if your overarching goal here is to make clang support glibc's FORTIFY as-is 
a bit better, that's great and I'm personally cool with this patch (again, not 
being deeply aware of what interactions this may have in a non-FORTIFY'ed 
context). If the intent is to build off of this to try and make glibc's FORTIFY 
story with clang a great one, I'm unsure how useful this patch will be in the 
long run.



================
Comment at: clang/lib/AST/Decl.cpp:3007
+bool FunctionDecl::isReplaceableSystemFunction() const {
+  FunctionDecl const *Definition;
+  if (hasBody(Definition)) {
----------------
style nit: `const FunctionDecl *` is the preferred ordering in most of LLVM


================
Comment at: clang/lib/AST/Decl.cpp:3179
+  // C library do that to implement fortified version.
+  if (isReplaceableSystemFunction()) {
+    return 0;
----------------
style nit: please don't use braces here or below, for consistency with nearby 
code.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71082



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

Reply via email to