[PATCH] D111443: [Driver] Fix ToolChain::getSanitizerArgs

Artem Belevich via Phabricator via cfe-commits cfe-commits at lists.llvm.org
Fri Oct 8 15:37:58 PDT 2021


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



More information about the cfe-commits mailing list