This is an archive of the discontinued LLVM Phabricator instance.

[libc] Switch functions to using global headers
ClosedPublic

Authored by michaelrj on Nov 12 2020, 4:31 PM.

Details

Summary

This switches all of the files in src/string, src/math, and
test/src/math from using relative paths (e.g. #include “include/string.h”)
to global paths (e.g. #include <string.h>) to make bringing up those
functions on other platforms, such as fuchsia, easier.

Diff Detail

Event Timeline

michaelrj created this revision.Nov 12 2020, 4:31 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 12 2020, 4:31 PM
michaelrj requested review of this revision.Nov 12 2020, 4:31 PM

This does not sound right if standard system includes are not excluded (-nostdinc)

sivachandra added a comment.EditedNov 13 2020, 12:32 PM

I think Michael is trying to satisfy a Fuchsia use case which is effectively like taking sources from LLVM libc and dropping them into Fuchsia's libc. In which case, the internal paths currently used by LLVM libc will not work. After going through all the combinations in this change, I think we should probably not do this for all headers. Take threads.h for example. It has type definitions which cannot be mixed with threads.h from elsewhere. This is effectively what Fangrui is pointing out - mixing is fraught with landmines.

So, Michael, we should probably limit this to just math.h and string.h as the functions from these headers are not ABI or platform sensitive. That is, you should only replace #include "include/math.h" with #include <math.h>, and #include "include/string.h with #include <string.h> from within implementations of math and string functions only. Also, I think the current includes of include/string.h can be replaced with includes of stddef.h? If so, then may be do that instead of including the public header.

By limiting to just string.h and math.h, we are not only saying that it is OK to take sources of LLVM libc's string.h and math.h functions and drop them into another libc's source tree, we are also saying that doing the same with other functions is not OK. That doesn't mean this situation is completely fine. We need to have a more principled way of doing this, at the least add a document somewhere explaining this inconsistency. LLVM libc'c build itself is protected by the clang tidy check llvmlibc-restrict-system-libc-headers. May be, we should additionally have some check which catches inclusion of public headers in implementations of ABI sensitive functions. All of these can be taken up outside this patch.

reset the #includes for everything not in src/string and src/math

michaelrj edited the summary of this revision. (Show Details)Nov 16 2020, 10:14 AM

switch the tests to using the global headers to match the sources they're testing.

michaelrj edited the summary of this revision. (Show Details)Nov 17 2020, 11:05 AM
sivachandra accepted this revision.Nov 19 2020, 11:26 PM

I will consider this experimental for now. We should follow up with a plan for a tool based build time guidance which will tell us if including the header <...> is OK.

This revision is now accepted and ready to land.Nov 19 2020, 11:26 PM
This revision was automatically updated to reflect the committed changes.