This is an archive of the discontinued LLVM Phabricator instance.

[libc] Move strdup implementation to a new header
ClosedPublic

Authored by jhuber6 on Nov 23 2022, 1:44 PM.

Details

Summary

The strdup family of functions rely on malloc to be implemented.
Its presence in the string_utils.h header meant that compiling many of
the string functions relied on malloc being implementated as well.
This patch simply moves the implementation into a new file to avoid
including stdlib.h from the other string functions. This was a barrier
for compiling string functions for the GPU where there is no malloc
currently.

Diff Detail

Event Timeline

jhuber6 created this revision.Nov 23 2022, 1:44 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 23 2022, 1:44 PM
jhuber6 requested review of this revision.Nov 23 2022, 1:44 PM

I would think that GPU is one example of a platform where malloc isn't (yet) available. So, the mechanical aspects of this patch LGTM. I think we should do a few things more:

  1. At the least, add comments warning developers that allocators should not be called from string_utils.h. Likewise, add comments explaining what exactly is string_utils_malloc.h (or whatever it will be called) for.
  2. Does not have to be done in this patch, but would be nice if a developer mistake/mixup can be caught during build + test cycle. Worst case, the GPU builder will catch it I believe, but would be nice to be able to do it during a normal host build + test development cycle.
libc/src/string/string_utils_malloc.h
1

malloc is one among a bunch of standard allocator functions. So, may be call this allocating_string_utils.h?

jhuber6 updated this revision to Diff 478240.Nov 28 2022, 7:28 AM

Addressing comments

sivachandra accepted this revision.Nov 28 2022, 9:19 AM

You should add another header library for the new header file like this: https://github.com/llvm/llvm-project/blob/main/libc/src/string/CMakeLists.txt#L3. Otherwise LGTM.

This revision is now accepted and ready to land.Nov 28 2022, 9:19 AM
This revision was automatically updated to reflect the committed changes.