Page MenuHomePhabricator

abrachet (Alex Brachet)
User

Projects

User does not belong to any projects.

User Details

User Since
May 16 2019, 3:50 PM (75 w, 12 h)

Recent Activity

Aug 12 2020

abrachet added inline comments to D85779: [libc] Add strtok_r implementation..
Aug 12 2020, 4:48 PM · Restricted Project
abrachet added inline comments to D85779: [libc] Add strtok_r implementation..
Aug 12 2020, 1:47 PM · Restricted Project
abrachet accepted D85790: [libc][NFC] Extend <ASSERT|EXPECT>_STR* macros to compare with nullptr..
Aug 12 2020, 8:55 AM · Restricted Project

Aug 11 2020

abrachet added inline comments to D85790: [libc][NFC] Extend <ASSERT|EXPECT>_STR* macros to compare with nullptr..
Aug 11 2020, 3:28 PM · Restricted Project

Aug 6 2020

abrachet requested review of D85407: [libc][NFC] Fix build when using UBSan.
Aug 6 2020, 12:28 AM

Aug 5 2020

abrachet accepted D85103: [libc] Add strspn implementation..

Missing definition in spec/stdc.td

Aug 5 2020, 12:32 PM · Restricted Project

Aug 4 2020

abrachet added a comment to D85103: [libc] Add strspn implementation..

FWIW, I think in most real world scenarios the O(N*M) approach would be faster. Granted I don't know what those scenarios are I had never heard of this function, I have no idea where this is useful.

Aug 4 2020, 12:55 AM · Restricted Project

Jul 31 2020

abrachet added inline comments to D84875: [libc] Adds strrchr implementation..
Jul 31 2020, 1:54 PM · Restricted Project
abrachet added a comment to D84875: [libc] Adds strrchr implementation..

Clearly I'm too late but there are some problems with this revision.

Jul 31 2020, 12:35 PM · Restricted Project

Jul 30 2020

abrachet accepted D84848: [libc] Add a tool called WrapperGen..

This looks good. I wonder though how we will deal with global variables. Those can't be easily wrapped like functions can.

Yes, global variables question is open if we have to support them in some manner. The one we have currently, errno which not really global but like one, is accessed by the extension function __errno_location. So, we are good in this case. If there are other global variables we need to support (nothing comes to my mind), we can take that up at that time.

Jul 30 2020, 12:58 PM · Restricted Project

Jul 29 2020

abrachet added a comment to D84848: [libc] Add a tool called WrapperGen..

This looks good. I wonder though how we will deal with global variables. Those can't be easily wrapped like functions can.

Jul 29 2020, 9:34 AM · Restricted Project

Jul 24 2020

abrachet added inline comments to D84575: [libc] Add scaffolding for ctype and implementation of isalpha.
Jul 24 2020, 7:15 PM · Restricted Project

Jul 21 2020

abrachet added inline comments to D83931: Update Test (EXPECT_EQ and friends) to accept __uint128_t and floating point types (float, double, long double)..
Jul 21 2020, 10:56 AM · Restricted Project
abrachet accepted D83931: Update Test (EXPECT_EQ and friends) to accept __uint128_t and floating point types (float, double, long double)..
Jul 21 2020, 10:52 AM · Restricted Project

Jul 20 2020

abrachet added inline comments to D83931: Update Test (EXPECT_EQ and friends) to accept __uint128_t and floating point types (float, double, long double)..
Jul 20 2020, 11:30 PM · Restricted Project

Jul 17 2020

abrachet added inline comments to D75487: [libc] [UnitTest] Add Matchers.
Jul 17 2020, 11:43 AM · Restricted Project

Jul 16 2020

abrachet accepted D83980: [libc][NFC] Use RemoveCVType to implement IsIntegral and IsPointerType..
Jul 16 2020, 3:55 PM · Restricted Project
abrachet added a comment to D83931: Update Test (EXPECT_EQ and friends) to accept __uint128_t and floating point types (float, double, long double)..

Why add support to the TEST macros for this when we have the MPFR macros which I think handle floats better than we can with TEST. Comparisons with floating points are not fun, its why we use the mpfr library in the first place. Is there even a need to use the TEST macros with floats?

Jul 16 2020, 1:30 AM · Restricted Project

Jun 26 2020

