[compiler-rt] r266295 - [sanitizer] [SystemZ] Implement internal_mmap.

[Kostya Serebryany via llvm-commits]

Making such changes is not easy, but please try to find ways to minimize
the number of ifdefs.
The more stuff you move to separate SystemZ-specific file(s) -- the better.

On Fri, Apr 15, 2016 at 10:20 AM, Marcin Kościelnicki <koriakin at 0x04.net>
wrote:

> On 15/04/16 18:48, Kostya Serebryany wrote:
>
>> I'd prefer if you could move this and similar pieces of code into
>> separate SystemZ-specific file(s).
>> The sanitizer run-time is already a hairy ball of #ifdefs, let's try not
>> to make it worse.
>>
>>
> Very well, but what exactly are you suggesting instead?
>
> s390 is just another architecture.  All the others already have lots of
> ifdefs in the code, I'm merely following the current style:
>
> projects/compiler-rt/lib $ grep -R __x86_64 *san* | wc -l
> 79
> projects/compiler-rt/lib $ grep -R __powerpc *san* | wc -l
> 69
> projects/compiler-rt/lib $ grep -R __aarch *san* | wc -l
> 70
> projects/compiler-rt/lib $ grep -R __s390 *san* | wc -l
> 60
> projects/compiler-rt/lib $ grep -R __i386 *san* | wc -l
> 34
> projects/compiler-rt/lib $ grep -R __arm *san* | wc -l
> 29
> projects/compiler-rt/lib $ grep -R __mips *san* | wc -l
> 80
>
> This is with quite a few unsubmitted s390 patches on top of what's already
> in review or svn.  s390 is certainly on the heavier side, but not out of
> the ordinary.
>
> If you want to have arch-specific files for arch-specific functions, fine
> with me.  But that's not how things are currently done, and you'll still
> have a maze of #ifdefs left over - there's no sane way to expunge them from
> eg. D18888.  For things like D18895, you could split it off, but only at
> the cost of duplicating code.  You'll also still have #ifdefs for disabling
> the common versions of functions overriden by arch-specific ones (eg.
> internal_mmap will need #ifndef __s390__).
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20160415/62daced4/attachment.html>