Page MenuHomePhabricator

[Sanitizers] Basic sanitizer Solaris support (PR 33274)
ClosedPublic

Authored by ro on Dec 6 2017, 7:50 AM.

Details

Summary

This is the first mostly working version of the Sanitizer port to 32-bit Solaris/x86.
It is currently based on Solaris 11.4 Beta.

This part was initially developed inside libsanitizer in the GCC tree and should apply to
both. Subsequent parts will address changes to clang, the compiler-rt build system
and testsuite.

I'm not yet sure what the right patch granularity is: if it's profitable to split the patch
up, I'd like to get guidance on how to do so.

Most of the changes are probably straightforward with a few exceptions:

  • The Solaris syscall interface isn't stable, undocumented and can change within an OS release. The stable interface is the libc interface, which I'm using here, if possible using the internal _-prefixed names.
  • While the patch primarily target 32-bit x86, I've left a few sparc changes in. They cannot currently be used with clang due to a backend limitation, but have worked fine inside the gcc tree.
  • Some functions (e.g. largefile versions of functions like open64) only exist in 32-bit Solaris, so I've introduced a separate SANITIZER_SOLARIS32 to check for that.

The patch (with the subsequent ones to be submitted shortly) was tested
on i386-pc-solaris2.11. Only a few failures remain, some of them analyzed, some
still TBD:

 AddressSanitizer-i386-sunos :: TestCases/Posix/concurrent_overflow.cc
 AddressSanitizer-i386-sunos :: TestCases/init-order-atexit.cc
 AddressSanitizer-i386-sunos :: TestCases/log-path_test.cc
 AddressSanitizer-i386-sunos :: TestCases/malloc-no-intercept.c
 AddressSanitizer-i386-sunos-dynamic :: TestCases/Posix/concurrent_overflow.cc
 AddressSanitizer-i386-sunos-dynamic :: TestCases/Posix/start-deactivated.cc
 AddressSanitizer-i386-sunos-dynamic :: TestCases/default_options.cc
 AddressSanitizer-i386-sunos-dynamic :: TestCases/init-order-atexit.cc
 AddressSanitizer-i386-sunos-dynamic :: TestCases/log-path_test.cc
 AddressSanitizer-i386-sunos-dynamic :: TestCases/malloc-no-intercept.c

SanitizerCommon-Unit :: ./Sanitizer-i386-Test/MemoryMappingLayout.DumpListOfModules
 SanitizerCommon-Unit :: ./Sanitizer-i386-Test/SanitizerCommon.PthreadDestructorIterations

Maybe this is good enough the get the ball rolling.

Diff Detail

Event Timeline

ro created this revision.Dec 6 2017, 7:50 AM
krytarowski added inline comments.Dec 6 2017, 8:19 AM
lib/sanitizer_common/sanitizer_internal_defs.h
94

Is this supported in SmartOS?

lib/sanitizer_common/sanitizer_linux.cc
484–485

I would make this list of OSes generic (we could include there NetBSD as well).

