[PATCH] D41159: [asan] LIT: Add lld testing config.

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: test/asan/CMakeLists.txt:124
+
+  if(COMPILER_RT_HAS_LLD AND arch STREQUAL "x86_64" AND NOT (APPLE OR WIN32))
+    add_asan_testsuite(${arch} True False)
----------------
vitalybuka wrote:
> so if complier rt has LLD we are going to run 2x+ as much tests?
> cfi has small set of tests, but for entire compiler-rt could be noticeable.
> @eugenis, any concerns?
> so if complier rt has LLD we are going to run 2x+ as much tests?

I believe so. It has already caught one lld problem (see msan differential), and this diff shows something too.

> cfi has small set of tests, but for entire compiler-rt could be noticeable.
Yes, see next comment.


================
Comment at: test/asan/CMakeLists.txt:125
+  if(COMPILER_RT_HAS_LLD AND arch STREQUAL "x86_64" AND NOT (APPLE OR WIN32))
+    add_asan_testsuite(${arch} True False)
+  endif()
----------------
vitalybuka wrote:
> if future you are going to add add_asan_testsuite(${arch} True **True**) call here, right?
> 
Now //this// is an awesome question. No, i'm serious.
I don't know what will be the best course of action **yet**.
I do understand that simply mimicking cfi, and having all the combinations will just not work.
I think having **something** like this in the end may be the best compromise:
```
add_asan_testsuite(${arch} False False)
add_asan_testsuite(${arch} ${COMPILER_RT_HAS_LLD} True)
```
i.e. just two combinations - old plain simple; and lld if it is there + lto (thinlto if it is there)


================
Comment at: test/asan/lit.site.cfg.in:12
 config.target_arch = "@ASAN_TEST_TARGET_ARCH@"
+config.use_lld = @ASAN_TEST_USE_LLD@
+config.use_thinlto = @ASAN_TEST_USE_THINLTO@
----------------
vitalybuka wrote:
> lebedev.ri wrote:
> > vitalybuka wrote:
> > > lebedev.ri wrote:
> > > > vitalybuka wrote:
> > > > > Why just asan? can we go for entire compiler-rt?
> > > > I'm not sure what you mean. This is per-sanitizer-config option.
> > > > I.e. there is a clang lit config, and a clang lit config with LLD. (and later, with LTO)
> > > > I'm not sure how to do that for the entire compiler-rt.
> > > >  
> > > I think we should put it into compiler-rt/test/lit.common.cfg and try to support the option by all sanitizes
> > That is great to know, but can you explain in a few more words, please? :)
> > For the reference, i'm modelling this after CFI - it has several lit configs, with lld&thinlto, just lld, just thinlto, etc.
> > 
> please ignore my comment. I've checked patches from description. msan is already done this way
>msan is already done this way
And ubsan


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D41159