[llvm-dev] Use of the C++ standard library in XRay compiler-rt

[David Blaikie via llvm-dev]

On Sun, Mar 12, 2017, 4:10 PM Dean Michael Berris <dean.berris at gmail.com>
wrote:

>
> > On 9 Mar 2017, at 09:32, David Blaikie via llvm-dev <
> llvm-dev at lists.llvm.org> wrote:
> >
> > I agree that we should clean up the standard library usage even just for
> consistency.
> >
>
> +1 -- now that I think about it, it should be fairly doable (also happy to
> help with reviews if that helps).
>
> > Searching the xray directory for dependencies:
> > ...compiler-rt/lib/xray % grep '#include <[^>.]*>' -oh `find . -type
> f|grep -v 'tests'` | sort | uniq -c
> >       1 #include <algorithm>
> >      10 #include <atomic>
> >       1 #include <bitset>
> >       6 #include <cassert>
> >       1 #include <cerrno>
> >       1 #include <cstddef>
> >       7 #include <cstdint>
> >       2 #include <cstdio>
> >       1 #include <cstdlib>
> >       2 #include <cstring>
> >       1 #include <deque>
> >       2 #include <iterator>
> >       2 #include <limits>
> >       2 #include <memory>
> >       4 #include <mutex>
> >       1 #include <system_error>
> >       1 #include <thread>
> >       2 #include <tuple>
> >       1 #include <unordered_map>
> >       1 #include <unordered_set>
> >       3 #include <utility>
> > I think the biggest part is containers, and they are mostly in
> ./xray_buffer_queue.h and ./xray_fdr_logging.cc.
>
> Yes, buffer_queue can definitely live without using system_error,
> unordered_map, and unordered_set. It might make it a bit more complex (we'd
> need to implement a correct and fairly efficient hash set) but if it means
> the deployment model is simpler then I'm happy with that trade-off. When we
> were implementing this, we made a decision to make it so that the "mismatch
> of standard library implementations" was treated as a lower priority issue
> -- something we don't think comes up as often, and is easily solvable by
> re-building the runtime with the standard library the end
> application/binary will be using anyway. Since XRay is only ever
> statically-linked (we don't have a dynamic version of it), I thought the
> rebuild option is slightly simpler than trying to implement the whole XRay
> runtime in a constrained version of C++ and libc-only functions.
>

Except that's not how llvm is distributed. In releases it will ship with
the compiler and runtime libraries but can be used with any c++ standard
library.

This isn't a quality if implementation thing, this is more a correctness
issue.


> >
> > dependencies without buffer queue and fdr logging:
> > ...compiler-rt/lib/xray % grep '#include <[^>.]*>' -oh `find . -type
> f|egrep -v 'tests|buffer|fdr'` | sort | uniq -c
> >       9 #include <atomic>
> >       4 #include <cassert>
> >       1 #include <cerrno>
> >       1 #include <cstddef>
> >       6 #include <cstdint>
> >       2 #include <cstdio>
> >       1 #include <cstring>
> >       2 #include <iterator>
> >       2 #include <limits>
> >       1 #include <memory>
> >       3 #include <mutex>
> >       1 #include <thread>
> >       2 #include <tuple>
> >       2 #include <utility>
> > I believe that this is relatively easy to cleanup. I can do that.
> >
> > I don't know how hard it is to rewrite buffer queue and fdr logging
> using compiler_rt infrastructure.
> >
>
> The crucial things we need in FDR mode logging are:
>
> - Aligned storage (I suspect this could be done without
> std::aligned_storage<...>)
> - memcpy (we use std::memcpy there, but probably didn't need to)
> - thread-local storage (using C++'s `thread_local` keyword)
>
> The buffer queue can be re-written to not use std::system_error in the
> APIs (use XRay-specific enums instead), internally not use std::atomic<...>
> types, and have to implement the FIFO queue, a lookup table for ownership,
> etc. -- while none of these seem hard, they didn't seem worth it at the
> time of implementation.
>
> > I think buffer_queue's probably sufficiently well bounded that it
> shouldn't be drastically hard to replace it with a custom implementation.
> Haven't looked at fdr_logging.
> >
> > Maps/dictionary-like things might be a bit of a pain in particular. Not
> sure if the sanitizers already have some reusable idioms/libraries for that.
> >
> > I'm also not really clear on where the boundary is - which headers or
> language features ('new'?) can be used, and which can't. Can't say I've
> ever tried to make code library agnostic.
> >
>
> One thing we rely on heavily on in the FDR mode implementation is C++'s
> `thread_local` keyword. I'm not sure what that entails runtime-wise (does
> it need pthreads? or something else?) but I'm sure a functional replacement
> would be alright too.
>

No doubt we can find some common API for that, I'd guess tsan probably has
already had to figure out things like that.


> >
> >
> > (this came up for me due to what's probably a bug in the way compiler-rt
> is built - where the lib itself is built with the host compiler but the
> tests are built/linked with the just-bulit clang. My host compiler uses
> libstdc++ 6, whereas the just-built clang will use libstdc++ 4.8. So it
> fails to link due to this mismatch)
>
> How hard would it be to fix that bug?
>

I've started a separate thread on that, but even if that's fixed it's still
necessary to fix the dependency/distribution model here.


> I would think this error would come up in the wild too, but could be
> remedied by just rebuilding XRay with the right C++ standard library.
>
> -- Dean
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20170313/91fb45c4/attachment.html>