abrachet updated subscribers of D82404: [libc] Minimal Darwin support.

Would you mind looking if it is possible for a program not to be linked against LibSystem.dylib which contains Apple's libc? I thought it was the case that it wasn't possible, but I might be wrong.

Jun 26 2020, 5:43 PM · Restricted Project

Jun 23 2020

abrachet added inline comments to D82376: [libc] Add strdup implementation..
Jun 23 2020, 8:31 AM · Restricted Project

Jun 18 2020

abrachet added a comment to D82134: [libc] Add strcmp implementation..

FWIW, this is a good candidate for fuzz testing

Jun 18 2020, 6:35 PM · Restricted Project
abrachet added a comment to D82133: Add strcmp function to libc..

Are you using the arc tool to create these? I've always had trouble with it too. You can create a diff and upload that diff or copy and paste it in Phab, which I think is a lot easier and less error prone. git diff origin -U99999 > patch then you can upload patch from reviews.llvm.org. Hopefully thats helpful

Jun 18 2020, 5:31 PM · Restricted Project

Jun 9 2020

abrachet accepted D81533: [libc] Add a simple linux aarch64 config..

This makes sense to me. We should look to do this later then. It makes sense to move on now.

Jun 9 2020, 11:57 PM · Restricted Project
abrachet added a comment to D81533: [libc] Add a simple linux aarch64 config..

I don't know the solution here, but there is so much which we end up copying between configs of architectures. It's most apparent with the math.h targets because our implementations are architecture and OS agnostic, but most of libc except for syscall like you mention is too. I think it's clear that having separate build config files for every OS * architecture is untenably difficult for us to maintain. I should have mentioned this earlier, it's not this patch which introduced this, but I didn't catch it then.

Jun 9 2020, 11:06 PM · Restricted Project

May 30 2020

abrachet added a comment to D80495: Fixing the build command for benchmarks..

Oh yes you two are right last I looked must have been before that change. Sorry for the noise.

May 30 2020, 12:43 PM · Restricted Project
abrachet accepted D80612: [libc] Add implementations of ceil[f], floor[f] and trunc[f] from math.h..

LGTM

May 30 2020, 12:43 PM · Restricted Project

May 28 2020

abrachet added a comment to D80495: Fixing the build command for benchmarks..

FWIW, we should do something about clang and clang-tools-extra being required. It also causes the Harbormaster builds to fail because it not longer builds all top level targets anymore.

May 28 2020, 12:38 PM · Restricted Project

May 21 2020

abrachet accepted D80291: [libc][NFC] Simplify memcpy implementation.
May 21 2020, 2:38 PM · Restricted Project

May 16 2020

abrachet added a comment to D79828: [libc] Add implementation of call_once from threads.h..

This looks good to me but it might be worth waiting for @MaskRay if he has anything to say because I missed this the first time.

May 16 2020, 8:40 PM · Restricted Project

May 15 2020

abrachet added inline comments to D80010: [libc] Add memset and bzero implementations.
May 15 2020, 11:57 AM · Restricted Project

May 13 2020

abrachet accepted D79828: [libc] Add implementation of call_once from threads.h..
May 13 2020, 10:50 AM · Restricted Project
abrachet accepted D79826: [libc] Call mtx_init in mtx_test..
May 13 2020, 1:02 AM · Restricted Project
abrachet added inline comments to D79828: [libc] Add implementation of call_once from threads.h..
May 13 2020, 1:02 AM · Restricted Project

May 7 2020

abrachet added inline comments to D79192: [libc] Add integration tests..
May 7 2020, 3:14 PM · Restricted Project

May 6 2020

abrachet added inline comments to D79149: [libc] Move implementations of expf and exp2f from the AOR to src/math..
May 6 2020, 12:30 AM · Restricted Project

May 5 2020

abrachet accepted D79278: [libc] Fix how math results are compared with MPFR results..
May 5 2020, 11:57 PM · Restricted Project
abrachet accepted D79469: [libc] Fix warnings on release build..
May 5 2020, 11:57 PM · Restricted Project

May 4 2020

abrachet accepted D79256: [libc] Improve information printed on failure of a math test which uses MPFR..

LGTM

