Query Driver -- Oddities in Log #1378
Comments
|
[@HighCommander4, I'm going to start by tagging you if that's okay, only since you've been great about narrowing these down in the past, and we just discussed an include_next symptomed issue of a different type.] |
cpsauer
changed the title
|
Aha. Okay, so I think (1) might be fixed by needing to preserve --target in the system includes extraction, cross-compiling being broken without that. Executing without reproduces the problematic include paths. Executing with fixes them. [(2) and (3) are separate, I think.] vs. |
|
I do also see another bug in that code, just reading quickly. Also heads that the phabricator links seem to be messed up somehow. Like look at llvm/llvm-project@6e8d6bc (which added this code) and click on the differential revision link. It points to some unrelated cmake change. So no additional context there. |
|
I started spinning up a quick patch to fix (1) (plus a typo), but decided I should check in with someone more experienced with the codebase. It occurred that maybe it'd be better to switch things over to reuse clang's arg parsing, like with ArgStripper in CompileCommands.cpp, unifying logic so we have fewer issues like the isysroot one, above? Note that, like isysroot, the target flag doesn't fit cleanly into the ArgsToPreserve abstraction and does indeed have a different number of - in its = and non = forms. (ref) Also, scrolling down quickly, I'm seeing more concerning things. There's another isysroot bug ( Anyway, saving out the mini patch here in public, just in case it's useful. Maybe we should push it as a stopgap? I want to flag the issues I'm seeing, but also want to keep us moving incrementally, rather than getting bogged down in trying to fix everything here atomically. --- a/clang-tools-extra/clangd/SystemIncludeExtractor.cpp
+++ b/clang-tools-extra/clangd/SystemIncludeExtractor.cpp
@@ -184,13 +184,18 @@ extractSystemIncludesAndTarget(llvm::SmallString<128> Driver,
const llvm::StringRef FlagsToPreserve[] = {
"-nostdinc", "--no-standard-includes", "-nostdinc++", "-nobuiltininc"};
// Preserves these flags and their values, either as separate args or with an
- // equalsbetween them
+ // equals between them
const llvm::StringRef ArgsToPreserve[] = {"--sysroot", "-isysroot"};
for (size_t I = 0, E = CommandLine.size(); I < E; ++I) {
llvm::StringRef Arg = CommandLine[I];
if (llvm::is_contained(FlagsToPreserve, Arg)) {
Args.push_back(Arg);
+ } else if (Arg.startswith("--target=")) {
+ Args.push_back(Arg);
+ } else if (I + 1 < E && Arg.equals("-target")) {
+ Args.push_back(CommandLine[I]);
+ Args.push_back(CommandLine[++I]);
} else {
const auto *Found =
llvm::find_if(ArgsToPreserve, [&Arg](llvm::StringRef S) { |
I think what happens is that, since the workflows for submitting a patch for review and then merging the reviewed version are not fully automated/scripted, developers sometimes end up copying the Phabricator link into the commit message manually, leading to occasional errors. The correct link in this case is https://reviews.llvm.org/D73453. |
I'd guess it's once per file type (e.g. C header, C source file, C++ header, etc.), since the file type is part of the memoization key. |
+1 -- I would say post it for review, and we can consider the larger changes to (1) reuse more proper argument parsing code and (2) include the preserved args (or a hash of them or something similar) in the memoization key, as follow-up improvements. |
(Not sure on this one, I will defer to someone who uses and is more familiar with Mac.) |
|
Replying to each in turn :) Thanks for being great as usual, @HighCommander4. Re (2), maybe it's once per language type. But it still seems curious to have multiple before building a c++ source file. Shouldn't the file type always be C++ source? Anyway, this one's the least of the problems. Re (1) and the patch: Thanks for encouraging the incremental improvement. Done. https://reviews.llvm.org/D138546 Please do coach me! This is LLVM review number 2 for me. Re the many other bugs in that file: Happy to help, but someone with more experience in the codebase should probably take the lead here, esp if we're trying to use existing parsing. That said, your utility libraries and online docs are oh-so-nice. Re (3): I can be that person if you want :). It's a bug. I ran some quick experiments passing them in via Anyway, thanks again, Nathan :) |
It seems to, yeah. |
|
Sg. So on all platforms, then. One less special case to consider. |
|
Hi all, i've put together the concerns around caching in #1403. I think we need to first get that sorted out to make progress here, patches welcome. i've also created #1404 for tracking But in addition to that, i'd like to hear why you need query-driver in the first place @cpsauer, considering the actual driver you're querying is clang in the end (as I've pointed out in the review as well). This is not related to handling |
|
Thanks, @kadircet, for being great :) Re query-driver: Without it, I was seeing a host sysroot get incorrectly inferred, unfortunately. I'm guessing this has something to do, similarly, with the target affecting the default include directories in the driver? I don't yet know exactly how this works. To give an example from the clangd log, as requested, ndk compilation, without query-driver, takes the form: But more generally, Bazel folks (at least) have been finding they need to use query driver by default--both because of difficulties inferring, like the above, but also since toolchains just keep having custom driver wrappers scripts for one reason or another (annoying, I know, but prevalent), and query driver helps penetrate. |
|
So you're saying that Let me ask a couple more questions to clarify the situation here,
A general side note, https://github.com/llvm/llvm-project/blob/main/clang-tools-extra/clangd/CompileCommands.cpp#L335 is where we inject If not having |
|
Yep. The macOS SDK isn't what one would expect to see inferred as the sysroot for the Android NDK. One would hope instead to just infer the default system include directories for the NDK. The include directories one gets by querying the driver with That is, to succeed without querying the driver, we want to infer just the system include paths (where $DRIVER_DIR is just a stand in for the directory containing the driver, not a environment var clangd should depend on or anything.) Those paths look like they're probably just standard? And, from browsing around, clangd's suggestions do already seem to infer them correctly, additionally rejecting host-specific includes, so it seems like things are working somehow, ignoring the host sysroot, though I'm not sure where or why. Basically, on closer inspection, things do seem to work without query-driver, I'd just been spooked by the log specifying a host sysroot for cross-compilation. Still seems a little wrong to infer a host sysroot for cross-compiling and then ignore it somehow, but it seems like the issue doesn't make its way into the editor. Just to recap and make sure I'm explicitly answering each of your questions: Yes, the macOS SDK sysroot does seem wrong, though it seems to get ignored before it breaks things for the user.
Anyway, thanks @kadircet for taking an interest, and for being so nice and thoughtful. |
|
IIUC, everything is working as expected on your end (somewhat incidentally though). So glad to hear that. Regarding the include paths you mentioned, yes those are the standard ones that would be heuristically detected by any clang (assuming argv[0] is mentioned correctly in compile commands, which seem to be the case for you). the "../sysroot" pieces look a little surprising, but apparently android toolchain does that. The only action that we haven't discussed yet, coming out of this issue, seems like not setting I'd suggest creating a new issue for not setting |
|
Hey, @kadircet! Indeed, the non-query driver case does seem to work for Android after all. (But I'd wanted to answer your questions about why people were using query-driver--and I hadn't realized the extent to which the problematic-looking host flag, logged, was ignored.) Thanks for the link--and your kindness and interest. I had no idea there was that much special casing on triples for include paths! Re the automatic mac isysroot, might we need logic to see if the triple is set to something besides macOS, so we don't get it if the Apple clang is being used for other targets? I figure we should resolve that before filing. Plus, you seem to have special magic for calling in help, given people had submitted diffs before I'd even read the new issues! Maybe just the help-wanted tag. Could I ask you do file this one, too, with that tag? I think you have a significantly better understanding of what's going on behind the scenes with this one than I do, so could you file? I think we've also got one other outstanding sub-issue: The need to strip " (framework directory)" and put those paths under -iframework. That is, (3) in the original post Perhaps we should keep this one open until the core, target -> query-driver change lands? Thanks again, |
Hey wonderful clangd folks,
I'm seeing some oddities in the clangd log that makes me think there are at some issues with --query-driver, compounding each other. They end up leading, I think maybe, to a bunch of extraneous standard library warnings, degrading autocomplete.
Logs
I'm seeing things that look like this
There are some fairly strange things in there.
[It also logs that it thinks the target is darwin, which is the host. I can see that that is indeed the default target the compiler emits when run with -v and no other arguments, but, looking at the implementation, it looks like this value is then subsequently ignored because, e.g.,
--target=aarch64-linux-android21is in the command.]-isystem "/System/Library/Frameworks (framework directory)" -isystem "/Library/Frameworks (framework directory)"gets added to the command verbatim. The framework directories would need to instead be added via-For-iframework, (with " (framework directory)" stripped--not that they belong in an Android build command anyway.FWIW, the original problem that got me looking in the logs was spurious errors like
In included file: no member named 'int8_t' in the global namespace, which, when clicked down into, also shows a failed include_next in stdint.hMain file cannot be included recursively when building a preamble. Not sure how related those issues are to the problems spotted in the log--the top level false errors do disappear when --query-driver is removed...although an erroneous MacOSX.sdk does remain in the command in the log. Anyway, I figured you all would want a heads up about the oddities in the log.System information
I'm seeing this with clangd 15.0.3 and the latest weekly snapshot, 20221113, in VSCode on macOS 12.2.1.
Thanks for reading and for all you do!
Chris
The text was updated successfully, but these errors were encountered: