Hi @vvereschaka, thank you for the quick approval! I'm not entirely sure why the tests are so flaky especially since I was able to pass all LIT tests under Windows and Linux locally. I've re-ran the pre-merge build 5 times and it appears that the three configurations pass if you combine runs 1 and 3. Is this enough to safely merge or should I continue rebuilding until all configurations passes?

Improved test compatibility for Windows.

Address @morehouse comments:

• Ignore chmod exit code for improved compatibility when running the test fuzzer-dirs.test on Windows
• Check for dummy_dir creation

Unchecked some mistakenly checked Dones.

Addressed @morehouse comments:

• const int to bool for argument in ValidateDirectoryExists
• Simplified MkDirRecursiveInner
• Spelling

Address @morehouse comment.

Ping.

Fixed the diff

In D84808#2200407, @kcc wrote:

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.

First reason is consistency. We currently don't create DIRs if they don't exist.
The second reason is avoiding surprise.
Suppose a user has a typo in a directory name.
Crashing because a wrongly named dir doesn't exist is not a surprise.
Working and using a wrongly named dir *is* a surprise.

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.

In D84808#2194438, @kcc wrote:

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

Ping.

Applied clang-format to some areas of the code but left others alone to be consistent with the rest of the libFuzzer code base. Please let me know if it's appropriate to apply all suggested clang-tidy and clang-format changes.

Thanks for the review & approval @vitalybuka! Before I request someone to commit this patch, can you please take a look at my response to your concern regarding the changes made to __sanitizer_symbolize_code?

In D79280#2021148, @vitalybuka wrote:

These tests makes me think that maybe our bot is not running what is expected. I'll check.

Ping.

@vitalybuka thanks for the approval! Can someone with commit access please push these changes?

Added spaces around "&&".

In D79280#2019393, @vitalybuka wrote:

How are those tests are broken? We run InternalSymbolizer on the http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux

Ran git-clang-format HEAD to resolve CI errors.

Require little-endian byte order when executing tests which read in byte order sensitive .sancov files. This should prevent LIT test failure on big-endian systems such as s390x which is what caused this commit to be reverted as per https://reviews.llvm.org/rL374636.

@MaskRay sure.
Name: Douglas Gliner
E-mail:

@MaskRay, is my updated description okay? If so, can someone with commit access please push this change?

@MaskRay thanks for the approval. I just want to make sure you saw my earlier question since I haven't yet updated the description or added a comment:

Changed sys::Process::FileDescriptorIsDisplayed(FD) to raw_fd_ostream::is_displayed().

@MaskRay thanks for the feedback. Before I update the summary and code with a comment, I'd like to first confirm that from our discussion about this function, are Windows, Minix, and fully POSIX compliant OS the only hosts supported by LLVM so far?

At the least, the target OS should be mentioned.

Ping.

So I've finally taken the time to figure out why these tests fail on s390x using QEMU. When .sancov files are loaded into memory and the header is checked in isCoverageFile, the check fails presumably due to endianess. For example, the sancov header magic for test-linux_x86_64.0.sancov is expected to be 0xC0BFFFFF but is read as ‭0xFFFFBFC0‬ on big endian systems. I assume the same issue is causing the tests to fail when ran on ppc64be. I am not at all familiar with how other parts of LLVM handle endian differences when reading files from memory using MemoryBuffer::getFile or equivalent. I'd guess there'd be a portable method since llvm-objdump -d /test/tools/sancov/Inputs/test-linux_x86_64 returns the identical output when ran on both s390x and x86_64, but perhaps there are other factors in play that I'm overlooking. What are the expectations regarding cross platform compatibility with sancov? Is it reasonable to expect sancov to be able to parse and provide coverage information for any .sancov and binary compiled for a target supported by LLVM on any platform? Or should users be expected to use .sancov files on the same host that generated them?

@vitalybuka I plan to debug this failure as soon as I can. I imagine I'd have to use QEmu to emulate ppc64/s390x since I don't have such a machine on hand. Thanks for committing my other diff though!

In D68752#1705235, @kcc wrote:

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

Hi @kcc,

@vitalybuka thanks for the explanation. I believe I did it correctly, please let me know otherwise. It is my first time submitting a change to the LLVM project!

Split out JSON changes to D68752.

In D51018#1702330, @vitalybuka wrote:

In D51018#1700811, @dgg5503 wrote:

@vsk thanks for the review! It looks like the JSON support library implements what JSONWriter does in this tool. To reduce maintenance, I've updated sancov to use the JSON support library implementation instead. The only downside to this change is that the JSON text format differs compared to the original implementation. I'm open to reverting this diff and simply adding your suggested change which also worked. Let me know what you think.

EDIT:
I've also updated the title and description to better describe the changes in this diff.

I like the change.

Could you move JSONWriter -> JSON refactoring into separate patch and rebase win stuff ontop?
If you don't have commiter access, someone will need to commit it for you
So I don't mind to split the patch and commit myself.

@vsk thanks for the review! It looks like the JSON support library implements what JSONWriter does in this tool. To reduce maintenance, I've updated sancov to use the JSON support library implementation instead. The only downside to this change is that the JSON text format differs compared to the original implementation. I'm open to reverting this diff and simply adding your suggested change which also worked. Let me know what you think.

Ping.

Ping.

Ping.

Ping.

Ping.

Ping.

Ping.

Ping.

Ping.

Rebase over mono-repo.