[PATCH] D115904: [DSE] Remove calls with known writes to dead memory

[Philip Reames via Phabricator via llvm-commits]

reames added a comment.

In D115904#3201217 <https://reviews.llvm.org/D115904#3201217>, @nikic wrote:

> Reverted this because it does break the strncpy-overflow.cpp test case in asan (https://github.com/llvm/llvm-project/blob/main/compiler-rt/test/asan/TestCases/strncpy-overflow.cpp). The strncpy is now getting optimized away (correctly). Not sure what the policy in this case is, but I assume it's updating the test case to be more robust rather than trying to avoid the optimization in sanitized functions?

First, thank you for reverting.  I'd seen the build failures, but since we all coming from bots I've mentally marked as unstable (as in, they fail often enough there's no signal in the messages), I didn't see the real failure here.

Second, yeah, that test is simply wrong/unstable.  I will take a shot at trying to stabilize it, but I see at least two other transform paths which can break it as written.  I'll probably just end up deferring the problem to the next person, but hey, that's how this sometimes works.

> I found this change a bit surprising because I thought we'd already do this based on the previous attribute code in InstCombine. The reason we didn't is that strncpy returns the first argument, which means that it's not nocapture. But we do provide getForDest() information for this libcall, and that got exposed to InstCombine now.

Well, not quite.  So long as the return value is unused, the strncpy is in fact nocapture on both arguments.

This is confirming my suspicion that nocapture is too conservative for this case.  If the only capture can be a write to the dead memory, then the callee is effectively nocapture.

Given DSE wasn't previously checking for the use on the return value, I suspect we may have had a latent miscompile before your recent change

> I guess this is also why you say failures when dropping the libcall handling: We do still need it for libcalls that return the argument.

I went back and skimmed the tests in libcalls.ll (the ones which failed without the libcall handling).  None of them cover the case where a strncpy actually captures by returning.  It's possibly I'd have noticed that while updating tests, but equally likely I'd have missed it.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115904