LGTM, thanks!

I spotted a typo

thanks! :)

LGTM

Cool!
please add the documentation and the run-time test to the same CL.

In D132611#3747406, @fmayer wrote:

In D132611#3747396, @kcc wrote:

not arguing for or against, but I wonder how many output parsers will break.

not sure about asan, for hwasan we did some quite significant changes to the output format, and no one complained at us.

not arguing for or against, but I wonder how many output parsers will break.

dropping the function name is fine, we have it from the stack traces.
the variable name can't be found anywhere else because in most cases
msan is used with -gline-tables-only, i.e. DWARF doesn't have the names of locals.

Where can I get the name of the variable? (I think it is buried in the debug info some place, but I would need an example to help me extract it.)
An alternative I thought of was putting this behind a flag.

please show an error message on a non-trivial test case
(lots of variables, some defined on the same line) before and after.
Code size is important, but we cannot decrease the report quality

needed to add the same callbacks in more places.
Now the tests pass.

removed unneeded (void)CBRes;

LGTM, but please wait for Vitaly

addressed review comments

one more test fix (can't expect xxd to be present)

fix a test

fix dfsan build

What's the code size implications?

limit the test to x86_64 linux as it is too experimental to be used elsewhere,
and __libfuzzer_extra_counters are linux-only anyway, currently.

fixing incomplete fix, sorry

addressed one more review comment

addressed review comments

One comment that I have is a request to limit the number of #ifdefs in the code to at most one.
We typically achieve this by having platform-specific code in a platform-specific file, guarded with an ifdef.

LGTM

Thanks for the change!
Indeed, the current single-pass merge is far from perfect, and it's nice to see your numbers.

Yey, great idea! :)
(I am not reviewing the code; but the change looks straightforward)

+Vitaly Buka <vitalybuka@google.com> +Matt Morehouse <mascasa@google.com>

We can't possibly maintain two variants of scudo.
All effort is currently spent on the newer (standalone) version.
I am afraid we will have to delete the older (non-standalone) variant entirely.
(And the sooner the better)

ugh..
If I were the maintainer of this file, I would run away from this change.
Not because there is something wrong with it functionality-wise, but because of the ifdefs :(
We ourselves in the sanitizer land would reject a change with this many ifdefs w/o looking further.

(2 bytes per bit!)

1 byte per bit, hopefully. (for the new 8-bit mode only)

In D96842#2569364, @ldionne wrote:

The reason why we can't make this change as-is is that it modifies the API of std::vector when instantiated with bool when the dataflow sanitizer is used. Specifically, the specialization of vector<bool> has a different interface, it's not only an optimization. For example std::vector<bool>::reference has a .flip() method. So if someone is doing something like v[3].flip() (which is legal), now their code won't compile when they turn on the dataflow sanitizer. That's not acceptable, and it would make us non-conforming when the dataflow sanitizer is used.

Why not?
DFSan is a separate ABI, you can't mix DFSan-ified code with non-DFSan-ified code.
There is no annotation that we can imagine to work in this case.
DFSan's metadata is per-byte, if we mix different taints in a single byte (8 packet bits) we get an overtaint, i.e. a false positive.
The only other solution for our users is to not use vector<bool>

LGTM, thanks for the better fix!

yea, I am afraid that removing -O1 weakens our ability to find subtle bugs in how sanitizers work with the optimized code.
After all, most of the uses for the sanitizers are with -O1 and higher, so by testing with -O0 we are hiding potential problems.
I think the best is to prevent inlining (noinline attribute, or a command line if available)

I am reluctant to extend the public interface in ways that
a) are likely to be useful for only few cases
b) are likely to remain libFuzzer-specific
c) already have an existing functionality that can be used instead). I mean the existing -dict flag (it's not exactly what you describe though)

This header is intentionally private, so that the fuzz targets remain engine-neutral.

This worked for us for many years.
Changing the default is likely to break some of the existing users.

did you consider approaches where the emitted code doesn't change, but the binary contains a debug-like metadata that corresponds to the trap instructions?
Matt (CC-ed) has a patch if this kind (for a different purpose) in the works .

LGTM, thanks!

But I'm not sure how best to integrate this -- are there existing crashing tests somewhere I should add this to?

compiler-rt/test/fuzzer

please no #ifdefs.
please add a test.

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/