User Details
- User Since
- Oct 10 2016, 10:44 AM (223 w, 3 d)
Wed, Jan 20
Thanks @alexfh ! Should be good now.
Address doc + tool-extra issues
Tue, Jan 19
As suggested by @aaron.ballman base the detection of top-level-ness on Sema::LookupName to avoid re-implementing the wheel.
ping ? This change looks harmless to me, I'd appreciate an ack though.
Up ?
Thu, Jan 14
Tue, Jan 12
First review round. I'm pretty excited by this work, thanks for handling this!
Mon, Jan 11
Fri, Jan 8
Ignore forward declaration of tagdecl
Update codebase and testbed to reflect recent discussion.
Up?
Address some of the review
Tue, Jan 5
Mon, Jan 4
Rebased on main.
Rebased on main.
Dec 22 2020
Instead of pretending to be smart, just consider that if the considered decl is at the top level, then it's a top-level decl, otherwise it's not. There may be some interpretation wrt. the standard, but this seems both simple and conservative to me. It gies a few false negative, but it's a warning here, so that should be fine.
LGTM, thanks!
Dec 21 2020
Commited as of 5e31e226b5b2b682607a6578ff5adb33daf4fe39, I totally messed the differential revision :-)
lgtm, thanks!
Dec 18 2020
@siddhesh what anout the following?
Dec 17 2020
Take review into account, some extra tests suggested by @aaron.ballman and the according modification in Decl.cpp
Dec 16 2020
@rnk what do you find of this approach?
Take review into account and add more tests
Dec 15 2020
I do think it would be good for this to somehow handle macros with reserved names, since macros are one of the primary dangers of using reserved names (if I #define _Tp 0 and then later #include <vector>
When __builtin_dynamic_object_size returns a non-constant expression, it cannot be -1 since that is an invalid return value for object size
Dec 14 2020
More test cases and updated reserved isReserved()
As suggested by @rnk, spot the pattern more accurately at clang level, and use a combination of nobuiltin / builtin attributes to flag it at LLVM IR level.
What I like with that approach is that we're not preventing the emission a function Body, but just handling it in a decent way.
Up? I'd like to land that one this week if possible.
@sylvestre.ledru I: double checked and there's nothing in the original advisory against Darwin, but nothing that clearly states it's protected either (unlike Windows-based system). And there's also nothing specific to Darwin in the stack clash protection implementation, I think it's ok to only warn on Windows.
Dec 11 2020
Dec 9 2020
@rnk the new approach doesn't have the compilation time impact the previous had, still passes validation and a new test case has been added.
Dec 8 2020
Updated approach, much less costly: match the pattern of functions forwarding to self, as detecting if they're recursive doesn't match the reality of inline builtins.
The rational could be that if it's doing anything other than forwarding to self, then it's there on purpose and we shouldn't skip it.
Dec 7 2020
Update test case
Improve trivially recursive function detection
Dec 5 2020
Sure! Can you send me an email at sguelton@redhat.com with the email you want to use for authorship? Thanks!
Dec 4 2020
Some survivors!
@nickdesaulniers I added you to the review because you bumped into the non-inling issue at the kernel level at some point.
Update documentation and tests
Dec 3 2020
Update test
Nov 30 2020
Make the ascii test for isprintable locale-independent
The backend implementation of stack-clash-protection is incompatible with Windows, see https://github.com/llvm/llvm-project/blob/master/llvm/lib/Target/X86/X86FrameLowering.cpp#L486
As the state of LastUser are closely tied, it would make sense to encapsulate the two members in a class that would maintain the consistency of these two states, so that the two new operations recordNewUser(Pass, User) and updatelastUser(Pass, User) become explicit ?
If that's okay with @leonardchan in terms of RISCV validation, I'm fine with that one. Any branch removed is less path to cover!
Nov 26 2020
Compiles fine with
I'm going to run a bunch of tests with different configuration before accepting :-)
We already discussed this change on IRC, I'm fine with it, once the suggested renaming is done.
Nov 25 2020
Nov 19 2020
Some history on that particular problem: back to https://reviews.llvm.org/D54472 when we moved away from llvm::isPodLike<> in favor of llvm::is_trivially_copyable<>, the goal was to move to is_trivially_copyable once the minimal compiler requirements reaches a supported state.
So I'm not in favor of a diverging between the llvm and std implementation, but either
Interestingly, gcc and clang have different behavior here: https://godbolt.org/z/hdadhY
And clang already honors the user-defined bcopy why is -D_FORTIFY_SOURCE different from the example here?
Nov 18 2020
Nov 17 2020
! In D91677#2401785, @siddhesh wrote:
Maybe because that D71082 is limited to clang? The bcopy survives clang but llvm forces it into a memmove without this patch.
@sbc100 Can you share your cmake configuration?
The problem you're trying to solve doesn't seem specific to bcopy: any function with an inline definition should probably *not* be replaced, and the inlined definition should be favored. This was the spirit of that patch https://reviews.llvm.org/D71082. i wonder why your test case doesn't fall into that configuration.
@sbc100: This should be fixed by 6795984a47dfd2da4f451ad86a99021d6c9f85b5
Can you give an example of how this would be used to select the gcc-toolchain?
I was thinking of `-Wl,--error-handling-script=/etc/clang/linker-script.sh` or something along the line. The initial goal would be to suggest installing the asan runtime if some asan symbols are missing.