Page MenuHomePhabricator

[clangd] Filter out plugin related flags and move all commandline manipulations into OverlayCDB.
ClosedPublic

Authored by kadircet on Jan 17 2019, 3:29 AM.

Details

Summary

Some projects make use of clang plugins when building, but clangd is
not aware of those plugins therefore can't work with the same compile command
arguments.

There were multiple places clangd performed commandline manipulations,
this one also moves them all into OverlayCDB.

Event Timeline

kadircet created this revision.Jan 17 2019, 3:29 AM
ioeric added inline comments.Jan 17 2019, 3:34 AM
clangd/ClangdUnit.cpp
429

This doesn't seem to be clangd specific; clang-tidy seems to have the same issue. Could we share the filtering logic (e.g. in lib/Tooling/ArgumentsAdjusters.cpp)?

LG, just a few comments. Supporting compiler plugins in clangd is non-trivial and we don't support them so far, so filtering out the options make sense.

It feels these helper belong in the Tooling library, that would allow to reuse code between clang-tidy, clangd, etc. Could even be the default behavior, since most of the tools won't handle compiler plugins well, unless compiled with the same exact version of the compiler.
I'd be interested in what other people think, @sammccall, @klimek?

clangd/ClangdUnit.cpp
429

We need a comment mentioning that we are filtering out the plugin options and why we're doing that here

433

Maybe move this code into ClangdServer::getCompileCommand to keep all argument adjustments that we do in clangd in one place.
We add -resource-dir there.

ilya-biryukov added inline comments.Jan 17 2019, 5:31 AM
clangd/ClangdUnit.cpp
429

+1 to the suggestion.
WDYT about landing it into clangd as a quick workaround and moving into tooling later?
The reason I think it might be better is because that would unblock usage of clangd in Chromium.

ioeric added inline comments.Jan 17 2019, 5:40 AM
clangd/ClangdUnit.cpp
429

WDYT about landing it into clangd as a quick workaround and moving into tooling later?
The reason I think it might be better is because that would unblock usage of clangd in Chromium.

I don't see why it would take much more effort to move the shared logic into tooling now?

kadircet marked 3 inline comments as done.Jan 17 2019, 5:42 AM

It seems like the most relevant place in tooling is ArgumentsAdjuster as @ioeric pointed out. Happy to move it to there if @sammccall and @klimek agrees as well.

As a side note ArgumentsAdjusters unfortunately causes a copy of the original command line arguments. Not sure how important this factor is compared to spinning up a compiler instance, just wanted to point it out.

clangd/ClangdUnit.cpp
433

As mentioned in the change description, we also have a different compiler invocation in BackgroundIndex which is not aware of ClangdServer::getCompileCommand

kadircet updated this revision to Diff 182260.Jan 17 2019, 5:42 AM
kadircet marked an inline comment as done.
  • Address comments
ilya-biryukov added inline comments.Jan 17 2019, 5:43 AM
clangd/ClangdUnit.cpp
429

My approximation would be half an hour to finish this and land in clangd and 2-3 hours to land this in tooling, update clangd, clang-tidy, etc.
Not a big difference, though, up to Kadir to actually quantify how hard he thinks this would be.

It seems like the most relevant place in tooling is ArgumentsAdjuster as @ioeric pointed out. Happy to move it to there if @sammccall and @klimek agrees as well.

As a side note ArgumentsAdjusters unfortunately causes a copy of the original command line arguments. Not sure how important this factor is compared to spinning up a compiler instance, just wanted to point it out.

