This is an archive of the discontinued LLVM Phabricator instance.

[libc++] Add .fuzz.cpp tests and move the fuzzing tests to the normal locations
AcceptedPublic

Authored by philnik on Jun 29 2023, 12:50 PM.

Details

Reviewers
ldionne
Group Reviewers
Restricted Project

Diff Detail

Event Timeline

philnik created this revision.Jun 29 2023, 12:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2023, 12:50 PM
philnik requested review of this revision.Jun 29 2023, 12:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 29 2023, 12:50 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript
ldionne added a subscriber: kcc.Jul 5 2023, 10:11 AM
ldionne added inline comments.
libcxx/test/libcxx/fuzzing/unique.pass.cpp
1

From https://buildkite.com/llvm-project/libcxx-ci/builds/27285#018908b8-92d3-42af-9094-090e2ac9773b:

St11char_traitsIcESaIcEEE[_ZNK6fuzzer7Command12getFlagValueERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE]+0x2c9): undefined reference to `std::__throw_length_error(char const*)'
/usr/bin/ld: (.text._ZNK6fuzzer7Command12getFlagValueERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE[_ZNK6fuzzer7Command12getFlagValueERKNSt7__cxx1112basic_stringIcSt11char_traitsIcESaIcEEE]+0x2e1): undefined reference to `std::__throw_out_of_range_fmt(char const*, ...)'
/usr/bin/ld: /usr/lib/llvm-17/lib/clang/17/lib/linux/libclang_rt.fuzzer-x86_64.a(FuzzerFork.cpp.o): in function `std::vector<unsigned long, std::allocator<unsigned long> >::insert(__gnu_cxx::__normal_iterator<unsigned long const*, std::vector<unsigned long, std::allocator<unsigned long> > >, unsigned long const&)':
(.text._ZNSt6vectorImSaImEE6insertEN9__gnu_cxx17__normal_iteratorIPKmS1_EERS4_[_ZNSt6vectorImSaImEE6insertEN9__gnu_cxx17__normal_iteratorIPKmS1_EERS4_]+0x280): undefined reference to `std::__throw_length_error(char const*)'
/usr/bin/ld: /usr/lib/llvm-17/lib/clang/17/lib/linux/libclang_rt.fuzzer-x86_64.a(FuzzerFork.cpp.o): in function `void std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile> >::_M_realloc_insert<fuzzer::SizedFile const&>(__gnu_cxx::__normal_iterator<fuzzer::SizedFile*, std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile> > >, fuzzer::SizedFile const&)':
(.text._ZNSt6vectorIN6fuzzer9SizedFileESaIS1_EE17_M_realloc_insertIJRKS1_EEEvN9__gnu_cxx17__normal_iteratorIPS1_S3_EEDpOT_[_ZNSt6vectorIN6fuzzer9SizedFileESaIS1_EE17_M_realloc_insertIJRKS1_EEEvN9__gnu_cxx17__normal_iteratorIPS1_S3_EEDpOT_]+0xe9): undefined reference to `std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >::_M_create(unsigned long&, unsigned long)'
/usr/bin/ld: 
...

So it turns out that on our CI machine (which runs Linux), /usr/lib/llvm-17/lib/clang/17/lib/linux/libclang_rt.fuzzer-x86_64.a has been compiled against libstdc++. I guess that's because compiler-rt is compiled against libstdc++ by default in LLVM releases. However, the result is that we can't use that part of compiler-rt with -nostdlib++.

@kcc Did you folks think about a way that the stdlib itself could use fuzzing? Are we doing something fundamentally wrong, or do we need to basically build our own compiler-rt against libc++ in order to do this?

ldionne added inline comments.Jul 5 2023, 10:26 AM
libcxx/test/libcxx/fuzzing/unique.pass.cpp
1

Other options:

  1. Only run the fuzzing tests when we're doing a bootstrapping build in the CI, since then we can (?) easily control which stdlib compiler-rt is built against (I think). I think COMPILER_RT_CXX_LIBRARY="libcxx" will do that.
  2. Always build a version of compiler-rt against libc++ in our CI and pass that to the test suite so it can set the right --rtlib= flag in Clang. We'd only want to do that for fuzzing tests, since we definitely don't want to customize the compiler-rt in use for our general tests.
philnik updated this revision to Diff 537545.Jul 5 2023, 4:52 PM
philnik marked 2 inline comments as done.

Address comments

philnik added inline comments.Jul 5 2023, 4:52 PM
libcxx/test/libcxx/fuzzing/unique.pass.cpp
1

I went for just running it in the bootstrapping build for now. We can try to build compiler-rt everywhere later.

