search

llvm / llvm-project

The LLVM Project is a collection of modular and reusable compiler and toolchain technologies.
http://llvm.org
Other
28.65k stars 11.84k forks source link

C_INCLUDE_DIRS feature is messed up #61328

Open pogo59 opened 1 year ago

pogo59 commented 1 year ago

When doing some driver work a little while ago, I bumped into code handling C_INCLUDE_DIRS, which appears to be replicated across many driver toolchain classes. My first thought was to refactor all those duplicates into a common utility function, but on closer inspection they aren't all the same.

First off, C_INCLUDE_DIRS is a CMake variable that is a PATH-like string that lets you specify system include directories; the driver handling typically skips adding $sysroot/include (and maybe others, varies with target) and use what's in the C_INCLUDE_DIRS string instead. This appears to be legacy going back to the Autoconf days, with references to configure --with-c-include-dirs popping up. I have no idea whether anyone actually uses this.

Even so, there are issues.

  1. Some targets prepend the sysroot to absolute paths from C_INCLUDE_DIRS, and leave relative paths along. Other targets do it the other way around. (This inconsistency stopped me from doing the refactor. The code for all targets is otherwise identical, except for the name of the variable containing the sysroot to use.)
  2. Not all targets support this. I removed PS4/PS5 support it because it makes no sense for us (and it existed in the first place only because I was cargo-culting other targets). I don't know what the story is for other targets.
  3. The separator is hard-coded to be a colon, which totally fails on a Windows hosted cross compiler. (Windows targets do not support C_INCLUDE_DIRS at all.) It should use llvm::sys::EnvPathSeparator instead. But, that means updating all the 8-10 places that do this bit of code, because factoring into a utility routine would change behavior (see item 1).

I know too little about how different environments and distros handle where they put things, so I have no opinion about what if anything should be done here. But item 1 in particular seems very weird. Personally I'd be inclined to rip it all out, but maybe it's useful to someone.

Tagging @MaskRay as the current Driver owner, and @echristo as the former Driver and Autoconf-stuff owner.

llvmbot commented 1 year ago

@llvm/issue-subscribers-clang-driver

tstellar commented 1 year ago

I would be in favor of removing all of the C_INCLUDE_DIRS usage. Recently, we've been encouraging distributors to use the config files for these kind of customization rather than CMake variables.

brad0 commented 1 year ago

@MaskRay @echristo Ping.

MaskRay commented 1 year ago

Personally I'd be inclined to rip it all out, but maybe it's useful to someone.

+1. C_INCLUDE_DIRS is not recognized by GCC.

C_INCLUDE_DIRS is a popular substring. If I use https://sourcegraph.com/search?q=context:global+%5CbC_INCLUDE_DIRS%3D+-f:clang/&patternType=regexp&case=yes&sm=1&groupBy=repo to find the use cases, one project has a use like add_opt C_INCLUDE_DIRS=$C_INCLUDES, but the search patch seems likely redundant.

brad0 commented 1 year ago

I posted a diff here for review to rip it all out.. https://reviews.llvm.org/D159054