This is an archive of the discontinued LLVM Phabricator instance.

[Sanitizers, test] Fix sanitizer tests on Solaris (PR 33274)
ClosedPublic

Authored by ro on Dec 6 2017, 8:14 AM.

Details

Summary

This patch (on top of the previous two (https://reviews.llvm.org/D40898 and
https://reviews.llvm.org/D40899) complete the compiler-rt side of the the Solaris
sanitizer port.

It contains the following sets of changes:

  • For the time being, the port is for 32-bit x86 only, so reject the various tests on x86_64.
  • When compiling as C++, <setjmp.h> resp. <iso/setjmp_iso.h> only declares _setjmp and _longjmp inside namespace std.
  • MAP_FILE is a Windows feature. While e.g. Linux <sys/mman.h> provides a no-op compat define, Solaris does not.
  • test/asan/TestCases/Posix/coverage.cc was initially failing like this:

/vol/gcc/src/llvm/llvm/local/projects/compiler-rt/lib/sanitizer_common/scripts/sancov.py: 4 files merged; 2 PCs total
rm: cannot remove '/var/gcc/llvm/local/projects/compiler-rt/test/asan/I386SunOSConfig/TestCases/Posix/Output/coverage': Invalid argument

Further digging revealed that the rm was trying to remove the running test's working
directory which failed as observed.  cd'ing out of the dir before let the test pass.
  • Two tests needed a declaration of alloca. I've now copied the existing code from test/asan/TestCases/alloca_constant_size.cc, but it may be more profitable and maintainable to have a common testsuite header where such code is collected.
  • Similarly, Solaris' printf %p format doesn't include the leading 0x.
  • In test/asan/TestCases/malloc-no-intercept.c, I had to undef EXTENSIONS (predefined by clang for no apparent reason) to avoid conflicting declarations for memalign.
  • test/ubsan/TestCases/Float/cast-overflow.cpp has different platform dependent ways to define BYTE_ORDER and friends. Why not just use BYTE_ORDER and friends as predefined by clang and gcc?

Diff Detail

Event Timeline

ro created this revision.Dec 6 2017, 8:14 AM
krytarowski added inline comments.Dec 6 2017, 8:24 AM
test/asan/TestCases/Posix/ioctl.cc
13

Do we need to check for __svr4__? Are there other alive SunOS systems?

test/asan/TestCases/alloca_loop_unpoisoning.cc
17

<stdlib.h> is included above.

test/asan/TestCases/alloca_vla_interact.cc
17

<stdlib.h> is already included

test/asan/TestCases/debug_double_free.cc
20

Is there an option to use %#p?

kcc edited reviewers, added: alekseyshl; removed: samsonov.Dec 6 2017, 9:59 AM
ro added inline comments.Dec 7 2017, 1:44 AM
test/asan/TestCases/Posix/ioctl.cc
13

It's still the customary way to check for Solaris and derivatives,
though you'll be hard pressed to find a SunOS 4 system around.

test/asan/TestCases/alloca_loop_unpoisoning.cc
17

Missed that, thanks. We can either change the section to

#elif defined(FreeBSD) || defined(NetBSD)
-#include <stdlib.h>
+// alloca is declared in <stdlib.h>.
#else
#include <alloca.h>

since the BSDs lack <alloca.h> or add a specific guard
for alloca.h inclusion.

My general point about moving such code to a common
test header to avoid duplication in several places still stands.

test/asan/TestCases/debug_double_free.cc
20

No: although printf(3C) accepts it, it only adds 0x/0X for the
%x and %X formats, not %p.

ro updated this revision to Diff 127506.Dec 19 2017, 5:32 AM

Incorporate review comments.

alekseyshl accepted this revision.Dec 22 2017, 10:40 AM
alekseyshl added inline comments.
test/asan/TestCases/alloca_loop_unpoisoning.cc
17

The common test headers make sense, but as a separate refactoring patch.

This revision is now accepted and ready to land.Dec 22 2017, 10:40 AM
ro added a comment.Jan 13 2018, 6:36 AM

Thanks. Could you please commit this for me?

test/asan/TestCases/alloca_loop_unpoisoning.cc
17

Certainly. I'll have a look once I have a chance.

krytarowski added inline comments.Jan 13 2018, 9:26 AM
test/asan/TestCases/alloca_vla_interact.cc
17

I think we should reverse this check.

Include <alloca.h> for SunOS only.

<stdlib.h> is enough for all BSDs, Linux, Windows, MacOSX.

ro added inline comments.Jan 13 2018, 9:29 AM
test/asan/TestCases/alloca_vla_interact.cc
17

Will do.

ro updated this revision to Diff 129771.Jan 13 2018, 1:47 PM

Only include <alloca.h> on Solaris.

ro marked 3 inline comments as done.Jan 13 2018, 1:47 PM

Once accepted I can land this for you.

alekseyshl accepted this revision.Jan 16 2018, 10:42 AM
This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.

@ro For your interest, just in case that it's in the scope of your interest:
there is a work towards merging your work with Illumos and ship in pkgsrc.

http://mail-index.netbsd.org/tech-pkg/2018/01/16/msg019322.html

There are also patches generated locally for SmartOS... it would be great to merge them
instead of keeping in downstream branches.

test/asan/TestCases/alloca_constant_size.cc
14–15

I've added #endif here as a hotfix.

ro added a comment.Jan 18 2018, 6:41 AM

@ro For your interest, just in case that it's in the scope of your interest:
there is a work towards merging your work with Illumos and ship in pkgsrc.

http://mail-index.netbsd.org/tech-pkg/2018/01/16/msg019322.html

That's great news: I saw something like this flying by on Twitter. There will certainly be some
areas where Solaris and Illumos differ, but the majority will be the same.

There are also patches generated locally for SmartOS... it would be great to merge them
instead of keeping in downstream branches.

Absolutely: keeping large patch sets local for a long time creates an incredible amount of
work for everyone. I was totally horrified when I saw the old Solaris userland llvm 3.9 patches:
nobody would have been able to get those upstream.

In my case, getting the sanitizer port upstream was the only way to get it into gcc at all :-)

test/asan/TestCases/alloca_constant_size.cc
14–15

Thanks so much for doing this: this one time I thought I could
things right without a quick check-all. Certainly the last time
I tried that!

I propose to get in touch with Jonathan (jperkin) from SmartOS, in order to merge the patches. He doesn't seem to like to upstream patches, so he won't push them here (and he declared so not bother about upstreaming).

I agree that some patches might be local to a building system, but these differences could be managed with generic switches on the cmake/make command line level. Especially in the context that for some reason patching is not needed for stable releases of the toolchain for NetBSD at all.