[PATCH] D39508: [ubsan] lit changes for lld testing, future lto testing.

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis added inline comments.


================
Comment at: test/ubsan/CMakeLists.txt:35
+  endif()
+endmacro()
+
----------------
lebedev.ri wrote:
> eugenis wrote:
> > I'm not sure we want to test all sanitizers with LLD - they don't really interact with the linker, and it doubles testing time. @pcc what do you think?
> For the context, https://github.com/google/oss-fuzz/issues/295#issuecomment-340847238 did sound like the more testing there is, the better.
> While in this case, if needed, it is possible to test only some combinations, i'm not sure (did not look yet) it will be possible for other sanitizers.
> 
> TBN the addition of LTO can double the testing time yet again, but i suppose it is possible to always test lld and lto as one configuration.
lld and lto as one configuration sounds good.


================
Comment at: test/ubsan/lit.common.cfg:68
+  config.unsupported = True
+
+def build_invocation(compile_flags):
----------------
vitalybuka wrote:
> lebedev.ri wrote:
> > eugenis wrote:
> > > This code should be shared between cfi/ubsan/asan/msan/sanitizer_common. Please move as much of it as possible one level up.
> > > 
> > Could you please clarify, do you mean to move `build_invocation()` into `test/lit.common.cfg`, or the variables?
> > Moving the function seemed cleaner, but if i do that, i get `NameError: name 'build_invocation' is not defined`, so i'm most likely missing something...
> > `
> I think it's about moving entire use lld and lto stuff from ubsan to parent dir so all sanitizers can use it
Yes. Maybe just the variables. Basically, adding "use_lld" to the config should switch any sanitizer's tests to lld, no reason for this to be specific to ubsan.



================
Comment at: test/ubsan/lit.common.cfg:70
+def build_invocation(compile_flags):
+  return " " + " ".join(run_wrapper + [config.compile_wrapper, config.clang] + compile_flags) + " "
+
----------------
"run_wrapper" is a misnomer, it is actually a compiler wrapper, as I understand. I'm not sure why it is not part of config.compile_wrapper.


Repository:
  rL LLVM

https://reviews.llvm.org/D39508