a drive-by comment -- I would really appreciate *not* adding any new uses of C preprocessor.

+Matt

Would it be possible to add a threaded test that fails w/o this change?
LGTM otherwise, thanks!

would it be acceptable to have an environment variable or launch parameter that could allow the silent creation of these directories?

In D84808#2195169, @dgg5503 wrote:

In D84808#2194844, @kcc wrote:

From the description:

this PR adds automatic directory creation for locations in which libFuzzer expects to write data.

I'd prefer libFuzzer to not create directories, but instead err-and-exit if those don't exist.

I can make this change, but is there a reason why this shouldn't be done? It seems more convenient for the end user but perhaps I'm overlooking a larger issue.

O, wow, thanks for catching this.
Could you please add a test (in compiler-rt/test/fuzzer) that would reliably fail currently
and reliably pass with this change?

From the description:

this PR adds automatic directory creation for locations in which libFuzzer expects to write data.

I'd rather fail instead of silently creating new dirs, to be consistent with the other behavior

Please fix two nits, then good to go.
Thanks!

In D84947#2186106, @IanPudney wrote:

Sticking just with x86_64 is possible; I actually have the code for that here, but it's a bit ugly:
https://reviews.llvm.org/differential/diff/281467/

Do we need a version for 32-bit at all?
Not having a private version of libc++ is likely to cause subtle stability issues.

The compiler change seems to be completely independent from the libFuzzer change.
Please split this change into two.

LGTM.
Matt, please help land it

Code LGTM, thanks!
Please add a section in docs/LibFuzzer.html.
I'd add it after "Startup initialization", something like "Using libFuzzer as a library".

Yep, cool.
LGTM from me, but please get another pair if eyes (Vitaly?)

I am concerned about this change.
We've essentially exposed an implementation detail (both the function FuzzerDriver
and this header file, with all of its other internal details) to outside users.
This means we have more things to support as an API.
Maybe we could revert it and get back to the drawing board?

In what cases do we still call __dfsan_union?

LGTM

Also, we don't have to err in these functions at all, it's fine to just return silently.

and that's fine. I want this mode to be as simple as possible.

I think this is an overkill.
fast16labels mode should be even simpler:
there are always 16 primary labels, they don't have any descriptions or properties controlled by dfsan.

No strong opinion on whether this needs to be done.
If you feel strong, and if it will help, sure. (you may indeed have to test on various platforms, or rely on the post-commit bots)
OTOH, the new profiler should not require all of these functions, you can probably get away with a custom-tailored variant of MapDynamicShadow.

Will adding attribute((no_sanitize("address"))) to your global solve the problem you are trying to solve?
(sorry for being too terse last time)

can we instead slap an attribute on these special variables?

In D82685#2133646, @Dor1s wrote:

In D82685#2133565, @kcc wrote:

My preference would be to reject weird file names instead of adding this extra complexity.

so we'll have a list of allowed (or disallowed) characters and error out if any of the arguments passed do not comply?

My preference would be to reject weird file names instead of adding this extra complexity.

LGTM (even though it's sad...)

also, please avoid #ifdefs.
OS-specific code should go to an OS-specific file.

Hi, 
This made our ubsan bots red. Please fix or revert ASAP
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/42256

Submitted: https://github.com/llvm/llvm-project/commit/2e6c3e3e7b5eb46452b1819c69919fab820b4233
(had some trouble with arc... pushed via git push instead of arc land)

(update)

(fix typo)

Vitaly, please review (and land if ok).

(let me land it)

Thanks for this work, and the effort to make the code better!

Sorry for the delay. Mostly naming/style nits left.

Please take out the time-related changes for now. If anything, extra changes make the code review process quadratic.

This change causes a performance regression in tsan, as detected on our LLVM buildbot:
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-autoconf/builds/49850/steps/tsan%20analyze/logs/stdio

Commenting on just to issues, not the hole patch.

The code is ok, but I'd like to see an ACK from Dmitry.

We *may* need to add more arguments to these callbacks (with the data pointers) later.
Right now I am not sure.

Code LGTM, but please add some comments near the flag definition.

LGTM

thanks!

Sounds good. Max and I will do the next round(s) of review.

LGTM, thanks!

It's exciting that such a small change can bring such a great improvement.
Thanks for the contribution.

It looks like this patch has at least two independent changes.
Please prefer to send two+ individual changes next time. (even if the patches are tiny).

In D69537#1799790, @johanengelen wrote:

In D69537#1799478, @kcc wrote:

How is this going to work when one thread calls __sanitizer_for_each_extra_stack_range with another thread's ID,
while that other thread creates and discards frames, or while that other thread is being destroyed?

What is assumed is that the other threads have been stopped,

How is this going to work when one thread calls __sanitizer_for_each_extra_stack_range with another thread's ID,
while that other thread creates and discards frames, or while that other thread is being destroyed?

E.g. we have -print_stats that prints machine-readable output, we can do something like that and guarantee it doesn't change.

We want the user to also have user-readable output, though.

We're using an autogenerated trait for this anyway, so we get this for free.

BTW, may I ask you to provide some details of your Rust fuzz target examples?
(like the code of the fuzz target and the output with your patch)

Understood.
Before agreeing with the approach I'd like to hear from more users who need this and some details about their use cases. I've pinged some users.

In D70738#1772158, @Manishearth wrote:

I can totally see how this is helpful in some cases when running libFuzzer manually, but it can also be very annoying when the reproducer is large.
In any kind of automated scenario, it should be easy to add a separate binary that prints the inputs in human readable form.

This requires parsing the human-readable libfuzzer output though, which could change, and is also brittle

[sorry for delay, I was OOO]
So, this patch will cause LLVMFuzzerCustomOutput to be called on the reproducer input
which in turn will cause an arbitrarily large input to be printed to stderr (stdin)?
Or in fact, it will cause an arbitrary action to be performed with {Data,Size}

I'd prefer to not overload the public interface and this use-case sounds a bit too corner-case-ish.
Am I wrong?

please avoid #ifdefs -- they are pure evil.
Can you move this function to FuzzerUtil*.cpp?
In this case it will have it's own implementation in FuzzerUtilFuchsia.cpp

LGTM

LGTM, thanks!

I was actually hoping to get rid of this code entirely.
Why do you need this change?

LGTM

but please update the commit message to explain why this enables us to use __GLIBC_PREREQ

The asan bot is unhappy:
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/35078/steps/check-llvm%20asan/logs/stdio

LGTM with one nit, also asking Matt for a second review.

I would prefer to not introduce this complexity.
For periodic pruning we can use an empty dir, like you describe.
For stats, we can use the overal corpus size (in bytes and in files)

LGTM

With a build command line, of course

clang++ -g -std=c++17 string-view-use-after-free.cc  -fsanitize=scudo

Maybe add an example?
something like my favorite

#include <iostream>
#include <string>
#include <string_view>

Vitaly, please take a look as well.

Frankly, I would like to see a parallel effort in Musl.
I've seen the recent comment at https://github.com/google/sanitizers/issues/1080#issuecomment-506565699
suggesting that Musl is not likely to accept this approach.
But please look at the situation from our perspective:
this amount of #ifdefs in the code will increase the cost of maintenance for us, w/o giving much to anyone who doesn't use Musl.
And this is a wrong technical decision anyway, as we've known in 2011 (just didn't have other choice)
and as we've confirmed with our current implementation of HWASAN in Bionic.