[libc++abi] Limit libc++ header search to specified paths

[smeenai]

Summary

Right now, when libc++abi is locating libc++ headers, it specifies
several search locations, but it also doesn't prevent CMake from looking
for those headers in system directories. I don't know if this was
intentional or an oversight, but it has several issues:

• We're looking specifically for the vector header, which could just as easily be found in a libstdc++ (or other C++ library) installation.
• No system I know of places their C++ headers directly in system include directories (they're always under a C++ subdirectory), so the system search will never succeed.
• find_path searches system paths before the user-specified PATHS, so if some system does happen to have C++ headers in its system include directories, those headers will be preferred, which doesn't seem desirable.

It makes sense to me to limit this header search to the explicitly
specified paths (using NO_DEFAULT_PATH, as is done for the other
find_path call in this file), but I'm putting it up for review in case
there's some use case I'm not thinking of.

[dexonsmith]

What's the effective change on the command-line? Does this map to -nostdinc? To -nostdinc++?

[smeenai]

This doesn't map to any command line change; it's purely a CMake thing. It just changes where libc++abi's build system looks for libc++ headers. Before this patch, it would look for a file named "vector" in all the standard system include directories (/usr/local/include, /usr/include, anything else in your PATH, etc.) and then look for it in the paths specified by the PATHS argument. After this patch it will only look for that file in the paths specified by the PATHS argument. The actual value of LIBCXXABI_LIBCXX_INCLUDES gets added to the header search path when compiling the libc++abi sources, but this diff isn't changing that part (it just potentially changes the value that gets computed for LIBCXXABI_LIBCXX_INCLUDES, but as I detailed in the description I don't think that should actually change on any system I'm aware of, and if it does cause a change then there's a good chance the old value was incorrect).

[EricWF]

LGTM, but I'm a bit confused. You seem to argue that no system places C++ headers on the default search paths, but also that it would be a problem if such a system did.
Why wouldn't the conclusion be true? That is, although C++ includes aren't normally found along the default paths, when they are found, we should still consider them?

That being said, the current behavior of searching certian default include paths first does seem incorrect. So I'm OK with disabling it.

I also agree that we shouldn't necessarily be looking for vector header, how about looking for __config instead, since it's libc++ specific?

[ldionne]

I guess I'm echoing some of Eric's comments, but does it make sense to build libc++abi against a system-installed libc++? If not, this change makes sense.

[smeenai]

In D48694#1146232, @EricWF wrote:

LGTM, but I'm a bit confused. You seem to argue that no system places C++ headers on the default search paths, but also that it would be a problem if such a system did.
Why wouldn't the conclusion be true? That is, although C++ includes aren't normally found along the default paths, when they are found, we should still consider them?

That being said, the current behavior of searching certian default include paths first does seem incorrect. So I'm OK with disabling it.

I also agree that we shouldn't necessarily be looking for vector header, how about looking for __config instead, since it's libc++ specific?

Switching to __config is a good idea. I can do that separately, since it should be pretty uncontroversial.

For whether it makes sense to search for includes in the system path, that boils down to @ldionne's question of whether building libc++abi against a system-installed libc++ is supported. I actually don't know the answer to that myself ... @dexonsmith and @EricWF, what are your thoughts on that? The current search won't find the system-installed libc++ headers on Darwin anyway though, where they're placed in the compiler's include directory rather than a system include directory.

If we do decide that searching for C++ headers in the system is important, we can do two separate find_path calls, to ensure our specified paths are searched before the system paths. (We may also want to tweak the second call to actually look for the system C++ headers in the right places in that case.)

[EricWF]

For whether it makes sense to search for includes in the system path, that boils down to @ldionne's question of whether building libc++abi against a system-installed libc++ is supported. I actually don't know the answer to that myself ... @dexonsmith and @EricWF, what are your thoughts on that? The current search won't find the system-installed libc++ headers on Darwin anyway though, where they're placed in the compiler's include directory rather than a system include directory.

Building libc++abi against an installed libc++ doesn't make sense IMO, so I don't care if we try to support it. Libc++abi is a lower lever component with libc++ being built on top of it.
If you want to update libc++abi, you should be building a new libc++ as well. (Note: Using system libc++abi headers to build libc++ would make sense though).

As an aside, libc++abi should really live in the libc++ repository. That way it would always have the correct headers available. But every time I pitch that idea I get a ton of push back.

[smeenai]

In D48694#1148718, @EricWF wrote:

For whether it makes sense to search for includes in the system path, that boils down to @ldionne's question of whether building libc++abi against a system-installed libc++ is supported. I actually don't know the answer to that myself ... @dexonsmith and @EricWF, what are your thoughts on that? The current search won't find the system-installed libc++ headers on Darwin anyway though, where they're placed in the compiler's include directory rather than a system include directory.

Building libc++abi against an installed libc++ doesn't make sense IMO, so I don't care if we try to support it. Libc++abi is a lower lever component with libc++ being built on top of it.
If you want to update libc++abi, you should be building a new libc++ as well. (Note: Using system libc++abi headers to build libc++ would make sense though).

As an aside, libc++abi should really live in the libc++ repository. That way it would always have the correct headers available. But every time I pitch that idea I get a ton of push back.

All right. In that case I'll go ahead and commit this and make the __config change in a follow-up.

[ldionne]

In D48694#1148718, @EricWF wrote:

As an aside, libc++abi should really live in the libc++ repository. That way it would always have the correct headers available. But every time I pitch that idea I get a ton of push back.

FWIW, I think I would support that. I'd be curious to hear other people's concerns about this.

libcxxabi/trunk/CMakeLists.txt

 find_path(
   LIBCXXABI_LIBCXX_INCLUDES
   vector
   PATHS ${LIBCXXABI_LIBCXX_INCLUDES}
         ${LIBCXXABI_LIBCXX_PATH}/include
         ${CMAKE_BINARY_DIR}/${LIBCXXABI_LIBCXX_INCLUDES}
         ${LIBCXXABI_LIBCXX_INCLUDE_DIRS}
         ${LLVM_INCLUDE_DIR}/c++/v1
+  NO_DEFAULT_PATH
   NO_CMAKE_FIND_ROOT_PATH
   )
 
 set(LIBCXXABI_LIBCXX_INCLUDES "${LIBCXXABI_LIBCXX_INCLUDES}" CACHE PATH
     "Specify path to libc++ includes." FORCE)
 
 find_path(
   LIBCXXABI_LIBCXX_PATH