[PATCH] D92012: [clangd][query-driver] Extract target

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.

Sorry, code reviews are racy :-)



================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:134
     elog("System include extraction: end marker missing: {0}", Output);
-    return {};
-  }
-
-  for (llvm::StringRef Line : llvm::make_range(StartIt, EndIt)) {
-    SystemIncludes.push_back(Line.trim().str());
-    vlog("System include extraction: adding {0}", Line);
+    return llvm::None;
   }
----------------
ArcsinX wrote:
> Unsure about this.
> Should we toss all collected data if end markes was not found?
Yup, at least for this patch (this is the existing behavior).
I think the logic is "this is a fragile text protocol, we've never seen this happen, so don't really know what it would mean".


================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:212
+  if (!Target.empty()) {
+    Cmd.CommandLine.push_back("-target");
+    Cmd.CommandLine.push_back(Target);
----------------
sammccall wrote:
> kadircet wrote:
> > ArcsinX wrote:
> > > ArcsinX wrote:
> > > > kadircet wrote:
> > > > > sammccall wrote:
> > > > > > `clang -target foo test.cc` seems to be a hard error in the driver if the target is unknown.
> > > > > > (vs likely *some* functionality if we just didn't set the driver)
> > > > > > 
> > > > > > so this could regress some scenarios. Can we mitigate that?
> > > > > > (It's possible that we're running the driver in a mode where we proceed anyway, but I can't remember :-()
> > > > > what if target already exists in `Cmd`?
> > > > > 
> > > > > also it would be nice to use `--target=X` format to be consistent with target inference from invocation name as in https://github.com/llvm/llvm-project/blob/master/clang/lib/Tooling/Tooling.cpp#L278.
> > > > I failed to find options to process in case of invalid target,  so added target validation.
> > > We could specify several --target options, the last one will be used. But I am not sure should we override existing or not.
> > > We could specify several --target options, the last one will be used. But I am not sure should we override existing or not.
> > 
> > Having multiple targets sounds confusing. I would prefer keeping a target specifically mentioned by the user but it is also possible for the target to be inferred by clang (and that might be wrong). This is similar to our stand against predefined macros though, i think we should fix clang's inference logic if it has any false positives. So i am slightly leaning towards not overriding the target if it exists.
> I think the question of "target already explicitly set" should probably be answered.
> Checking for an arg equal to "-target" or beginning with "--target" is probably enough.
I think we should definitely not override the target if it already exists.

The scenario is: compile_commands.json has `/my/driver --target=bar foo.cc`.
`/my/driver reports "Target: foo"`

So foo is the default target, but it's being overridden on the command-line as part of the build.

Now if we add the extracted target to the end of the compile command, to produce `/my/driver --target=bar foo.cc --target=foo`, we're effectively reversing the precedence: now the default target is used even if the compile command overrode it.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D92012/new/

https://reviews.llvm.org/D92012