Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Wrong include path to Clang 16 on Windows #17863

Closed
stevenlr opened this issue Mar 23, 2023 · 6 comments
Closed

Wrong include path to Clang 16 on Windows #17863

stevenlr opened this issue Mar 23, 2023 · 6 comments
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: bug

Comments

@stevenlr
Copy link

Description of the bug:

Building C++ with Clang 16.0.0 and Bazel 6 on Windows spits out "missing dependency declarations" for include files:

ERROR: .../BUILD:10:10: Compiling main.cpp failed: undeclared inclusion(s) in rule '//:main':
this rule is missing dependency declarations for the following files included by 'main.cpp':
  'C:/Program Files/LLVM/lib/clang/16/include/vadefs.h'

The clang lib is installed in C:/Program Files/LLVM/lib/clang/16, but Bazel uses clang-cl -v to detect the version used in the path, which is 16.0.0 here, hence wrongfully using the C:/Program Files/LLVM/lib/clang/16.0.0 path.

In external/local_config_cc/BUILD:

cc_toolchain_config(
    name = "clang_cl_x64",
    cpu = "x64_windows",
    compiler = "clang-cl",
    # ...
    msvc_env_include = "...;C:\\Program Files\\LLVM\\lib\\clang\\16.0.0\\include",
    msvc_env_lib = "...;C:\\Program Files\\LLVM\\lib\\clang\\16.0.0\\lib\\windows",
    # ...
    cxx_builtin_include_directories = [
        # ...
        "C:\\Program Files\\LLVM\\lib\\clang\\16.0.0\\include"],
    # ...
)

What's the simplest, easiest way to reproduce this bug? Please provide a minimal example if possible.

Use the following C++ file

#include <stdio.h>

int main()
{
    printf("Hello, world! " __clang_version__ "\n");
    return 0;
}

With the Bazel config described in https://bazel.build/configure/windows#clang with --incompatible_enable_cc_toolchain_resolution.

Which operating system are you running Bazel on?

Windows 10

What is the output of bazel info release?

release 6.1.1

If bazel info release returns development version or (@non-git), tell us how you built Bazel.

No response

What's the output of git remote get-url origin; git rev-parse master; git rev-parse HEAD ?

No response

Have you found anything relevant by searching the web?

No response

Any other information, logs, or outputs that you want to share?

No response

@buildbreaker2021 buildbreaker2021 added P3 We're not considering working on this, but happy to review a PR. (No assignee) and removed untriaged labels Apr 3, 2023
@e8y
Copy link

e8y commented Apr 10, 2023

Also hit this. I manually renamed the directory 'C:/Program Files/LLVM/lib/clang/16 to 'C:/Program Files/LLVM/lib/clang/16.0.0 to get around it for now.

ohodson added a commit to cloudflare/workerd that referenced this issue Aug 29, 2023
Broken by actions/runner-images#812

Unforunately upgrading to LLVM16 per scikit-image/scikit-image#7109 appears
to be hampered by bazelbuild/bazel#17863. Working
around this, there is a static assertion failure in Google benchmark.
ohodson added a commit to cloudflare/workerd that referenced this issue Aug 31, 2023
This is required due to actions/runner-images#8125

