[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

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

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

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

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

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

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

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

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:

[PATCH] D114213: Compilation Database: Point Bazel users to a solution

cpsauer added a comment. Wahoo! Thank you Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114213/new/ https://reviews.llvm.org/D114213 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D114213: Compilation Database: Point Bazel users to a solution

cpsauer added a comment. @sammccall (or others), could I ask for your help landing the change now that it's approved? (I think I need to ask someone with commit access to do so, per https://llvm.org/docs/Phabricator.html#committing-a-change) Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D114213: Compilation Database: Point Bazel users to a solution

cpsauer added a comment. Good call, @nridge. Will do as soon as we land this one. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D114213/new/ https://reviews.llvm.org/D114213 ___ cfe-commits mailing list

[PATCH] D114213: Compilation Database: Point Bazel users to a solution

cpsauer created this revision. cpsauer added a reviewer: sammccall. cpsauer created this object with visibility "All Users". Herald added subscribers: usaxena95, kadircet. cpsauer requested review of this revision. Herald added subscribers: cfe-commits, ilya-biryukov. Herald added a project: