Page MenuHomePhabricator

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

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.Tue, Oct 8, 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
287–288

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

dgg5503 updated this revision to Diff 223973.EditedTue, Oct 8, 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.Wed, Oct 9, 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.Wed, Oct 9, 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.Thu, Oct 10, 6:16 PM
vitalybuka edited the summary of this revision. (Show Details)
This revision is now accepted and ready to land.Thu, Oct 10, 6:18 PM
vitalybuka edited the summary of this revision. (Show Details)Thu, Oct 10, 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.Fri, Oct 11, 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.Wed, Oct 16, 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.Wed, Oct 16, 5:15 PM