[PATCH] [LLVM] [sanitizer] add conditionals for libc

[Kostya Serebryany]

On Wed, Apr 23, 2014 at 3:11 PM, Bernhard Reutner-Fischer <
rep.dot.nop at gmail.com> wrote:

> On 23 April 2014 12:45, Kostya Serebryany <kcc at google.com> wrote:
> >
> > On Wed, Apr 23, 2014 at 2:38 PM, Bernhard Reutner-Fischer
> > <rep.dot.nop at gmail.com> wrote:
> >>
> >> On 23 April 2014 10:57, Kostya Serebryany <kcc at google.com> wrote:
>
> >> These things (e.g. glob_t members) do not change between glibc-releases,
> >> so
> >> i fail to see how this would be costly to maintain? It's pretty much a
> >> one-time
> >> thing to do IMO.
> >>
> >> > So, before we continue with review I would like to hear the motivation
> >> > and
> >>
> >> The motivation behind this patch is that there are libc's apart from
> >> glibc, and
> >> these libc implementations, being POSIX compliant, may lack certain GNU
> >> extensions (as implemented in glibc). As you can see in your own code,
> you
> >> have gazillions of conditionals (preprocessor conditionals, which i add
> >> just add
> >> a couple of more, generic ones, btw, to answer your comment from above)
> to
> >> handle these other libc's.
> >
> >
> > Ok. What particular libc implementation are you trying to use?
>
> I'm targeting uClibc.
>

Are you saying that asan works with uClibc with your patches? Good to know!
Or it just builds?


>
> > And what particular tool are you trying to use (asan, tsan, msan)?
>
> When bootstrapping GCC the build fails IIRC in asan.
>

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.
This may or may not be simpler though.


>
> > Most of the things you hide under ifdefs are important only/mostly for
> msan,
> > which does not exist in the GCC variant anyway.
> >
> >>
> >>
> >> The route this patch takes it to probe these non-standard things and
> >> act accordingly.
> >>
> >> > whether you are welling to help us maintain this code in future (= set
> >> > up a public build bot)
> >>
> >> I'm certainly willing to help maintain this code in the future since i
> >> want to use
> >> your nice sanitizers via gcc, sure. I cannot, however, setup a public
> >> build-bot since
> >> my notebook is not prepared to handle this task on a regular basis.
> >> If you break stuff upstream and i find the backported gcc version to
> >> be dysfunctional, i
> >> will send you fixes if time permits.
> >
> >
> > This round trip has very long latency and brings heavy burden on us (code
> > maintainers).
>
> I do not really understand where you see this heavy burden or a burden
> at all TBH.
> If you break my bootstrap with my libc then i'll fix it up, it will
> not affect you folks at
> all.
>

We will have to review all these changes, retest them on all platforms we
care about and commit.
Then repeat the exercise for the GCC merge.  It takes time.


>
> > We are very reluctant to pay this price.
> > Having a public bot for llvm upstream code will reduce this cost.
>
> If you can provide a box where i can setup cross-compilers to act as
> build-bots then that'd be doable, yes.
>
> I personally do not have such a computer, unfortunately.
>

Well, this is you who want these patches in, not us. Right?


> >
> >>
> >>
> >> I think the patch itself is pretty straight forward,
> >
> >
> > It is straight forward, but it is also huge.
>
> I can break it up into small, logical pieces if you would accept those
> more easily.
>

This will help A LOT.
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

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)
We should not do any probes at build time

Then, please follow the code style around (e.g. no /**/ comments)

Is nanosleep always available? Is so, just replace usleep with nanosleep
w/o any more ifdefs.



>
> > Depending on your actual needs (see above) we might be able to do
> something
> > *much* simpler.
>
> The patch probes for very small, distinct aspects of GNU-extensions of
> POSIX
> functions and disables them on an individual basis, reducing long-term
> maintenance
> cost by adopting to different target-platforms seamlessly in
> established manner (by
> using autoconf resp. existing cmake Modules that are maintained in
> cmake/autoconf
> upstream projects) instead of hard-coding meta-knowledge -- like
> "INTERCEPT_FUNCTION_LINUX_OR_FREEBSD" or "(SI_MAC && !SI_IOS) ||
> SI_LINUX_NOT_ANDROID".
>

asan sources are build by *many* different build systems: two (!) in LLVM,
one in GCC and a few proprietary.
So, we do not want any autoconf-style checks at build time.

--kcc


>
> If you have a better idea or simpler idea then i'm all ears :)
>
> thanks,
> >
> > --kcc
> >
> >>
> >> please let me
> >> know if you have specific
> >> questions about a technical aspect of the patch.
> >>
> >> Please apply.
> >> thanks,
> >> >
> >> > http://reviews.llvm.org/D3464
> >> >
> >> >
> >
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20140423/fefb9689/attachment.html>