May 4 2020, 11:07 PM · Restricted Project
abrachet added inline comments to D79256: [libc] Improve information printed on failure of a math test which uses MPFR..
May 4 2020, 7:55 PM · Restricted Project

May 3 2020

abrachet added a comment to D79256: [libc] Improve information printed on failure of a math test which uses MPFR..

I don't want to make any suggestions yet because I don't fully understand why we need a MPFR_TEST. Would you mind clarifying the need for TEST_WITH_BASE_CLASS? Instead of MPFR_TEST could we have TEST_EXTENSION's first argument take a class with compare, would that simplify it? I'm by no means a gtest expert, I'm not sure how someone would use gtest to fill these kinds of needs. Maybe they would make their own macros which end up calling FAIL or using matchers. We aren't limited to that though so perhaps these would end up being more complicated than this solution.

May 3 2020, 1:00 AM · Restricted Project
abrachet accepted D79185: [libc] Include object files from alias entrypoints also in entrypoint libraries..

LG to me too

May 3 2020, 1:00 AM · Restricted Project

May 1 2020

abrachet added a comment to D79192: [libc] Add integration tests..

Few high level comments and questions:

  • Instead of creating a target for every entrypoint, have you considered generating a single file which:
    1. Includes all header files listed in the api.td.
    2. Lists cpp:Function pointers to all functions as required by api.td.

There definitely are problems that need to be solved with this approach. For example, how do you ensure all public header files are already present in the build/.../libc/include directory. But, the appeal of this approach is that one does not have to create explicit or implicit targets for "integration tests". There would be just one target which would test everything in one go.

May 1 2020, 7:16 PM · Restricted Project

Apr 30 2020

abrachet accepted D79189: [libc][NFC] Rename cpp::function to cpp::Function..
Apr 30 2020, 11:47 AM · Restricted Project
abrachet accepted D79150: [libc] Add definitions of double_t and float_t to math.h..
Apr 30 2020, 11:47 AM · Restricted Project

Apr 29 2020

abrachet added inline comments to D79150: [libc] Add definitions of double_t and float_t to math.h..
Apr 29 2020, 9:05 PM · Restricted Project

Apr 28 2020

abrachet accepted D79016: [libc] Add strlen to library entrypoints..
Apr 28 2020, 10:11 AM · Restricted Project

Apr 26 2020

abrachet added a comment to D78886: [libc] Wrap unittests in __llvm_libc namespace. NFC.

If the goal is to not need to explicitly use the __llvm_libc:: prefix in our tests we could change the TEST macro to put the class it makes in __llvm_libc instead of needing to remember to explicitly put our tests in __llvm_libc. Granted it isn't difficult to remember to put new tests in the namespace but I don't think there is any clear advantage to requiring it either.

Apr 26 2020, 6:35 PM · Restricted Project

Apr 25 2020

abrachet committed rG69dad324db3a: [TableGen] [NFC] Make argv0 const (authored by abrachet).
[TableGen] [NFC] Make argv0 const
Apr 25 2020, 1:48 PM
abrachet closed D78840: [TableGen] [NFC] Make argv0 const.
Apr 25 2020, 1:48 PM · Restricted Project
abrachet updated the summary of D78840: [TableGen] [NFC] Make argv0 const.
Apr 25 2020, 1:48 PM · Restricted Project

Apr 24 2020

abrachet created D78840: [TableGen] [NFC] Make argv0 const.
Apr 24 2020, 5:21 PM · Restricted Project
abrachet added inline comments to D77281: [libc] Add libc-tidy..
Apr 24 2020, 12:58 PM · Restricted Project
abrachet added inline comments to D77281: [libc] Add libc-tidy..
Apr 24 2020, 12:29 AM · Restricted Project

Apr 23 2020

abrachet added inline comments to D77281: [libc] Add libc-tidy..
Apr 23 2020, 8:07 PM · Restricted Project
abrachet accepted D78703: [libc] Add spec for sigdelset and sigfillset..

Thanks!

Apr 23 2020, 3:48 PM · Restricted Project
abrachet added a comment to D78703: [libc] Add spec for sigdelset and sigfillset..

Oops, thanks for fixing this

Apr 23 2020, 10:15 AM · Restricted Project

Apr 22 2020

abrachet added a comment to D78612: [libc] Add sanitizer instrumentation to the SIMD strlen implementation..

