<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Apr 15, 2016 at 11:59 AM, Marcin Kościelnicki <span dir="ltr"><<a href="mailto:koriakin@0x04.net" target="_blank">koriakin@0x04.net</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">On 15/04/16 20:56, Kostya Serebryany wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Making such changes is not easy, but please try to find ways to minimize<br>
the number of ifdefs.<br>
The more stuff you move to separate SystemZ-specific file(s) -- the better.<br>
</blockquote>
<br></span>
Fair enough, I'll split off internal_mmap + internal_clone + the CVE 2016-2143 business to a separate file.  However, you'll now have a #ifndef __s390__ around the "normal" internal_mmap.<br>
<br>
What should be done about the existing arch-specific functions?  Should I break off all the internal_clone implementations to separate files?<br></blockquote><div><br></div><div>Well, it's hard to tell exactly what would be better. (they problem isn't trivial)</div><div>Try something to miniimze the number of #ifdefs and let us look at it. </div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">
<br>
On Fri, Apr 15, 2016 at 10:20 AM, Marcin Kościelnicki <<a href="mailto:koriakin@0x04.net" target="_blank">koriakin@0x04.net</a><br></span><div><div class="h5">
<mailto:<a href="mailto:koriakin@0x04.net" target="_blank">koriakin@0x04.net</a>>> wrote:<br>
<br>
    On 15/04/16 18:48, Kostya Serebryany wrote:<br>
<br>
        I'd prefer if you could move this and similar pieces of code into<br>
        separate SystemZ-specific file(s).<br>
        The sanitizer run-time is already a hairy ball of #ifdefs, let's<br>
        try not<br>
        to make it worse.<br>
<br>
<br>
    Very well, but what exactly are you suggesting instead?<br>
<br>
    s390 is just another architecture.  All the others already have lots<br>
    of ifdefs in the code, I'm merely following the current style:<br>
<br>
    projects/compiler-rt/lib $ grep -R __x86_64 *san* | wc -l<br>
    79<br>
    projects/compiler-rt/lib $ grep -R __powerpc *san* | wc -l<br>
    69<br>
    projects/compiler-rt/lib $ grep -R __aarch *san* | wc -l<br>
    70<br>
    projects/compiler-rt/lib $ grep -R __s390 *san* | wc -l<br>
    60<br>
    projects/compiler-rt/lib $ grep -R __i386 *san* | wc -l<br>
    34<br>
    projects/compiler-rt/lib $ grep -R __arm *san* | wc -l<br>
    29<br>
    projects/compiler-rt/lib $ grep -R __mips *san* | wc -l<br>
    80<br>
<br>
    This is with quite a few unsubmitted s390 patches on top of what's<br>
    already in review or svn.  s390 is certainly on the heavier side,<br>
    but not out of the ordinary.<br>
<br>
    If you want to have arch-specific files for arch-specific functions,<br>
    fine with me.  But that's not how things are currently done, and<br>
    you'll still have a maze of #ifdefs left over - there's no sane way<br>
    to expunge them from eg. D18888.  For things like D18895, you could<br>
    split it off, but only at the cost of duplicating code.  You'll also<br>
    still have #ifdefs for disabling the common versions of functions<br>
    overriden by arch-specific ones (eg. internal_mmap will need #ifndef<br>
    __s390__).<br>
<br>
<br>
</div></div></blockquote>
<br>
</blockquote></div><br></div></div>