philnik retitled this revision from [libc++] Run fuzzing tests in the CI and move the tests to the normal locations to [libc++] Add .fuzz.cpp tests and move the fuzzing tests to the normal locations.Jul 5 2023, 4:53 PM
philnik updated this revision to Diff 537824.Jul 6 2023, 11:56 AM

Try to fix CI

ldionne requested changes to this revision.Jul 7 2023, 11:55 AM
ldionne added inline comments.
libcxx/utils/libcxx/test/features.py
103–111
libcxx/utils/libcxx/test/format.py
1

You should take a look at libcxx/utils/ci/oss-fuzz.sh, it will need to be fixed. LIBCPP_OSS_FUZZ won't be needed anymore.

220–225

Let's remove -O3.

221–223
377–380

Let's not hardcode -O3 in fuzz tests. We should instead run the test suite with optimizations enabled when we mean to. And we should also run these fuzzing tests under OSS fuzz.

379

Instead of introducing %{run_flags}, let's hardcode -max_total_time here in the test format. Eventually we might make it something we can customize via the lit config (globally) if there's a use case for it.

Otherwise we're adding complexity (RUN_FLAGS: support) for a very tiny use case.

This revision now requires changes to proceed.Jul 7 2023, 11:55 AM
philnik updated this revision to Diff 538284.Jul 7 2023, 3:40 PM
philnik marked 6 inline comments as done.

Address comments

Pretty much LGTM after applying feedback.

libcxx/utils/ci/run-buildbot
343

Let's add - "**/crash-" to the artifacts paths in buildkite-pipeline.yml. Also we probably want to use a different crash file pattern like libcxx-fuzz-crash-XXXXXXXXXXXXXXXXX if that's possible.

libcxx/utils/libcxx/test/dsl.py
208

Let's add a docstring explaining what this does.

208–213

This also means you're missing tests for this function.

libcxx/utils/libcxx/test/format.py
379

Let's also add a timeout for each function to execute, that way we won't blow up if calling e.g. std::regex with some crazy input leads to an infinite loop.

ldionne added inline comments.Jul 10 2023, 8:49 AM
libcxx/test/libcxx/selftest/fuzz.cpp/compile-error.fuzz.cpp
2

Not attached: we should mark the tests that are failing right now as UNSUPPORTED temporarily and then we can enable them again as we fix those issues. Otherwise this review might be blocked for a long time.

Thanks for working on this! Should we mention fuzzing on the libc++ contributing page. Until this patch I wasn't even aware we had a fuzzing framework. I would say that format should have fuzzing too, @philnik already asked about it, and I will do it.

Thanks for working on this! Should we mention fuzzing on the libc++ contributing page. Until this patch I wasn't even aware we had a fuzzing framework. I would say that format should have fuzzing too, @philnik already asked about it, and I will do it.

I think we should first find out what we actually want to fuzz and how to properly do it before we mention anything on the contributing page. After all, these should be just additional coverage, not the main thing we use for testing.

Thanks for working on this! Should we mention fuzzing on the libc++ contributing page. Until this patch I wasn't even aware we had a fuzzing framework. I would say that format should have fuzzing too, @philnik already asked about it, and I will do it.

I think we should first find out what we actually want to fuzz and how to properly do it before we mention anything on the contributing page. After all, these should be just additional coverage, not the main thing we use for testing.

Fair point. My general idea is to fuzz everything where the library acts on user defined input. I guess that is mainly parsers, where I consider format specifiers and regexes also parsing.

philnik updated this revision to Diff 538731.Jul 10 2023, 10:38 AM
philnik marked 5 inline comments as done.

Address comments

ldionne accepted this revision.Jul 10 2023, 2:51 PM

LGTM w/ CI and comments applied.

libcxx/test/libcxx/selftest/dsl/dsl.sh.py
243

Please add simple tests for compileAndRunSucceeds.

libcxx/test/std/re/regex.fuzz.cpp
13
14
This revision is now accepted and ready to land.Jul 10 2023, 2:51 PM
philnik updated this revision to Diff 539181.Jul 11 2023, 10:35 AM
philnik marked 3 inline comments as done.

Address comments

OSS-Fuzz pulls these fuzzing tests and runs them all the time.
If you move them, then you should update the settings at OSS-Fuzz.

OSS-Fuzz pulls these fuzzing tests and runs them all the time.
If you move them, then you should update the settings at OSS-Fuzz.

IIUC this is handled through the changes to libcxx/utils/ci/oss-fuzz.sh.

philnik updated this revision to Diff 540204.Jul 13 2023, 3:57 PM

Try to fix CI