This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Link and initialize target infos
ClosedPublic

Authored by kadircet on Jun 12 2019, 6:02 AM.

Details

Reviewers
ilya-biryukov
Summary

So that clang can infer target from argv[0] of command line invocation

Event Timeline

kadircet created this revision.Jun 12 2019, 6:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 12 2019, 6:02 AM
ilya-biryukov added inline comments.Jun 12 2019, 6:32 AM
clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
36

Do we actually need this if we provide a proper Argv[0] to the driver?
My understanding is that the driver should figure out the targets from custom binary names on its own.

clang-tools-extra/clangd/test/target_info.test
2

Could you add a short human-readable description of the test setup here?
Reading the bash scripts is not fun. Something like

Try to mock 'compile_commnands.json' for fuchsia-based builds and check the corresponding target flags
are being passed into clang.
9

This is added unconditionally on all platforms, right?
Why not just add this extra slash directly into the input file?

kadircet updated this revision to Diff 204278.Jun 12 2019, 6:42 AM
kadircet marked 3 inline comments as done.
  • Add comments to the test
kadircet added inline comments.Jun 12 2019, 6:42 AM
clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
36

unfortunately yes.

because if the "-target" is missing, driver tries to deduce it from the "running binary" which is "clangd" in our case.

clang-tools-extra/clangd/test/target_info.test
9

no it is not, this checks whether we have a drive letter in the file uri ("[A-Z]:") and adds the slash in that case.

ilya-biryukov added inline comments.Jun 12 2019, 6:54 AM
clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
36

I wonder if it would sense to change this? Where does this code live?

That's exactly what argv[0] is for in compile_commands.json. Fixing that at the driver level would "fix" all libclang- and libtooling-based binaries, like clang-tidy, etc.

ilya-biryukov marked an inline comment as done.Jun 12 2019, 6:55 AM
ilya-biryukov added inline comments.
clang-tools-extra/clangd/test/target_info.test
9

Ah, ok, LGTM. Missed that.

kadircet updated this revision to Diff 204484.Jun 13 2019, 4:34 AM
kadircet marked an inline comment as done.
  • Move target name deduction into Driver
clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
36

ah sorry, I've mixed that one actually. the behavior is: if the "-target" is missing, we always use the default target(depends on the host). even though ClangExecutable is available in the Driver and set correctly to argv[0].
tools that care about target deduction needs to pass "-target" to driver explicitly. currently the driver only deduces "driver-mode" from the executable name.

adding this to rather driver side, looks like nothing is broken.

ilya-biryukov marked an inline comment as done.Jun 13 2019, 5:20 AM

Nice, thanks! The clangd parts obviously LGTM, but let's be more conservative with the driver bits (see the relevant comment)

clang-tools-extra/clangd/test/target_info.test
29

This is a marvellous hack!

clang/lib/Driver/Driver.cpp
1055 ↗(On Diff #204484)

I strongly think the driver is the right place to do this, but we have to watch out for breakages coming from this. The drivers code has way more uses than clangd code, so we should expect that people rely on all kinds of things there.

Could you separate the driver pieces into a separate change and add driver tests for them? To make it more focused and easier to revert in case of breakages.

kadircet marked 3 inline comments as done.
kadircet added inline comments.
clang/lib/Driver/Driver.cpp
1055 ↗(On Diff #204484)

sent out D63264

kadircet marked an inline comment as done.Jun 21 2019, 6:31 AM

ping

Sorry about the confusion, I can now see why the original version of the patch was actually simpler.
I was put off by the fact that we override by adding command-line arguments instead of passing things around in the code, but it appears that's actually simpler than adding yet another mechanism to override target tripple in the driver.

The only suggestion from me would be maybe try moving this to a layer that would also make also benefit other compilation-db-based things like clang-tidy, etc?
Any ideas on how to do this? Would this potentially break anything?

The only suggestion from me would be maybe try moving this to a layer that would also make also benefit other compilation-db-based things like clang-tidy, etc?
Any ideas on how to do this? Would this potentially break anything?

I've made it a wrapper for JSON-based compilation database(as inferring cdb does), there were no breakages in tests.
Hopefully all clang-based tools can benefit from it by linking in target infos and enabling them at startup.

kadircet updated this revision to Diff 206193.Jun 24 2019, 5:06 AM
  • Introduce a new wrapper CDB that adds target and mode info to returned compile commands

Thanks, this looks very good!
Leaving a few nitpicky comments, but nothing important really.

Two more general NITs:

  • could we update the title and description of this revision to mirror that it focuses on code in tooling rather than in clangd?
  • maybe land the clangd parts into a separate change for nicer VCS history?
clang/include/clang/Tooling/CompilationDatabase.h
220 ↗(On Diff #206193)

NIT: in the spirit of a previous name, we could shorten the name inferTargetAndDriverMode(…)

clang/lib/Tooling/CMakeLists.txt
25 ↗(On Diff #206193)

NIT: GuessTargetAndDriverModeCompilationDatabase would align better with the names of other files that define compilation databases.

It looks a little long, though, maybe there's a better word to capture TargetAndMode

clang/lib/Tooling/TargetAndModeAdderDatabase.cpp
39 ↗(On Diff #206193)

NIT: maybe accept by value to simplify the code?
A cost of an extra move constructor here is negligible.

kadircet updated this revision to Diff 206378.Jun 25 2019, 12:52 AM
kadircet marked 4 inline comments as done.
  • Move clang related changes to a different patch
kadircet retitled this revision from [clangd] Link in target infos and pass target and mode while invoking driver to [clangd] Link and initialize target infos.Jun 25 2019, 12:52 AM
kadircet edited the summary of this revision. (Show Details)
ilya-biryukov accepted this revision.Jun 25 2019, 1:15 AM

LGTM

clang-tools-extra/clangd/GlobalCompilationDatabase.cpp
14

looks like accidental leftover from a previous revision. Remove?

This revision is now accepted and ready to land.Jun 25 2019, 1:15 AM
kadircet updated this revision to Diff 206598.Jun 26 2019, 12:28 AM
kadircet marked an inline comment as done.
  • revert leftover change