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.

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

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

Open
brad0 opened this issue Dec 15, 2023 · 5 comments · Fixed by #75841
Open

[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
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
Collaborator

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
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
Development

Successfully merging a pull request may close this issue.

[clang][Darwin] Remove legacy framework search path logic in the frontend ldionne/llvm-project
6 participants
@MaskRay @ldionne @brad0 @jroelofs @EugeneZelenko @llvmbot