[PATCH] D71082: Allow system header to provide their own implementation of some builtin

manojgupta added a comment. I opened https://github.com/ClangBuiltLinux/linux/issues/979 to see if we can fix this in Linux kernel. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71082/new/ https://reviews.llvm.org/D71082

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

nickdesaulniers added a comment. In D71082#1960815 , @george.burgess.iv wrote: > It'd be nice if the kernel's FORTIFY were more like all of the other existing > ones in this way. Please file a bug with some more details, and let's make it happen.

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

george.burgess.iv added a comment. For a more direct comparison, I offer https://godbolt.org/z/fqAhUC . The lack of optimization in the later case is because we're forced to mark the call to `__builtin_memcpy` in the inline memcpy as `nobuiltin`. If we instead rename things, this issue doesn't

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille added a comment. In D71082#1958597 , @manojgupta wrote: > Yes, it'd be nice if all of the FORTIFY handling can be improved. For a > simple call like memcpy of 8 bytes in the example, there is no reason to emit > all these stack/range

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

manojgupta added a comment. Yes, it'd be nice if all of the FORTIFY handling can be improved. For a simple call like memcpy of 8 bytes in the example, there is no reason to emit all these stack/range checks since they'd degrade memcpy performance. I still think this change should be reverted

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

rnk added a comment. It's unsurprising that the Linux kernel's own Fortify implementation is incompatible with Clang. The whole feature should never have been implemented in the library to begin with, but here we are. I think the Linux kernel folks would prefer it if we can fix forward and

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille added a comment. @manojgupta prior to this patch, clang was using its version of `memcpy` and not the one provided inline. I slightly modified your reproducer to showcase that aspect: https://godbolt.org/z/NmxC0v Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

manojgupta added a comment. Unfortunately, cherry-picking the kernel patches didn't work including latest memcpy for x86 (https://github.com/torvalds/linux/commit/170d13ca3a2fdaaa0283399247631b76b441cca2 and https://github.com/torvalds/linux/commit/c228d294f2040c3a5f5965ff04d4947d0bf6e7da ).

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

manojgupta added subscribers: nickdesaulniers, llozano, srhines. manojgupta added a comment. I was able to reduce to following: typedef unsigned int u32; typedef unsigned long long u64; typedef unsigned long size_t; void fortify_panic(const char *name) __attribute__((noreturn)) ;

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

manojgupta added a comment. Sure, I am trying to root cause the issue. Will report back hopefully soon. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71082/new/ https://reviews.llvm.org/D71082 ___

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

efriedma added a comment. I'd prefer not to revert a change that's three months old without some sort of evidence the issue is actually the compiler's fault. If nobody else has seen an issue, it's probably okay to let it sit for a day or two. Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

manojgupta added a comment. We believe this change (relanded as https://reviews.llvm.org/rGd437fba8ef626b6d8b7928540f630163a9b04021) is causing a mis-compile in Linux kernel 4.4 builds resulting in local test failures. Chrome OS bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1066638

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

george.burgess.iv added a comment. Until recently, both CrOS and Android disabled FORTIFY wholesale when any of asan/msan/tsan were active. Bionic recently got support for FORTIFY playing nicely with sanitizers, but that support boils down to "turn off all the FORTIFY runtime checks, but leave

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

thakis added a comment. This caused a regression when using `-fsanitize=memory -D_FORTIFY_SOURCE=2`. MemorySanitizer requires all code, including system libraries, to be instrumented. Else it reports false positives. For glibc, it has lots of interceptors to make sure glibc doesn't have to be

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille added a comment. >> The simplest one I can think of is to make `__warn_memset_zero_len` a >> recognized builtin that generates no code. > > +1, simple and easy. See https://reviews.llvm.org/D72869 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille added a comment. In D71082#1824563 , @rnk wrote: > The simplest one I can think of is to make `__warn_memset_zero_len` a > recognized builtin that generates no code. +1, simple and easy. > I believe we also branched 10.0 between this

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

rnk added a comment. I agree, raising -fgnuc-version is not an acceptable workaround, and we can't ship a clang that doesn't work with old versions of glibc. I think we need a different workaround. The simplest one I can think of is to make `__warn_memset_zero_len` a recognized builtin that

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

george.burgess.iv added a comment. (It's interesting to me if gcc doesn't warn about that libcxx code, since the whole point of the gnuc 5.0 check there was "the compiler should check this for us now"...) If a glibc-side fix for this does materialize, IMO `-fgnuc-version=5.0` isn't a good

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille added a comment. @akhuang : I've reported https://sourceware.org/bugzilla/show_bug.cgi?id=25399 to glibc. According to me this is more of a glibc problem than this patch, and as we have a workaround with `-fgnuc-version=5.0` I think we could apply the patch back.

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille added a comment. In D71082#1822925 , @akhuang wrote: > This caused a linker error in chromium: > > ld.lld: error: undefined symbol: __warn_memset_zero_len > > > Apparently now that the glibc memset is being used, __warn_memset_zero_len

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

akhuang added a comment. Reverted in 3d210ed3d1880c615776b07d1916edb400c245a6 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71082/new/ https://reviews.llvm.org/D71082

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

akhuang added a comment. This caused a linker error in chromium: ld.lld: error: undefined symbol: __warn_memset_zero_len Apparently now that the glibc memset is being used, __warn_memset_zero_len gets called from libc++ code

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

This revision was automatically updated to reflect the committed changes. Closed by commit rG921f871ac438: Allow system header to provide their own implementation of some builtin (authored by serge-sans-paille). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

efriedma accepted this revision. efriedma added a comment. This revision is now accepted and ready to land. LGTM Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71082/new/ https://reviews.llvm.org/D71082

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille added a comment. @sylvestre.ledru : I'd like a confirmation from @efriedma first! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71082/new/ https://reviews.llvm.org/D71082 ___

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

sylvestre.ledru added a comment. @serge-sans-paille any blocker for this to land? Would be great to have it in version 10. Thanks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71082/new/ https://reviews.llvm.org/D71082

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille added a comment. @efriedma validation passes without the checks you were pointed out, see https://github.com/serge-sans-paille/llvm-project/pull/4/checks Thanks for making the code simpler! Comment at: clang/lib/AST/Decl.cpp:3019 +if (SL.isValid()) +

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille updated this revision to Diff 233924. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71082/new/ https://reviews.llvm.org/D71082 Files: clang/include/clang/AST/Decl.h clang/lib/AST/Decl.cpp clang/lib/CodeGen/CGExpr.cpp

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille updated this revision to Diff 233923. serge-sans-paille marked an inline comment as done. serge-sans-paille added a comment. Take comment into account. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71082/new/

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille marked an inline comment as done. serge-sans-paille added inline comments. Comment at: clang/lib/AST/Decl.cpp:3019 +if (SL.isValid()) + return SM.isInSystemHeader(SL); + } efriedma wrote: > I'm a little concerned about this; we're

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

