Page MenuHomePhabricator

[dfsan] Add "DataFlow" option to LLVM_USE_SANITIZER
ClosedPublic

Authored by zbrid on Apr 17 2020, 11:09 AM.

Details

Reviewers
morehouse
EricWF
Group Reviewers
Restricted Project
Commits
rG0f12480bd13a: [dfsan] Add "DataFlow" option to LLVM_USE_SANITIZER
Summary

This patch add the dataflow option to LLVM_USE_SANITIZER and documents
it.

I built the libcxx and cxx targets with this option and then ran ninja check-cxx.
Edit: Some failing tests: (see bug: https://bugs.llvm.org/show_bug.cgi?id=45621)

Unexpected failures
libcxx/atomics/atomics.align/align.pass.sh.cpp
std/strings/string.conversions/stoull.pass.cpp
std/utilities/charconv/charconv.to.chars/integral.pass.cpp
std/utilities/tuple/tuple.tuple/tuple.apply/apply_large_arity.pass.cpp

Diff Detail

Event Timeline

zbrid created this revision.Apr 17 2020, 11:09 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptApr 17 2020, 11:09 AM
Herald added a reviewer: Restricted Project. · View Herald Transcript
zbrid edited the summary of this revision. (Show Details)

Oh I found some lit tests that fail by running check-cxx. I'll address that.

zbrid updated this revision to Diff 258383.Apr 17 2020, 11:39 AM

Update config.py to support data flow sanitizer

zbrid edited the summary of this revision. (Show Details)Apr 17 2020, 11:44 AM
zbrid edited the summary of this revision. (Show Details)
zbrid retitled this revision from [dfsan] Add dataflow option to LLVM_USE_SANITIZER to [dfsan] Add "DataFlow" option to LLVM_USE_SANITIZER.Apr 17 2020, 11:52 AM
morehouse added inline comments.Apr 17 2020, 12:17 PM
clang/docs/DataFlowSanitizer.rst
24

Let's add some explanation here about why we want to build libc++ with dfsan.

26

This sentence doesn't seem too helpful. I think most people reading this doc will know how to build LLVM/Clang.

I suggest either removing this sentence or expanding it to explain that we want to build LLVM/Clang separately from libc++ since we only want dfsan instrumentation on libc++.

libcxx/utils/libcxx/test/config.py
913

I'm not 100% sure what the sanitizer-new-delete feature is used for, but I don't think we want it for dfsan since it doesn't use a custom allocator.

zbrid updated this revision to Diff 258447.Apr 17 2020, 4:52 PM

Update documentation based on Matt's comments.

zbrid marked 3 inline comments as done.Apr 17 2020, 4:56 PM
broadwaylamb added inline comments.Apr 17 2020, 5:04 PM
libcxx/utils/libcxx/test/config.py
912

I'm not sure we need a new feature if none of the tests actually use the feature.

morehouse accepted this revision.Apr 17 2020, 5:16 PM
morehouse added inline comments.
clang/docs/DataFlowSanitizer.rst
23

Nit: "How to build libc++"

zbrid marked an inline comment as done.Apr 17 2020, 5:58 PM
zbrid added inline comments.
libcxx/utils/libcxx/test/config.py
912

What is are these features used for? Will not having this mean some tests fail as a result of dfsan being enabled?

zbrid updated this revision to Diff 258462.Apr 17 2020, 5:59 PM

Update nit from Matt

zbrid marked an inline comment as not done.Apr 18 2020, 2:20 PM
zbrid updated this revision to Diff 258608.Apr 19 2020, 9:16 AM
  • Remove dfsan feature based on review comment
EricWF accepted this revision.Apr 20 2020, 10:10 AM
This revision is now accepted and ready to land.Apr 20 2020, 10:10 AM
zbrid edited the summary of this revision. (Show Details)Apr 20 2020, 10:16 AM

Gonna land this and file a bug for the failing tests. The tests shouldn't block anyone upstream since I'm only adding a build mode. I'll do some more digging into those failures in the future.

zbrid edited the summary of this revision. (Show Details)Apr 20 2020, 10:32 AM
This revision was automatically updated to reflect the committed changes.