[PATCH] D71082: Allow system header to provide their own implementation of some builtin

[George Burgess IV via Phabricator via cfe-commits]

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