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

[Siddhesh Poyarekar via Phabricator via llvm-commits]

siddhesh added inline comments.


================
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;
----------------
siddhesh wrote:
> 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.
> 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.

I misspoke here; there is a subtle difference between the idea of the interposition library and interposition/overriding of functions.  I meant to specifically refer to overriding of functions here, i.e. making sure that the current TU uses the local version of the function and not the global one.  It is not prohibited or even discouraged.


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

https://reviews.llvm.org/D91677