[PATCH] [LLVM] [sanitizer] add conditionals for libc
Bernhard Reutner-Fischer
rep.dot.nop at gmail.com
Wed Apr 23 07:45:42 PDT 2014
On 23 April 2014 13:26, Kostya Serebryany <kcc at google.com> wrote:
>
>
>
> 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?
I did not complete my testing so for a start i'll just say that it 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.
For reference, i'm fixing stuff like:
libtool: compile: /scratch/obj.x86_64/gcc-4.9.mine/./gcc/xgcc -shared-libgcc -
B/scratch/obj.x86_64/gcc-4.9.mine/./gcc -nostdinc++ -L/scratch/obj.x86_64/gcc-4
.9.mine/x86_64-mine-linux-uclibc/libstdc++-v3/src -L/scratch/obj.x86_64/gcc-4.9
.mine/x86_64-mine-linux-uclibc/libstdc++-v3/src/.libs -L/scratch/obj.x86_64/gcc
-4.9.mine/x86_64-mine-linux-uclibc/libstdc++-v3/libsupc++/.libs -B/opt/x86_64-m
ine-linux-uclibc/usr/x86_64-mine-linux-uclibc/bin/ -B/opt/x86_64-mine-linux-ucl
ibc/usr/x86_64-mine-linux-uclibc/lib/ -isystem /opt/x86_64-mine-linux-uclibc/us
r/x86_64-mine-linux-uclibc/include -isystem /opt/x86_64-mine-linux-uclibc/usr/x
86_64-mine-linux-uclibc/sys-include --sysroot=/opt/x86_64-mine-linux-uclibc/tc-
sysroot -D_GNU_SOURCE -D_DEBUG -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS
-D__STDC_LIMIT_MACROS -I. -I../../../../../src/gcc-4.9.mine/libsanitizer/saniti
zer_common -I.. -I ../../../../../src/gcc-4.9.mine/libsanitizer/include -isyste
m ../../../../../src/gcc-4.9.mine/libsanitizer/include/system -Wall -W -Wno-unu
sed-parameter -Wwrite-strings -pedantic -Wno-long-long -fPIC -fno-builtin -fno-
exceptions -fno-rtti -fomit-frame-pointer -funwind-tables -fvisibility=hidden -
Wno-variadic-macros -I../../libstdc++-v3/include -I../../libstdc++-v3/include/x
86_64-mine-linux-uclibc -I../../../../../src/gcc-4.9.mine/libsanitizer/../libst
dc++-v3/libsupc++ -include ../config.h -I ../../../../../src/gcc-4.9.mine/libsa
nitizer -DSANITIZER_LIBBACKTRACE -DSANITIZER_CP_DEMANGLE -I ../../../../../src/
gcc-4.9.mine/libsanitizer/../libbacktrace -I ../libbacktrace -I ../../../../../
src/gcc-4.9.mine/libsanitizer/../include -include ../../../../../src/gcc-4.9.mi
ne/libsanitizer/libbacktrace/backtrace-rename.h -O2 -g3 -ggdb3 -MT sanitizer_pl
atform_limits_posix.lo -MD -MP -MF .deps/sanitizer_platform_limits_posix.Tpo -c
../../../../../src/gcc-4.9.mine/libsanitizer/sanitizer_common/sanitizer_platfo
rm_limits_posix.cc -fPIC -DPIC -o .libs/sanitizer_platform_limits_posix.o
../../../../../src/gcc-4.9.mine/libsanitizer/sanitizer_common/sanitizer_platfor
m_limits_posix.cc:131:51: error: invalid application of ‘sizeof’ to incompl
ete type ‘__sanitizer::stat64’
unsigned struct_stat64_sz = sizeof(struct stat64);
^
../../../../../src/gcc-4.9.mine/libsanitizer/sanitizer_common/sanitizer_platfor
m_limits_posix.cc:182:55: error: invalid application of ‘sizeof’ to incompl
ete type ‘__sanitizer::rlimit64’
unsigned struct_rlimit64_sz = sizeof(struct rlimit64);
^
../../../../../src/gcc-4.9.mine/libsanitizer/sanitizer_common/sanitizer_platfor
m_limits_posix.cc:187:57: error: invalid application of ‘sizeof’ to incompl
ete type ‘__sanitizer::statvfs64’
unsigned struct_statvfs64_sz = sizeof(struct statvfs64);
^
In file included from ../../../../../src/gcc-4.9.mine/libsanitizer/sanitizer_co
mmon/sanitizer_platform_limits_posix.cc:17:0:
../../../../../src/gcc-4.9.mine/libsanitizer/sanitizer_common/sanitizer_platfor
m_limits_posix.cc:882:55: error: ‘dirent64’ was not declared in this scope
COMPILER_CHECK(sizeof(__sanitizer_dirent64) <= sizeof(dirent64));
^
uClibc may not have these *64() structs/functions, depending on configury.
>
>>
>>
>> > 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.
Yes, it takes a bit of time for sure. But the net-effect of the patch for
glibc-based systems is to do nothing at all, the code is effectively the same
after the preprocessor as it was before the patch.
Only effect is if you build against a libc (say bionic or on *BSD or
uClibc or you-name-it)
where there is no e.g. statfs64. In this case, the instrumentation is
skipped (as before the patch in this particular case, btw), just that
you do not have to hardcode
LINUX_OR_FREEBSD_OR_NETBSD3ONWARD_OR_WHATEVER but can
just use the (cpp-)token obtained during configury. This is IMO simpler and
reduces the maintenance effort to add a new target OS, for example.
>
>>
>>
>> > 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?
Yes, it would be nice to be able to use your sanitizer on more platforms, sure.
But i was not aware that you *require* public build-bots to be provided by
contributors. I read the wiki page to state that it would be nice to have a bot,
not being a requirement for patch-acceptance.
>
>>
>> >
>> >>
>> >>
>> >> 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
Not probing at build time will not work, i fear. I cannot determine
the characteristics
of the target otherwise:
How do you suggest can i determine if e.g. netrom/netrom.h exists
otherwise? I cannot.
As an alternative to the -DHAVE_BLAH on the command-line, i can use a
cmake "configure_file", see
http://reviews.llvm.org/D3464#20ab9453
What do you think?
>
> Then, please follow the code style around (e.g. no /**/ comments)
oops, i'll fix the comments in c++ code, sorry for that.
>
> Is nanosleep always available? Is so, just replace usleep with nanosleep w/o
> any more ifdefs.
nanosleep is required by POSIX1.2008, yes.
thanks,
>
>
>
>>
>>
>> > 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
>> >> >
>> >> >
>> >
>> >
>
>
More information about the llvm-commits
mailing list