[PATCH] D123626: [InstCombine] Fold memrchr calls with constant size of zero or one.

[Martin Sebor via Phabricator via llvm-commits]

msebor added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp:873
+  Value *Size = CI->getArgOperand(2);
+  annotateNonNullAndDereferenceable(CI, 0, Size, DL);
+  Value *CharVal = CI->getArgOperand(1);
----------------
xbolva00 wrote:
> xbolva00 wrote:
> > nikic wrote:
> > > nikic wrote:
> > > > I'm not entirely sure this is correct. This is definitely incorrect for memchr() and I submitted D123665 to fix that. memrchr() is a GNU extension, so I'm not sure whether the same rules apply there. I guess one could argue that memrchr() should require a fully dereferenceable object, as otherwise the base pointer will already point outside the object.
> > > As a data point, sanitizer interception for these functions is implemented as follows: https://github.com/llvm/llvm-project/blob/ee44896cf42d3360f7e0229c360258c92262a47b/compiler-rt/lib/sanitizer_common/sanitizer_common_interceptors.inc#L984-L1021 So while the interceptor does not assume full dereferenceability for memchr(), it does assume so for memrchr(). So your current implementation would be consistent with sanitizers.
> > Any sanitizer developer here?
> > 
> > It would be great to know why there is a such differenr behaviour.
> “ I guess one could argue that memrchr() should require a fully dereferenceable object, as otherwise the base pointer will already point outside the object”
> 
> But yeah this makes sense
Good catch on the `memchr` annotation!  For `memrchr`, as you note, the whole range [base, base + n) must be dereferenceable even if it's not necessarily always accessed.


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

https://reviews.llvm.org/D123626