Page MenuHomePhabricator

Add VFS support for sanitizers' blacklist

Authored by jkorous on Sep 18 2019, 5:29 PM.

Diff Detail


Event Timeline

jkorous created this revision.Sep 18 2019, 5:29 PM

Hi Jan, what's the is motivation for the sanitizer blacklists to support a VFS overlay in the driver?

jkorous updated this revision to Diff 221112.Sep 20 2019, 2:00 PM

We discussed offline and agreed that we don't need to use overlay in driver at all. I simplified the patch.

jkorous updated this revision to Diff 221126.Sep 20 2019, 3:08 PM

Added happy-path test.

I completely forgot about error handling which would be tricky in current place where I devirtualize files. We discussed offline and seems like it will be better to use VFS in SanitizerBlacklist anyway.

I took a look at SanitizerSpecialCaseList and I'm now thinking it might not be necessary to plumb VFS there. It's derived from llvm::SpecialCaseList whose sole interaction with filesystem is that it has couple named constructors that read a file and init an instance with its content.
It feels like dealing with paths and files is actually out of scope for SpecialCaseList and the natural interface is probably just a buffer.

jkorous updated this revision to Diff 221131.Sep 20 2019, 4:04 PM

The test was incorrect.

jkorous planned changes to this revision.Sep 20 2019, 4:06 PM
jkorous changed the visibility from "Public (No Login Required)" to "No One".Sep 20 2019, 4:19 PM
jkorous updated this revision to Diff 221397.Sep 23 2019, 1:16 PM
jkorous edited the summary of this revision. (Show Details)
jkorous changed the visibility from "No One" to "Public (No Login Required)".
delcypher added inline comments.Oct 1 2019, 11:09 AM
7 ↗(On Diff #221397)

For template substitutions I prefer to have placeholders delimited by @ symbols e.g. @REAL_FILE@. This helps avoid situations where text like REAL_FILE might appear in the template where we don't want them substituted. Having a @ symbol at the end avoids issue where a placeholder is a prefix of another (e.g. REAL_FILE is a prefix of REAL_FILE_ABSOLUTE).

It doesn't matter for this patch but I think it's a good convention to follow.

10 ↗(On Diff #221397)

Are we allowed to use sed here? For Windows builds can we assume it's available?

jkorous marked 2 inline comments as done.Oct 7 2019, 2:14 PM
jkorous added inline comments.
10 ↗(On Diff #221397)

Good question! Apparently I should just use %T etc. and echo to %t-vfsoverlay.yaml. However, since we use sed in other clang-scan-deps tests too - would you be fine with me addressing this in all tests at once as a separate patch?

jkorous updated this revision to Diff 223655.Oct 7 2019, 2:16 PM

Addressed comments.

jkorous retitled this revision from [WIP] Add VFS support for sanitizers' blacklist to Add VFS support for sanitizers' blacklist.Oct 7 2019, 3:21 PM
delcypher added inline comments.Oct 7 2019, 3:22 PM
10 ↗(On Diff #221397)

Sure separate patch sounds good.

JDevlieghere accepted this revision.Oct 7 2019, 3:24 PM


833 ↗(On Diff #223655)

You could reserve the number of elements in Paths.

This revision is now accepted and ready to land.Oct 7 2019, 3:24 PM
delcypher accepted this revision.Oct 7 2019, 3:30 PM
rnk added a subscriber: rnk.Oct 7 2019, 4:40 PM

Broke Windows:

Looks related to this:

The fix is probably to follow the pattern I started using in rC371663, use %/T and %/S instead of %T and %S.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptOct 7 2019, 10:22 PM