Page MenuHomePhabricator

Fix missing memcpy, memmove and memset tail calls
ClosedPublic

Authored by sanwou01 on Mar 7 2019, 1:45 AM.

Details

Summary

If a wrapper around one of the mem* stdlib functions bitcasts the returned
pointer value before returning it (e.g. to a wchar_t*), LLVM does not emit a
tail call.

Add a check for this scenario so that we emit a tail call.

Diff Detail

Event Timeline

ramred01 created this revision.Mar 7 2019, 1:45 AM
ramred01 updated this revision to Diff 192592.Mar 28 2019, 3:00 AM
ramred01 edited the summary of this revision. (Show Details)

Updated the summary to properly reflect the issue and the solution.

Added a test case.

wmi accepted this revision.Apr 1 2019, 2:47 PM

LGTM.

This revision is now accepted and ready to land.Apr 1 2019, 2:47 PM
sanwou01 commandeered this revision.Oct 30 2019, 5:16 AM
sanwou01 added a reviewer: ramred01.
sanwou01 added a subscriber: sanwou01.

Hi, I'd like to take over this change to get it committed. There is a crash lurking when llvm::returnTypeIsEligibleForTailCall is called with a function that takes no arguments, so I'd like to make some changes before committing. Thanks.

sanwou01 updated this revision to Diff 227067.Oct 30 2019, 5:17 AM
sanwou01 retitled this revision from memcpy is not tailcalled to Fix missing memcpy, memmove and memset tail calls.
sanwou01 edited the summary of this revision. (Show Details)

Fix crash and refactor for readability

Herald added a project: Restricted Project. · View Herald TranscriptOct 30 2019, 5:17 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
dmgreen accepted this revision.Oct 30 2019, 9:08 AM

Yeah. LGTM. Thanks for carrying this forward!

llvm/lib/CodeGen/Analysis.cpp
617 ↗(On Diff #227067)

I don't think these two asserts are required if you wanted to leave them out. It's pretty much implied from the signature that the two values will not be null.

If you do want to keep them in, run clang format and add a message to the asserts. Something like:
assert(a && b && "Expected non-null inputs!");

This revision was automatically updated to reflect the committed changes.
sanwou01 marked an inline comment as done.