missed optimization leading to compiletime assert with address sanitizer

[arndb]

Bugzilla Link	42580
Version	trunk
OS	Linux
Blocks	#4440
CC	@arndb,@efriedma-quic,@nathanchance,@nickdesaulniers,@zygoloid

Extended Description

The current clang-9 prerelease causes one file in the linux kernel to get fail compilation with an assertion triggering when building with -sanitize=kernel-address:

void __iwl_err(char *, ...);
int a;
void iwl_fw_dbg_apply_point() {
  const char b[] = "WRT ext=%d. Invalid apply point %d for %s\n";
  if (a) {
    void __compiletime_assert_2790(void);
    if (b[sizeof(b) - 2] != '\n')
      __compiletime_assert_2790();
  }
  __iwl_err(0, b);
}

$ clang-9 -O2 -fsanitize=kernel-address -S dbg.i -o- | grep compiletime
callq __compiletime_assert_2790

both clang-8 and gcc-8 pass the assertion here.

See also https://godbolt.org/z/cPrkv4

[nickdesaulniers]

See also: ClangBuiltLinux/linux#580

[nathanchance]

I bisected using Arnd's reproducer:

3bf72d7 is the first bad commit
commit 3bf72d7
Author: Eli Friedman efriedma@quicinc.com
Date: Fri Feb 8 21:18:46 2019 +0000

[Sema] Make string literal init an rvalue.

This allows substantially simplifying the expression evaluation code,
because we don't have to special-case lvalues which are actually string
literal initialization.

This currently throws away an optimization where we would avoid creating
an array APValue for string literal initialization.  If we really want
to optimize this case, we should fix APValue so it can store simple
arrays more efficiently, like llvm::ConstantDataArray.  This shouldn't
affect the memory usage for other string literals.  (Not sure if this is
a blocker; I don't think string literal init is common enough for this
to be a serious issue, but I could be wrong.)

The change to test/CodeGenObjC/encode-test.m is a weird side-effect of
these changes: we currently don't constant-evaluate arrays in C, so the
strlen call shouldn't be folded, but lvalue string init managed to get
around that check.  I this this is fine.

Fixes llvm/llvm-project#39776  .

llvm-svn: 353569

clang/lib/AST/ExprConstant.cpp | 69 +++++++++---------------
clang/lib/CodeGen/CGExprConstant.cpp | 10 ----
clang/lib/Sema/SemaInit.cpp | 1 +
clang/test/AST/ast-dump-wchar.cpp | 8 +--
clang/test/CodeGenObjC/encode-test.m | 3 +-
clang/test/SemaCXX/constant-expression-cxx11.cpp | 8 +++
6 files changed, 40 insertions(+), 59 deletions(-)

$ git bisect log
git bisect start

good: [7b55654] Fix build breakage from llvm r351317

git bisect good 7b55654

bad: [617df20] [CodeGen] Add larger vector types for i32 and f32

git bisect bad 617df20

bad: [5e165fb] [NFC] Add missing revision number in libc++ ABI changelog

git bisect bad 5e165fb

bad: [7f78d47] [DebugInfo] Apply subprogram attributes on behalf of owner CU

git bisect bad 7f78d47

good: [bd3adbb] [X86][SSE] Add tests showing missing SimplifyDemandedVectorElts support for X86ISD::BLENDV

git bisect good bd3adbb

bad: [9e5e868] AMDGPU/GlobalISel: Fix RegBankSelect for GEP.

git bisect bad 9e5e868

bad: [a2f6093] llvm-lib: Implement /list flag

git bisect bad a2f6093

good: [baf2f35] sanitizers: Introduce ThreadType enum

git bisect good baf2f35

good: [340cb87] [llvm-objcopy] Add --redefine-syms

git bisect good 340cb87

good: [2add627] [analyzer] Opt-in C Style Cast Checker for OSObject pointers

git bisect good 2add627

bad: [b041a18] Temporarily disable calls to getgrnam/getgrnam_r in test due to it hitting unrelated issues in EGLIBC 2.19.

git bisect bad b041a18

bad: [1386d99] [x86] add test for miscompiling setcc transform (#40657 ); NFC

git bisect bad 1386d99

bad: [3edf63c] [lld-link] better error message when failing to open archive members

git bisect bad 3edf63c

bad: [3bf72d7] [Sema] Make string literal init an rvalue.

git bisect bad 3bf72d7

good: [57e60a5] Fix typo

git bisect good 57e60a5

first bad commit: [3bf72d7] [Sema] Make string literal init an rvalue.

[efriedma-quic]

It looks like without my patch, "b[sizeof(b) - 2]" got constant-folded by clang; with my patch, it doesn't get constant-folded by clang. This is basically the same thing that happened to encode-test.m, and wasn't really intentional, as far as I can tell.

Without asan, the load is eventually optimized away by GVN. With asan, we explicitly disable the optimization in question (see https://reviews.llvm.org/D14763).

I see a few options here to move forward:

1. Enable constant folding for all arrays in C, which would again catch this case. This would probably require fixing the performance issue called out in the "FIXME" in r353569.
2. Some more restricted version of the above that specifically applies to string initialization, or a related hack.
3. Fix GVN so it's less conservative with asan enabled.
4. Ask the kernel team to fix their code, by adding "static" to the definition of the string. This is probably a good idea anyway.

[nickdesaulniers]

4. Ask the kernel team to fix their code, by adding "static" to the
   definition of the string. This is probably a good idea anyway.

Thanks for the tip, will send such a patch (I've confirmed it works around the issue).

[nickdesaulniers]

https://lkml.org/lkml/2019/7/11/751