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

Wed Nov 23 02:18:25 PST 2022

sammccall added a comment.

In D138546#3946046 <https://reviews.llvm.org/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 for review rather than getting bogged down in trying to fix multiple things atomically.

Sure. At first glance the design looks like it's been changed in a way that's broken, but maybe there's some deeper reason that it's safe. That reason may or may not also apply to `-target` (e.g. if `-target` could plausibly differ across a project but other flags couldn't). I wanted to understand whether/why it's broken today before concluding it's safe to break it further. Probably @kadircet is the best person to make a call here.

> 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 get you (and others) to quickly read through that issue if you haven't already? (The relevant section to this part: "If we think sysroots, targets, and the other flags enumerated effect the system includes, we'd better include them as part of the memoization key.") )

The discussion in this bug makes sense to me. I agree with the need for memoization, and the handling of `-isysroot` indeed looks dodgy and could probably be fixed.
Framework support sounds important for Mac, and we'd be happy to take a contribution from a Mac person who can explain the issues there.
Reusing clang's arg parser is tricky: it's a large change, it's often more complex than naive string-bashing, and it's also significantly slower.

Unfortunately at the moment I'm not really able to spend work time on improving these things, beyond reviewing patches.
(The system include extractor is mostly useful for things that don't build with stock clang, which at $employer is a minority).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D138546