This is an archive of the discontinued LLVM Phabricator instance.

[sancov] Accommodate sancov and coverage report server for use under Windows
ClosedPublic

Authored by dgg5503 on Aug 20 2018, 5:27 PM.

Details

Summary

This patch makes the following changes to SanCov and its complementary Python script in order to resolve issues pertaining to non-UNIX file paths in JSON symbolization information:

  • Convert all paths to use forward slash.
  • Update coverage-report-server.py to correctly handle paths to sources which contain spaces.
  • Remove Linux platform restriction for all SanCov unit tests. All SanCov tests passed when ran on my local Windows machine.

Patch by Douglas Gliner.

Diff Detail

Event Timeline

dgg5503 created this revision.Aug 20 2018, 5:27 PM
dgg5503 updated this revision to Diff 213398.Aug 5 2019, 10:23 AM

Rebase over mono-repo.

Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2019, 10:23 AM
vsk added subscribers: Dor1s, vsk.Oct 8 2019, 1:50 PM

@Dor1s - any chance you know more folks actively working on sancov who have the bandwidth to review?

llvm/tools/sancov/sancov.cpp
289–290

Perhaps this whole function should be simplified to OS << json::Value(S) (see 'llvm/Support/JSON.h')?

dgg5503 updated this revision to Diff 223973.EditedOct 8 2019, 6:58 PM
dgg5503 retitled this revision from [sancov] Fixed malformed JSON when symbolizing coverage information to [sancov] Accommodate sancov and coverage report server for use under Windows.
dgg5503 edited the summary of this revision. (Show Details)

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

Dor1s edited reviewers, added: morehouse, vitalybuka, metzman; removed: aizatsky.Oct 9 2019, 1:05 PM

@Dor1s - any chance you know more folks actively working on sancov who have the bandwidth to review?

Added Matt and Vitaly from "sanitizers" team, + Jonathan who has Windows expertise.

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

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.

Sure thing! Would I be creating a separate diff for review or would this be taken from the history? Also, I do not have commiter access so I will need someone to commit it for me.

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

Sure thing! Would I be creating a separate diff for review or would this be taken from the history? Also, I do not have commiter access so I will need someone to commit it for me.

yes, you need to upload new patch with "arc diff"
then you can chain them with "Edit Related Revisions..." on this site

dgg5503 updated this revision to Diff 224223.Oct 9 2019, 6:06 PM
dgg5503 edited the summary of this revision. (Show Details)

Split out JSON changes to D68752.

@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!

In other news, I've slightly modified the test symbolize.test to actually test the case I present in the initial description.

vitalybuka accepted this revision.Oct 10 2019, 6:16 PM
vitalybuka edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Oct 10 2019, 6:18 PM
vitalybuka edited the summary of this revision. (Show Details)Oct 10 2019, 6:20 PM

I don't consider myself a Windows expert but I don't see anything problematic from a Windows point of view.

This revision was automatically updated to reflect the committed changes.
This revision is now accepted and ready to land.Oct 11 2019, 10:22 PM

@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!

vitalybuka requested changes to this revision.Oct 16 2019, 5:15 PM

@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!

I suspect it never worked on these platform and we just enabled the test with x86-registered-target

This revision now requires changes to proceed.Oct 16 2019, 5:15 PM

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?

For reference, all of these tests passed on Windows when parsing the Linux sancov test file. So I guess what I'm really asking is, should we expect "cross-endian" support? If not, I wouldn't mind adding some .sancov binary files created on a big endian system just so these tests pass accordingly.

dgg5503 updated this revision to Diff 261526.May 1 2020, 1:33 PM

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.

vitalybuka accepted this revision.May 9 2020, 12:50 AM
vitalybuka added inline comments.
llvm/test/tools/sancov/blacklist.test
1

In cases like this we usually put spaces around &&

This revision is now accepted and ready to land.May 9 2020, 12:50 AM
dgg5503 updated this revision to Diff 263484.May 12 2020, 11:21 AM

Added spaces around "&&".

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

This revision was automatically updated to reflect the committed changes.