Page MenuHomePhabricator

[clangd] (take 2) Try harder to find a plausible `clang` as argv0, particularly on Mac.
ClosedPublic

Authored by sammccall on Dec 4 2019, 11:06 AM.

Details

Summary

This was originally committed in 88bccded8fa169481fa367debf5ec615640635a1,
and reverted in 93f77617abba512d2861e2fc50ce385883f587b6.

This version is now much more testable: the "detect toolchain properties" part
is still not tested but also not active in tests.
All the command manipulation based on the detected properties is
directly tested, and also not active in other tests.

Fixes https://github.com/clangd/clangd/issues/211
Fixes https://github.com/clangd/clangd/issues/178

Diff Detail

Event Timeline

sammccall created this revision.Dec 4 2019, 11:06 AM

@kbobyrev would you mind verifying this version of the patch also works?
I don't expect changes vs the last one: it's been restructured a lot but the logic should be the same.

sammccall updated this revision to Diff 232183.Dec 4 2019, 11:11 AM

Revert unneeded changes to tests.

Build result: fail - 60479 tests passed, 1 failed and 726 were skipped.

failed: libc++.std/thread/thread_mutex/thread_mutex_requirements/thread_mutex_requirements_mutex/thread_mutex_recursive/lock.pass.cpp

Log files: console-log.txt, CMakeCache.txt

Build result: pass - 60480 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

The use of xcrun looks sound to me.

clang-tools-extra/clangd/CompileCommands.cpp
54

The trim() is probably safe - just wondering if that could cause issues in some weird case.

66

Just wondering why you decided to not somehow propagate the error - are there scenarios where the path is correct yet not resolvable on real FS?

92

Maybe this warrants a mention in the header file?

sammccall updated this revision to Diff 232274.Dec 5 2019, 1:34 AM
sammccall marked 5 inline comments as done.

Address review comments

Thanks for looking at this Jan, I should have put you on the reviewers in the first place...

clang-tools-extra/clangd/CompileCommands.cpp
54

Only if the path ends with a space, AFAICS. I don't think it's worth worrying about.

66

By definition probably not *correct* cases, but this will be hit if /usr/bin/clang is a dangling symlink, or xcrun lies to us, etc.

There's no error *propagation* because error recovery is here: even if we can't work out whether clang is a symlink, the best thing to do is create a mangler that assumes it isn't. (Or maybe better to create a mangler that ignores this path entirely, but I'm not convinced).
There's no value in knowing about this failure at the site where the mangler is created, or where the compile command is mangled, or where the file is opened - we're always going to want to recover and forge ahead, and we have the best information to recover here.

sammccall marked an inline comment as done.Dec 5 2019, 1:36 AM
sammccall added inline comments.
clang-tools-extra/clangd/CompileCommands.cpp
66

(Forgot to mention: I added a log here, since this shouldn't happen.)

Build result: pass - 60480 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

@kbobyrev would you mind verifying this version of the patch also works?
I don't expect changes vs the last one: it's been restructured a lot but the logic should be the same.

I can confirm that the latest version of the patch works.

Thanks. Can someone approve the patch and I'll land it again :-)

This revision is now accepted and ready to land.Dec 5 2019, 7:24 AM
This revision was automatically updated to reflect the committed changes.