- User Since
- Oct 10 2016, 10:44 AM (244 w, 3 d)
Tried another not-so-intrusive approach. @rsmith?
Tue, Jun 15
Mon, Jun 14
Sun, Jun 13
Thanks for the fast review!
Fri, Jun 11
Thu, Jun 10
Added test + fix suggested by @aaron.ballman
Wed, Jun 9
LGTM, sorry for the late review.
Tue, Jun 8
Do not cache the (potential) function call.
Mon, Jun 7
Thu, Jun 3
Tue, May 25
@kcc any thoughts on that one?
Mon, May 24
Sat, May 22
Fri, May 21
@eugenis any thought on that one? Without this patch, clang -fsanitize=address main.c fails because it needs C++ symbol.
Thu, May 20
Remove clang-formatted noise
Wed, May 19
Tue, May 18
Patch updated with the advised comment.
May 18 2021
@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?
May 14 2021
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...
May 13 2021
May 12 2021
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.
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
May 11 2021
@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 ?
May 10 2021
May 6 2021
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?
May 5 2021
May 4 2021
Apr 30 2021
Take into account latest reviews
Apr 29 2021
Apr 22 2021
Apr 20 2021
Apr 19 2021
@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?
Apr 16 2021
Apr 15 2021
@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 ;-)
Apr 14 2021
Apr 13 2021
Take review into account.
Fix and test verifier
Apr 8 2021
Add verifier step
Apr 7 2021
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!
Apr 6 2021
Take @MaskRay comments into account
Fix handling of final character
Apr 5 2021
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.
Apr 3 2021
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