Skip to content

Properly set up C++ include paths and similar environment settings when parsing imported C++. #5767

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

Merged

Conversation

zygoloid
Copy link
Contributor

@zygoloid zygoloid commented Jul 3, 2025

Stop using the clang tooling library to build an ASTUnit; that library is set up to process clang frontend arguments, assuming that something has already built frontend arguments from the compiler arguments. It is also too encapsulated and doesn't let us inspect and modify the compiler invocation before it's executed.

Instead, build the AST unit directly in two phases:

  • FIrst, take a list of clang driver arguments and convert them into a list of compiler arguments, using clang::createInvocation. Internally, this uses the clang driver to build a frontend invocation, including building system-specific include paths as needed.
  • Then, directly build an ASTUnit from this compiler invocation.

I've factored this so that we can split out the createInvocation step, with the intention that we may want to move it out of check and into the carbon driver with the rest of the driver-level argument handling, and we may want to customize some of the clang options before we invoke the clang frontend with that set of options.

In order to make the invocation reusable, it no longer depends on the name of the carbon file importing the C++ code. In place of synthesizing a header file name as <foo.carbon>.generated.cpp_imports.h, we now insert line marker directives into the generated header so that errors in that header cause Clang to point a diagnostic back at the Carbon source file itself. This results in a minor improvement in the diagnostic output: we no longer refer to a nonexistent generated file. But the snippet still contains text that doesn't match the source code, so it remains imperfect.

zygoloid added 4 commits July 2, 2025 22:40
The tooling interface doesn't work well here -- it expects frontend
arguments, not driver arguments, and is a bit too high-level. Create an
ASTUnit directly instead, and pass driver arguments to its construction.
Plumb through the clang path rather than specifying a resource directory
manually.

Add line marker directives to point diagnostics from clang back at the
corresponding Carbon import.
@chandlerc
Copy link
Contributor

Unsure if you were aware of #5543 which is also touching how we invoke Clang. How would you like to proceed in terms of merging the two approaches?

references to different files.

Base the line delta on the file mentioned in that particular line number
reference, not the file number associated with the check line.
@zygoloid
Copy link
Contributor Author

zygoloid commented Jul 8, 2025

Unsure if you were aware of #5543 which is also touching how we invoke Clang. How would you like to proceed in terms of merging the two approaches?

I think they're mostly independent, though they do overlap in that they both contain the change to switch to passing driver arguments and mapping them to frontend arguments. That's a necessary part of this change, but seems to be more of an incidental fix in #5543, so I suggested reverting that part of that PR, but I'm also happy with that landing as-is and rebasing this on top (or the opposite way around).

@chandlerc
Copy link
Contributor

Unsure if you were aware of #5543 which is also touching how we invoke Clang. How would you like to proceed in terms of merging the two approaches?

I think they're mostly independent, though they do overlap in that they both contain the change to switch to passing driver arguments and mapping them to frontend arguments. That's a necessary part of this change, but seems to be more of an incidental fix in #5543, so I suggested reverting that part of that PR, but I'm also happy with that landing as-is and rebasing this on top (or the opposite way around).

Sounds good, let's work on landing this and then #5543 can rebase on top of this one I think.

Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First pass comments...

Copy link
Contributor Author

@zygoloid zygoloid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Split out #5783 with a mostly-separable piece of this (though I've not removed the changes from this PR yet). Let me know if there are any other parts you'd like split out.

@zygoloid zygoloid requested a review from chandlerc July 8, 2025 23:18
Copy link
Contributor

@chandlerc chandlerc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just minor nits, LG with suggested edits.

zygoloid and others added 5 commits July 8, 2025 17:37
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
Co-authored-by: Chandler Carruth <chandlerc@gmail.com>
@zygoloid zygoloid enabled auto-merge July 9, 2025 01:19
@zygoloid zygoloid added this pull request to the merge queue Jul 9, 2025
Merged via the queue into carbon-language:trunk with commit 3776e46 Jul 9, 2025
8 of 9 checks passed
@zygoloid zygoloid deleted the toolchain-lower-set-up-clang-env branch July 9, 2025 03:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants