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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. yaxunl marked an inline comment as done. Closed by commit rG0309e50f33f6: [Driver] Fix ToolChain::getSanitizerArgs (authored by yaxunl). Herald added a project: clang.

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

eugenis accepted this revision. eugenis added a comment. This revision is now accepted and ready to land. LGTM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111443/new/ https://reviews.llvm.org/D111443 ___ cfe-commits mailing list cfe-commits

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

yaxunl marked an inline comment as done. yaxunl added inline comments. Comment at: clang/lib/Driver/ToolChain.cpp:124 + } + return SanitizerArgs(*this, JobArgs, /*DiagnoseErrors=*/false); } eugenis wrote: > SanitizerArgs SanArgs(*this, JobArgs, !SanitizerArgsC

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

yaxunl updated this revision to Diff 386518. yaxunl marked an inline comment as done. yaxunl added a comment. Revised by Evgenii's comments CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111443/new/ https://reviews.llvm.org/D111443 Files: clang/include/clang/Basic/DiagnosticDriverKind

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

eugenis added inline comments. Comment at: clang/lib/Driver/ToolChain.cpp:124 + } + return SanitizerArgs(*this, JobArgs, /*DiagnoseErrors=*/false); } SanitizerArgs SanArgs(*this, JobArgs, !SanitizerArgsChecked); SanitizerArgsChecked = true; return SanArgs;

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

yaxunl added a comment. @eugenis Any further changes needed? Thanks. 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

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

tra added a comment. I'll defer to @eugenis. Overall it looks OK to be. 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/

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

yaxunl added a comment. @tra Any further concerns? Thanks. 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/mailm

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

eugenis added a comment. The approach looks reasonable to me. 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/ma

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

yaxunl added a comment. ping. I have made changes so that the diagnostics about sanitizer args are only emitted once. Any further changes or concerns? Thanks. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111443/new/ https://reviews.llvm.org/D111443 ___

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

yaxunl marked an inline comment as done. yaxunl added a comment. ping 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/cg

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

yaxunl updated this revision to Diff 379806. yaxunl edited the summary of this revision. yaxunl added a comment. diagnose sanitizer args only once. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D111443/new/ https://reviews.llvm.org/D111443 Files: clang/include/clang/Basic/DiagnosticDr

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

yaxunl added a comment. In D111443#3062139 , @eugenis wrote: > Right, but a cache for SanitizerArgs is not enough to avoid repeated > diagnostics, is it? Ex. if I request a non-existing sanitizer, I think I > would get errors from host arg parsing, as w

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

eugenis added a comment. Right, but a cache for SanitizerArgs is not enough to avoid repeated diagnostics, is it? Ex. if I request a non-existing sanitizer, I think I would get errors from host arg parsing, as well as from each of device1 and device2, because each device will have a unique ArgL

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

yaxunl added a comment. In D111443#3052697 , @tra wrote: > In D111443#3052381 , @yaxunl wrote: > >> In D111443#3052099 , @tra wrote: >> >>> I'm curious why we need the cac

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

eugenis added a comment. 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 ca

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

tra added a reviewer: eugenis. tra added a subscriber: eugenis. tra added a comment. In D111443#3052381 , @yaxunl wrote: > In D111443#3052099 , @tra wrote: > >> I'm curious why we need the cache at all. While the

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

yaxunl added a comment. In 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 e

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

tra added a comment. 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. Comment at: clang/include/clang/Driver/ToolChain.h:165 -

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

yaxunl created this revision. yaxunl added a reviewer: tra. Herald added subscribers: abrachet, kerbowa, mstorsjo, jgravelle-google, sbc100, nhaehnle, jvesely, dschuff, emaste. yaxunl requested review of this revision. Herald added a subscriber: aheejin. The driver uses class SanitizerArgs to sto