This is an archive of the discontinued LLVM Phabricator instance.

[ubsan] lit changes for lld testing, future lto testing.
ClosedPublic

Authored by lebedev.ri on Nov 1 2017, 1:47 PM.

Details

Summary

As discussed in https://github.com/google/oss-fuzz/issues/933,
it would be really awesome to be able to use ThinLTO for fuzzing.
However, as @kcc has pointed out, it is currently undefined (untested)
whether the sanitizers actually function properly with LLD and/or LTO.

This patch is inspired by the cfi test, which already do test with LTO
(and/or LLD), since LTO is required for CFI to function.

I started with UBSan, because it's cmakelists / lit.* files appeared
to be the cleanest. This patch adds the infrastructure to easily add
LLD and/or LTO sub-variants of the existing lit test configurations.

Also, this patch adds the LLD flavor, that explicitly does use LLD to link.
The check-ubsan does pass on my machine. And to minimize the [initial]
potential buildbot breakage i have put some restrictions on this flavour.

Please review carefully, i have not worked with lit/sanitizer tests before.

Diff Detail

Event Timeline

lebedev.ri created this revision.Nov 1 2017, 1:47 PM
lebedev.ri updated this revision to Diff 121182.Nov 1 2017, 2:21 PM

Move config.lto_launch param to before config.compile_wrapper param in the invocation line, same as in cfi/lit.cfg.

eugenis added a subscriber: pcc.Nov 1 2017, 2:38 PM
eugenis added inline comments.
test/ubsan/CMakeLists.txt
35

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?

test/ubsan/lit.common.cfg
70

This code should be shared between cfi/ubsan/asan/msan/sanitizer_common. Please move as much of it as possible one level up.

lebedev.ri added inline comments.Nov 2 2017, 10:51 AM
test/ubsan/CMakeLists.txt
35

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.