I personally think this is too clever. The whole templating thing seems like a good overall QOL improvement - but the whole unsanitized safe_check_word that we have to manually sanitize seems challenging to maintain. You would need to do sanitizer instrumentation manually for ASan, MSan, UBSan, HWASan, MTE, DFSan, and any other fun toys we come up with. Each of these has a different interface, and different quirks. Why not just let the compiler do these quirks?

Sorry, the templating thing is probably a distraction at this point.

With respect to the sanitizer parts: In this patch, strlen is an example. With the present sanitizer API, wouldn't hand-crafting instrumentation mean that we have to manually deal with multiple sanitizers in general anyway?

Instead, can't we just use the slow path always under sanitizers? i.e.:

#if __has_feature(address_sanitizer) || defined(__SANITIZE_ADDRESS__)
  static constexpr bool word_optimized = false;
#else // ASan
  static constexpr bool word_optimized = Mask<sizeof(uintptr_t)>::opt;
#endif

Then, the whole dancing-around-managing sanitizers is solved as the compiler just does normal instrumentation of the slow path.

My view is that this just defeats the purpose of sanitizing. Also, we will be setting an example which says, "it is OK to keep fast paths unsanitized."

I agree, sanitizers are useful for detecting problems in programs using libc and detecting problems with the libc itself. It's really the "fast path" where we want the second kind, because it's more complicated and harder to see if it's safe. @sivachandra is right, and thats not an example I would want to set here.

Apr 22 2020, 11:26 AM · Restricted Project
abrachet added a comment to D78612: [libc] Add sanitizer instrumentation to the SIMD strlen implementation..

I'm not sure if this discussion belongs here or D77949, but as this complicates that patch even more I will do so here.

Apr 22 2020, 1:35 AM · Restricted Project
abrachet added a comment to D78611: [libc] Add a library of low level utils..

Do these all belong in a linux specific directory? Wouldn't the two which aren't x86 specific be fine to put into libc/src/__support/?

Apr 22 2020, 1:03 AM · Restricted Project

Apr 21 2020

abrachet accepted D78585: [libc][NFC] Cleanup dependencies in src/signal and test/src/signal..

LG

Apr 21 2020, 2:40 PM · Restricted Project

Apr 20 2020

abrachet accepted D78537: [libc] Propagate entrypoint deps to downstream targets..

Excited about this one!

Apr 20 2020, 11:18 PM · Restricted Project
abrachet accepted D78536: [libc] [NFC] Split the CMake rules into multiple files..
Apr 20 2020, 11:18 PM · Restricted Project

Apr 17 2020

