<div dir="ltr">I'm currently not entirely sure we want to do this class of optimizations for a few reasons:<div><br></div><div>a) This ends up being an interesting educational problem on the contents of a particular platform's libc - in particular, people that write small libc interceptors are going to have to update just for this in order to successfully build. I updated a few recently, but this seems to be something that's going to be frustrating at llvm release time. It will require anyone that's using a libc interceptor to run into some odd problems if they don't -fno-builtin some new functions or make sure that they add the unlocked versions which are technically not required, but may exist on a system.</div><div><br></div><div>b) I believe I've found a bug in the current (theoretical) basis of this which is: fopen could capture the file argument and do whatever it wants with it. (and even after my fix, -fno-builtin-fopen still didn't have individual -fno-builtin support which means you really couldn't distinguish this at the time from clang - I've since fixed this). This may not be the case anymore, but...</div><div><br></div><div>What's the overall gain we're getting for doing this optimization? It seems to be a lot of pain for not a lot of win in general applications? Is there some benchmark here?</div><div><br></div><div>Thoughts?</div><div><br></div><div>-eric</div><div><br><div class="gmail_quote"><div dir="ltr">On Thu, May 17, 2018 at 3:13 PM Dávid Bolvanský via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="auto">You are right so I would like to apologize you.<div dir="auto"><br></div><div dir="auto">If you revert it, I would be fine too. I do my best to do/fix things in this patch anyway :)</div></div><br><div class="gmail_quote"><div dir="ltr">Dňa pi 18. 5. 2018, 0:09 Friedman, Eli <<a href="mailto:efriedma@codeaurora.org" target="_blank">efriedma@codeaurora.org</a>> napísal(a):<br></div></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<div text="#000000" bgcolor="#FFFFFF">
<div class="m_2810723606690624818m_-4218521709411879700moz-cite-prefix">On 5/17/2018 2:51 PM, Eric Christopher
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr"><br>
<br>
<div class="gmail_quote">
<div dir="ltr">On Wed, May 16, 2018 at 3:09 PM Friedman, Eli
via llvm-commits <<a href="mailto:llvm-commits@lists.llvm.org" rel="noreferrer" target="_blank">llvm-commits@lists.llvm.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">On
5/16/2018 2:54 PM, Benjamin Kramer via llvm-commits wrote:<br>
> This triggered a really annoying miscompile (see
r332531), and it's even<br>
> present in the test case. Please double check your
tests, especially when<br>
> using update_test_checks.py.<br>
><br>
> Also you want to get these things reviewed by someone
who's actually<br>
> familiar with the LLVM before they land.<br>
<br>
I looked at the patch multiple times, and never spotted this
issue. <br>
Granted, maybe I should have looked a little more carefully
at that part <br>
of the patch (I was more focused on the actual transform).<br>
</blockquote>
<div><br>
</div>
<div>Probably would have helped here if you'd ack'd the patch
yourself. The only approval on the patch is an anonymous
person with no llvm history (as far as I can tell).</div>
<div><br>
</div>
<div>-eric <br>
</div>
</div>
</div>
</blockquote>
<p>The timeline was something like this: I reviewed it on May 8, and
gave it sort of tentative LGTM, pending the discussion on
llvmdev. The discussion happened on llvmdev, David posted a
slightly updated path in response to that discussion, and I meant
to review that, but it sort of sank to the bottom of my inbox, and
David merged on the 15th (?) without a review on the updated
version. (I really shouldn't have let it sit that long, but you
know how reviews tend to pile up.)<br>
</p>
<p>David shouldn't have merged it without an explicit ack from me or
another qualified reviewer, but I don't think we need to revert at
this point.<br>
</p>
<p>-Eli<br>
</p>
<pre class="m_2810723606690624818m_-4218521709411879700moz-signature" cols="72">--
Employee of Qualcomm Innovation Center, Inc.
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project</pre>
</div>
</blockquote></div><div class="gmail_quote"></div>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank">llvm-commits@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote></div></div></div>