test/ubsan/lit.common.cfg
70

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...
`

vitalybuka added inline comments.Nov 6 2017, 4:22 PM
test/ubsan/lit.common.cfg
70

I think it's about moving entire use lld and lto stuff from ubsan to parent dir so all sanitizers can use it

eugenis added inline comments.Nov 7 2017, 2:29 PM
test/ubsan/CMakeLists.txt
35

lld and lto as one configuration sounds good.

test/ubsan/lit.common.cfg
70

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.

72

"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.

Aha, i see.
How about this approach?

There is something weird going on with config.cxx_mode_flags in cfi/lit.cfg.
If i *don't* append it at the end (but use config.clangxx, which already contains it), then test/cfi/overwrite.cpp breaks, maybe others.

eugenis added inline comments.Nov 9 2017, 3:09 PM
test/lit.common.cfg
339

Now asan will add target_cflags twice: first here, then in asan/lig.cfg build_invocation.

Perhaps its better to keep compiler and flags in separate config variables, and let individual test projects join them. How about adding run_wrapper and compile_wrapper to config.clang, and -fuse-ld and lto_flags to config.target_cflags?

eugenis edited edge metadata.Nov 9 2017, 3:10 PM

About cxx_mode_flags - I'm not sure what's wrong. You'd need to compare the generated test command lines.

lebedev.ri added inline comments.Nov 10 2017, 3:38 AM
test/lit.common.cfg
339

Now asan will add target_cflags twice: first here, then in asan/lig.cfg build_invocation.

Yes, because i have only touched cfi and ubsan.
If these changes make sense, i would look into asan/msan next.

How about adding run_wrapper and compile_wrapper to config.clang

https://reviews.llvm.org/D39508#inline-345840 - i.e. how do i do that?

lebedev.ri marked 5 inline comments as done.
lebedev.ri changed the repository for this revision from rL LLVM to rCRT Compiler Runtime.

Rebased.
Changed the code to simply update the config.clang and config.target_cflags, and projects will have to join them by themselves.
Does this look better?

Herald added a subscriber: Restricted Project. · View Herald TranscriptNov 28 2017, 11:50 AM

Looks fine.
You need to update other sanitizers like asan to not add compile_wrapper the second time.

test/lit.common.cfg
345

why not keep target_cflags as a list of strings? Looks like all users are joining them anyway.

Looks fine.

Great, finally onto something here :)

You need to update other sanitizers like asan to not add compile_wrapper the second time.

Yep, done.

For UBSan, the LLD testing is added, for others - nothing yet.
Should i do that in this patch, or split the non-cleanup stuff into the next patch?

lebedev.ri added inline comments.Nov 29 2017, 4:10 AM
test/lit.common.cfg
345

Not *all* of them.
There are some places that treat it as a string (via re.search e.g.)
So i *think* it is best to keep it as string, not list of strings.

re.search('mthumb', config.target_cflags)

can be rewritten as

'-mthumb' in config.target_cflags

if target_cflags is a list. It's IMO both easier to read and less likely to match something unexpected, say part of a --sysroot path.

It's fine to add ubsan+lld testing in the same patch.

lebedev.ri marked 2 inline comments as done.

Changed target_cflags to be a list.
The differential are starting to become a little bit messy :)

eugenis added inline comments.Nov 30 2017, 10:52 AM
test/asan/lit.cfg
172

I don't think "is not None" part is necessary.

test/lit.common.cfg
330

Ah, I did not realize that cmake passes multiple flags as a string. Then mthumb in target_cflags check would not work.

Also, if there are no flags, this would create a 1-element [None] list, which would fail to stringify later.

Sorry for the extra work. Let's revert to target_cflags being a string.

lebedev.ri marked an inline comment as done.

And back to target_cflags being a string.

Re-uploaded previous diff (124711).

eugenis accepted this revision.Nov 30 2017, 1:37 PM

Looks good!

This revision is now accepted and ready to land.Nov 30 2017, 1:37 PM

ninja check-sanitizer check-asan check-asan-dynamic check-cfi check-cfi-and-supported check-scudo check-tsan check-ubsan check-ubsan-minimal check-msan passes, not sure what else to check, i'm gonna land this and revert if any bot is not happy.

This revision was automatically updated to reflect the committed changes.
lebedev.ri reopened this revision.Dec 1 2017, 2:19 AM

Reverted due to some *strange* buildbot failure:

This change has introduced a problem with the Lit tests build for compiler-rt using Gold: http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux/builds/6047/steps/test%20standalone%20compiler-rt/logs/stdio

llvm-lit: /b/sanitizer-x86_64-linux/build/llvm/utils/lit/lit/TestingConfig.py:101: fatal: unable to parse config file '/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/test/profile/Linux/lit.local.cfg', traceback: Traceback (most recent call last):
  File "/b/sanitizer-x86_64-linux/build/llvm/utils/lit/lit/TestingConfig.py", line 88, in load_from_path
    exec(compile(data, path, 'exec'), cfg_globals, None)
  File "/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/test/profile/Linux/lit.local.cfg", line 37, in <module>
    if root.host_os not in ['Linux'] or not is_gold_linker_available():
  File "/b/sanitizer-x86_64-linux/build/llvm/projects/compiler-rt/test/profile/Linux/lit.local.cfg", line 27, in is_gold_linker_available
    stderr = subprocess.PIPE)
  File "/usr/lib/python2.7/subprocess.py", line 390, in __init__
    errread, errwrite)
  File "/usr/lib/python2.7/subprocess.py", line 1024, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory
This revision is now accepted and ready to land.Dec 1 2017, 2:19 AM

Ok, reproduced with (ld is gold)

$ CC=clang CXX=clang++ cmake ../compiler-rt -DLLVM_CONFIG_PATH=/usr/bin/llvm-config-6.0 -DCOMPILER_RT_INCLUDE_TESTS=ON -DLLVM_MAIN_SRC_DIR=/build/llvm
$ make -j9
$ make check-all -j9
...
[100%] Running all regression tests
llvm-lit: /build/llvm/utils/lit/lit/TestingConfig.py:101: fatal: unable to parse config file '/build/compiler-rt/test/profile/Linux/lit.local.cfg', traceback: Traceback (most recent call last):
  File "/build/llvm/utils/lit/lit/TestingConfig.py", line 88, in load_from_path
    exec(compile(data, path, 'exec'), cfg_globals, None)
  File "/build/compiler-rt/test/profile/Linux/lit.local.cfg", line 37, in <module>
    if root.host_os not in ['Linux'] or not is_gold_linker_available():
  File "/build/compiler-rt/test/profile/Linux/lit.local.cfg", line 27, in is_gold_linker_available
    stderr = subprocess.PIPE)
  File "/usr/lib/python2.7/subprocess.py", line 390, in __init__
    errread, errwrite)
  File "/usr/lib/python2.7/subprocess.py", line 1025, in _execute_child
    raise child_exception
OSError: [Errno 2] No such file or directory

And the reason is very simple, config.clang is /usr/local/bin/clang (yes, with spaces) there, so naturally such path/binary/executable does not exist.
I guess we now know why`config.compile_wrapper` and run_wrapper were separate before.
Let's see if python has some less safe subprocess.Popen() variant that would be ok with it.

lebedev.ri updated this revision to Diff 125101.Dec 1 2017, 3:55 AM

Ok, so this does fix the standalone build, but it is extra-crappy because of shell=True.
Thoughs, ideas?

I'm OK with shell=True.
Another option to make config.clang a list of strings, like we tried with target_cflags.

I'm OK with shell=True.

Ok, will re-commit, let's see if there are other failure modes i did not find yet :/

Another option to make config.clang a list of strings, like we tried with target_cflags.

Hmm, could work actually, but let's not cram everything into one differential :)

This revision was automatically updated to reflect the committed changes.

Alright, no new failures thus far :)

I will finish the simple stuff of modifying the rest of the tests to set the config.use_lld/config.use_lto
(and enable the lld testing at the same time) in the next differential and leave the fun lto stuff until after that.

Roman.