An assortment of changes to enable building with the LLVM16 toolchain on Windows:
- add Windows-2022 back to the test strategy matrix in test.yml.
- add an LLVM upgrade step to test.yml to install LLVM 16
- add command to test.yml to fix the include path for LLVM includes
  (Bazel issue bazelbuild/bazel#17863).
- add a macro definition to .bazelrc to prefer __builtin_offsetof
  (the offsetof macro without this is broken under LLVM16).
- add enforced include path override to files including emmintrin.h.
  clang-cl.exe in LLVM15 picks up the LLVM version of this file, but
  LLVM16 was picking up the MSFT version and preventing the compiler
  from replacing intrinsics. It looks like there is a change in
  precedence between LLVM15 and LLVM16 for the include path setup by
  Bazel in the INCLUDE environment variable.

Test: Install LLVM16, fix the LLVM include directory location for bazel, then
      run `bazel test //...`
ohodson added a commit to cloudflare/workerd that referenced this issue Aug 31, 2023
This is required due to actions/runner-images#8125

An assortment of changes to enable building with the LLVM16 toolchain on Windows:
- add Windows-2022 back to the test strategy matrix in test.yml.
- add an LLVM upgrade step to test.yml to install LLVM 16
- add command to test.yml to fix the include path for LLVM includes
  (Bazel issue bazelbuild/bazel#17863).
- add a macro definition to .bazelrc to prefer __builtin_offsetof
  (the offsetof macro without this is broken under LLVM16).
- add enforced include path override to files including emmintrin.h.
  clang-cl.exe in LLVM15 picks up the LLVM version of this file, but
  LLVM16 was picking up the MSFT version and preventing the compiler
  from replacing intrinsics. It looks like there is a change in
  precedence between LLVM15 and LLVM16 for the include path setup by
  Bazel in the INCLUDE environment variable.

Test: Install LLVM16, fix the LLVM include directory location for bazel, then
      run `bazel test //...`
ohodson added a commit to cloudflare/workerd that referenced this issue Aug 31, 2023
This is required due to actions/runner-images#8125

An assortment of changes to enable building with the LLVM16 toolchain on Windows:
- add Windows-2022 back to the test strategy matrix in test.yml.
- add an LLVM upgrade step to test.yml to install LLVM 16
- add command to test.yml to fix the include path for LLVM includes
  (Bazel issue bazelbuild/bazel#17863).
- add a macro definition to .bazelrc to prefer __builtin_offsetof
  (the offsetof macro without this is broken under LLVM16).
- add enforced include path override to files including emmintrin.h.
  clang-cl.exe in LLVM15 picks up the LLVM version of this file, but
  LLVM16 was picking up the MSFT version and preventing the compiler
  from replacing intrinsics. It looks like there is a change in
  precedence between LLVM15 and LLVM16 for the include path setup by
  Bazel in the INCLUDE environment variable.

Test: Install LLVM16, fix the LLVM include directory location for bazel, then
      run `bazel test //...`
ohodson added a commit to cloudflare/workerd that referenced this issue Aug 31, 2023
This is required due to actions/runner-images#8125

An assortment of changes to enable building with the LLVM16 toolchain on Windows:
- add Windows-2022 back to the test strategy matrix in test.yml.
- add an LLVM upgrade step to test.yml to install LLVM 16
- add command to test.yml to fix the include path for LLVM includes
  (Bazel issue bazelbuild/bazel#17863).
- add a macro definition to .bazelrc to prefer __builtin_offsetof
  (the offsetof macro without this is broken under LLVM16).
- add enforced include path override to files including emmintrin.h.
  clang-cl.exe in LLVM15 picks up the LLVM version of this file, but
  LLVM16 was picking up the MSFT version and preventing the compiler
  from replacing intrinsics. It looks like there is a change in
  precedence between LLVM15 and LLVM16 for the include path setup by
  Bazel in the INCLUDE environment variable.

Test: Install LLVM16, fix the LLVM include directory location for bazel, then
      run `bazel test //...`
@ohodson
Copy link
Contributor

ohodson commented Aug 31, 2023

Also hit this. I manually renamed the directory C:/Program Files/LLVM/lib/clang/16 to C:/Program Files/LLVM/lib/clang/include/16.0.0 to get around it for now.

This is a super helpful suggestion, thank you. This got us a long way when trying to workaround the fallout from github's runner image changes (latest fix actions/runner-images#8125).

We have a project that includes intrinsics and files with intrinsics need a bit more than the suggest. clang-cl.exe uses the include path bazel provides, but also prefixes the include path that it applies with the installed location of headers, e.g. C:/Program Files/LLVM/lib/clang/16. AFAICT, this prefixing is done because the include directory includes headers that have header files with appropriately annotated intrinsics for clang. When that directory is moved, things break because headers in the Windows SDK paths have the same names as the clang intrinsics headers, but different contents. Moving the directory means the (incompatible) Windows headers are picked up and intrinsics aren't desugared into CPU instructions, but left as unresolved functions for the linker to croak on.

Simply copying the include path, instead of moving it, to have an include/16 and include/16.0.X falls foul of the "undeclared inclusion" logic in bazel.

One workaround is to add --per_file_copt entries in .bazelrc to include the clang header path before the INCLUDE path set by bazel. It is very ugly (cloudflare/workerd#1092).

ohodson added a commit to cloudflare/workerd that referenced this issue Aug 31, 2023
This is required due to actions/runner-images#8125

An assortment of changes to enable building with the LLVM16 toolchain on Windows:
- add Windows-2022 back to the test strategy matrix in test.yml.
- add an LLVM upgrade step to test.yml to install LLVM 16
- add command to test.yml to fix the include path for LLVM includes
  (Bazel issue bazelbuild/bazel#17863).
- add a macro definition to .bazelrc to prefer __builtin_offsetof
  (the offsetof macro without this is broken under LLVM16).
- add enforced include path override to files including emmintrin.h.
  clang-cl.exe in LLVM15 picks up the LLVM version of this file, but
  LLVM16 was picking up the MSFT version and preventing the compiler
  from replacing intrinsics. It looks like there is a change in
  precedence between LLVM15 and LLVM16 for the include path setup by
  Bazel in the INCLUDE environment variable.

Test: Install LLVM16, fix the LLVM include directory location for bazel, then
      run `bazel test //...`
ohodson added a commit to cloudflare/workerd that referenced this issue Sep 1, 2023
This is required due to actions/runner-images#8125

An assortment of changes to enable building with the LLVM16 toolchain on Windows:
- add Windows-2022 back to the test strategy matrix in test.yml.
- add an LLVM upgrade step to test.yml to install LLVM 16
- add command to test.yml to fix the include path for LLVM includes
  (Bazel issue bazelbuild/bazel#17863).
- add a macro definition to .bazelrc to prefer __builtin_offsetof
  (the offsetof macro without this is broken under LLVM16).
- add enforced include path override to files including emmintrin.h.
  clang-cl.exe in LLVM15 picks up the LLVM version of this file, but
  LLVM16 was picking up the MSFT version and preventing the compiler
  from replacing intrinsics. It looks like there is a change in
  precedence between LLVM15 and LLVM16 for the include path setup by
  Bazel in the INCLUDE environment variable.

Test: Install LLVM16, fix the LLVM include directory location for bazel, then
      run `bazel test //...`
ohodson added a commit to cloudflare/workerd that referenced this issue Sep 1, 2023
This is required due to actions/runner-images#8125

An assortment of changes to enable building with the LLVM16 toolchain on Windows:
- add Windows-2022 back to the test strategy matrix in test.yml.
- add an LLVM upgrade step to test.yml to install LLVM 16
- add command to test.yml to fix the include path for LLVM includes
  (Bazel issue bazelbuild/bazel#17863).
- add a macro definition to .bazelrc to prefer __builtin_offsetof
  (the offsetof macro without this is broken under LLVM16).
- add enforced include path override to files including emmintrin.h.
  clang-cl.exe in LLVM15 picks up the LLVM version of this file, but
  LLVM16 was picking up the MSFT version and preventing the compiler
  from replacing intrinsics. It looks like there is a change in
  precedence between LLVM15 and LLVM16 for the include path setup by
  Bazel in the INCLUDE environment variable.

Test: Install LLVM16, fix the LLVM include directory location for bazel, then
      run `bazel test //...`
ohodson added a commit to ohodson/bazel that referenced this issue Sep 1, 2023
Clang install on Windows started using just the major version
number in the install path rather than the full version number.

Fixes bazelbuild#17863
fmeum added a commit to CodeIntelligenceTesting/jazzer that referenced this issue Sep 6, 2023
Fixes an issue with clang-16 on Windows (bazelbuild/bazel#17863), which is now live on the GitHub Actions runners.
@fmeum
Copy link
Collaborator

fmeum commented Sep 6, 2023

@bazel-io flag

@bazel-io bazel-io added the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Sep 6, 2023
fmeum added a commit to CodeIntelligenceTesting/jazzer that referenced this issue Sep 6, 2023
Fixes an issue with clang-16 on Windows (bazelbuild/bazel#17863), which is now live on the GitHub Actions runners.
@iancha1992
Copy link
Member

@bazel-io fork 6.4.0

@bazel-io bazel-io removed the potential release blocker Flagged by community members using "@bazel-io flag". Should be added to a release blocker milestone label Sep 6, 2023
bazel-io pushed a commit to bazel-io/bazel that referenced this issue Sep 6, 2023
Clang install on Windows started using just the major version number in the install path rather than the full version number.

Fixes bazelbuild#17863

Closes bazelbuild#19391.

PiperOrigin-RevId: 562480023
Change-Id: Iebd5d3cedff48739747fa8668d56ff8f1d9350b9
iancha1992 pushed a commit that referenced this issue Sep 7, 2023
Clang install on Windows started using just the major version number in
the install path rather than the full version number.

Fixes #17863

Closes #19391.

Commit
0377bad

PiperOrigin-RevId: 562480023
Change-Id: Iebd5d3cedff48739747fa8668d56ff8f1d9350b9

Co-authored-by: Orion Hodson <[email protected]>
keith pushed a commit to keith/bazel that referenced this issue Sep 8, 2023
Clang install on Windows started using just the major version number in
the install path rather than the full version number.

Fixes bazelbuild#17863

Closes bazelbuild#19391.

Commit
bazelbuild@0377bad

PiperOrigin-RevId: 562480023
Change-Id: Iebd5d3cedff48739747fa8668d56ff8f1d9350b9

Co-authored-by: Orion Hodson <[email protected]>
@iancha1992
Copy link
Member

A fix for this issue has been included in Bazel 6.4.0 RC1. Please test out the release candidate and report any issues as soon as possible. Thanks!

@ohodson
Copy link
Contributor

ohodson commented Sep 21, 2023

The project we have using Clang 16 and Bazel, workerd, builds successfully with a clean Clang 16 installation and Bazel 6.4.0 RC1.

Great going, thanks for incorporating the change. Looking forward to dropping the fragile workaround we have in place. 👍🙏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
P3 We're not considering working on this, but happy to review a PR. (No assignee) team-Rules-CPP Issues for C++ rules type: bug
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants