This is an archive of the discontinued LLVM Phabricator instance.

[OSX][TargetLibraryInfo] Mark memrchr as unavailable on OSX
ClosedPublic

Authored by kpdev42 on Jul 13 2023, 3:12 AM.

Details

Summary

On Mac OSX (tested version macOS 12.4, sdk 12.1) llvm can replace call to strrchr() with call to memrchr() when string length is known at compile time. This results in link error, because memrchr is not present in libSystem. It is needed to disable this optimization in TargetLibraryInfo for affected OSX versions.

This non-standard function (memrchr) is not present on (at least) several versions of MacOS https://www.gnu.org/software/gnulib/manual/html_node/memrchr.html , so, in this patch memrchr is marked as unavailable for all versions. If someone knows versions where it should be available - please let me know.

Github issue: https://github.com/llvm/llvm-project/issues/62254

Tests for this feature also cannot be easily added: https://reviews.llvm.org/D134134#3801747

~~~

Huawei RRI, OS Lab

Diff Detail

Event Timeline

kpdev42 created this revision.Jul 13 2023, 3:12 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 13 2023, 3:13 AM
kpdev42 requested review of this revision.Jul 13 2023, 3:13 AM
nikic added a comment.Jul 13 2023, 3:18 AM

Tests for this feature also cannot be easily added: https://reviews.llvm.org/D134134#3801747

You should be able to test that InstCombine does not perform the strrchr -> memrchr fold if a macos triple is given.

llvm/lib/Analysis/TargetLibraryInfo.cpp
206

Isn't this supposed to be memrchr?

kpdev42 updated this revision to Diff 539922.Jul 13 2023, 3:23 AM
kpdev42 retitled this revision from [OSX][TargetLibraryInfo] Mark memchr as unavailable on OSX to [OSX][TargetLibraryInfo] Mark memrchr as unavailable on OSX.

Fix typo (memchr -> memrchr)

kpdev42 marked an inline comment as done.Jul 13 2023, 3:23 AM
kpdev42 added inline comments.
llvm/lib/Analysis/TargetLibraryInfo.cpp
206

Thank you, fixed

kpdev42 marked an inline comment as done.Jul 13 2023, 3:43 AM

Tests for this feature also cannot be easily added: https://reviews.llvm.org/D134134#3801747

You should be able to test that InstCombine does not perform the strrchr -> memrchr fold if a macos triple is given.

Ok, will add it

@nikic Is this good to merge? This issue is present in LLVM 15 and 16 is breaking packages that use strrchr on macOS with a cryptic error (see eg. https://github.com/macports/macports-ports/pull/19868). Hopefully this can be merged before LLVM 17 is released.

xbolva00 edited the summary of this revision. (Show Details)Aug 13 2023, 12:01 PM
nikic accepted this revision.Aug 13 2023, 1:25 PM

LGTM

This revision is now accepted and ready to land.Aug 13 2023, 1:25 PM

@nikic @mohd-akram Thank you for the review