[PATCH] D109967: Simplify handling of builtin with inline redefinition

serge-sans-paille added a comment. In D109967#3044464 , @rnk wrote: > Thanks for doing this, this approach looks like it incorporated my feedback > on the previous approach. Yeah, listened to the wise men... and struggled a lot doing so ;-)

[PATCH] D109967: Simplify handling of builtin with inline redefinition

rnk added a comment. Thanks for doing this, this approach looks like it incorporated my feedback on the previous approach. Comment at: clang/lib/CodeGen/CodeGenModule.cpp:3177 + // PR9614. Avoid cases where the source code is lying to us. An available // externally

[PATCH] D109967: Simplify handling of builtin with inline redefinition

kees added a comment. Is https://bugs.llvm.org/show_bug.cgi?id=50322 and dup of https://bugs.llvm.org/show_bug.cgi?id=23280 ? (Can both be closed?) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109967/new/ https://reviews.llvm.org/D109967

[PATCH] D109967: Simplify handling of builtin with inline redefinition

serge-sans-paille added a comment. And on Windows: http://45.33.8.238/win/46077/summary.html ! Thanks for pointing those. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109967/new/ https://reviews.llvm.org/D109967

[PATCH] D109967: Simplify handling of builtin with inline redefinition

serge-sans-paille added a comment. @thakis: fine on OSX: http://45.33.8.238/macm1/18808/summary.html Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109967/new/ https://reviews.llvm.org/D109967 ___

[PATCH] D109967: Simplify handling of builtin with inline redefinition

serge-sans-paille added a comment. @thakis if rG1ecb1bc3e214 doesn't work, I'll revert. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109967/new/

[PATCH] D109967: Simplify handling of builtin with inline redefinition

thakis added a comment. For macOS, see eg http://45.33.8.238/macm1/18806/step_7.txt Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109967/new/ https://reviews.llvm.org/D109967 ___ cfe-commits mailing

[PATCH] D109967: Simplify handling of builtin with inline redefinition

thakis added a comment. This breaks tests on windows and macOS eg http://45.33.8.238/win/46075/step_7.txt Please take a look and revert for now if it takes a while to fix. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109967/new/

[PATCH] D109967: Simplify handling of builtin with inline redefinition

serge-sans-paille added a comment. @kda : thanks for the builbot reference. The issue you're pointing at occurs *before* bd379915de38a9af3d65e19075a6a64ebbb8d6db which specifically fixes the issue spotted by the buildbot.

[PATCH] D109967: Simplify handling of builtin with inline redefinition

kda added a comment. Broke fast buildbot: https://lab.llvm.org/buildbot/#/builders/5/builds/12360 reverting... A guide to reproducing sanitizer bots can be found here: https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D109967: Simplify handling of builtin with inline redefinition

serge-sans-paille added a comment. for the record: I had to apply that extra patch: bd379915de38a9af3d65e19075a6a64ebbb8d6db which enforces the `always_inline` attribute Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D109967: Simplify handling of builtin with inline redefinition

serge-sans-paille added a comment. @kees great, thanks for confirming - I just landed the patch. Can you share a pointer to the missing pieces? I'm interested in implemented the missing parts. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D109967: Simplify handling of builtin with inline redefinition

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG3d6f49a56995: Simplify handling of builtin with inline redefinition (authored by serge-sans-paille). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D109967: Simplify handling of builtin with inline redefinition

kees added a comment. Yeah, I can confirm that many cases are improved with this patch (others are more sensitive and depend on the __bos behavior I mentioned). With the 17 kernel FORTIFY self-tests, all 17 fail without this patch. With this patch, 9 start passing. Nice! CHANGES SINCE LAST

[PATCH] D109967: Simplify handling of builtin with inline redefinition

kees added a comment. I'm setting up to test this patch (thank you!) using my current kernel FORTIFY improvements. Right now I have a bunch of compile-time behavior selftests written:

[PATCH] D109967: Simplify handling of builtin with inline redefinition

serge-sans-paille updated this revision to Diff 374290. serge-sans-paille added a comment. Set default as suggested by @nickdesaulniers CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109967/new/ https://reviews.llvm.org/D109967 Files: clang/lib/CodeGen/CodeGenFunction.cpp

[PATCH] D109967: Simplify handling of builtin with inline redefinition

serge-sans-paille marked 3 inline comments as done. serge-sans-paille added a comment. In D109967#3013552 , @nickdesaulniers wrote: > Looks reasonable. Can you give us some time to test this on the Linux kernel? Sure, who can refuse some extra testing?

[PATCH] D109967: Simplify handling of builtin with inline redefinition

nickdesaulniers accepted this revision. nickdesaulniers added a comment. This revision is now accepted and ready to land. Looks reasonable. Can you give us some time to test this on the Linux kernel? Comment at: clang/test/CodeGen/memcpy-inline-builtin.c:3 + +// RUN:

[PATCH] D109967: Simplify handling of builtin with inline redefinition

serge-sans-paille marked 6 inline comments as done. serge-sans-paille added inline comments. Comment at: clang/test/CodeGen/memcpy-inline-builtin.c:1 +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s -disable-llvm-passes | FileCheck %s +//

[PATCH] D109967: Simplify handling of builtin with inline redefinition

serge-sans-paille updated this revision to Diff 373801. serge-sans-paille added a comment. Herald added a subscriber: whisperity. Update formatting / extra comments Update test to be more explicit about their intent / run them through update_cc_test_checks CHANGES SINCE LAST ACTION

[PATCH] D109967: Simplify handling of builtin with inline redefinition

nickdesaulniers added inline comments. Comment at: clang/test/CodeGen/memcpy-inline-builtin.c:1 +// RUN: %clang_cc1 -triple x86_64-unknown-unknown -S -emit-llvm -o - %s -disable-llvm-passes | FileCheck %s +// nickdesaulniers wrote: > Would `-emit-codegen-only`

[PATCH] D109967: Simplify handling of builtin with inline redefinition

nickdesaulniers added a comment. Please amend the bug description to link to: - https://bugs.llvm.org/show_bug.cgi?id=50322 - https://lore.kernel.org/lkml/20210822075122.864511-17-keesc...@chromium.org/ (You can do `git commit --amend; arc diff --verbatim` to have `arc` update the patch

[PATCH] D109967: Simplify handling of builtin with inline redefinition

nickdesaulniers added a comment. Thanks for the patch! Please fix the lint checks. `git-clang-format HEAD~` should help, and IIRC there is a git hook when using `arc diff` (though maybe that requires one time setup? I seem to have an `.arclint` file in my `llvm-projects` checkout.

[PATCH] D109967: Simplify handling of builtin with inline redefinition

serge-sans-paille added a comment. @kees this does fix https://bugs.llvm.org/show_bug.cgi?id=50322 , but only if the memcpy "inline definition" is flagged as a `__attribute__((always_inline))` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D109967: Simplify handling of builtin with inline redefinition

kees added a comment. Does this address https://bugs.llvm.org/show_bug.cgi?id=50322 ? (I assume this is a new version of https://reviews.llvm.org/D92657 ?) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D109967/new/ https://reviews.llvm.org/D109967

[PATCH] D109967: Simplify handling of builtin with inline redefinition

serge-sans-paille created this revision. serge-sans-paille added reviewers: rnk, nickdesaulniers, efriedma. serge-sans-paille requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. It is a common practice in glibc header to provide an inline