Page MenuHomePhabricator

Fix sanitizer tool list used to generate sanitizer_common tests to be up-to-date.
ClosedPublic

Authored by delcypher on Dec 15 2018, 3:50 PM.

Details

Summary

This replaces the sanitizer tool list (used for generating
sanitizer_common configurations) with a tool list derived from
existing build system information.

Previously sanitizer_common had its own list of supported sanitizer
tools. This was bad because it was out of sync with the rest of the
build system. Notably it meant that the sanitizer_common runtime was
only being tested on Darwin the ASan dylib and not the other sanitizer
dylibs that are built for Darwin (LSan, TSan, and UBSan).

Unfortunately enabling the tests against other sanitizer dylibs has lead
to some test failures on Darwin. For now they've been marked as
unsupported until the failures can investigated properly.

Event Timeline

delcypher created this revision.Dec 15 2018, 3:50 PM

I propose to keep UNSUPPORTED for tests that are not applicable for Darwin or are randomly hanging/crashing.

I propose to keep UNSUPPORTED for tests that are not applicable for Darwin or are randomly hanging/crashing.

Are you saying you'd rather I use XFAIL?

I propose to keep UNSUPPORTED for tests that are not applicable for Darwin or are randomly hanging/crashing.

Are you saying you'd rather I use XFAIL?

Yes, unless a test is flaky and produces random success.

delcypher marked an inline comment as done.Dec 16 2018, 3:44 AM
delcypher added inline comments.
test/sanitizer_common/CMakeLists.txt
44

Should we guard against ANDROID and WINDOWS here? We weren't running on those platforms previously and it's likely that this change would cause the tests to be run for those platforms.

I propose to keep UNSUPPORTED for tests that are not applicable for Darwin or are randomly hanging/crashing.

Are you saying you'd rather I use XFAIL?

Yes, unless a test is flaky and produces random success.

Okay. I'll change it. The intention is that I'll fix these tests at a later date because some of the failures look like real bugs to me.

delcypher updated this revision to Diff 178389.Dec 16 2018, 3:59 AM

Switch from annotating failing tests on Darwin from UNSUPPORTED to XFAIL.

delcypher marked an inline comment as not done.Dec 16 2018, 3:59 AM
delcypher updated this revision to Diff 178390.Dec 16 2018, 5:02 AM

Fix up abort_on_error.cc test. It won't work for LSan.

delcypher updated this revision to Diff 178391.Dec 16 2018, 5:26 AM

Don't run on Android or Windows for now.

delcypher updated this revision to Diff 178395.Dec 16 2018, 7:15 AM

Don't run LSan-i386 tests on Darwin by default because LSan is broken in that case.

vitalybuka added inline comments.Dec 19 2018, 1:22 AM
test/sanitizer_common/CMakeLists.txt
14

we run sanitizer_common tests with asan on Android
I guess we have COMPILER_RT_HAS_UBSAN on android as well, but we don't sanitizer_common tests there

delcypher marked an inline comment as done.Dec 19 2018, 1:38 AM
delcypher added inline comments.
test/sanitizer_common/CMakeLists.txt
14

@vitalybuka Oops. Sorry I screwed up there. What would like to do here? If you do ship UBSan on Android I would suggest running the sanitizer_common tests against UBSan. However, I don't have any android devices so I can't easily fix any breakages that this introduces, so perhaps for Android I should use the existing behaviour of only running against ASan?

@waltl Does this have any impact for RTEMS ?
@phosek Does this have any impact on Fuchsia?

I don't know how testing is performed for those platforms.

vitalybuka added inline comments.Dec 19 2018, 2:36 AM
test/sanitizer_common/CMakeLists.txt
20

Could you please and submit patch add printing of SUPPORTED_TOOLS into existing code and wait some time
so we will have something on buildbots to compare after the current one is landed

delcypher marked an inline comment as done.Dec 19 2018, 3:35 AM
delcypher added inline comments.
test/sanitizer_common/CMakeLists.txt
20

@vitalybuka Just to clarify what you're asking for (I've got this wrong before)

  • Submit a separate patch that prints out what SUPPORTED_TOOLS is now (i.e. not what this patch tries to make SUPPORTED_TOOLS become).
  • Wait for some builds to happen
  • Rebase this patch and then look at the output of several bots to see in which scenarios we are running tests today and try to decide if we should change this patch to try and preserve the existing scenarios or expand on them.

Correct?

waltl added a comment.Dec 19 2018, 9:09 AM

@waltl Does this have any impact for RTEMS ?

We don't use CMake for RTEMS so this change doesn't affect it.

vitalybuka added inline comments.Dec 19 2018, 3:20 PM
test/sanitizer_common/CMakeLists.txt
20

yes.

delcypher marked 2 inline comments as done.
delcypher added inline comments.
test/sanitizer_common/CMakeLists.txt
20

@vitalybuka Sorry for the delay. Here's the CMake patch --> https://reviews.llvm.org/D55939

delcypher marked an inline comment as done and an inline comment as not done.Dec 20 2018, 9:43 AM
vitalybuka added inline comments.Jan 8 2019, 4:26 PM
test/sanitizer_common/CMakeLists.txt
18
  1. Remove: "if (NOT (ANDROID OR WINDOWS))"
  2. Create the list
  3. Print the list
  4. for windows and android:
    • replace the list and with hardcoded one
    • print the new list and with some FIXME warning about this replacement
test/sanitizer_common/TestCases/symbolize_stack.cc
6

why is this not XFAIL?

delcypher updated this revision to Diff 180837.Jan 9 2019, 6:51 AM
  • Put legacy list back but only use it for Windows and Android.
  • Print warning when we use the legacy list.
  • Print the generated SUPPORTED_TOOLS list.
delcypher marked 3 inline comments as done.Jan 9 2019, 6:54 AM
delcypher added inline comments.
test/sanitizer_common/CMakeLists.txt
18

@vitalybuka I've updated the patch with what (I think) you asked for.

test/sanitizer_common/TestCases/symbolize_stack.cc
6

I think the original reason was the for LSan on i386 it would hang (crashed while doing stop-the-world). However because I prevent those tests being run from ninja check-sanitizer in the CMakeLists.txt file, using XFAIL, is probably okay.

vitalybuka added inline comments.Jan 9 2019, 11:58 AM
test/sanitizer_common/CMakeLists.txt
35

For simplicity you can drop SUPPORTED_TOOLS_LEGACY and just use:
If Windows SUPPORTED_TOOLS=""
If Android SUPPORTED_TOOLS="asan"

test/sanitizer_common/TestCases/symbolize_stack.cc
6

Thanks.
Yes, question was just about XFAIL vs UNSOPPORTED. Former has some benefits if we expect to fix it (and it fails consistently).

delcypher updated this revision to Diff 181806.Jan 15 2019, 9:23 AM
delcypher marked an inline comment as done.
  • Remove SUPPORTED_TOOLS_LEGACY
delcypher marked an inline comment as done.Jan 15 2019, 9:24 AM

@vitalybuka Sorry the delay in getting to this. Is this good to go?

vitalybuka accepted this revision.Jan 16 2019, 1:46 PM

Thank you!
LGTM

This revision is now accepted and ready to land.Jan 16 2019, 1:46 PM
This revision was automatically updated to reflect the committed changes.