[PATCH] D40898: [Sanitizers] Basic sanitizer Solaris support (PR 33274)

[Rainer Orth via Phabricator via llvm-commits]

ro added a comment.

In https://reviews.llvm.org/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



================
Comment at: lib/sanitizer_common/sanitizer_common_interceptors.inc:4188
+      sz = sizeof(__sanitizer_shmid_ds);
+#if !SANITIZER_SOLARIS
+    else if (cmd == shmctl_shm_stat)
----------------
kcc wrote:
> Here, above and below, please avoid #ifdefs inside the code. 
> These are very bad. 
> When possible, move the functionality to solaris-specific files. 
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.


Repository:
  rCRT Compiler Runtime

https://reviews.llvm.org/D40898