abrachet committed rG5793c84925fa: [libc] Add write(2) implementation for Linux and FDReader test utility (authored by abrachet).
[libc] Add write(2) implementation for Linux and FDReader test utility
Apr 17 2020, 10:48 AM
abrachet closed D78184: [libc] Add write(2) implementation for Linux and FDReader test utility.
Apr 17 2020, 10:48 AM · Restricted Project
abrachet committed rGd9e96b6a0267: [libc] Add spec/*.td as dependencies to add_gen_header (authored by abrachet).
[libc] Add spec/*.td as dependencies to add_gen_header
Apr 17 2020, 10:15 AM
abrachet closed D78349: [libc] Add spec/*.td as dependencies to add_gen_header.
Apr 17 2020, 10:15 AM · Restricted Project
abrachet updated the diff for D78184: [libc] Add write(2) implementation for Linux and FDReader test utility.

Move "hello" to a variable

Apr 17 2020, 10:14 AM · Restricted Project
abrachet committed rG91c10f50f38d: Use proper dependency name for libc.include.stdio (authored by abrachet).
Use proper dependency name for libc.include.stdio
Apr 17 2020, 2:41 AM
abrachet created D78349: [libc] Add spec/*.td as dependencies to add_gen_header.
Apr 17 2020, 2:08 AM · Restricted Project
abrachet added inline comments to D78184: [libc] Add write(2) implementation for Linux and FDReader test utility.
Apr 17 2020, 2:07 AM · Restricted Project
abrachet updated the diff for D78184: [libc] Add write(2) implementation for Linux and FDReader test utility.

Add ssize_t to __posix-types.h

Apr 17 2020, 2:07 AM · Restricted Project

Apr 15 2020

abrachet created D78184: [libc] Add write(2) implementation for Linux and FDReader test utility.
Apr 15 2020, 2:08 AM · Restricted Project

Apr 14 2020

abrachet accepted D76825: [libc] Move implementations of cosf, sinf, sincosf to src/math directory..

Thanks a lot for the detailed review.

I did not address some of the comments. The plan is to quickly absorb the AOR code into the normal LLVM-libc way and try to bring AOR developers to work with LLVM-libc. To that extant, I want to keep their structuring as intact as possible so that they do not have to go through a learning phase to understand their own code. We do want the higher-level structuring to adhere to LLVM libc's style so I tried to get that in shape with this change.

Apr 14 2020, 10:17 PM · Restricted Project
abrachet added a comment to D77949: [libc] Add SIMD strlen implementation ..

I guess this is one of those times where we would have to give up correctness for speed. Quick question - do we have performance numbers that indicate this is faster? I did some quick runs on quick-bench and found it had improvements, but was surprised that the compiler didn't produce a repnz for the old strlen variant...

Bearing in mind to keep the LLVM libc goal here of being sanitizer-friendly, you'd have to implement the sanitizer behaviour yourself. It would basically be:

  1. Foreach bounds-based sanitizer: -fsanitize=address, -fsanitize=bounds, -fsanitize=memory, -fsanitize=hwasan, -fsanitize=memtag:
    • Mark the function as unsanitizeable: __attribute__(no_sanitize("address"))
    • Guard the following code behind an if has_feature(address_sanitizer): (note that the interfaces will slightly differ depending on the sanitizer)
      1. Check the shadow before reading each word (__asan_region_is_poisoned(ptrPtr, sizeof(uintptr_t)))
      2. If the string still looks like it should continue, but the sanitizer reported that it's OOB, you should trap (__asan_report_error)
      3. If the string terminates, and the sanitizer reported that it's OOB, you should check that the string termination point is strictly less than the sanitizer OOB point (fallback to a single-byte slow path using __asan_address_is_poisoned)
      4. If the string terminates, and sanitizers don't report OOB, continue as usual.

Alternatively you could range-check based on the return value, which would probably be simpler (this is what we do with the interceptors currently, see sanitizer_common_interceptors.inc:364).

Apr 14 2020, 3:45 PM · Restricted Project
abrachet committed rG99aea5792841: [libc] Add very basic stdio FILE and fwrite (authored by abrachet).
[libc] Add very basic stdio FILE and fwrite
Apr 14 2020, 1:33 AM
abrachet closed D77626: [libc] Add very basic stdio FILE and fwrite.
Apr 14 2020, 1:33 AM · Restricted Project
abrachet updated the diff for D77626: [libc] Add very basic stdio FILE and fwrite.

Remove fwrite_unlocked from internal header
Internally refer to FILE by its fully qualified name __llvm_libc::FILE NFC

Apr 14 2020, 1:33 AM · Restricted Project
abrachet committed rG54d13b5b2d92: [libc] Remove <functional> dependency in syscall_test.cpp (authored by abrachet).
[libc] Remove <functional> dependency in syscall_test.cpp
Apr 14 2020, 12:29 AM
abrachet closed D77948: [libc] Remove <functional> dependency in syscall_test.cpp.
Apr 14 2020, 12:29 AM · Restricted Project

Apr 13 2020

abrachet added inline comments to D77626: [libc] Add very basic stdio FILE and fwrite.
Apr 13 2020, 2:08 PM · Restricted Project
abrachet updated the diff for D77626: [libc] Add very basic stdio FILE and fwrite.

Remove cookie from the base FILE type

Apr 13 2020, 2:07 PM · Restricted Project
abrachet added a comment to D77949: [libc] Add SIMD strlen implementation ..

As a high level comment, this scheme is technically safe because no mmu that I know of can deal make fault pages on non word boundaries (of course they all do at page boundaries) so readying some extra bytes past the end of the string will never cause SIGBUS or SIGSEGV. However it is still undefined behavior and sanitizers will complain. If you build with LLVM_USE_SANITIZER=Address I'm pretty sure that FourBytes and TwoBytes will fail. And certainly __llvm_libc::strlen(__llvm_libc::strcpy(::malloc(6), "12345")); will get the sanitizers to complain.

I think it is worth discussing if we think it's valuable to turn off sanitizers for this function because we can conclude that it is safe to read past the string in the name of speed. I'd like to hear what the others have to say.

Sorry - I fail to see how this is safe. There are no guarantees about the alignment of the string, the OOB read could trivially cross a page boundary (and even then, I don't think we should be relying on UB).

Apr 13 2020, 2:07 PM · Restricted Project
abrachet added inline comments to D77626: [libc] Add very basic stdio FILE and fwrite.
Apr 13 2020, 10:12 AM · Restricted Project
abrachet updated the diff for D77626: [libc] Add very basic stdio FILE and fwrite.
  • Use incomplete struct FILE as FILE's definition not void
  • Remove cookie from write_function_t
Apr 13 2020, 10:12 AM · Restricted Project
abrachet added inline comments to D77626: [libc] Add very basic stdio FILE and fwrite.
Apr 13 2020, 12:29 AM · Restricted Project

Apr 12 2020

abrachet updated the diff for D77626: [libc] Add very basic stdio FILE and fwrite.
  • Change FILE's external definition from empty struct to void
  • Remove change to syscall_test.cpp
Apr 12 2020, 11:57 PM · Restricted Project

Apr 11 2020

abrachet added a comment to rG04a309dd0be3: [libc] Adding memcpy implementation for x86_64.

Gentle ping that this patch is still broken when building with UBSan

Apr 11 2020, 10:24 PM
abrachet added a comment to D77949: [libc] Add SIMD strlen implementation ..

As a high level comment, this scheme is technically safe because no mmu that I know of can deal make fault pages on non word boundaries (of course they all do at page boundaries) so readying some extra bytes past the end of the string will never cause SIGBUS or SIGSEGV. However it is still undefined behavior and sanitizers will complain. If you build with LLVM_USE_SANITIZER=Address I'm pretty sure that FourBytes and TwoBytes will fail. And certainly __llvm_libc::strlen(__llvm_libc::strcpy(::malloc(6), "12345")); will get the sanitizers to complain.

Apr 11 2020, 10:39 AM · Restricted Project
abrachet updated subscribers of D77949: [libc] Add SIMD strlen implementation ..
Apr 11 2020, 10:39 AM · Restricted Project
abrachet created D77948: [libc] Remove <functional> dependency in syscall_test.cpp.
Apr 11 2020, 9:35 AM · Restricted Project
abrachet added inline comments to D77626: [libc] Add very basic stdio FILE and fwrite.
Apr 11 2020, 1:02 AM · Restricted Project

Apr 10 2020

abrachet added inline comments to D76825: [libc] Move implementations of cosf, sinf, sincosf to src/math directory..
Apr 10 2020, 7:57 PM · Restricted Project
abrachet added inline comments to D77861: [libc] Add cmake target for linting libc..
Apr 10 2020, 7:57 PM · Restricted Project

Apr 6 2020

abrachet created D77626: [libc] Add very basic stdio FILE and fwrite.
Apr 6 2020, 8:44 PM · Restricted Project
abrachet accepted D77533: [libc][NFC] Make all top of file comments consistent..

Just some nits inline.

One thing though, this change is now wider than the original description. So, update the description here (and also in your local git repo if you use git llvm push.)

It might also be useful to add [NFC] to the name while you're at it.

Apr 6 2020, 3:48 PM · Restricted Project
abrachet added a comment to D77533: [libc][NFC] Make all top of file comments consistent..

Also, although we only want header files to have the C++ tag, shouldn't we also left justify the text in our .cpp files too?

Apr 6 2020, 10:51 AM · Restricted Project
abrachet added inline comments to D77533: [libc][NFC] Make all top of file comments consistent..
Apr 6 2020, 10:18 AM · Restricted Project

Apr 3 2020

abrachet accepted D77340: [libc] Add fully-qualified target names..

LGTM. I think the shortened name with the . prefix is a little confusing personally, but I can also see how it is useful.

Apr 3 2020, 3:43 PM · Restricted Project
abrachet added inline comments to D76676: [libc] Unblock SIGABRT before raising in abort.
Apr 3 2020, 3:43 PM
abrachet added inline comments to D76676: [libc] Unblock SIGABRT before raising in abort.
Apr 3 2020, 9:40 AM