Missing definition in spec/stdc.td

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.

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

In D84848#2182500, @sivachandra wrote:

In D84848#2182387, @abrachet wrote:

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.

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

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?

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.

FWIW, this is a good candidate for fuzz testing

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

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

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.

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

LGTM

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.

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.

LGTM

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.

LG to me too

In D79192#2015295, @sivachandra wrote:

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.

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.

Thanks!

Oops, thanks for fixing this

In D78612#1997364, @sivachandra wrote:

In D78612#1997305, @hctim wrote:

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.

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

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/?

LG

Excited about this one!

Move "hello" to a variable

Add ssize_t to __posix-types.h

In D76825#1979742, @sivachandra wrote:

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.

In D77949#1981766, @hctim wrote:

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).

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

Remove cookie from the base FILE type

In D77949#1978750, @hctim wrote:

In D77949#1976185, @abrachet wrote:

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).

• Use incomplete struct FILE as FILE's definition not void
• Remove cookie from write_function_t

• Change FILE's external definition from empty struct to void
• Remove change to syscall_test.cpp

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

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.

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.

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?

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