Skip to content

[Driver][Darwin] Move the remaining bits for header path handling over to the Driver. #75638

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.

Sign up for GitHub

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

Jump to bottom
Closed
brad0 opened this issue Dec 15, 2023 · 5 comments · Fixed by #75841 or #138234
Closed

[Driver][Darwin] Move the remaining bits for header path handling over to the Driver. #75638

brad0 opened this issue Dec 15, 2023 · 5 comments · Fixed by #75841 or #138234
Assignees
@ldionne
Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' platform:macos

Comments

@brad0
Copy link
Contributor

brad0 commented Dec 15, 2023

The remaining bits for the header path handling need to be moved over to the Driver within AddClangSystemIncludeArgs() as has been done for other OS's to date.

clang/lib/Lex/InitHeaderSearch.cpp

llvm-project/clang/lib/Lex/InitHeaderSearch.cpp

Line 343 in 7113c80

if (triple.isOSDarwin()) { 

  // NOTE: some additional header search logic is handled in the driver for
  // Darwin.
  if (triple.isOSDarwin()) {
    if (HSOpts.UseStandardSystemIncludes) {
      // Add the default framework include paths on Darwin.
      if (triple.isDriverKit()) {
        AddPath("/System/DriverKit/System/Library/Frameworks", System, true);
      } else {
        AddPath("/System/Library/Frameworks", System, true);
        AddPath("/Library/Frameworks", System, true);
      }
    }
    return;
  }
@EugeneZelenko EugeneZelenko added the clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' label Dec 15, 2023
@llvmbot
Copy link
Member

llvmbot commented Dec 15, 2023

@llvm/issue-subscribers-clang-driver

Author: Brad Smith (brad0)

The remaining bits for the header path handling need to be moved over to the Driver within AddClangSystemIncludeArgs() as has been done for other OS's to date.

clang/lib/Lex/InitHeaderSearch.cpp

llvm-project/clang/lib/Lex/InitHeaderSearch.cpp

Line 343 in 7113c80

if (triple.isOSDarwin()) { 

  // NOTE: some additional header search logic is handled in the driver for
  // Darwin.
  if (triple.isOSDarwin()) {
    if (HSOpts.UseStandardSystemIncludes) {
      // Add the default framework include paths on Darwin.
      if (triple.isDriverKit()) {
        AddPath("/System/DriverKit/System/Library/Frameworks", System, true);
      } else {
        AddPath("/System/Library/Frameworks", System, true);
        AddPath("/Library/Frameworks", System, true);
      }
    }
    return;
  }

@MaskRay
Copy link
Member

MaskRay commented Dec 16, 2023 •
edited
Loading

@jansvoboda11 @jroelofs

https://reviews.llvm.org/D158376 (DragonFlyBSD) is a migration example.

@brad0
Copy link
Contributor Author

brad0 commented Dec 17, 2023

@MaskRay I had spoken to jroelofs on Discourse and he said to file an issue so he could pass it around to someone appropriate at Apple.

@jroelofs
Copy link
Contributor

cc: @ldionne @cachemeifyoucan

ldionne added a commit to ldionne/llvm-project that referenced this issue Dec 18, 2023
@ldionne
[clang][Darwin] Remove legacy framework search path logic in the fron…
ca2a858 
…tend

This removes a long standing piece of technical debt. Most other platforms
have moved all their header search path logic to the driver, but Darwin
still had some logic for setting framework search paths present in the
frontend. This patch moves that logic to the driver alongside existing
logic that already handles part of these search paths.

This is intended to be a pure refactor without any functional change
visible to users, since the search paths before and after should be the
same, and in the same order. The change in the tests is necessary because
we would previously add the DriverKit framework search path in the frontend
regardless of whether we actually need to, which we now handle correctly
because the driver checks for ld64-605.1+.

Fixes llvm#75638
@ldionne ldionne mentioned this issue Dec 18, 2023
Merged
@ldionne ldionne self-assigned this Dec 18, 2023
ldionne added a commit that referenced this issue Dec 31, 2023
@ldionne
[clang][Darwin] Remove legacy framework search path logic in the fron…
61999b1 
…tend (#75841)

This removes a long standing piece of technical debt. Most other
platforms have moved all their header search path logic to the driver,
but Darwin still had some logic for setting framework search paths
present in the frontend. This patch moves that logic to the driver
alongside existing logic that already handles part of these search
paths.

This is intended to be a pure refactor without any functional change
visible to users, since the search paths before and after should be the
same, and in the same order. The change in the tests is necessary
because we would previously add the DriverKit framework search path in
the frontend regardless of whether we actually need to, which we now
handle correctly because the driver checks for ld64-605.1+.

Fixes #75638
@ldionne
Copy link
Member

ldionne commented Jan 8, 2024

Reverted in d34901f due to LSan issues.

@ldionne ldionne reopened this Jan 8, 2024
ldionne added a commit to ldionne/llvm-project that referenced this issue Dec 16, 2024
@ldionne
[clang][Darwin] Remove legacy framework search path logic in the fron…
e578bd7 
…tend

This removes a long standing piece of technical debt. Most other platforms
have moved all their header search path logic to the driver, but Darwin
still had some logic for setting framework search paths present in the
frontend. This patch moves that logic to the driver alongside existing
logic that already handles part of these search paths.

To achieve parity with the previous search path order, this patch
introduces the -internal-iframework flag which is used to pass
system framework paths from the driver to the frontend. These paths
are handled specially in that they are added after normal framework
search paths, which preserves the old frontend behavior for system
frameworks.

This patch is a re-application of llvm#75841
which was reverted in d34901f because it broke framework search paths.
In fact, the original patch was only adding framework search paths to
the linker job, but was not adding search paths for finding headers.
That issue is resolved in this version of the patch, with added tests.

Fixes llvm#75638
@ldionne ldionne mentioned this issue Dec 16, 2024
Closed
ldionne added a commit to ldionne/llvm-project that referenced this issue Jan 7, 2025
@ldionne
[clang][Darwin] Remove legacy framework search path logic in the fron…
ac16507 
…tend

This removes a long standing piece of technical debt. Most other platforms
have moved all their header search path logic to the driver, but Darwin
still had some logic for setting framework search paths present in the
frontend. This patch moves that logic to the driver alongside existing
logic that already handles part of these search paths.

To achieve parity with the previous search path order, this patch
introduces the -internal-iframework flag which is used to pass
system framework paths from the driver to the frontend. These paths
are handled specially in that they are added after normal framework
search paths, which preserves the old frontend behavior for system
frameworks.

This patch is a re-application of llvm#75841
which was reverted in d34901f because it broke framework search paths.
In fact, the original patch was only adding framework search paths to
the linker job, but was not adding search paths for finding headers.
That issue is resolved in this version of the patch, with added tests.

Fixes llvm#75638
ian-twilightcoder added a commit to ian-twilightcoder/llvm-project that referenced this issue May 2, 2025
@ian-twilightcoder
[clang][Darwin] Remove legacy framework search path logic in the fron…
4fbb2e0 
…tend

Move the Darwin framework search path logic from InitHeaderSearch::AddDefaultIncludePaths to DarwinClang::AddClangSystemIncludeArgs. Add a new -internal-iframework cc1 argument to support the tool chain adding these paths.
Now that the tool chain is adding search paths via cc1 flag, they're only added if they exist, so the Preprocessor/cuda-macos-includes.cu test is no longer relevant.
Change Driver/driverkit-path.c and Driver/darwin-subframeworks.c to do -### style testing similar to the darwin-header-search and darwin-embedded-search-paths tests. Rename darwin-subframeworks.c to darwin-framework-search-paths.c and have it test all framework search paths, not just SubFrameworks.
Add a unit test to validate that the myriad of search path flags result in the expected search path list.

Fixes llvm#75638
@ian-twilightcoder ian-twilightcoder mentioned this issue May 2, 2025
Merged
ian-twilightcoder added a commit to ian-twilightcoder/llvm-project that referenced this issue May 2, 2025
@ian-twilightcoder
[clang][Darwin] Remove legacy framework search path logic in the fron…
59f8d68 
…tend

Move the Darwin framework search path logic from InitHeaderSearch::AddDefaultIncludePaths to DarwinClang::AddClangSystemIncludeArgs. Add a new -internal-iframework cc1 argument to support the tool chain adding these paths.
Now that the tool chain is adding search paths via cc1 flag, they're only added if they exist, so the Preprocessor/cuda-macos-includes.cu test is no longer relevant.
Change Driver/driverkit-path.c and Driver/darwin-subframeworks.c to do -### style testing similar to the darwin-header-search and darwin-embedded-search-paths tests. Rename darwin-subframeworks.c to darwin-framework-search-paths.c and have it test all framework search paths, not just SubFrameworks.
Add a unit test to validate that the myriad of search path flags result in the expected search path list.

Fixes llvm#75638
ian-twilightcoder added a commit to ian-twilightcoder/llvm-project that referenced this issue May 2, 2025
@ian-twilightcoder
[clang][Darwin] Remove legacy framework search path logic in the fron…
6c55939 
…tend

Move the Darwin framework search path logic from InitHeaderSearch::AddDefaultIncludePaths to DarwinClang::AddClangSystemIncludeArgs. Add a new -internal-iframework cc1 argument to support the tool chain adding these paths.
Now that the tool chain is adding search paths via cc1 flag, they're only added if they exist, so the Preprocessor/cuda-macos-includes.cu test is no longer relevant.
Change Driver/driverkit-path.c and Driver/darwin-subframeworks.c to do -### style testing similar to the darwin-header-search and darwin-embedded-search-paths tests. Rename darwin-subframeworks.c to darwin-framework-search-paths.c and have it test all framework search paths, not just SubFrameworks.
Add a unit test to validate that the myriad of search path flags result in the expected search path list.

Fixes llvm#75638
ian-twilightcoder added a commit to ian-twilightcoder/llvm-project that referenced this issue May 8, 2025
@ian-twilightcoder
[clang][Darwin] Remove legacy framework search path logic in the fron…
ba40d11 
…tend

Move the Darwin framework search path logic from InitHeaderSearch::AddDefaultIncludePaths to DarwinClang::AddClangSystemIncludeArgs. Add a new -internal-iframework cc1 argument to support the tool chain adding these paths.
Now that the tool chain is adding search paths via cc1 flag, they're only added if they exist, so the Preprocessor/cuda-macos-includes.cu test is no longer relevant.
Change Driver/driverkit-path.c and Driver/darwin-subframeworks.c to do -### style testing similar to the darwin-header-search and darwin-embedded-search-paths tests. Rename darwin-subframeworks.c to darwin-framework-search-paths.c and have it test all framework search paths, not just SubFrameworks.
Add a unit test to validate that the myriad of search path flags result in the expected search path list.

Fixes llvm#75638
llvm-sync bot pushed a commit to arm/arm-toolchain that referenced this issue May 8, 2025
@ian-twilightcoder @github-actions
Automerge: [clang][Darwin] Remove legacy framework search path logic …
6c689e5 
…in the frontend (#138234)

Move the Darwin framework search path logic from
InitHeaderSearch::AddDefaultIncludePaths to
DarwinClang::AddClangSystemIncludeArgs. Add a new -internal-iframework
cc1 argument to support the tool chain adding these paths.
Now that the tool chain is adding search paths via cc1 flag, they're
only added if they exist, so the Preprocessor/cuda-macos-includes.cu
test is no longer relevant.
Change Driver/driverkit-path.c and Driver/darwin-subframeworks.c to do
-### style testing similar to the darwin-header-search and
darwin-embedded-search-paths tests. Rename darwin-subframeworks.c to
darwin-framework-search-paths.c and have it test all framework search
paths, not just SubFrameworks.
Add a unit test to validate that the myriad of search path flags result
in the expected search path list.

Fixes llvm/llvm-project#75638
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@ldionne ldionne

Labels
clang:driver 'clang' and 'clang++' user-facing binaries. Not 'clang-cl' platform:macos
Projects
None yet
Milestone
No milestone
6 participants
@MaskRay @ldionne @brad0 @jroelofs @EugeneZelenko @llvmbot