<div dir="ltr"><br><div class="gmail_extra"><br><br><div class="gmail_quote">On Wed, Apr 23, 2014 at 3:11 PM, Bernhard Reutner-Fischer <span dir="ltr"><<a href="mailto:rep.dot.nop@gmail.com" target="_blank">rep.dot.nop@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex"><div class="">On 23 April 2014 12:45, Kostya Serebryany <<a href="mailto:kcc@google.com">kcc@google.com</a>> wrote:<br>
><br>
> On Wed, Apr 23, 2014 at 2:38 PM, Bernhard Reutner-Fischer<br>
> <<a href="mailto:rep.dot.nop@gmail.com">rep.dot.nop@gmail.com</a>> wrote:<br>
>><br>
>> On 23 April 2014 10:57, Kostya Serebryany <<a href="mailto:kcc@google.com">kcc@google.com</a>> wrote:<br>
<br>
</div><div class="">>> These things (e.g. glob_t members) do not change between glibc-releases,<br>
>> so<br>
>> i fail to see how this would be costly to maintain? It's pretty much a<br>
>> one-time<br>
>> thing to do IMO.<br>
>><br>
>> > So, before we continue with review I would like to hear the motivation<br>
>> > and<br>
>><br>
>> The motivation behind this patch is that there are libc's apart from<br>
>> glibc, and<br>
>> these libc implementations, being POSIX compliant, may lack certain GNU<br>
>> extensions (as implemented in glibc). As you can see in your own code, you<br>
>> have gazillions of conditionals (preprocessor conditionals, which i add<br>
>> just add<br>
>> a couple of more, generic ones, btw, to answer your comment from above) to<br>
>> handle these other libc's.<br>
><br>
><br>
> Ok. What particular libc implementation are you trying to use?<br>
<br>
</div>I'm targeting uClibc.<br></blockquote><div><br></div><div>Are you saying that asan works with uClibc with your patches? Good to know!</div><div>Or it just builds? </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><br>
> And what particular tool are you trying to use (asan, tsan, msan)?<br>
<br>
</div>When bootstrapping GCC the build fails IIRC in asan.<br></blockquote><div><br></div><div>Sure. What I am saying is that you can disable most of the stuff in e.g. sanitizer_common/sanitizer_platform_limits_posix.cc and things will just work for asan.</div>
<div>This may or may not be simpler though. </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><br>
> Most of the things you hide under ifdefs are important only/mostly for msan,<br>
> which does not exist in the GCC variant anyway.<br>
><br>
>><br>
>><br>
>> The route this patch takes it to probe these non-standard things and<br>
>> act accordingly.<br>
>><br>
>> > whether you are welling to help us maintain this code in future (= set<br>
>> > up a public build bot)<br>
>><br>
>> I'm certainly willing to help maintain this code in the future since i<br>
>> want to use<br>
>> your nice sanitizers via gcc, sure. I cannot, however, setup a public<br>
>> build-bot since<br>
>> my notebook is not prepared to handle this task on a regular basis.<br>
>> If you break stuff upstream and i find the backported gcc version to<br>
>> be dysfunctional, i<br>
>> will send you fixes if time permits.<br>
><br>
><br>
> This round trip has very long latency and brings heavy burden on us (code<br>
> maintainers).<br>
<br>
</div>I do not really understand where you see this heavy burden or a burden<br>
at all TBH.<br>
If you break my bootstrap with my libc then i'll fix it up, it will<br>
not affect you folks at<br>
all.<br></blockquote><div><br></div><div>We will have to review all these changes, retest them on all platforms we care about and commit. </div><div>Then repeat the exercise for the GCC merge. It takes time.</div><div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><br>
> We are very reluctant to pay this price.<br>
> Having a public bot for llvm upstream code will reduce this cost.<br>
<br>
</div>If you can provide a box where i can setup cross-compilers to act as<br>
build-bots then that'd be doable, yes.<br>
<br>
I personally do not have such a computer, unfortunately.<br></blockquote><div><br></div><div>Well, this is you who want these patches in, not us. Right? </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class="">><br>
>><br>
>><br>
>> I think the patch itself is pretty straight forward,<br>
><br>
><br>
> It is straight forward, but it is also huge.<br>
<br>
</div>I can break it up into small, logical pieces if you would accept those<br>
more easily.<br></blockquote><div><br></div><div>This will help A LOT. </div><div>First, try to minimize the number of ifdefs. It is much better to move some code into another file and hide the entire file under a single ifdef</div>
<div><br></div><div>Second, the build should not require any additional -DHAVE_BLAH -- all this logic should be done inside the sources (e.g. sanitizer_common/sanitizer_platform.h)</div><div>We should not do any probes at build time</div>
<div><br></div><div>Then, please follow the code style around (e.g. no /**/ comments)</div><div><br></div>Is nanosleep always available? Is so, just replace usleep with nanosleep w/o any more ifdefs. <br><div><br></div><div>
</div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<div class=""><br>
> Depending on your actual needs (see above) we might be able to do something<br>
> *much* simpler.<br>
<br>
</div>The patch probes for very small, distinct aspects of GNU-extensions of POSIX<br>
functions and disables them on an individual basis, reducing long-term<br>
maintenance<br>
cost by adopting to different target-platforms seamlessly in<br>
established manner (by<br>
using autoconf resp. existing cmake Modules that are maintained in<br>
cmake/autoconf<br>
upstream projects) instead of hard-coding meta-knowledge -- like<br>
"INTERCEPT_FUNCTION_LINUX_OR_FREEBSD" or "(SI_MAC && !SI_IOS) ||<br>
SI_LINUX_NOT_ANDROID".<br></blockquote><div><br></div><div>asan sources are build by *many* different build systems: two (!) in LLVM, one in GCC and a few proprietary. </div><div>So, we do not want any autoconf-style checks at build time. </div>
<div><br></div><div>--kcc </div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
If you have a better idea or simpler idea then i'm all ears :)<br>
<br>
thanks,<br>
<div class=""><div class="h5">><br>
> --kcc<br>
><br>
>><br>
>> please let me<br>
>> know if you have specific<br>
>> questions about a technical aspect of the patch.<br>
>><br>
>> Please apply.<br>
>> thanks,<br>
>> ><br>
>> > <a href="http://reviews.llvm.org/D3464" target="_blank">http://reviews.llvm.org/D3464</a><br>
>> ><br>
>> ><br>
><br>
><br>
</div></div></blockquote></div><br></div></div>