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

[Sam McCall via Phabricator via cfe-commits]

sammccall accepted this revision.
sammccall added a comment.
This revision is now accepted and ready to land.

Thanks! just nits, apart from the "-target already explicitly set" issue Kadir raised.



================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:99
+      }
+      if (!Info.Target.empty())
+        break;
----------------
I'm not sure these early returns are worth the complexity


================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:102
+      LLVM_FALLTHROUGH;
+    case IncludesExtracted:
+      if (Line.trim().startswith(TS)) {
----------------
also not sure whether initial vs includesextracted is worth distinguishing - this doesn't scale well if we add a third thing we're tracking (do we need a state for each combination that we could have seen already?)

Maybe just a bool for SeenIncludes or something to detect duplicate/missing sections would be simpler


================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:106
+        TargetLine.consume_front(TS);
+        // Use only valid target
+        if (!isValidTarget(TargetLine)) {
----------------
this mostly echoes the code: "Only detect targets that clang understands"?


================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:108
+        if (!isValidTarget(TargetLine)) {
+          vlog("Invalid target \"{0}\", ignoring", TargetLine);
+          break;
----------------
I'd prefix with "System include extraction: " and bump up to elog - this is fairly likely to break things (and we'll only log it ~once thanks to caching)


================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:112
+        Info.Target = TargetLine.str();
+        vlog("Target extracted: \"{0}\"", TargetLine);
+      }
----------------
also add a prefix here so it's clear what the context is in the log


================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:227
+  if (!Info->SystemIncludes.empty())
+    log("System includes extractor: successfully executed {0}, got includes: "
+        "\"{1}\"",
----------------
we want to log this even if the set is empty, so the reader can tell the extraction worked
(it's fine to only optionally log target)


================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:231
+  if (!Info->Target.empty())
+    log("Target extractor: successfully executed {0}, got target: "
+        "\"{1}\"",
----------------
The target extractor isn't a separate component from the system includes extractor, so the log message seems a bit misleading.
I'd suggest just using "System includes extractor" for both, even if it's not totally accurate


================
Comment at: clang-tools-extra/clangd/QueryDriverDatabase.cpp:212
+  if (!Target.empty()) {
+    Cmd.CommandLine.push_back("-target");
+    Cmd.CommandLine.push_back(Target);
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92012