[libc-commits] [PATCH] D74397: [libc] Adding memcpy implementation for x86_64

Wed Feb 12 14:50:53 PST 2020

sivachandra added inline comments.

================
Comment at: libc/test/src/string/memory_utils/memcpy_utils_test.cpp:14
+
+#include <cassert> // assert
+#include <cstdint> // uintptr_t
----------------
abrachet wrote:
> sivachandra wrote:
> > gchatelet wrote:
> > > sivachandra wrote:
> > > > abrachet wrote:
> > > > > D74091 Was also wanting to use `assert`/`abort` we should add them. Also that comment is funny to me because its the only thing in assert.h :)
> > > > Point taken. I will prepare something to address this.
> > > I think that would be nice to have the functions in `memcpy_utils.h` have some precondition checking, an `abort` function would be useful indeed,
> > > ```
> > > #if not defined(NDEBUG)
> > > // check precondition
> > > #endif
> > > 
> > > ```
> > Okay. There are two kinds of `abort` here. One to use in the implementation, another to use in the test. For the `abort` used in the test, we should use the one coming from the system libc. For the one going into the implementation, we should use the one from llvm-libc. The abort function is fairly involved and needs other pieces to be in place for us to build llvm-libc's implementation. Let me do my homework get back to you on that.
> > The abort function is fairly involved and needs other pieces to be in place for us to build llvm-libc's implementation.
> I was actually working on writing to the list about getting started on signals, which is how abort should be implemented I think, `for(;;) raise(SIGABRT);`. I'd be happy to get started on signals :)
> 
> I think for now though, it is fine to have abort be `__builtin_trap()` because we are mainly focused on x86 at the moment. (I remember on ARM32 __builtin_trap() calls abort for example)
It would be awesome if you can start work on signals.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D74397/new/

https://reviews.llvm.org/D74397