[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

nridge added a comment. This can be closed, as the change has been made in https://github.com/llvm/llvm-project/pull/65824 CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138546/new/ https://reviews.llvm.org/D138546 ___ cfe-commits mailing

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

kadircet added a comment. Sorry I was waiting for D139277 to land, but it seems to be stuck so sent out D146941 . Let's take a look at this again after that one lands CHANGES SINCE LAST ACTION

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer added a comment. @kadircet, could I get your review on this one? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138546/new/ https://reviews.llvm.org/D138546 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer added a comment. @kadircet, friendly check in, since I got reminded of this: How would you like to proceed from here? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138546/new/ https://reviews.llvm.org/D138546 ___ cfe-commits mailing

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer added a comment. Thanks, Nathan. Makes sense; sounds good. @kadircet, I think this one is ready for you, whenever you want it. Lmk how you'd like to proceed. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138546/new/ https://reviews.llvm.org/D138546

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

nridge added a comment. In D138546#4075681 , @cpsauer wrote: > Sounds like you're not concerned, then, that the > JSONCompilationDatabasePlugin will get invoked and the results then passed to > a SystemIncludeExtractor-enabled mangler? > (I'd seen some

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer updated this revision to Diff 491593. cpsauer added a comment. Rebased CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138546/new/ https://reviews.llvm.org/D138546 Files: clang-tools-extra/clangd/CompileCommands.cpp clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer updated this revision to Diff 491592. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138546/new/ https://reviews.llvm.org/D138546 Files: clang-tools-extra/clangd/CompileCommands.cpp clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer added a comment. Yay. Thanks, Nathan, for guiding, replying, and just generally being so kind, great, and thorough. I'll back out the plugin part of the changes changes momentarily, then. Just to confirm: Sounds like you're not concerned, then, that the JSONCompilationDatabasePlugin

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

nridge added a comment. In D138546#4053538 , @cpsauer wrote: > @nridge, I took a shot at the change you suggested. Confirming that this what > you were thinking of, removing `inferTargetAndDriverMode` from database load > and replacing it with a call

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer added a comment. (If anyone knows what's going on with the (unrelated seeming?) testing timeouts please do say.) CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138546/new/ https://reviews.llvm.org/D138546 ___ cfe-commits mailing list

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer updated this revision to Diff 489317. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138546/new/ https://reviews.llvm.org/D138546 Files: clang-tools-extra/clangd/CompileCommands.cpp clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer updated this revision to Diff 489217. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138546/new/ https://reviews.llvm.org/D138546 Files: clang-tools-extra/clangd/CompileCommands.cpp clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer updated this revision to Diff 489216. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138546/new/ https://reviews.llvm.org/D138546 Files: clang-tools-extra/clangd/CompileCommands.cpp clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer updated this revision to Diff 489214. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138546/new/ https://reviews.llvm.org/D138546 Files: clang-tools-extra/clangd/CompileCommands.cpp clang-tools-extra/clangd/GlobalCompilationDatabase.cpp

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer updated this revision to Diff 489208. cpsauer added a comment. Herald added a project: LLVM. Herald added a subscriber: llvm-commits. @nridge, I took a shot at the change you suggested. Confirming that this what you were thinking of, removing `inferTargetAndDriverMode` from database load

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer added a comment. @nridge, yep, confirming: For Android, --target is being added explicitly as part of the command and we'll need to pass through explicit (but not implicit) --target flags to the system includes extractor to get the correct paths. CHANGES SINCE LAST ACTION

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

nridge added a comment. In D138546#4017356 , @ArcsinX wrote: > In other words, with this patch we preserve `--target=...`, but this option > can appear from >

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer added a subscriber: nridge. cpsauer added a comment. Ooh interesting. Thanks for your careful look, @ArcsinX. To check my understanding: Sounds like, more generally, commands get massaged into clang argument form before they're passed into the query-driver machinery, which can break

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

ArcsinX added a comment. It seems this patch can break GCC toolchains support This patch expects that we never get target-related options in `CommandLine` passed to `extractSystemIncludesAndTarget()` for **GCC** toolchain. But seems this is not always true because of

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer added a comment. Sweet. Thanks again, Nathan. Guys, lmk how you'd like to go from here. If you approve; feel free to copy in/land as part of the other change if that would save time. (I don't have commit access anyway.) CHANGES SINCE LAST ACTION

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

nridge resigned from this revision. nridge added a comment. In D138546#3980350 , @cpsauer wrote: > @nridge, I took a shot at amending the test. Thanks for the pointer! Please > me know if that looks good to you! The test changes look good, thanks. I'm

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer added a comment. @kadircet, you were asking about what's requiring the use of query-driver--I'll reply over in the issue (as you suggest), keeping everything in one place. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138546/new/ https://reviews.llvm.org/D138546

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer added a comment. @nridge, I took a shot at amending the test. Thanks for the pointer! Please me know if that looks good to you! The host-target cross-compile situation you hypothesize is exactly what caused me to notice this. Super common with Bazel. CHANGES SINCE LAST ACTION

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer updated this revision to Diff 481141. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138546/new/ https://reviews.llvm.org/D138546 Files: clang-tools-extra/clangd/SystemIncludeExtractor.cpp clang-tools-extra/clangd/test/system-include-extractor.test Index:

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer added a comment. Sorry for being so slow this time myself, guys. Took me a while to dig myself out of a deep inbox hole after Thanksgiving (US holiday). Really appreciate you all and your friendly responsiveness, especially as I get up to speed here. Repository: rG LLVM Github

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

kadircet added a comment. Sorry for noticing this so late. Yes the initial reason we left `sysroot` and `std/builtin-inc` related flags were exactly that, in theory they could vary but in practice they were applied to all or none of the project. Regarding this change itself, surely preserving

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

nridge added a comment. In D138546#3946144 , @sammccall wrote: > In D138546#3946046 , @cpsauer wrote: > >> Sam, my read, too, is that the memoizing design isn't safe--also that the >> key bug is preexisting. >>

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer added a comment. I suppose, if it ever might help make the case with said employer (Google, right?) the broken, non-stock-clang use case that's motivating this is Android (and also usage from Bazel/Blaze). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer added a comment. Makes sense! Thanks for your time, Sam, and for being great, as always. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138546/new/ https://reviews.llvm.org/D138546 ___

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

sammccall added a comment. In D138546#3946046 , @cpsauer wrote: > Sam, my read, too, is that the memoizing design isn't safe--also that the key > bug is preexisting. > Nathan and I were thinking, though, that we'd should post this incremental > fix

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer added a comment. Sam, my read, too, is that the memoizing design isn't safe--also that the key bug is preexisting. I had the same reaction reading through this file after spotting problems in the log. That's what spawned https://github.com/clangd/clangd/issues/1378. Any chance I could

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

sammccall added a comment. My biggest concern here is propagating more information here without including it in the cache key. Originally the computation was strictly using the cache key as input, but started using the command-line flags in D73453 . @kadircet

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

cpsauer created this revision. cpsauer added reviewers: nridge, sammccall, kadircet. Herald added a subscriber: arphaman. Herald added a project: All. cpsauer requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: