Page MenuHomePhabricator

Add VFS support for sanitizers' blacklist' 2
ClosedPublic

Authored by jkorous on Oct 30 2019, 3:47 PM.

Details

Summary

This is another approach to using VFS for blacklists.

The previous one got reverted as it didn't work with in-memory file systems.
https://reviews.llvm.org/D67742

Adding original reviewers + people who got broken by the previous version.

Diff Detail

Event Timeline

jkorous created this revision.Oct 30 2019, 3:47 PM
jkorous marked an inline comment as done.Oct 30 2019, 3:49 PM
jkorous added inline comments.
llvm/lib/Support/SpecialCaseList.cpp
120

Technically, this isn't really necessary and could be split in a separate patch - I just kept it here so the motivation is obvious.

ilya-biryukov added inline comments.Oct 31 2019, 3:00 AM
clang/include/clang/Basic/SanitizerSpecialCaseList.h
18

NIT: #include "llvm/Support/VirtualFileSystem.h" should be enough

clang/lib/Basic/SanitizerSpecialCaseList.cpp
26

Why not pass VFS to the SpecialCaseList::createInternal instead?
We're duplicating the code of createInternal here.

27

NIT: maybe use auto. The type is quite long, spelling it out seems to hurt readability

29

NIT: it's probably more idiomatic to use a boolean conversion here:

if (!MaybeBuffer) {
  Error = (Twine("can't open file '") + Path + "': " + MaybeBuffer.getError().message()).str();
  return;
}

But up to you, obviously

jkorous marked 4 inline comments as done.Oct 31 2019, 12:09 PM
jkorous added inline comments.
clang/lib/Basic/SanitizerSpecialCaseList.cpp
26

I intended to keep the scope of this patch small but open to discussion. Seems like the only other place where SpecialCaseList is used is XRayFunctionFilter. The options I see here are:

  1. Get rid of anything filesystem-related in SpecialCaseList and just require buffers in the interface.
  2. Require VFS in SpecialCaseList interface and pass it from XRayFunctionFilter (seems like VFS would be used just in the constructor and not in the actual methods?).
  3. Add VFS to only some method/s in SpecialCaseList. I'd consider this a regression in interface "quality".

I'd prefer #1 and unless someone can confirm that the XRay change is trivial I'd prefer to not do #2.

27

I feel that spelling the type out here makes it a bit easier to understand what's going on later (MaybeBuffer.get().get()).

29

Thanks! I copied this from SpecialCaseList. Let's discuss eventual changes to SpecialCaseList interface first, I'd change this after.

ilya-biryukov added inline comments.Oct 31 2019, 1:16 PM
clang/lib/Basic/SanitizerSpecialCaseList.cpp
26

I would argue the minimal change is actually #2.

createInternal(std::vector<string> Paths)

gets turned into

createInternal(std::vector<string> Paths, vfs::FileSystem *FS = getRealFileSystem());

The implementation is updated accordingly, callsites in XRay don't need an update.

I see what you mean - I didn't realize I could use the real fs as the default arg.
I still feel that handling files is out of scope for SpecialCaseList but I don't think those couple lines are a big deal either way - updating the patch now.

jkorous updated this revision to Diff 227378.Oct 31 2019, 4:59 PM
ilya-biryukov added inline comments.Nov 5 2019, 3:35 AM
llvm/lib/Support/SpecialCaseList.cpp
102

Should this call into VFS?

Do the tests pass with the current revision?
Since we're not actually calling into the VFS, is there a chance we're not actually properly testing this?

jkorous marked an inline comment as done.Nov 6 2019, 11:08 AM
jkorous added inline comments.
llvm/lib/Support/SpecialCaseList.cpp
102

Good catch! Thanks.

jkorous updated this revision to Diff 228107.Nov 6 2019, 11:09 AM

Actually use the VFS that got passed.

The test seems to be fine (fails on master, passes with the patch). I most probably just forgot to run it.

This revision is now accepted and ready to land.Nov 7 2019, 1:12 AM

Just wanted to give early warning that this might still break our integrate and buildcops might be keen to revert it. The change itself LG, but our infrastructure might need time to pick it up and get broken for some time.
We're discussing how to properly land this internally, so hopefully this shouldn't happen, but still wanted to make sure that does not come as a surprise.

I am in no particular rush and happy to help you as much as I can but at some point not too far in the future I just want this to land and not get reverted.

Is there anything I can do to help you with this? Can't you guys just cherry-pick the patch internally, do whatever needs to be done on your side and ping me when it's safe to land?

It seems we should be fine, now that the file accesses actually go through the VFS.
Feel free to submit the patch and sorry for the disturbance!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptNov 8 2019, 11:03 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript

It turned out there are more places that attempt to construct SpecialCaseList and access corresponding file via the usual filesystem APIs.
D70440 should take care of those.

Ahh, right. I originally wanted to support -ivfsoverlay in Driver as it seemed reasonable to check the existence of blacklist in Driver and open it in cc1 using the same fs. I got talked out of it and I didn't touch the Driver but seems like I should've replaced the calls to native fs with VFS to keep things consistent. Thanks for picking this up!