[PATCH] D91677: Avoid simplification of library functions when callee has an implementation

[Siddhesh Poyarekar via Phabricator via llvm-commits]

siddhesh added a comment.

In D91677#2405137 <https://reviews.llvm.org/D91677#2405137>, @serge-sans-paille wrote:

> Interestingly, gcc and clang have different behavior here: https://godbolt.org/z/hdadhY

Hmm, looks like I opened a pandora's box box with this one; the behaviour seems to be very different for different ways in which one implements bcopy and some of them seem incorrect for both compilers.

Without `<string.h>` for example, both compilers converge and ignore the locally defined `bcopy` and replace the call with a memmove.  This seems incorrect but I'll take a closer look.  It shouldn't impact this patch though since the bcopy signature is different.  I can restrict the change to non-empty *and* inline if it makes sense.

> And clang already honors the user-defined `bcopy` why is -D_FORTIFY_SOURCE different from the example here?

I suspect clang is doing something weird there because it doesn't actually inline the bcopy; it simply removes the call; see: https://godbolt.org/z/PEssMe . I think this will be avoided altogether if bcopy simplification is avoided whenever a local definition is available.  Even if the local definition is preemptable, it is always going to be selected first and the compiler should not assume that it will behave the same as the library bcopy.

That said, the _FORTIFY_SOURCE definition is not just an inline hint, it is: `extern inline __attribute__((always_inline)) __attribute__((__gnu_inline__))`.  This should ensure that bcopy is inlined, except that clang doesn't actually inline it and replaces it with a memmove instead.  This one shows more clearly how the behaviour is different between gcc and llvm for fortify-source: https://godbolt.org/z/3baWfY



================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:3076-3079
+  // Simplification is for external library calls, so bail out if the callee
+  // has an implementation.
+  if (!Callee->empty())
+    return nullptr;
----------------
serge-sans-paille wrote:
> siddhesh wrote:
> > lebedev.ri wrote:
> > > And if it happens to be in another TU?
> > It does the simplification; it is not relevant for fortify-source since those implementations are always inline.
> Would it make sense to check that the function is marked inline?
> Redefining a libc function locally is probably not a good thing?
Strictly speaking, I think the local function should always get precedence but if for this patch we want to limit scope, we could limit this to non-empty, inline functions.

Redefining a libc function locally is not uncommon; in fact it is a common way to build an interposition library and take advantage of symbol resolution precedence.


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

https://reviews.llvm.org/D91677