efriedma added inline comments. Comment at: clang/lib/AST/Decl.cpp:3019 +if (SL.isValid()) + return SM.isInSystemHeader(SL); + } I'm a little concerned about this; we're subtly changing our generated code based on the "system-headerness" of the

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille updated this revision to Diff 233753. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71082/new/ https://reviews.llvm.org/D71082 Files: clang/include/clang/AST/Decl.h clang/lib/AST/Decl.cpp clang/lib/CodeGen/CGExpr.cpp

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille updated this revision to Diff 233746. serge-sans-paille marked 6 inline comments as done. serge-sans-paille added a comment. Handle comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71082/new/

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

george.burgess.iv added a comment. Just a few more nits, and this LGTM. Thanks again! IDK if Eli wanted to have another look at this; please wait until tomorrow before landing to give him time to comment. Comment at: clang/lib/AST/Decl.cpp:3006 +bool

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille updated this revision to Diff 233648. serge-sans-paille added a comment. Improve test case portability. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71082/new/ https://reviews.llvm.org/D71082 Files:

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille updated this revision to Diff 233585. serge-sans-paille added a comment. @george.burgess.iv : take into account reviews, extra testing and function renaming. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71082/new/

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

george.burgess.iv added inline comments. Comment at: clang/lib/AST/Decl.cpp:3006 +bool FunctionDecl::isReplaceableSystemFunction() const { + FunctionDecl const *Definition; serge-sans-paille wrote: > george.burgess.iv wrote: > > serge-sans-paille wrote: > > >

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille marked an inline comment as done. serge-sans-paille added inline comments. Comment at: clang/lib/AST/Decl.cpp:3006 +bool FunctionDecl::isReplaceableSystemFunction() const { + FunctionDecl const *Definition; george.burgess.iv wrote: >

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

george.burgess.iv added a comment. Should we also have a quick test-case in Sema/ verifying that we still get the warnings that Eli mentioned? Comment at: clang/lib/AST/Decl.cpp:3006 +bool FunctionDecl::isReplaceableSystemFunction() const { + FunctionDecl const

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille added a comment. @george.burgess.iv : up? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71082/new/ https://reviews.llvm.org/D71082 ___ cfe-commits mailing list

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille added a comment. Validation OK: https://github.com/serge-sans-paille/llvm-project/pull/4/checks Comment at: clang/lib/AST/Decl.cpp:3006 +bool FunctionDecl::isReplaceableSystemFunction() const { + FunctionDecl const *Definition; Note to

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille updated this revision to Diff 232894. serge-sans-paille added a comment. - Be less intrusive in the getBuiltinID() system, just hook inside CGExpr.cpp - Take into account @george.burgess.iv remark on builtins referencing themselves in an override. Add a test case for that

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille updated this revision to Diff 232667. serge-sans-paille added a comment. Fix interaction with fortify warnings (cc @efriedma) Also I've checked interactions with the test suite I wrote here https://github.com/serge-sans-paille/fortify-test-suite/ and clang now passes all the

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

efriedma added a comment. It looks like this patch accidentally suppresses certain warnings when _FORTIFY_SOURCE is enabled. For example, Sema::CheckMemaccessArguments only gets called for recognized builtin functions. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

george.burgess.iv added a comment. Thanks for looking into this! I dunno what implications this has elsewhere, but I'd like to note a few things about this general approach: - AIUI, this'll only work for FORTIFY functions which are literally calling `__builtin___foo_chk`; an equivalent

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille updated this revision to Diff 232425. serge-sans-paille marked an inline comment as done. serge-sans-paille added a comment. test case updated Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D71082/new/

[PATCH] D71082: Allow system header to provide their own implementation of some builtin

serge-sans-paille created this revision. serge-sans-paille added reviewers: mstorsjo, george.burgess.iv, efriedma. Herald added a project: clang. Herald added a subscriber: cfe-commits. If a system header provides an (inline) implementation of some of their function, clang still matches on the