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

[Yaxun Liu via Phabricator via cfe-commits]

yaxunl added a comment.

In D111443#3052697 <https://reviews.llvm.org/D111443#3052697>, @tra wrote:

> 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?



In D111443#3055522 <https://reviews.llvm.org/D111443#3055522>, @eugenis wrote:

> Don't we want to diagnose the problems in the job's command line? What kind of changes can the driver do there that would affect SanitizerArgs?
>
> I wonder if diagnostics should still be performed on the job args, but presented to the user differently, mainly because we can no longer refer to the user-provided command line?

For offloading toolchain, the driver could remove -fsanitize for certain devices if they do not support them. Consider the following use case:

-fsanitize=address is passed by the user to clang driver, clang driver constructs 4 jobs by passing translated arguments to the tools:

1. clang for host: -fsanitize=address is passed to clang tool

2. ld for host: -fsantize=address is passed to host linker tool

3. clang for device 1: -fsanitize=address is not passed to clang tool

4. clang for device 2: -fsanitize=address is passed to clang tool

This demonstrates the necessity to parse the sanitizer arguments per job instead of per toolchain, since the same toolchain may parse different sanitizer arguments for different jobs.

If we need not avoid repeated diagnostics, we just need to parse the arguments for each job.

However, if we passed an invalid sanitizer argument to jobs 1, 2, and 4, we will get 3 identical diagnostics. This is annoying. That is why there are lit tests to prevent that.

Basically we need a way to know if the same argument list has been diagnosed. The simplest way is to check the pointer to the argument list since toolchain will reuse the original argument list if it does not change it. In this case, jobs 1, 2 and 4 share the same pointer to argument list, therefore they will be diagnosed only once. Since they are the same, they can be cached by pointer to the argument list.

This is also why separating diagnosing and parsing of sanitizer args does not help avoid repeated diagnostics for different jobs, since different jobs may have different sanitizer args. Then you have to have a map IsDiagnosed[JobArg] to track whether a job arg has been diagnosed. However, if you have that, then it is not too much different than having a cache for SanitizerArgs.


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

https://reviews.llvm.org/D111443