From my point of view, it looks like this isn't enough to be a huge thing in tooling. That said, if we can find a nice abstraction to pull out, I'm generally in favor :) (I know, I know, I'm not a big help)

It seems like the most relevant place in tooling is ArgumentsAdjuster as @ioeric pointed out. Happy to move it to there if @sammccall and @klimek agrees as well.

As a side note ArgumentsAdjusters unfortunately causes a copy of the original command line arguments. Not sure how important this factor is compared to spinning up a compiler instance, just wanted to point it out.

I do think this should be a standard arguments adjuster available in Tooling.
This is a pattern that's likely to recur (clang-tidy will need the same fix to work with chromium). The fix is small but fiddly enough that it might warrant some improvement over time - I think we want to avoid people cloning their own copies of the fix.
The ArgumentsAdjuster interface is a little unfortunate regarding the copy, but I don't think this is significant in practice.

Once verifying this fixes the issue, we should consider requesting the tooling and clangd patches be cherrypicked to the LLVM-8 branch.
(And possibly do the same for clang-tidy)

clang-tidy also has the same fix in place, https://reviews.llvm.org/D18806

(I know, I know, I'm not a big help)

That's fine, mostly wanted to hear your opinion :-)

That said, if we can find a nice abstraction to pull out, I'm generally in favor :)

Eric suggested putting this into lib/Tooling/ArgumentsAdjusters.cpp, looks like a proper place for this kind of thing.

As a side note ArgumentsAdjusters unfortunately causes a copy of the original command line arguments. Not sure how important this factor is compared to spinning up a compiler instance, just wanted to point it out.

Yeah, running the frontend later would definitely outweigh this inefficiency, so we don't care.
I do agree that's an unfortunate design, though.

clangd/ClangdUnit.cpp
433

Ah, right :-( That's annoying. Does it also manually override -resource-dir? I'd move both into a single place, maybe this function is a better place to do so.

kadircet planned changes to this revision.Jan 17 2019, 7:41 AM
kadircet marked an inline comment as done.

Moving stripper to tooling.

clangd/ClangdUnit.cpp
433

It turned out to be more complicated actually, for example CodeCompletion also invokes a frontend action and it doesn't go through this function.....

kadircet updated this revision to Diff 182479.Jan 18 2019, 2:03 AM
  • Use getStripPluginsAdjuster and move manipulations into OverlayCDB
kadircet retitled this revision from [clangd] Filter out plugin related flags. to [clangd] Filter out plugin related flags and move all commandline manipulations into OverlayCDB..Jan 18 2019, 2:04 AM
kadircet edited the summary of this revision. (Show Details)

just a few nits

clangd/GlobalCompilationDatabase.cpp
23 ↗(On Diff #182479)

naming NIT: use adjustArguments

24 ↗(On Diff #182479)

NIT: use StringRef

136 ↗(On Diff #182479)

NIT: avoid an unnecessary copy: ResourceDir ? std::move(*ResourceDir) : …

kadircet updated this revision to Diff 182489.Jan 18 2019, 3:19 AM
kadircet marked 3 inline comments as done.
  • Address comments
kadircet added inline comments.Jan 18 2019, 3:19 AM
clangd/GlobalCompilationDatabase.cpp
24 ↗(On Diff #182479)

Need a string for the concatenation below, when pushing to vector. Otherwise we get a Twine

ilya-biryukov added inline comments.Jan 18 2019, 10:44 AM
clangd/GlobalCompilationDatabase.cpp
24 ↗(On Diff #182479)

Why not call .str() on a Twine?

kadircet updated this revision to Diff 182765.Jan 21 2019, 2:28 AM
kadircet marked 3 inline comments as done.

Address comments

clangd/GlobalCompilationDatabase.cpp
24 ↗(On Diff #182479)

Sure it is also an option but didn't think it would look any better, but there you go.

ilya-biryukov accepted this revision.Jan 21 2019, 7:59 AM

LGTM

clangd/GlobalCompilationDatabase.cpp
24 ↗(On Diff #182479)

Looks kinda ugly, but so efficient... (Not that we care much here)

This revision is now accepted and ready to land.Jan 21 2019, 7:59 AM
This revision was automatically updated to reflect the committed changes.
kadircet marked an inline comment as done.