Modify the AIX clang toolchain to include AIX dependencies in the search path
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
clang/lib/Driver/ToolChains/AIX.cpp | ||
---|---|---|
169 | I don't think this is the revision you intended to post. |
clang/lib/Driver/ToolChains/AIX.cpp | ||
---|---|---|
169 | This is a stale comment, please ignore. |
clang/test/Driver/aix-toolchain-include.cpp | ||
---|---|---|
9 | I don't think we need a separate check prefix for 32/64-bit cases, just the RUN statements. Since the expected output is the same they can share the same prefix. |
LGTM other than minor nit
clang/test/Driver/aix-toolchain-include.cpp | ||
---|---|---|
14 | nit: we usually put the 32-bit / 64-bit run lines together before the CHECK block for readabliity |
clang/lib/Driver/ToolChains/AIX.cpp | ||
---|---|---|
181 | This does not observe --sysroot. |
clang/lib/Driver/ToolChains/AIX.cpp | ||
---|---|---|
174 | Please investigate the handling of OPT_nostdlibinc. |
clang/lib/Driver/ToolChains/AIX.cpp | ||
---|---|---|
174 | If we are looking at this, should we also consider adding /usr/local/include? |
clang/lib/Driver/ToolChains/AIX.cpp | ||
---|---|---|
174 | It seems that, unlike what I found on Linux, the AIX versions of GCC and XL C/C++ do not include /usr/local/include. It seems this is a legitimate platform difference that we should respect. |
Modified changes:
- Add sysroot
- return if options::OPT_nostdlibinc is specified, and will not add /usr/local/include to the path since it's different from other system (linux,darwin etc.)
clang/lib/Driver/ToolChains/AIX.cpp | ||
---|---|---|
181 | Minor nit: Use full sentences in comments with proper capitalization for English writing. | |
183 | My understanding is that -nostdlibinc does not suppress the inclusion of the "builtin" headers. | |
clang/test/Driver/aix-toolchain-include.cpp | ||
2 | s/that AIX/that the AIX/; | |
4 | Minor nit: Remove the space after the slash. | |
41 | There should be testing for -nostdlibinc as well. |
- nostdlibinc should not suppress clang builtin headers
- Add tests for nostdlibinc and nobuiltininc
- fix lint
Aside from one comment; LGTM. Thanks.
clang/test/Driver/aix-toolchain-include.cpp | ||
---|---|---|
5 | I think there should be a clang (as opposed to clang++) test as well. Duplicate this invocation and use clang with -xc. |
LGTM with minor nit.
I took a look at the failed tests in premerge checks, they didn't seem to be related, but please double check before you commit.
clang/lib/Driver/ToolChains/AIX.cpp | ||
---|---|---|
197 | Can we rewrite this block of code so that it's in consistent with the previous one? if (!DriverArgs.hasArg(options::OPT_nostdlibinc)) { // Add <sysroot>/usr/include. SmallString<128> UP(Sysroot); path::append(UP, "/usr/include"); addSystemInclude(DriverArgs, CC1Args, UP.str()); } |
LGTM
clang/lib/Driver/ToolChains/AIX.cpp | ||
---|---|---|
197 | I tend to agree, everything below this point should be standard system includes that all should get ignored with nostdlibinc so I think the early exit makes sense. |
I don't think this is the revision you intended to post.