Skip to content

@molar's issue -- Let's work together! #33

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

Closed
cpsauer opened this issue Mar 20, 2022 · 9 comments · May be fixed by #248
Closed

@molar's issue -- Let's work together! #33

cpsauer opened this issue Mar 20, 2022 · 9 comments · May be fixed by #248

Comments

@cpsauer
Copy link
Contributor

cpsauer commented Mar 20, 2022

Hi, @molar,

Thanks for giving this tool a whirl! I happened to notice that you'd pushed a commit to a fork--it looked like undoing the Grail LLVM toolchain's compiler wrapping.

Any chance we could work together to resolve in the main repo whatever problem you were running into? I would love, at least to understand what's going wrong. Ideally we'd get it fixed--for you and for others in the future.

Thanks!
Chris

P.S. Was it related to problems finding standard library headers? That's why we'd unwrapped things for Apple platforms, but it hadn't seemed to be a problem with Grail's LLVM toolchain in the quick macOS tests I ran. Or maybe on a first run, before the wrappers had been generated? In case it's useful, there's some discussion over in #15 (comment), from when we first opened up grailbio support.

@cpsauer
Copy link
Contributor Author

cpsauer commented Mar 23, 2022

Tagging also @mla, the author of that commit :)
[If that's not right, sorry! Strangely the username on that commit isn't clickable.]

@molar
Copy link

molar commented Mar 24, 2022

Hey @cpsauer ,

Thanks for reaching out, we can certainly collaborate to upstream my changes, if we cannot find the underlying issue. They are a bit of a hack.

If my compile_commands contains

"command": "external/llvm_toolchain/bin/cc_wrapper.sh  ....

Then clangd complains about cstdlib, but not filesystem,
Following the guide Here produces identical output given either of the paths to the compiler.

external/llvm_toolchain/bin/cc_wrapper.sh -v -c -stdlib=libc++ --language=c++ --target=x86_64-unknown-linux-gnu  -xc++ /dev/null >a.txt 2>&1
external/llvm_toolchain_llvm/bin/clang-13 -v -c -stdlib=libc++ --language=c++ --target=x86_64-unknown-linux-gnu  -xc++ /dev/null >b.txt 2>&1
diff a.txt b.txt # empty diff

However if i modify the path in the compile_commands.json and reload the language server it resolves the cstdlib just fine.
Hence the modification i made in my fork.

@cpsauer
Copy link
Contributor Author

cpsauer commented Mar 25, 2022

Likewise! Thanks for replying--and for using and fiddling with this tool :)

I think what you're saying is that some (but not all) standard library includes aren't getting found when the compiler (driver) specified is cc_wrapper.sh, but that they're all found when you specify clang-13. Right?

If so, I'm pretty sure I know the root issue. It's that the wrapper is inhibiting clangd's automatic introspection of the compiler driver--as in the P.S. above. We could add code to auto unwrap it (as we have to for Bazel's problematic Apple wrappers)--but let's first try just telling clangd to introspect harder.

Could you try passing --query-driver=/**/* to clangd to tell it to interrogate (all) wrapper scripts more aggressively? Please pass it the same way we pass the other flags from VSCode settings in the README. Let's see if that solves the problematic case you're hitting, without requiring code changes.

  • You may have to restart clangd to see results, as you mentioned doing previously.

I really appreciate your willingness to help. If thatworks, we could save a lot of future people pain, quite robustly, by adding it to the recommended settings in the README. And if not, it's not too hard to make a code change to automatically unwrap things and handle this case.

Thanks for being great!
Chris

@cpsauer
Copy link
Contributor Author

cpsauer commented Mar 25, 2022

Since --query-driver also fixed another issue--and I think it'll fix this one--I went ahead and added the workaround to the recommended settings in the README.

I've also (optimistically) closed this issue. But if that's premature and adding it doesn't fit things for you, please say and I'll open this right back up.

Either way, please do confirm whether or not this solves things for you!

@molar
Copy link

molar commented Mar 25, 2022

Hello, i can confirm it works when setting the --query-driver flag. then i get the following output in the task terminal from vscode

System includes extractor: successfully executed ..........

and the includes are resolved. Thank you for your help

@cpsauer
Copy link
Contributor Author

cpsauer commented Mar 25, 2022

Wahoo! Glad to hear.

In the future, please don't hesitate to write in if you encounter a problem!

That way we can solve it together for everyone :)

All the best,
Chris

@molar
Copy link

molar commented Apr 25, 2022

Hey, i found a use-case for rewriting the command.

SonarQube supports using a compile_commands.json, however it appears in my case to only work correctly if i rewrite the path to the compiler.

Would you consider a PR with the rewrite functionality - documenting it as a workaround for other tools than clangd?

@cpsauer
Copy link
Contributor Author

cpsauer commented Apr 26, 2022

Hey, Molar. Yes! SonarQube is new to me, but if it absolutely needs unwrapped compilers, then let's do it.

(Anything more I should know about why SonarQube fails with the wrapper? Is it the same thing as --query-driver, where it doesn't introspect the default include directories by default, or are we just hitting some assert? I assume there's no query-driver equivalent for SonarQube, where it'll just ask the compiler-driver wrapper what its default includes are? More generally, I want to make sure we've got you fully up to speed on what's going on with the query-driver/wrapping stuff before you dive in.)

Final thing: jun-sheaf came up with a great trick to automatically unwrap the compiler using --version in #40. He'd closed it down because --query-driver solved his problem. However, I think it'd be great place to pick back up from. The code and discussion are at least well worth a read, and you might want to copy over his _get_underlying_clang function. Gotta make sure we don't break things with e.g. gcc and apple, though. You might notice that we always unwrap on Apple--and I kinda think we should always unwrap with Grail, too, if you do indeed need it. That way there's no configuration for people, and everyone will be using the same codepath.

@cpsauer
Copy link
Contributor Author

cpsauer commented May 12, 2022

Also, @molar, did you ever resolve this issue (^) about a --query_driver equivalent for Sonarqube? If not, I still think the auto-unwrap could be a handy quick fix.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants