Pull request 1244 in include-what-you-use: Identify libcxx from clang ToolChain

1 view
Skip to first unread message

notifi...@include-what-you-use.org

unread,
Apr 19, 2023, 5:04:19 PM4/19/23
to include-wh...@googlegroups.com
New pull request 1244 by kilroyd: Identify libcxx from clang ToolChain
https://github.com/include-what-you-use/include-what-you-use/pull/1244

It seems like `HeaderSearchOpts` doesn't have UseLibcxx set for MacOS. Revert to getting this from `clang::driver::ToolChain`.

Compared to my original implementation, pass the whole Toolchain into iwyu_globals so we can extract any other useful arguments. This means iwyu_globals.cc is exposed to more from `clang::driver` than I'd like.



notifi...@include-what-you-use.org

unread,
Apr 21, 2023, 12:57:19 PM4/21/23
to include-wh...@googlegroups.com
Comment #1 on pull request 1244 by kimgr: Identify libcxx from clang ToolChain
https://github.com/include-what-you-use/include-what-you-use/pull/1244

This goes back to reading from toolchain in driver, hiding a magic value in globals, and then reading from globals in main. I really want to avoid that kind of temporal coupling, so I need to think about how to make the Toolchain available to `InitGlobals` again.


notifi...@include-what-you-use.org

unread,
Apr 22, 2023, 5:52:56 AM4/22/23
to include-wh...@googlegroups.com
Comment #2 on pull request 1244 by kilroyd: Identify libcxx from clang ToolChain
https://github.com/include-what-you-use/include-what-you-use/pull/1244

Sure. I tried to find a way to get back to `Toolchain` from `CompilerInstance`. I'm not sure the alternative of checking the command line arguments would work if the current code isn't picking it up (I assume the job isn't getting `-stdlib=` on Macos).

I'll dig a bit, but would appreciate your thoughts on the way forward.


notifi...@include-what-you-use.org

unread,
Apr 26, 2023, 3:30:14 PM4/26/23
to include-wh...@googlegroups.com
Comment #3 on pull request 1244 by kimgr: Identify libcxx from clang ToolChain
https://github.com/include-what-you-use/include-what-you-use/pull/1244

My head is itching on the inside, but I think I've figured out a way to do it. I'm not convinced it's better, but I think it turns all the arrows in a forward direction:

* `IwyuAction::CreateASTConsumer` currently calls `InitGlobals`
* But just above is the innocuous comment `// Do this first thing after getting our hands on a CompilerInstance`
* That's not really in `CreateASTConsumer`, that's actually in `iwyu_driver.cc`, `CreateCompilerInstance`
* `CreateCompilerInstance` also has access to the `Compilation`, which exposes `getDefaultToolChain`, and lets us get to the `CXXStdlibType` enum.
* So presumably we could call `InitGlobals` from `CreateCompilerInstance` and pass both compiler and toolchain, depending on what we need.

I think that makes sense. Not sure if there are drawbacks to calling `InitGlobals` from outside the action, but I don't think so. Feels better, even. I'll try to materialize a path when I get some time, unless you beat me to it :)


notifi...@include-what-you-use.org

unread,
Apr 26, 2023, 4:45:30 PM4/26/23
to include-wh...@googlegroups.com
Comment #4 on pull request 1244 by kilroyd: Identify libcxx from clang ToolChain
https://github.com/include-what-you-use/include-what-you-use/pull/1244

That does sound better. I gave that a spin, but unfortunately by the end of `CreateCompilerInstance()` the `CompilerInstance` doesn't have a `SourceManager`, so we get the following (from `InitGlobals` calling `getSourceManager()`:

```
Assertion failed: (SourceMgr && "Compiler instance has no source manager!"), function getSourceManager, file CompilerInstance.h, line 425.
```

Pushed what I have to kilroyd:fix_libcxx2 in case you want to play (sorry not sure how to link to branch...)


notifi...@include-what-you-use.org

unread,
Apr 26, 2023, 4:46:56 PM4/26/23
to include-wh...@googlegroups.com
Comment #4 on pull request 1244 by kilroyd: Identify libcxx from clang ToolChain
https://github.com/include-what-you-use/include-what-you-use/pull/1244

That does sound better. I gave that a spin, but unfortunately by the end of `CreateCompilerInstance()` the `CompilerInstance` doesn't have a `SourceManager`, so we get the following (from `InitGlobals` calling `getSourceManager()`:

```
Assertion failed: (SourceMgr && "Compiler instance has no source manager!"), function getSourceManager, file CompilerInstance.h, line 425.
```

Pushed what I have to kilroyd/include-what-you-use:fix_libcxx2 in case you want to play (sorry not sure how to link to branch...)


notifi...@include-what-you-use.org

unread,
Apr 26, 2023, 4:53:28 PM4/26/23
to include-wh...@googlegroups.com
Comment #5 on pull request 1244 by kilroyd: Identify libcxx from clang ToolChain
https://github.com/include-what-you-use/include-what-you-use/pull/1244

How do you feel about splitting `InitGlobals` in two?

Say `InitSourceManager(CompilerInstance&)` which does the search paths and mappings; and `InitToolchain(Toolchain&)` which gets the stdlibs and creates the `IncludePicker`


notifi...@include-what-you-use.org

unread,
Apr 26, 2023, 5:04:45 PM4/26/23
to include-wh...@googlegroups.com
Comment #5 on pull request 1244 by kilroyd: Identify libcxx from clang ToolChain
https://github.com/include-what-you-use/include-what-you-use/pull/1244

How do you feel about splitting `InitGlobals` in two?

Say `InitSearchPath(CompilerInstance&)` which does the search paths; and `InitIncludePicker(Toolchain&)` which gets the stdlibs and creates the `IncludePicker`


Reply all
Reply to author
Forward
0 new messages