tra added a reviewer: eugenis. tra added a subscriber: eugenis. tra added a comment.
In D111443#3052381 <https://reviews.llvm.org/D111443#3052381>, @yaxunl wrote: > In D111443#3052099 <https://reviews.llvm.org/D111443#3052099>, @tra wrote: > >> I'm curious why we need the cache at all. While the construction of >> sanitizer args is hairy, it's only called few times from the driver and will >> be lost in the noise compared to everything else. > > It is for avoiding repeated diagnostics. For example, -fsanitize=xxx is > passed to clang and ld. If it is parsed twice, the diagnostics are emitted > twice. There are lit tests to prevent the repeated diagnostics. > For this reason, using job action as cache key will not work since it will > not prevent repeated diagnostics. Fair enough. Perhaps that's what we need to refactor first. We could separate processing of the arguments from diagnosing issues there. I.e. something like `getSanitizerArgs(const llvm::opt::ArgList &JobArgs, bool diag = false)` and call it once with diag=true. Or just make diag a static local var and just enable diags on the first call. > For most non-offloading toolchain, ld and clang job action will not create > new ArgList, therefore they share the same ArgList and avoid repeated > parsing. The ArgList persists for the whole life span of driver, therefore no > life time issue. It may work now, but to me it looks like a dependence on an implementation detail. I do not think it's reasonable to assume that ArgList pointer is immutable and that it coexists with particular job. @eugenis -- WDYT? CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111443/new/ https://reviews.llvm.org/D111443 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