lib/sanitizer_common/sanitizer_platform_limits_posix.cc
17 ↗(On Diff #125728)

When I was adding NetBSD support I was prompted to move all the NetBSD definitions to a dedicated file: sanitizer_platform_limits_netbsd.cc. I think the same should be repeated for Solaris.

lib/sanitizer_common/sanitizer_posix_libcdep.cc
66

Can we cast all systems to (char*)?

kcc edited edge metadata.

Too many #ifdefs in the code -- we can not let this in.
Please find a way to reduce (to ~ zero) the number of #ifdefs inside the code.
Prefer to have solaris-specific functionality in separate files.

lib/sanitizer_common/sanitizer_common_interceptors.inc
4202

Here, above and below, please avoid #ifdefs inside the code.
These are very bad.
When possible, move the functionality to solaris-specific files.

ro added inline comments.Dec 7 2017, 2:12 AM
lib/sanitizer_common/sanitizer_internal_defs.h
94

I cannot tell: .preinit_array support was already in OpenSolaris
and is thus in the Illumos derivatives. However, when I tried
to switch gcc to .init_array and friends, there were issues that
had to be resolved first (don't remember the details).

If this would affect the current .preinit_array use at all I don't know.

This being a volunteer effort, I have neither the time nor the
hardware to do such testing myself (and on OmniOS and
OpenIndiana), so this will require help from those communities.

lib/sanitizer_common/sanitizer_linux.cc
484–485

Fine with me: it would be way more readable (and maintainable)
to use generic (say) USE_ENVIRON macros than maintaining
long SANITIZER_THIS || SANITIZER_THAT lists all over the
place ;-)

lib/sanitizer_common/sanitizer_platform_limits_posix.cc
17 ↗(On Diff #125728)

I see. When I started this port, that file (and convention)
didn't exist, I belive.

At first glance this seems still quite repetitive, but it's certainly
way clearer than the current #if/#ifdef maze. I'll have a look,
thanks.

lib/sanitizer_common/sanitizer_posix_libcdep.cc
66

Only testing will tell for certain, but I suspect so.

ro added a comment.Dec 7 2017, 2:32 AM
In D40898#946858, @kcc wrote:

Too many #ifdefs in the code -- we can not let this in.
Please find a way to reduce (to ~ zero) the number of #ifdefs inside the code.
Prefer to have solaris-specific functionality in separate files.

Could you be a bit more specific, please? Which uses are ok and which are not?

  • Most cases of augmenting #if SANITIZER_A || SANITIZER_B || SANITIZER_C by || SANITIZER_SOLARIS should be fine, I hope?
  • So should be inclusions of necessary Solaris-specific headers inside #if PLATFORM_SOLARIS when the same is done for other targets?
  • I'm totally unclear about situations where common code unconditionally uses say non-standard fields in standardized structures. Should I really duplicate the complete affected interceptors to avoid platform #ifdefs? This way, later ports are paying for unportable code in earlier ones.
  • What about cases of unportable ioctls? Is it ok to wrap those or not?

It would really help if you could point out a couple of problematic/unacceptable uses of #ifdef and explain why, so I can understand
where you're coming from.

However, I found the whole experience of porting the sanitizers a rather unpleasant one, in particular because it's mostly based on
platform ifdefs rather than feature ones. This way, you have to go over the whole code and see which SANITIZER_A || SANITIZER_B || SANITIZER_C
case applies to your port and which doesn't. In the feature-based approach (which is considered state-of-the-art for 20+ years, since
the advent of autoconf), you just check for the features (functions, fields in structures, ...) in one place and use the result in your code.
If this were done in the sanitizers, porters could concentrate on the few places that really need porting rather than spending lots of
time augmenting conditionals that would fall into place naturally otherwise.

Please enlighten me here.

Thanks.

Rainer
lib/sanitizer_common/sanitizer_common_interceptors.inc
4202

Do you really suggest to duplicate the whole shmctl interceptor
in a solaris-specific file to avoid this single #ifdef? Seems like
a maintenance nightmare of its own to me.

I see now that sanitizer_platform_limits_netbsd.h introduced
dummy definitions to deal with those non-standard values.
Seems a bit more maintainable to me.

ro updated this revision to Diff 125944.Dec 7 2017, 6:33 AM

I've now revised the patch, introducing sanitizer_platform_limits_solaris.{cc,h} along the
lines of the corresponding NetBSD files. While this certainly simplifies the code avoiding
the #ifdef maze in sanitizer_platform_limits_posix.{cc,h}, it would be good to handle the
Linux and FreeBSD contents in those files along the same lines to avoid confusing future
contributors. Further, it would be good to keep those parts that are common to all posix
platforms in the s_p_l_posix.* files instead of duplicating those into their platform-specific
counterparts.

ro marked 6 inline comments as done.Dec 7 2017, 6:36 AM

I believe I've now addressed the concrete review comments. Certainly need more guidance on additional #ifdef avoidance.

ro added a comment.Dec 7 2017, 6:37 AM

environ generalization done now.

alekseyshl edited edge metadata.Dec 11 2017, 6:11 PM

Mostly looks good, just a few minor changes.

lib/sanitizer_common/sanitizer_common_interceptors.inc
2192–2198

Can you rearrange the code this way:

#if SANITIZER_SOLARIS
Stripped down interceptor definition
#else  // SANITIZER_SOLARIS
Current glob definitions and interceptor code here, unchanged
#endif  // SANITIZER_SOLARIS
2551

#endif // SANITIZER_INTERCEPT_GETHOSTBYNAME

2556–2557

#endif // SANITIZER_INTERCEPT_GETHOSTBYNAME2

lib/sanitizer_common/sanitizer_linux.cc
1070

#endif // SANITIZER_SOLARIS

1810

Please indent this block of defines by one more space

lib/sanitizer_common/sanitizer_linux_libcdep.cc
136

#endif // SANITIZER_SOLARIS

659

So, it does not allocate memory on Solaris?

ro updated this revision to Diff 126542.Dec 12 2017, 6:23 AM

This addresses all review comments so far, I believe.

ro marked 7 inline comments as done.Dec 12 2017, 6:25 AM
ro added inline comments.
lib/sanitizer_common/sanitizer_linux_libcdep.cc
659

I've checked the OpenSolaris sources: it just calls an
internal/undocumented _sysconfig syscall and passes on
the result.

ro marked an inline comment as done.Dec 12 2017, 6:26 AM

I propose to rebase it to HEAD as this patch might not apply cleanly.

lib/sanitizer_common/sanitizer_internal_defs.h
144

Are there still !LP64 Solaris systems?

lib/sanitizer_common/sanitizer_mutex.h
99

I would remove ifdef here.

alekseyshl accepted this revision.Dec 12 2017, 11:45 AM
alekseyshl added inline comments.
lib/sanitizer_common/sanitizer_mutex.h
99

Yep, let's remove the #ifdef, but keep the comment.

This revision is now accepted and ready to land.Dec 12 2017, 11:45 AM
ro marked an inline comment as done.Dec 13 2017, 2:11 AM
ro added inline comments.
lib/sanitizer_common/sanitizer_internal_defs.h
144

Sure: while there are no 32-bit Solaris kernels beyond Solaris 10
(maybe in the OpenSolaris/Illumos space; I don't know), every
Solaris system can execute both 32 and 64-bit programs.
In fact, this first version of the port is 32-bit x86 only at the
moment, though this will change.

ro marked 2 inline comments as done.Dec 14 2017, 8:02 AM
ro updated this revision to Diff 126966.Dec 14 2017, 8:09 AM

Rebased two times in the last two days:

  • Account for NanoTime changes.
  • Use MADV_FREE on Solaris, too.

Tested on i386-pc-solaris2.11 and x86_64-pc-linux-gnu.

On the latter, I found a testsuite regression caused by merge failure

MemorySanitizer-Unit :: ./Msan-x86_64-Test/MemorySanitizer.gethostbyname2

where I had still used SI_NOT_WINDOWS instead of SI_POSIX.

Could someone please commit this for me then? Thanks.

This revision was automatically updated to reflect the committed changes.