This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Add support for stpncpy folding
ClosedPublic

Authored by msebor on Aug 1 2022, 11:42 AM.

Details

Summary

As in the case of stlcpy, the middle end support for stpncpy also consists solely of transforming calls to __stpncpy_chk to the standard function if the buffer overflow checking overhead can be avoided, but nothing else. Continuing in the same vein as D130666, this change extends the support for stpncpy to also optimize a subset of calls to it, analogously to strncpy. Both functions have the same semantics and differ only in the return value, with stpncpy returning a pointer either to the copied nul byte or to the last non-nul byte if the copy isn't a string. In addition, it extends the folding of calls to both functions to nonconstant bounds when the source string is empty.

Diff Detail

Event Timeline

msebor created this revision.Aug 1 2022, 11:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2022, 11:42 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
msebor requested review of this revision.Aug 1 2022, 11:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 1 2022, 11:42 AM
msebor planned changes to this revision.Aug 10 2022, 4:36 PM
msebor added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
699

Both stpncpy and strncpy access the destination only when Size is nonzero but not otherwise. (Annotating the argument unconditionally is a pre-existing bug in the code.)

784

stpncpy(D, S, N) returns D + min(strlen(S), N) as documented but not correctly verified in the new stpcpy-1.ll test.

msebor updated this revision to Diff 452807.Aug 15 2022, 2:29 PM

Changes in revision 1 of the patch include:

  • Avoid annotating destination argument when bound is not known to be nonzero.
  • Correct stpncpy return value to strnlen(S, N) rather than strlen(S).
  • Simplify common logic .
  • Add tests.

Ping: Is someone available to review this?

efriedma added inline comments.Sep 13 2022, 3:34 PM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
747

It's confusing to reason about what happens here if SrcLen == UINT64_MAX. Is there a reason you got rid of the early return?

msebor updated this revision to Diff 460150.Sep 14 2022, 10:35 AM

Restore early return when GetStringLength returns zero.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
747

I suspect I forgot that GetStringLength returns nonzero even for constants that aren't nul-terminated strings, including past-the-end pointers (besides also not actually returning their length). I find that confusing.

Ping: @efriedma do you (or anyone else) have any other questions or suggestions or is everyone okay with the last revision?

nikic accepted this revision.Sep 27 2022, 6:20 AM

LGTM

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
717

Can we keep the early return for non-constant size here? As far as I can tell, you don't do anything useful in that case, it just gets discarded later by the N > 128 check. That's a lot less obvious than immediately rejecting all non-constant sizes here.

llvm/test/Transforms/InstCombine/stpncpy-1.ll
18

These are probably intended to have CHECK prefixes? You can use --check-globals to generate them.

This revision is now accepted and ready to land.Sep 27 2022, 6:20 AM
This revision was landed with ongoing or failed builds.Sep 27 2022, 1:44 PM
This revision was automatically updated to reflect the committed changes.
msebor added inline comments.Sep 27 2022, 1:45 PM
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
717

We don't want to reject nonconstant sizes because it would prevent the st{p,r}ncpy(D, "", N) to memset(D, '\0', N) transformation that's done even for any N.

llvm/test/Transforms/InstCombine/stpncpy-1.ll
18

It took me a bit to understand what you meant here. I didn't mean to check the contents of these (it's a pre-existing optimization, and I didn't realize I could) but now that I know about --check-globals I agree it makes the test tighter.