Tried another not-so-intrusive approach. @rsmith?

In D104116#2816291, @RKSimon wrote:

This leaves the question - what hardware should we align each of the CK_x86_64_v* targets with?

Thanks for the fast review!

Added test + fix suggested by @aaron.ballman

LGTM, sorry for the late review.

Do not cache the (potential) function call.

@kcc @eugenis : up ? I applied the patch to the Fedora package because it ruins the user experience otherwise. I think it's worth considering applying it to main too.

@kcc any thoughts on that one?

@eugenis any thought on that one? Without this patch, clang -fsanitize=address main.c fails because it needs C++ symbol.

Remove clang-formatted noise

Patch updated with the advised comment.

@eugenis are you fine with that change? the maintenance cost seems low, and the benefit for the end user seems valuable enough (?)

@nikic here is the follow-up you were looking for

This change introduces a behavior change: when an attribute is not set, it used to mean `use the default value''. It now means `attribute unset''.

@rnk: does that look like an acceptable solution to you now?

In D102475#2759153, @jakubjelinek wrote:

This is certainly wrong. It will happily return 1 when some other thread is changing it, and the store is non-atomic.

It would be ideal to drop these llc flags completely, and adjust the tests that use them to use FMF flags. I don't think they serve any purpose apart from making bad tests simpler to write.

Agree. IIUC, we can do that part now without waiting for the next question to be resolved...

I've successfully tested that one and it works fine.

cc @tstellar I went ahead and ran a validation : these three symbols seems to be enough.

@rnk, let me double check that.

In D101972#2753900, @serge-sans-paille wrote:

@dcavalca can you confirm this fixes your issue?

Minor nits as suggested by reviewers + extend the list.

@dcavalca can you confirm this fixes your issue?

Force external visibilty on a limited set of symbols

@rnk setting the whole class as external is the less intrusive patch and I've tested it successfully. Maybe it's enough to set the default visibility to id() and template <typename T> const char Any::TypeId<T>::Id = 0 ?

This is what libcxx does for std::any when rti is not available.

@rnk it's used in the newPassManager. forcing the visibility of llvm::Any indeed fixes the bug I met (that was my original patch), but I also understand the concern of @tstellar concerning the possibility of having similar issue across the code base. A possible way to solve the issue the hard way would be to design a clang warning / a static analysis that spots this kind of issues. Thinking of it, maybe the check could be done at link time to, or on the final shared library?

Take into account latest reviews

In D99080#2723694, @nikic wrote:

Should this one be abandoned now?

@rnk I don't want that patch to bitrot , as it addresses a defect in _FORTIFY_SOURCE, i.e. it fills an (arguably small) security defect. Can you please have a look?

@saugustine thanks for the revert. I'll investigate that.
@thakis I checked the instruction count and overall execution time and didn't notice much change.
@dexonsmith Yeah that was the plan: switch to SmallVector once that one, hum, validates ;-)

Take review into account.

Fix and test verifier

Add verifier step

In D99299#2675071, @nikic wrote:

As you are now asserting that only certain values are allowed, I believe you also need to add a verifier check that checks this. It should not be possible to assert using a valid module.

Thanks @thakis, inestigating the issue is likely to take some time as it seems to be arch or system dependent

You may need to do the same change in llvm/tools/llvm-jitlink/CMakeLists.txt, which makes me think it may valuable to implement a decent check à la autoconf to check for the actual symbol you're looking for, probably based on a sequence of CHECK_LIBRARY_EXISTS. Would you be ok to do that change?

Reading through the source, it's relatively clear that -arch is specific to the Mach0 format, but -target seems applicable to OSX too, see https://godbolt.org/z/j8jvjbo84

@nikic gentle ping ;-)

git grep confirmed!

Take @MaskRay comments into account

Fix handling of final character

LGTM, thanks!

After a great deal of benchmarking on different architectures, I finally settled for the bithack version, as advised by @lattner . The usage of llvm::countTrailingZeros makes the implementation faster than the previous version and it's easier to read.

For complentess, I must mention another optimization: run a first memchr(Buf, BufLen, '\r') and if it finds nothing, use a fast path. That brings interesting speedup for the ref and seq version, but not for the SSE one. And it makes the code more difficult to read.

Thanks @lattner for the feedback. I wasn't super happy with the stability of the performance, so I isolated the function in a standalone file, and benchmarked several implementations. All the sources are available there: https://github.com/serge-sans-paille/mapping-line-to-offset